Opened 4 weeks ago

Closed 4 weeks ago

Last modified 7 days ago

#32097 closed defect (fixed)

Fix conflicts in mobile onbarding while rebasing to esr68.2.0

Reported by: gk Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords: TorBrowserTeam201910R, tbb-9.0-must
Cc: sysrqb Actual Points: 0.5
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

It turns out that there are a number of conflicts between our mobile onboarding code and the coming esr68.2.0 which we need to think about how to resolve. They stem in particular from

https://bugzilla.mozilla.org/show_bug.cgi?id=1570878

and

https://bugzilla.mozilla.org/show_bug.cgi?id=1587631

Not sure whether we want to just back out those changes or make them otherwise compatible with our modifications.

Child Tickets

Attachments (1)

bug32097_merge_conflict_resolution.patch (10.9 KB) - added by sysrqb 4 weeks ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 4 weeks ago by sysrqb

I pushed bug32097_00 which is tor-browser-68.1.0esr-9.0-2-build4 rebased onto gecko-dev/esr68 (Mozilla's github esr68 branch). Would you prefer this as a fixup! commit? I'm not sure the best way of solving this.

For reference, the only conflict is on Bug 25696 - Implement alpha onboarding for Tor Browser for Android and the diff with resolved conflicts is:

diff -u <(git show 3b3dbd5478641efe578054300018ec81935d4f2c) <(git show 369ed1ff2eb167eb2555125e73a05d27689c5584)

(I don't know why we have minor changes in the strings files.)

Changed 4 weeks ago by sysrqb

comment:2 Changed 4 weeks ago by sysrqb

hrm. The build fails with that branch - proguard symbols.

comment:3 Changed 4 weeks ago by sysrqb

Okay, I clobbered and rebuilt, and now it is happy. Unfortunately, now we have a "Sign Up" button on the botton of all onboarding panels and the Activity Stream (#31983) screen has a very large "Sign Up" button at the top, too. I'll fix those.

comment:4 Changed 4 weeks ago by sysrqb

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: newneeds_review

I used 22182890f20069201b37a0e8d9a849d2e54fcc25 as the new base. I reverted (in order): 484ec9407da4234000a73d2426f09de62de0274e ab8797462cefdaf99d10e1981f2a168750cc697a 303c1ef548d23dfd0d8aa1e9e61481c1baea02a8 896fa0df588de10b22e827b8f9999c69daa9e05d 1c54037a5bb7ded841b5a795041772a4571944f9 a7cc5967877636728e29ea865dc46a2889b7de58 a3f466a3ac73f0d11f3f3ba054173087e66a0ab5 and then rebased origin/tor-browser-68.1.0esr-9.0-3 onto the result.

I fear we may have more problems with this in the future if/when Mozilla modify the onboarding panels again. I felt resolving the merge conflict wasn't easy to review, but we can reconsider this if needed.

I pushed branch bug32097_02 based on 68.2.0esr.

comment:5 in reply to:  4 Changed 4 weeks ago by sysrqb

Replying to sysrqb:

I used 22182890f20069201b37a0e8d9a849d2e54fcc25 as the new base. I reverted (in order):

Sorry, it's based on 8b37b0197138897a766056fe3d4e6c53b891cdcb, branch bug32097_02.

comment:6 Changed 4 weeks ago by gk

Status: needs_reviewneeds_information

Okay, I went ahead in the morning and committed the 7 backouts to get the build going but I am still trying to understand why those 7 patches got chosen given that there are a bunch of more bugs involved.

For instance, the patch for https://bugzilla.mozilla.org/show_bug.cgi?id=1570880 got backed out, which seems right to me. However, the regression fix for that bug, tracked in https://bugzilla.mozilla.org/show_bug.cgi?id=1586770 did not. So, we are now actually using code that was only meant to get used with a fix we backed out... Are we good with that?

comment:7 Changed 4 weeks ago by sysrqb

Status: needs_informationneeds_review

I began with only resolving merge conflicts. That got me most of the way to creating a clean build. Then I noticed the new patches added new Sync "Sign Up" buttons on the onboarding panels and Activity Stream panel. I wrote fixup patches for deleting the button on the onboarding panels and I backed out the commit for the Activity Stream. Unfortunately, this resulted in a build failure due to undefined variables resulting from the backed out patch. At this point I decided I'd try backing out all the patches needed for successfully building 68.2.0. This is why I didn't backout all commits, like Bug 1586770. The patch for that bug didn't break any functionality or introduce conflicting code changes.

I looked at the patch and we can keep it. I'll look through the other Fennec patches for 68.2.0, as well, and confirm we didn't miss anything.

comment:8 Changed 4 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Okay, makes sense. Thanks for the explanation. The commits on tor-browser-68.2.0esr-9.0-1 and tor-browser-68.2.0esr-9.5-1 are

2ba949f9ba65ba71c9805635a904a68488404186 c7c1d14e1627aebb749dd5e0065e49c1436530d3
d05b3c8d35ac51311a8454e3e26edacbc45e3ce4 e3af40f4b3b7aecf53a2442ad3417055b87719a2
ec8b0640ed1a383b093dd044f95fdf9934070046 15c908b1c374b667acbf8711954b032b0bb4d8f8
565394ff86237cc3177f69a0f8c69f8a40c58168

We are done here. If there is anything else to fix up then please open a new ticket.

comment:9 Changed 7 days ago by sysrqb

Actual Points: 0.5
Note: See TracTickets for help on using tickets.