Opened 3 years ago

Closed 3 years ago

#21309 closed defect (fixed)

Fix Omnibox for TBB/52ESR

Reported by: arthuredelstein Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff52-esr, tbb-7.0-must, TorBrowserTeam201703R
Cc: Actual Points:
Parent ID: #20680 Points:
Reviewer: Sponsor: Sponsor4

Description

The Omnibox code different in Firefox 52ESR vs 45ESR. We need to rewrite our patch for Omnibox.

Child Tickets

Change History (19)

comment:1 Changed 3 years ago by arthuredelstein

Component: - Select a componentApplications/Tor Browser
Owner: set to tbb-team

comment:2 Changed 3 years ago by gk

Keywords: ff52-esr added; tbb-52esr removed

comment:4 Changed 3 years ago by gk

Keywords: TorBrowserTeam201702 added

comment:5 Changed 3 years ago by gk

Sponsor: Sponsor4

This is Sponsor4 work

comment:6 in reply to:  3 Changed 3 years ago by arthuredelstein

Replying to mcs:

Are you referring to the following two patches?

https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.7.0esr-7.0-1&id=773b70c590a5897de9a87c4ffb8b844f5eeb519b

https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.7.0esr-7.0-1&id=b839017ae214ec42cf203b711d371ab90bdc959b

Sorry, I somehow missed this question. Yes, the implementation patch needs to be fixed. I think the testing patch may still work.

Mozilla changed their search engine list format so I am currently grappling with that.

Last edited 3 years ago by arthuredelstein (previous) (diff)

comment:7 Changed 3 years ago by arthuredelstein

Status: newneeds_review

Here's my patch. Unfortunately it only applies currently to the US English locale. I will need to investigate how to update our tor-browser-bundle.git code (a la #18915). But if this patch looks OK I will add it to our #20680 branch:

https://github.com/arthuredelstein/tor-browser/commit/21309

comment:8 Changed 3 years ago by mcs

Do you know if the browser.search.* prefs are set from the values that are included in browser/locales/search/list.json?

Also, do we need to change the en-US US and CA entries in that file? I assume the browser uses the US entries if it detects that the user is located in the USA and CA if in Canada (although maybe we disabled that kind of location detection? I cannot remember).

For non-en-US locales, will it work to just patch browser/locales/search/list.json for each locale that we care to change? That won't work if the language packs override those settings though.

comment:9 in reply to:  8 ; Changed 3 years ago by arthuredelstein

Replying to mcs:

Do you know if the browser.search.* prefs are set from the values that are included in browser/locales/search/list.json?

Yes, they appear to be.

Also, do we need to change the en-US US and CA entries in that file? I assume the browser uses the US entries if it detects that the user is located in the USA and CA if in Canada (although maybe we disabled that kind of location detection? I cannot remember).

I believe that location detection is disabled, because we have browser.search.geoSpecificDefaults set to false.

For non-en-US locales, will it work to just patch browser/locales/search/list.json for each locale that we care to change? That won't work if the language packs override those settings though.

Unfortunately it doesn't work -- the language packs each contains a list.json file that overrides these settings. So we'll need to update the #18915 patch as I mentioned in comment:7.

comment:10 in reply to:  9 ; Changed 3 years ago by mcs

Replying to arthuredelstein:

Unfortunately it doesn't work -- the language packs each contains a list.json file that overrides these settings. So we'll need to update the #18915 patch as I mentioned in comment:7.

Here is a radical idea: modify browser/locales/search/list.json to only contain a "default" entry and remove the list.json file from each language pack during our packaging phase. I don't know if everything would work correctly if we did that, but it might.

comment:11 in reply to:  10 Changed 3 years ago by arthuredelstein

Replying to mcs:

Replying to arthuredelstein:

Unfortunately it doesn't work -- the language packs each contains a list.json file that overrides these settings. So we'll need to update the #18915 patch as I mentioned in comment:7.

Here is a radical idea: modify browser/locales/search/list.json to only contain a "default" entry and remove the list.json file from each language pack during our packaging phase. I don't know if everything would work correctly if we did that, but it might.

I think that's a good suggestion and it might work. I will give it a try.

comment:12 in reply to:  10 ; Changed 3 years ago by arthuredelstein

Replying to mcs:

Replying to arthuredelstein:

Unfortunately it doesn't work -- the language packs each contains a list.json file that overrides these settings. So we'll need to update the #18915 patch as I mentioned in comment:7.

Here is a radical idea: modify browser/locales/search/list.json to only contain a "default" entry and remove the list.json file from each language pack during our packaging phase. I don't know if everything would work correctly if we did that, but it might.

For some reason removing all but the default entry doesn't work, even for US English. nsSearchService.js is pretty convoluted so I'm not sure why. My current approach is to change all locales we deploy to include the same search engine list, and then rely on our #18915 patches for tor-browser-bundle.git to copy these search engines and the list to each locale xpi. It's awkward, but it seems like it should work. Unfortunately I'm having trouble getting tor-browser-bundle.git to build right now, so I haven't fully tested this. But here is the patch in any case:

https://github.com/arthuredelstein/tor-browser/commit/21309+1

comment:13 Changed 3 years ago by cypherpunks

The reason the Omnibox was redesigned is the Omnibox API riding the trains to Firefox 52 which has privacy implications and may be worth investigating.
https://bugzilla.mozilla.org/show_bug.cgi?id=1166831

comment:14 in reply to:  13 Changed 3 years ago by mcs

Replying to cypherpunks:

The reason the Omnibox was redesigned is the Omnibox API riding the trains to Firefox 52 which has privacy implications and may be worth investigating.
https://bugzilla.mozilla.org/show_bug.cgi?id=1166831

This API is for WebExtensions, which means there should be no privacy implications unless someone installs an add-on that they do not trust (something we definitely do not recommend).

comment:15 Changed 3 years ago by gk

Keywords: tbb-7.0-must added

comment:16 Changed 3 years ago by gk

Keywords: TorBrowserTeam201703 added; TorBrowserTeam201702 removed

Moving tickets to March

comment:17 Changed 3 years ago by gk

Keywords: TorBrowserTeam201703R added; TorBrowserTeam201703 removed

comment:18 in reply to:  12 ; Changed 3 years ago by mcs

Replying to arthuredelstein:

For some reason removing all but the default entry doesn't work, even for US English. nsSearchService.js is pretty convoluted so I'm not sure why. My current approach is to change all locales we deploy to include the same search engine list, and then rely on our #18915 patches for tor-browser-bundle.git to copy these search engines and the list to each locale xpi. It's awkward, but it seems like it should work. Unfortunately I'm having trouble getting tor-browser-bundle.git to build right now, so I haven't fully tested this. But here is the patch in any case:

https://github.com/arthuredelstein/tor-browser/commit/21309+1

This patch looks okay, except should the URLs in ddg-onion.xml be https instead of http?

Regarding the changes needed for packaging (builders/tor-browser-bundle.git), Kathy and I wonder if it will work to simply remove browser/chrome/$LOCALE/locale/browser/searchplugins from each of the language packs. But we have not tried it.

comment:19 in reply to:  18 Changed 3 years ago by arthuredelstein

Resolution: fixed
Status: needs_reviewclosed

Replying to mcs:

Replying to arthuredelstein:

For some reason removing all but the default entry doesn't work, even for US English. nsSearchService.js is pretty convoluted so I'm not sure why. My current approach is to change all locales we deploy to include the same search engine list, and then rely on our #18915 patches for tor-browser-bundle.git to copy these search engines and the list to each locale xpi. It's awkward, but it seems like it should work. Unfortunately I'm having trouble getting tor-browser-bundle.git to build right now, so I haven't fully tested this. But here is the patch in any case:

https://github.com/arthuredelstein/tor-browser/commit/21309+1

This patch looks okay, except should the URLs in ddg-onion.xml be https instead of http?

Good point. I changed that.

Regarding the changes needed for packaging (builders/tor-browser-bundle.git), Kathy and I wonder if it will work to simply remove browser/chrome/$LOCALE/locale/browser/searchplugins from each of the language packs. But we have not tried it.

I tried this and couldn't get it to work. It seems Firefox doesn't fall back to the default plugins list as I would have expected. But I could be missing something. In any case I added the patch to my 20680 branch.

Note: See TracTickets for help on using tickets.