Go over rebased patches again and reorder pieces where needed after rebase to esr78 is done

We've reordered/squashed bits of our patch set during #31918 (closed). We should do this again once we have the rebasing to esr78 done and are happy with the results.

@acat noted in #33533 (closed): One example that comes to my mind for this is the security level patch, which introduces a localization mecanism which is later replaced by TorStrings.jsm in the #31286 (closed) patch.

Additionally, there are firefox-branding.js changes that moved to the updater related patch. We might want to think about something that's less surprising.

---

• #40047 (closed)

changed milestone to %Sponsor 58 - Tor Browser Security, Performance, & Usability Improvements

added 1 deleted label

mentioned in issue #33533 (closed)

Another thing that we might want to move: in the patch for #27802 (enabling the limited UITour) we not only deal with uitour related permissions but touch the install ones, too, which are unrelated. Might be worth doing that elsewhere, in particular given that the old onboarding might soon go away anyway (but presumably not those permission changes).

#40025 (closed) is related.

Perhaps we can do this in #40025 (closed)? (moving the install permissions to a different patch).

Yes, sounds good.

added Next label

Another piece worth considering is solving the underlying problem that led to #40035 (closed). We might want to have all the TorString related pieces already in the first relevant commit, so that we don't have change the same things in more than one commit which seems quite error-prone...

mentioned in issue #40035 (closed)

added Doing label and removed Next label

mentioned in issue #40037 (closed)

Apart from the TorStrings.jsm and firefox-branding.js related changes, I tried to do some other changes aiming to remove dependencies between commits that should not be there (textual but not semantic dependencies). My idea was to make patches as independent as possible from each other, so that these can be cherry picked in any order and also individually (for example, to keep track of which Firefox tests fail for a specific patch). I did not do this in an exhaustive way, for example: the Onion-Location, onion alias, client auth patches and others are still dependent among other things because they share the same browser/components/onionservices path. If we think this is a good thing to pursuit, I could do another iteration trying to make the patches even more independent.

In the final branch 40024+8, we have (checked with https://github.com/aspiers/git-deps):

• 12 patches that depend (textually) on at least another patch.

• 49 patches that are independent (textually) with each other.

I tried to do one change per branch, to make it easier to compare (e.g. with range-diff):

• 40024: Started with 40037 and squashed #34196 (closed) onto #28005 (closed).

• 40024+1: Moved TorStrings.jsm to its own commit, and removed parts from:

  • acat/tor-browser@c25d9332
  • acat/tor-browser@20dac07f
  • acat/tor-browser@9ea8019c
  • acat/tor-browser@b3a3e2db

• 40024+2: Squashed "add torbutton as submodule" and "integrate torbutton" (should update processes document).

• 40024+3: "Integrate Tor Launcher" changes

  • Moved pref override and tbb-test for to pref override and unit test commits respectively.

• 40024+4: Moved pref override out of sec slider patch

• 40024+5: Patch for #13252 (closed) depends (textually) on #9173 and #14631 (closed) -> I grouped together this commits, since they touch similar parts of the code: #9173 then #14631 (closed) then #13252 (closed).

• 40024+6: Changed Bug 4234: Use the Firefox Update Process for Tor Browser: Moved firefox-branding.js changes to 000-tor-browser.js

  • startup.homepage_override_url: was already present in pref overrides (value set in nightly/alpha firefox-branding.js was ignored).
  • startup.homepage_welcome_url: was already present in pref overrides (value set in nightly/alpha firefox-branding.js was ignored).
  • app.update.promptWaitTime: same value for all builds.
  • app.update.url.manual: same value for all builds.
  • app.update.url.details: same value for all builds.
  • app.update.badgeWaitTime: same value for all builds.
  • app.releaseNotesURL: previous patch unset it, now overriding to 'about:blank' so that it's ignored by the UI code where it's used.

• 40024+7: Move 23104 test out of tbb-tests to remove dependency on "TorBrowser regression tests" patch.

• 40024+8: Remove textual dependency between "Integrate Tor Launcher" and "Don't block our unsigned extensions"

  • Separate changes in XPIDatabase.jsm as much as possible to avoid conflicts and make the two commits independent.

added Needs Review label

removed Doing label

assigned to @gk and unassigned @acat

Picked changes from #40037 (comment 2704065) in 40024+9. And rebased/squashed in 40024+10.

mentioned in issue #40023 (closed)

mentioned in merge request !20 (closed)

assigned to @acat and unassigned @gk

removed Needs Review label

40024+8: Remove textual dependency between "Integrate Tor Launcher" and "Don't block our unsigned extensions"

• Separate changes in XPIDatabase.jsm as much as possible to avoid conflicts and make the two commits independent.

If I see that right then the only possible side-effect of that change is a one-time warning about Tor Launcher not being properly signed after the update to a version containing your proposed re-ordering. If that's the case then how is the code you moved reachable anyway given that the failed signature check already results in a return false;? (I am assuming addon.isCorrectlySigned is actually false in that case but have not checked. But I know we always worked around that check in the past/present when dealing with signature exemptions)

(Thanks for the different branches and comments about what changed and why, splitting the changes up that way with explanations was super helpful.)

mentioned in merge request !16 (closed)