Opened 8 months ago

Last modified 29 hours ago

#31918 needs_review 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, TorBrowserTeam202006R
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 (27)

comment:1 Changed 8 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 8 months ago by gk

Keywords: TorBrowserTeam201910 tbb-9.0 added
Points: 1

comment:3 Changed 8 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 8 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 8 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 8 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 8 months ago by sysrqb

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

comment:8 Changed 7 months ago by sysrqb

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

comment:9 Changed 6 months ago by pili

Keywords: TorBrowserTeam201912 added; TorBrowserTeam201911 removed

Moving tickets to December

comment:10 Changed 5 months ago by sysrqb

Keywords: TorBrowserTeam202001 added; TorBrowserTeam201912 removed

comment:11 Changed 4 months ago by pili

Keywords: TorBrowserTeam202002 added; TorBrowserTeam202001 removed

Moving tickets to February

comment:12 Changed 4 months ago by sysrqb

Keywords: ReleaseTrainMigration added

comment:13 Changed 3 months ago by pili

Keywords: TorBrowserTeam202003 added; TorBrowserTeam202002 removed

We are no longer in February, moving tickets

comment:14 Changed 2 months ago by pili

Sponsor: Sponsor58

comment:15 Changed 2 months ago by pili

Keywords: TorBrowserTeam202004 added; TorBrowserTeam202003 removed

We are no longer in March

comment:16 Changed 2 months ago by pili

Reviewer: acat

comment:17 Changed 2 months ago by pili

Parent ID: #33656

comment:18 Changed 2 months ago by acat

Owner: changed from sysrqb to acat
Reviewer: acatsysrqb

comment:19 Changed 7 weeks 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 7 weeks ago by acat

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

comment:21 Changed 6 weeks ago by sysrqb

Reviewer: sysrqbsysrqb, gk

comment:22 Changed 6 weeks ago by pili

Sponsor: Sponsor58Sponsor58-must

comment:23 in reply to:  19 Changed 5 weeks 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 29 hours ago by gk (previous) (diff)

comment:24 Changed 5 weeks 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 5 weeks 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 4 weeks 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 6 days ago by gk

Keywords: TorBrowserTeam202006R added; TorBrowserTeam202005R removed

Moving review tickets.

Note: See TracTickets for help on using tickets.