THE Torbutton clean-up
That is the parent ticket for all things Torbutton clean-up, now that we included it into tor-browser. It's not clear yet how we'll be restructuring it but it's clear that a lot of old cruft has to go. This will be done in child tickets.
Activity
Some changes for review here: https://github.com/acatarineu/torbutton/commits/28745 (starting from 6023e0875c8085a58e6269801113a2a4600cb6d3, which is the last commit from #10760 (moved) patch). Corresponding tor-browser changes here: https://github.com/acatarineu/tor-browser/commits/28745 (last 4 commits).
Some comments:
Remove cookie-jar-selector component
- For
browser.privatebrowsing.autostart = true, cookie-jar-selector does nothing currently, so for new identity we justclearCookies(). - For
browser.privatebrowsing.autostart = false, some cookies json files are stored in the browser profile folder, which we clear on startup in the patch. Bug 30851: Move default preferences to 000-tor-browser.js - This has a corresponding patch in tor-browser. Bug 30888: move torbutton_util.js to modules/utils.js
- This also has a corresponding patch in tor-browser, to stop including torbutton_util.js in browser.xul. Make torbutton_open_network_settings global
- This will have to be called somewhere in UI at some point, for now we just expose it. Refactor code to always use tor-control-port.js
- Trying to use a single implementation of tor controller. Perhaps next step would be to do the same with tor-launcher.
- The code had to be adapted a bit, since tor-control-port.js is async while previous impl. was sync. Remove versioncheck from torbutton.js
- I'm assuming we don't need this, with the browser updater and having removed the blinking notification in torbutton UI. But perhaps we still want as telemetry, not sure.
Trac:
Status: new to needs_review
Keywords: tbb-torbutton deleted, tbb-torbutton TorBrowserTeam201907R added- For
We'd want to have this rebased I think (sorry for the delay).
Trac:
Keywords: TorBrowserTeam201910R deleted, TorBrowserTeam201910 added
Parent: N/A to #30506 (moved)
Status: needs_review to needs_revisionRebased in https://github.com/acatarineu/torbutton/commits/28745+1 and https://github.com/acatarineu/tor-browser/commits/28745+1.
I dropped some commits wrt the original
torbuttonchanges (Refactor code to always use tor-control-port.js,Tor controller promises should fail when there is an error,Move torbutton_do_new_identity to separate module,Move some code from torbutton to torCheckService) as I was not sure whether they fit in this ticket or they added much value, and the changes were quite big already. Maybe we can continue on these on later torbutton refactors?I think #13198 (moved) would be fixed with these changes, and perhaps #19256 (moved) too, although there might be more dead code that I missed.
Trac:
Keywords: TorBrowserTeam201910 deleted, TorBrowserTeam201910R added
Status: needs_revision to needs_review-
Alright I started reviewing and applying patches. The first two landed on
master(the latter closing #28746 (moved)): commit 2dfa0e0c9cff7cfad93664e0b0b6cdc05b24b7f2 and 938997fa6de418e423186a5fe5c3e8adf4d82a38. -
Commit 46b550c5550d3ad4a7d526a402a33bbb40dce5e2 on your
torbuttonbranch has- // Disable the External App Blocker on Android - if ((aPrefName === "extensions.torbutton.launch_warning") && - (Services.appinfo.OS === "Android")) { - defaultPrefBranch.setBoolPref(aPrefName, false); - }It seems we want to keep that part, though, and I don't see a matching counterpart in any of your related
tor-browsercommits.Commit f797d8ba19c5838032abff6da3c4b5e8cabc518f on your
tor-browserbranch:+pref("extensions.torbutton.cookie_protections",true); +pref("extensions.torbutton.cookie_auto_protect",false);That's already gone, I think.
+pref("extensions.torbutton.prompt_torbrowser", true);No need to add this anymore, see my previous comment.
+// TODO: This is just part of a stopgap until #14429 gets properly implemented. +// See #7255 for details. We display the warning three times to make sure the +// user did not click on it by accident.I think we should update the comment at least. We still have this mechanism for users that disable letterboxing, I think.
Have you tested that moving
browser.startup.homepageis fine now and that we can ditchnon-localized.propertiesintorbutton? The code mentioned in the comment is still there. But I guess we don't hit the codepath as we are not setting any default prefs in the extension anymore?Trac:
Status: needs_review to needs_revision
Keywords: TorBrowserTeam201911R deleted, TorBrowserTeam201911 added Revised branches: https://github.com/acatarineu/tor-browser/commits/28745+2 and https://github.com/acatarineu/torbutton/commits/28745+2.
I moved the
extensions.torbutton.launch_warning = falsemobile override to000-tor-browser-android.js.Have you tested that moving browser.startup.homepage is fine now and that we can ditch non-localized.properties in torbutton? The code mentioned in the comment is still there. But I guess we don't hit the codepath as we are not setting any default prefs in the extension anymore? Yes, I tested that it works fine.
Trac:
Status: needs_revision to needs_review
Keywords: TorBrowserTeam201911 deleted, TorBrowserTeam201911R added-
Thanks. I closed #30851 (moved) as the updated changes look good to me. And I applied the
m_tb_tbbone totorbutton'smaster(comit 6b1a5ded2cab7e51aeb504483fa0d8fbf0cae957). Getting closer... -
commit a12088fdc0d343d81b6ecf53faf06c91f8da27d5 looks good. I've applied it tor
torbutton'smaster(commit 65e1e8ba67ba865e7b3ded74b7e4bbc5b86c721c)Some nits and a question for commit 7e23b3cbf66717c1b9b3a107daf3147b54061a96:
+/* global ,I think you don't want to have a
,+ whitespace there?Are you sure we want to do
- let firstPartyDomain = getDomainForBrowser(gBrowser.selectedBrowser); + let firstPartyDomain = getDomainForBrowser(gBrowser);? In
utils.jsit says// Assuming this is called with gBrowser.selectedBrowser var getDomainForBrowser = (browser) => {Please remove the superfluous whitespaces in
+ // So we can use it in boolean expressions to determine where theand
+function torbutton_get_property_string(propertyname) +{ + try {(the
tryline)commit 38720bfe8f88248534d4d211a40274c613a6ade6:
-
Do we really need the remaining
k_tb_browser_update_needed_prefparts (including the pref) now that the pref observer is gone (and the code acting upon pref changes)? What about the respective button CSS parts? Can't we get rid of them as well in this commit? For instance, if we don't settbStatusanymore it seems to me we don't need the[tb-status="on"]and[tb-status="off"]parts anymore either etc. -
Please adapt the commit message to what has happened since you wrote it. :)
Please base your new branch on current
master.Trac:
Status: needs_review to needs_revision
Keywords: TorBrowserTeam201911R deleted, TorBrowserTeam201911 added -
Thanks for the review.
Revised branch: https://github.com/acatarineu/torbutton/commits/28745+3.
With respect to the
k_tb_browser_update_needed_prefcomment, I decided to squashRemove versioncheck from torbutton.jsandRemove code dealing with torbutton UI button in toolbar, as some of the requested changes were already included in the other commit, and I think these are related (we can remove versioncheck because there is not torbutton UI anymore).There were some conflicts with the last commit of 28745+2 (2d318efde8faccf3980c6d7da163c32103202b26) and the 46efc92348dbed06fc31ddfb0a5ac2e4e8554de2 commit in master (#30237 (moved)). I think these are not straightforward to solve, as in that commit I moved
m_tb_control_ipc_file, m_tb_control_host, m_tb_control_port, m_tb_control_pass, m_tb_control_descto a service while in the master commit those were moved to a different module and initialized viaconfigureControlPortModule. I would suggest dropping that commit for now, and perhaps do it later in #30850 (moved) (some of the previous dropped commits are also related to that one).Trac:
Keywords: TorBrowserTeam201911 deleted, TorBrowserTeam201911R added
Status: assigned to needs_review-
Thanks. While I am preparing a patch for #30787 (moved) I realized that it seems we forgot to clean up the respective locale files, too, while removing the code. E.g. I am right now still seeing
torbutton.cookieDialog.*entries. Could you go over the locale files and get rid of the strings we don't need anymore due to this clean-up?Trac:
Keywords: TorBrowserTeam201911R deleted, TorBrowserTeam201911 added
Status: needs_review to needs_revision -
I closed #30888 (moved) as the changes look good to me now.
Done in https://github.com/acatarineu/torbutton/commits/28745+4.
Trac:
Keywords: TorBrowserTeam201911 deleted, TorBrowserTeam201911R added
Status: needs_revision to needs_review-
Okay, let's move on with the review process, commit 8269902a22357697c46c7052c9d946bd874c3f55:
Looks mostly good. It misses a
tor-browserchange to remove theextensions.torbutton.updateNeededandextensions.torbutton.versioncheck_enabledpref, now that we moved the preferences. Additionally, please remove thetorbutton-update-needed.svgas well. It seems to be obsolete now, too.For 8c0c18a09a30f14dd0b4a99fe67238fec0ad3bac what command did you run/tool did you use to check for those errors (and fix them)?
-
Replying to acat:
[snip]
There were some conflicts with the last commit of 28745+2 (2d318efde8faccf3980c6d7da163c32103202b26) and the 46efc92348dbed06fc31ddfb0a5ac2e4e8554de2 commit in master (#30237 (moved)). I think these are not straightforward to solve, as in that commit I moved
m_tb_control_ipc_file, m_tb_control_host, m_tb_control_port, m_tb_control_pass, m_tb_control_descto a service while in the master commit those were moved to a different module and initialized viaconfigureControlPortModule. I would suggest dropping that commit for now, and perhaps do it later in #30850 (moved) (some of the previous dropped commits are also related to that one).Sounds good to me.
Replying to gk:
Okay, let's move on with the review process, commit 8269902a22357697c46c7052c9d946bd874c3f55:
Looks mostly good. It misses a
tor-browserchange to remove theextensions.torbutton.updateNeededandextensions.torbutton.versioncheck_enabledpref, now that we moved the preferences. Additionally, please remove thetorbutton-update-needed.svgas well. It seems to be obsolete now, too.For 8c0c18a09a30f14dd0b4a99fe67238fec0ad3bac what command did you run/tool did you use to check for those errors (and fix them)?
Hmm, i can't find
8269902a22357697c46c7052c9d946bd874c3f55commit, I assume it was fromtorbutton(I hope it was notpush --forceagain).In any case, I removed the mentioned prefs and some more that are now unused from
000-tor-browser-prefs.js: https://github.com/acatarineu/tor-browser/commit/28745+3. Some are unused because of changes already commited, some are due totorbuttonchanges from this ticket not yet commited.I also rebased the torbutton changes from
28745+4to current master, and removedtorbutton-updated-needed.svg. Besides, I split the style-changing commits into two, the ones automatically done by eslint tool, and the remaining ones that had to be addressed manually. Changes are in https://github.com/acatarineu/torbutton/commits/28745+5.The command to fix
eslintissues was./mach lint -l eslint --fix toolkit/torproject/torbutton, run in thetor-browserrepo with the righttorbuttonrepo temporarily checked in intoolkit/torproject/torbutton(this time I also had to run./mach bootstrap --no-system-changeswith Desktop selected before, which is new).Trac:
Status: needs_revision to needs_review
Keywords: TorBrowserTeam202001 deleted, TorBrowserTeam202001R added-
Replying to acat:
Replying to gk:
Okay, let's move on with the review process, commit 8269902a22357697c46c7052c9d946bd874c3f55:
Looks mostly good. It misses a
tor-browserchange to remove theextensions.torbutton.updateNeededandextensions.torbutton.versioncheck_enabledpref, now that we moved the preferences. Additionally, please remove thetorbutton-update-needed.svgas well. It seems to be obsolete now, too.For 8c0c18a09a30f14dd0b4a99fe67238fec0ad3bac what command did you run/tool did you use to check for those errors (and fix them)?
Hmm, i can't find
8269902a22357697c46c7052c9d946bd874c3f55commit, I assume it was fromtorbutton(I hope it was notpush --forceagain).Hrm. I still have that commit but not on any branch it seems?? Weird. Anyway, it is essentially
0a3ec4944f4d03bc05c6d557ea7008df81e4dcacon your28745+4. -
Resolved #10575 (moved) as duplicate.
mentioned in issue #28746 (moved)
mentioned in issue #33521 (moved)
mentioned in issue #33890 (moved)
mentioned in issue #28894 (moved)
mentioned in issue #30506 (moved)
mentioned in issue #30850 (moved)
mentioned in issue #30851 (moved)
mentioned in issue #30888 (moved)
