Opened 13 months ago

Last modified 5 months ago

#31918 needs_revision task

Rebase and squash mobile and desktop patches

Reported by: sysrqb Owned by: acat
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-9.5, ReleaseTrainMigration, TorBrowserTeam202006
Cc: tbb-team Actual Points:
Parent ID: #33656 Points: 1
Reviewer: sysrqb, gk Sponsor: Sponsor58-must

Description

The patches for mobile/android are separate from the patches for desktop. Some of these patches are similar, such as adding a mozconfig and overriding prefs. Now that Android is a first-class supported platform, we can squash some of these patches so we reduce the number of patches we need carry on top of Firefox.

Child Tickets

Change History (29)

comment:1 Changed 13 months ago by sysrqb

Making a note here:
ed7faf1c2b88cd025596151f2ad97892891965a4 contains:

diff --git a/toolkit/modules/AppConstants.jsm b/toolkit/modules/AppConstants.jsm
index 109f1f94e3d3..bc679fb6f9f0 100644
--- a/toolkit/modules/AppConstants.jsm
+++ b/toolkit/modules/AppConstants.jsm
@@ -387,4 +387,11 @@ this.AppConstants = Object.freeze({
 #else
     false,
 #endif
+
+  TOR_BROWSER_UPDATE:
+#ifdef TOR_BROWSER_UPDATE
+    true,
+#else
+    false,
+#endif

But TOR_BROWSER_UPDATE is used earlier in 14df73ecfdd88db89f196cecfb934dab7e552969. We should move this code block into the earlier patch.

comment:2 Changed 13 months ago by gk

Keywords: TorBrowserTeam201910 tbb-9.0 added
Points: 1

comment:3 Changed 13 months ago by sysrqb

During the 68esr rebase, I squashed the patch for configuring about:tor as the default homepage into the branding patch.

Unfortunately, that patch changes Fennec UI behavior. We shouldn't squash Gecko modifications with Fennec UI modifications. I opened #31983 for the Fennec UI change, so we can break that functionality into a separate patch and prevent the current inconsistency on Android.

comment:4 Changed 13 months ago by gk

Okay, tor-browser-68.1.0esr-9.0-3 is a squashed branch that could be used for this ticket. I already started moving commits around, e.g. moving the ones needed for actually building code even before our mozconfig commit.

We should think about more changes that would facilitate bisecting and readability of the branch. E.g. moving the updater/mar signing patches earlier, grouping Mozilla patches that belong together etc.

comment:5 Changed 13 months ago by gk

Something we could do while working on this bug, and making the patches less desktop vs. android, is to look for pieces we disabled on desktop but not on mobile, yet we would like to have them actually disabled on both. E.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1503402 comes to mind where we just no build the webcompat extension on desktop but still do so on mobile and include it there.

comment:6 Changed 13 months ago by gk

Another thing I am just dropping here to not forget about it: there are a number of securitySettings "fixup" commits we should squash while we are at it.

comment:7 Changed 13 months ago by sysrqb

Keywords: TorBrowserTeam201911 tbb-9.5 added; TorBrowserTeam201910 tbb-9.0 removed

comment:8 Changed 12 months ago by sysrqb

Cc: tbb-team added
Owner: changed from tbb-team to sysrqb
Status: newassigned

comment:9 Changed 11 months ago by pili

Keywords: TorBrowserTeam201912 added; TorBrowserTeam201911 removed

Moving tickets to December

comment:10 Changed 10 months ago by sysrqb

Keywords: TorBrowserTeam202001 added; TorBrowserTeam201912 removed

comment:11 Changed 9 months ago by pili

Keywords: TorBrowserTeam202002 added; TorBrowserTeam202001 removed

Moving tickets to February

comment:12 Changed 9 months ago by sysrqb

Keywords: ReleaseTrainMigration added

comment:13 Changed 8 months ago by pili

Keywords: TorBrowserTeam202003 added; TorBrowserTeam202002 removed

We are no longer in February, moving tickets

comment:14 Changed 7 months ago by pili

Sponsor: Sponsor58

comment:15 Changed 7 months ago by pili

Keywords: TorBrowserTeam202004 added; TorBrowserTeam202003 removed

We are no longer in March

comment:16 Changed 7 months ago by pili

Reviewer: acat

comment:17 Changed 7 months ago by pili

Parent ID: #33656

comment:18 Changed 7 months ago by acat

Owner: changed from sysrqb to acat
Reviewer: acatsysrqb

comment:19 Changed 6 months ago by acat

Keywords: TorBrowserTeam202004R added; TorBrowserTeam202004 removed
Status: assignedneeds_review

I'm using this one to also squash some of the desktop patches, not only mobile -> desktop. I used the 33533+4 branch, and the resulting rebased patches are in ​https://github.com/acatarineu/tor-browser/commits/31918.

I squashed some commits with the mozconfigs one which are not strictly mozconfigs, but I think they could all belong to the same category, something like "build-time options that we set or unset". And if we do #23656 then all the changes in this patch would be effective for tor-browser-builds.

Here is the list of changes from 33533+4 to 31918:

Bug 25741 - TBA: Add mobile-override of 000-tor-browser prefs
  squash! TB4: Tor Browser's Firefox preference overrides.

Bug 25741 - TBA: Add an AppConstant for TOR_BROWSER_VERSION
  fixup! Bug 4234: Use the Firefox Update Process for Tor Browser.

Bug 25741 - TBA: Add default configure options in dedicated file
  squash! TB3: Tor Browser's official .mozconfigs.

Bug 25741 - TBA: Disable features at compile-time
  squash! TB3: Tor Browser's official .mozconfigs.

Bug 32493: Disable MOZ_SERVICES_HEALTHREPORT
  squash! TB3: Tor Browser's official .mozconfigs.

Bug 31658: Changed the 'SECURITY LEVEL' text color to builtin --panel-disabled-color
  fixup! Bug 25658: Replace security slider with security level UI

Bug 32188: Change useLocalProxy string to tor-launcher's torsettings useProxy.checkbox in TorStrings.jsm
  fixup! Bug 31286: Implementation of bridge, proxy, and firewall settings in about:preferences#tor

Bug 31803: Replaced about:debugging logo with flat version
  squash! Bug 2176: Rebrand Firefox to TorBrowser

Bug 32111: Fixed issue parsing user-provided brige strings
  fixup! Bug 31286: Implementation of bridge, proxy, and firewall settings in about:preferences#tor

Bug 31749: Fix security level panel spawning events
  fixup! Bug 25658: Replace security slider with security level UI

Bug 31920: Fix Security Level panel when its toolbar button moves to overflow
  fixup! Bug 25658: Replace security slider with security level UI

Bug 31748: Fixed 'Learn More' links in Security Level preferences and panel
  fixup! Bug 25658: Replace security slider with security level UI

Bug 31935: Disable profile downgrade protection.
  squash! TB3: Tor Browser's official .mozconfigs.

Bug 28196: preparations for using torbutton tor-browser-brand.ftl
  squash! Bug 2176: Rebrand Firefox to TorBrowser

Bug 24653: merge securityLevel.properties into torbutton.dtd
  fixup! Bug 25658: Replace security slider with security level UI

Bug 31457: disable per-installation profiles
  squash! TB3: Tor Browser's official .mozconfigs.

Bug 31251: Security Level button UI polish
  fixup! Bug 25658: Replace security slider with security level UI

Bug 30631: Blurry Tor Browser icon on macOS app switcher
  squash! Bug 2176: Rebrand Firefox to TorBrowser

Bug 25702: Update Tor Browser icon to follow design guidelines
  took the aboutTBUpdateLogo.png changes (aboutTBUpdateLogo.png,
  browser/base/jar.mn) and added commit as fixup for
  `Bug 16940: After update, load local change notes.`

  the rest, squash! Bug 2176: Rebrand Firefox to TorBrowser

Bug 22548: Firefox downgrades VP9 videos to VP8.
  squash! TB4: Tor Browser's Firefox preference overrides.

Bug 14392: Make about:tor behave like other initial pages.
  squash! Bug 10760: Integrate TorButton to TorBrowser core

Some notes/questions:

I did not touch the onboarding patches, as I'm not sure what we are going to do with #31660. If we keep the current onboarding, I think it might be worth to squash them. I was thinking to keep two patches:

  • "Resurrect Firefox old onboarding"
    • Revert "Bug 1462415 - Delete onboarding system add-on r=Standard8,k88hudson
    • squash! Revert "Bug 1498378 - Actually remove the old onboarding add-on's prefs r=Gijs
    • squash! Bug 28822: Convert onboarding to webextension
    • squash! Partially revert 1564367 (controlCenter in UITour.jsm)
  • "Actual onboarding implementation"
    • Bug 26961: New user onboarding.
    • squash! Bug 27082: enable a limited UITour
    • squash! Bug 26962 - implement new features onboarding (part 1).
    • squash! Bug 27486 Avoid about:blank tabs when opening onboarding pages.
    • squash! Bug 31768: Introduce toolbar and network settings changes in onboarding

Given that now some "actual onboarding" patches go after "Bug 28822: Convert onboarding to webextension", to do this reordering + squashing we would have to fix some conflicts, but it should not be a big deal. The only thing is that "actual onboarding" patch should go after the most recent patch from all features, which I assume would be "Bug 27511: Add new identity button to toolbar".

If we agree with the mozconf squashes I suggested, would the patch "Bug 21849: Don't allow SSL key logging" fit in that too? (build-time configs that we set/change)

Is it fine to leave tests separated from patches, or do you think we could also squash those too?

comment:20 Changed 6 months ago by acat

Summary: Rebase and squash mobile/android patches into desktopRebase and squash mobile and desktop patches

comment:21 Changed 6 months ago by sysrqb

Reviewer: sysrqbsysrqb, gk

comment:22 Changed 6 months ago by pili

Sponsor: Sponsor58Sponsor58-must

comment:23 in reply to:  19 Changed 6 months ago by gk

Keywords: TorBrowserTeam202004 added; TorBrowserTeam202004R removed
Status: needs_reviewneeds_revision

Replying to acat:

I'm using this one to also squash some of the desktop patches, not only mobile -> desktop. I used the 33533+4 branch, and the resulting rebased patches are in ​https://github.com/acatarineu/tor-browser/commits/31918.

I squashed some commits with the mozconfigs one which are not strictly mozconfigs, but I think they could all belong to the same category, something like "build-time options that we set or unset". And if we do #23656 then all the changes in this patch would be effective for tor-browser-builds.

Sounds good to me.

Here is the list of changes from 33533+4 to 31918:

I think everything looks good just...

[snip]

Bug 25741 - TBA: Add an AppConstant for TOR_BROWSER_VERSION

fixup! Bug 4234: Use the Firefox Update Process for Tor Browser.

is a thing we should not do, I think. The mobile part does not really belong to the desktop updater patch. What about breaking the TOR_BROWSER_VERSION part of that patch out putting it into the "build-time options that we set or unset" one and merging the patch into that one?

[snip]

Additionally:

1) There are parts in a71ce964674ec47aed50cd322d40fdd7f6ec9c4a that can get squashed into TB3: Tor Browser's official .mozconfigs.
2) Can we squash bd5a3a8f8fa37fa3e877c4b6ca66c5a25b7cd3cc into 4234?
3) Can we squash 66197cbb3e3f6b72124a30ffc0e2a82819a52e9d into 4234?
4) Merge both Bug 19121 commits?
5) Squash the old.configure changes into TB3: Tor Browser's official .mozconfigs and just start using --enable-tor-browser-data-outside-app-dir in from that commit on? (Essentially similar to my TOR_BROWSER_VERSION idea from above)
6) Squash a0a69636e3074efcbf7c1fc80e07d77bd94a81ab into TB4 commit?

There might even be more :)

Some notes/questions:

I did not touch the onboarding patches, as I'm not sure what we are going to do with #31660. If we keep the current onboarding, I think it might be worth to squash them. I was thinking to keep two patches:

  • "Resurrect Firefox old onboarding"
    • Revert "Bug 1462415 - Delete onboarding system add-on r=Standard8,k88hudson
    • squash! Revert "Bug 1498378 - Actually remove the old onboarding add-on's prefs r=Gijs
    • squash! Bug 28822: Convert onboarding to webextension
    • squash! Partially revert 1564367 (controlCenter in UITour.jsm)
  • "Actual onboarding implementation"
    • Bug 26961: New user onboarding.
    • squash! Bug 27082: enable a limited UITour
    • squash! Bug 26962 - implement new features onboarding (part 1).
    • squash! Bug 27486 Avoid about:blank tabs when opening onboarding pages.
    • squash! Bug 31768: Introduce toolbar and network settings changes in onboarding

Bug 29768: Introduce new features to users needs to be somewhere in this picture as well.

Given that now some "actual onboarding" patches go after "Bug 28822: Convert onboarding to webextension", to do this reordering + squashing we would have to fix some conflicts, but it should not be a big deal. The only thing is that "actual onboarding" patch should go after the most recent patch from all features, which I assume would be "Bug 27511: Add new identity button to toolbar".

Sounds good. I think proceeding with that plan is a good idea. I would not bet us on getting a proper onboarding going. So, let's go for now with the squashing idea.

If we agree with the mozconf squashes I suggested, would the patch "Bug 21849: Don't allow SSL key logging" fit in that too? (build-time configs that we set/change)

Works for me.

Is it fine to leave tests separated from patches, or do you think we could also squash those too?

I think if we have tests belonging to particular code fixes in our patch set then the tests should get merged into those commits. All the other tests can get into one "test" commit.

Last edited 5 months ago by gk (previous) (diff)

comment:24 Changed 6 months ago by acat

Keywords: TorBrowserTeam202005R added; TorBrowserTeam202004 removed
Status: needs_revisionneeds_review

Thanks for the review: here is the revised branch: https://github.com/acatarineu/tor-browser/commits/31918+1 (4f94bc827bb8045b3c6504e8c65c5629d76c2313).

I did what you suggested, and also squashed Bug 29859: Disable HLS support for now, with mozconfs, as I thought it might belong there.

5) Squash the old.configure changes into TB3: Tor Browser's official .mozconfigs and just start using --enable-tor-browser-data-outside-app-dir in from that commit on? (Essentially similar to my TOR_BROWSER_VERSION idea from above)

Should we do this also with the other two flags that we define? (--disable-tor-launcher, --disable-tor-browser-update). I mean, define them in the mozconfig commit, and use them later.

I'll use the 31918+1 branch for next #33533 rebase, as essentially the 33533+4..31918+1 did not change. I can revise that later if necessary because of revision_needed here.

comment:25 in reply to:  24 Changed 6 months ago by gk

Replying to acat:

Thanks for the review: here is the revised branch: https://github.com/acatarineu/tor-browser/commits/31918+1 (4f94bc827bb8045b3c6504e8c65c5629d76c2313).

I did what you suggested, and also squashed Bug 29859: Disable HLS support for now, with mozconfs, as I thought it might belong there.

5) Squash the old.configure changes into TB3: Tor Browser's official .mozconfigs and just start using --enable-tor-browser-data-outside-app-dir in from that commit on? (Essentially similar to my TOR_BROWSER_VERSION idea from above)

Should we do this also with the other two flags that we define? (--disable-tor-launcher, --disable-tor-browser-update). I mean, define them in the mozconfig commit, and use them later.

I think so, yes.

I'll use the 31918+1 branch for next #33533 rebase, as essentially the 33533+4..31918+1 did not change.

Yeah, sounds good. Thanks!

comment:26 Changed 6 months ago by acat

Ok, I ended up doing a new squashed branch in ​https://github.com/acatarineu/tor-browser/commits/31918+2 (3ef48160be35fd86516031b6782b983b91366127).

  • Moved remaining build config definitions to mozconfig patch.
  • Squashed tests which did not have corresponding commit into same "unit tests" commit.
  • Squashed search engine tests with its corresponding patch.
  • Squashed two 14631 commits.
  • Split Bug 16441: Suppress "Reset Tor Browser" prompt: part went to the "tests patch" and the other to pref override.

comment:27 Changed 5 months ago by gk

Keywords: TorBrowserTeam202006R added; TorBrowserTeam202005R removed

Moving review tickets.

comment:28 in reply to:  24 Changed 5 months ago by gk

Keywords: TorBrowserTeam202006 added; TorBrowserTeam202006R removed
Status: needs_reviewneeds_revision

Replying to acat:

Thanks for the review: here is the revised branch: https://github.com/acatarineu/tor-browser/commits/31918+1 (4f94bc827bb8045b3c6504e8c65c5629d76c2313).

To follow-up here: this looks mostly good. One thing I think we should fix is in the squashed commit that brings the onboarding back (commit 025e0d950612a9a56cfcbb46069e5f94418e2abc). That patch contains references to torbutton's onboarding.properties which do not belong into resurrecting the old Firefox onboarding (see: BUNDLE_URI and the change to nsStringBundle.cpp) but rather in the follow-up commit which is doing our on onboarding.

Last edited 5 months ago by gk (previous) (diff)

comment:29 in reply to:  26 Changed 5 months ago by gk

Replying to acat:

Ok, I ended up doing a new squashed branch in ​https://github.com/acatarineu/tor-browser/commits/31918+2 (3ef48160be35fd86516031b6782b983b91366127).

  • Moved remaining build config definitions to mozconfig patch.
  • Squashed tests which did not have corresponding commit into same "unit tests" commit.
  • Squashed search engine tests with its corresponding patch.
  • Squashed two 14631 commits.
  • Split Bug 16441: Suppress "Reset Tor Browser" prompt: part went to the "tests patch" and the other to pref override.

That list looks good to me. Just two nits we should fix in the next revision:

9cbc26f2e923550183cf0f75c385424685ec7521 Bug 28044: Integrate Tor Launcher into tor-browser

The --disable-tor-launcher build option is not new anymore with that commit; I guess we can just omit that sentence in the commit message now?

f9e8153d4001291de490b0ecac8e2a1c7d0fc8d3 Bug 4234: Use the Firefox Update Process for Tor Browser

--enable-tor-browser-update is no new configure option at this point anymore (please adjust the commit message)

Note: See TracTickets for help on using tickets.