Closed Bug 930970 Opened 6 years ago Closed 6 years ago

Consider writing migration code on m-c/backout-branch-only to deal with migrating from Australis->non-Australis

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 28
Tracking Status
firefox26 --- wontfix
firefox27 + verified
firefox28 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P2][Australis:M9])

Attachments

(3 files, 1 obsolete file)

Because we got rid of some toolbar items in UX (notably, we merged the back/fwd/navbar/stop/reload stuff), when people switch back from UX to normal nightly, they often find that their toolbars look like ... well, it's not pretty.

In a world where, say, we have users on release/beta/aurora/possible-post-backout-aurora/nightly that end up trying nightly/aurora/beta/wherever *with* Australis and then shift back, this is sort of sadfaces, especially because the customize UI is a lot worse and it's not immediately obvious how to switch back.

I wonder if we should try to improve this story, or if that's not worth the effort. I'm thinking we could change the toolbar currentset stuff in a migration code path (ie replace our merged thing in the navbar with the appropriate sequence of back/fwd/navbar/whatever) that we land only on m-c/backout-branch (and possibly merge to earlier branches if necessary).

What do you think?

(I'm picking P3 because if we want this then sooner is better, but I'm not sure we want this enough, in which case maybe we should WONTFIX - I'm OK with that)
This seems worth fixing. How hard is it?
Without having written it, I suspect it shouldn't be super-hard. Just a question of reading the navbar's currentset and replacing the ID of the combined item with the IDs of the original set of items. I'd be tempted, in fact, to add it in the UI migration code but not associate it with a number, so that it will essentially always happen on builds that don't have the combined item.

We could land this on/via mainline branches, and back it out of the ux branch after having merged it. Assuming that the UI migration code runs before any windows open, I believe this approach should work. Would even be possible to low-risk backport it to beta/aurora.

I can see if I have time to try this approach later today.
... except in comment #2, I assumed that we'd renamed the container. We haven't. That makes it hard to tell if you've just switched back from Australis or not. We can't just always put (other) items back in the navbar, because that'd break people who've customized them away. :-(
This should work, though. Tested it briefly here, seems to do the job OK... I've had to mess with the rdf initialization a little, but nothing too bad. I'm deleting them because that's what the end of _migrateUI does, but I'm not sure why that's the case and if we really need to.
Attachment #822373 - Flags: review?(gavin.sharp)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Should we also do a hotfix for mozilla-release and mozilla-beta?
(In reply to Jared Wein [:jaws] from comment #5)
> Should we also do a hotfix for mozilla-release and mozilla-beta?

I think it's too late to get this into 25, but if we can bake this the coming week and then land on aurora/beta (27/26 at that point) that early in the cycle, I think that would be a good idea.
Comment on attachment 822373 [details] [diff] [review]
migrate based on pref,

Seems like making this._rdf and this._dataSource local variables (and passing them directly to any helper functions that currently reference them) would make ensuring that they're not referenced indefinitely less error-prone (and the code a bit easier to reason about). I don't much like the lazy getters.

What does prefHasUserValue("browser.uiCustomization.state") mean? "has been on australis" - is that true even if you haven't customized things?

Is the idea to only land this on m-c and get rid of the code on the Australis merge, and avoid merging it to UX in the interim?
Comment on attachment 822373 [details] [diff] [review]
migrate based on pref,

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> Comment on attachment 822373 [details] [diff] [review]
> migrate based on pref,
> 
> Seems like making this._rdf and this._dataSource local variables (and
> passing them directly to any helper functions that currently reference them)
> would make ensuring that they're not referenced indefinitely less
> error-prone (and the code a bit easier to reason about). I don't much like
> the lazy getters.

Hrm. I'm OK with that, but the current code avoids instantiating them if there is no need to do anything based on the current version. I'm not sure, offhand, how simple the code is if I keep that behaviour, and add this new non-version-dependent check, but I'll have a look. Clearing review and marking obsolete - I'll try and get a new patch together tomorrow morning.

> What does prefHasUserValue("browser.uiCustomization.state") mean? "has been
> on australis" - is that true even if you haven't customized things?

I believe so, but I'll check to make sure. The pref stores the customization state (essentially the equivalent of the localstore.rdf-based currentset attributes, although we do still save those for compatibility reasons). 
 
> Is the idea to only land this on m-c and get rid of the code on the
> Australis merge, and avoid merging it to UX in the interim?

My original thought was to land it on m-c, merge it to UX as usual, and immediately back it out on UX (ideally in the same push; maybe it could even be done in the merge cset, but I'm less sure about that and/or how well hg will deal with that). This would mean that when we merge UX to m-c, it'll automatically be removed there, too. I'm not sure there's really a good way to cherry-pick csets to *not* merge (while merging their parent and child) without altering cset IDs (as compared to m-c) and thereby making history really messy.

Does this sound like a plausible strategy?
Attachment #822373 - Attachment is obsolete: true
Attachment #822373 - Flags: review?(gavin.sharp)
Duplicate of this bug: 906727
Comment on attachment 822373 [details] [diff] [review]
migrate based on pref,

(In reply to :Gijs Kruitbosch from comment #8)
> Comment on attachment 822373 [details] [diff] [review]
> migrate based on pref,
> 
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #7)
> > Comment on attachment 822373 [details] [diff] [review]
> > migrate based on pref,
> > 
> > Seems like making this._rdf and this._dataSource local variables (and
> > passing them directly to any helper functions that currently reference them)
> > would make ensuring that they're not referenced indefinitely less
> > error-prone (and the code a bit easier to reason about). I don't much like
> > the lazy getters.
> 
> Hrm. I'm OK with that, but the current code avoids instantiating them if
> there is no need to do anything based on the current version. I'm not sure,
> offhand, how simple the code is if I keep that behaviour, and add this new
> non-version-dependent check, but I'll have a look. Clearing review and
> marking obsolete - I'll try and get a new patch together tomorrow morning.

Actually, this would require touching over half the lines of _migrateUI, because they all rely on this._rdf/this._dataSource, as do this._getPersist / this._setPersist. :-(
Attachment #822373 - Attachment is obsolete: false
Attachment #822373 - Flags: review?(gavin.sharp)
(In reply to :Gijs Kruitbosch from comment #8)
> Comment on attachment 822373 [details] [diff] [review]
> migrate based on pref,
> 
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #7)
> > What does prefHasUserValue("browser.uiCustomization.state") mean? "has been
> > on australis" - is that true even if you haven't customized things?
> 
> I believe so, but I'll check to make sure. The pref stores the customization
> state (essentially the equivalent of the localstore.rdf-based currentset
> attributes, although we do still save those for compatibility reasons). 

OK, so this isn't 100% correct. It's only saved if anything is non-default, so if you customize on Australis, or if things were customized before (which would be saved in currentset, which we migrate and save). If you take a vanilla nightly profile, and open it in UX, and then go back, nothing really happens. If you take a customized nightly profile (e.g. by installing adblockplus, or using the customize functionality, or whatever) and then put it in australis, and then take it back, this would run.
Whiteboard: [Australis:P3] → [Australis:P2]
Comment on attachment 822373 [details] [diff] [review]
migrate based on pref,

Approach sounds fine overall, but someone closer to Australis should review this more closely.
Attachment #822373 - Flags: review?(gavin.sharp) → feedback+
Attachment #822373 - Flags: review?(jaws)
Attachment #822373 - Flags: review?(jaws) → review+
Comment on attachment 822373 [details] [diff] [review]
migrate based on pref,

Review of attachment 822373 [details] [diff] [review]:
-----------------------------------------------------------------

Had it marked as r+ but forgot to hit Publish. 2 r+'s can't hurt though :)
Attachment #822373 - Flags: review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/ba95fffe62b9
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-fx-team]
Annnnd this broke a mochitest-browser test that assumed that we initialize _rdf and _dataSource within the test. Went back to doing that:

https://hg.mozilla.org/integration/fx-team/rev/564283498a6f
(In reply to :Gijs Kruitbosch from comment #15)
> Annnnd this broke a mochitest-browser test that assumed that we initialize
> _rdf and _dataSource within the test. Went back to doing that:

Which test? Shouldn't it be fixed to not do that?
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #16)
> (In reply to :Gijs Kruitbosch from comment #15)
> > Annnnd this broke a mochitest-browser test that assumed that we initialize
> > _rdf and _dataSource within the test. Went back to doing that:
> 
> Which test? Shouldn't it be fixed to not do that?

https://tbpl.mozilla.org/?tree=Fx-Team&rev=ba95fffe62b9

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_toolbar_migration.js | Exception thrown - [Exception... "'[JavaScript Error: "this._rdf is undefined" {file: "resource://app/components/nsBrowserGlue.js" line: 1348}]' when calling method: [nsIObserver::observe]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_toolbar_migration.js :: test_explicitly_collapsed_toolbar :: line 73" data: yes]


http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/browser_toolbar_migration.js

The whole tests seems to be making this assumption. I'm not sure how easy it is to fix that, but feel free to file a followup bug and I can have a look.
I don't really understand what the issue is - that test just sets the migration.version pref and causes _migrateUI to be called (via the magical observer service notification). Shouldn't that work regardless of whether there's a lazy getter?
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2][Australis:M9][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ba95fffe62b9
https://hg.mozilla.org/mozilla-central/rev/564283498a6f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][Australis:M9][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 28
Whiteboard: [Australis:P2] → [Australis:P2][Australis:M9]
Attached patch Combined patch for uplift (obsolete) — Splinter Review
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #18)
> I don't really understand what the issue is - that test just sets the
> migration.version pref and causes _migrateUI to be called (via the magical
> observer service notification). Shouldn't that work regardless of whether
> there's a lazy getter?

The issue was that we were deleting the the property off the object, which also deleted the getter. Actually initializing those variables when needed, inside _migrateUI, doesn't have that problem.
Comment on attachment 827525 [details] [diff] [review]
Combined patch for uplift

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: if users try out Australis (on UX branch, or soon Nightly, or Aurora/Beta when it rides trains), they might lose their back/forward/reload/stop buttons when returning to their "regular", non-Australis Firefox.
Testing completed (on m-c, etc.): Been on m-c since Nov. 5th. Nightly.
Risk to taking this patch (and alternatives if risky): Low risk, the impact should effectively be 0 unless people use their profile on Australis, too.
String or IDL/UUID changes made by this patch: none.
Attachment #827525 - Flags: approval-mozilla-beta?
Attachment #827525 - Flags: approval-mozilla-aurora?
Can QA verify this works as intended before we approve for uplift?
Flags: needinfo?(jbecerra)
Keywords: qawanted
Three paths to verify (hoping this helps QA):

A:
1. Create a new profile in regular nightly.
2. Open that profile in ux
3. Open that profile in regular nightly.
Nothing should really change

B:
1. Create a new profile in regular nightly.
2. Customize and swap the downloads button and the home button (Right click the navigation toolbar, click customize, drag the buttons to rearrange them). Don't customize anything else.
3. Open the profile in ux. The buttons should still be in their customized order. Nothing else should have changed.
4. Open the profile in regular nightly. The buttons should still be in their customized order. Nothing else should have changed (in particular, the back/forward button should be next to the url bar as usual, and the reload/stop button should be 'in' the urlbar as usual)

C:
1. Create a new profile in regular nightly.
2. Open the profile in ux.
3. Customize and swap the downloads button and the home button (click the hamburger/menu icon, click "Customize", drag to rearrange them). Don't customize anything else. Exit customize mode by clicking the menu button / customize button again, or hitting [esc].
4. Open the profile in regular nightly. The buttons should still be in their customized order. Nothing else should have changed (in particular, the back/forward button should be next to the url bar as usual, and the reload/stop button should be 'in' the urlbar as usual)


If you like, you could even verify that with a regular nightly before this fix landed, scenarios B and C don't work - that is, the back/fwd/reload/stop buttons will have disappeared from the navigation bar.
Flags: needinfo?(jbecerra)
QA Contact: jbecerra
Jared found an issue which is that currentset is sometimes null, which is caused by bug 938694, which needs fixing on the UX branch. Here's an update to the migration code which will ensure it doesn't break in case this happened, and that it saves and flushes only when necessary.
Attachment #832376 - Flags: review?(jaws)
Comment on attachment 827525 [details] [diff] [review]
Combined patch for uplift

Clearing approval requests until the followup has landed too.
Attachment #827525 - Flags: approval-mozilla-beta?
Attachment #827525 - Flags: approval-mozilla-aurora?
Attachment #832376 - Flags: review?(jaws) → review+
Depends on: 938961
Duplicate of this bug: 938961
Flagging Gijs to make sure he lands attachment #832376 [details] [diff] [review].
Flags: needinfo?(gijskruitbosch+bugs)
remote:   https://hg.mozilla.org/integration/fx-team/rev/6b335ebfebd9
Flags: needinfo?(gijskruitbosch+bugs)
Duplicate of this bug: 956241
Attachment #827525 - Attachment is obsolete: true
Comment on attachment 8355518 [details] [diff] [review]
Patch for beta uplift

Carrying over review, asking for beta approval:

IMO we should still uplift this to beta. People are now running into this in release and it can be pretty distressing (see also the bug I just duped). It's been baking on 28 for a long while now with no issues there - in fact, I should probably have ensured this made 26, but obviously it's too late for that...

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: users using a profile on both Australis and non-Australis miss navigation buttons when opening non-Australis Firefox
Testing completed (on m-c, etc.): Manual testing found an issue early on that was fixed immediately. Been on m-c for a while, now also been on m-a for well over a month, no known issues.
Risk to taking this patch (and alternatives if risky): Quite low, although not super-low. Alternatives would be publicizing the issue and its workarounds more widely, and waiting for 28 to hit release. In my opinion, we should do that anyway, but also still uplift this patch.
String or IDL/UUID changes made by this patch: None
Attachment #8355518 - Flags: review+
Attachment #8355518 - Flags: approval-mozilla-beta?
I'll touch base with QA in regard to comment#24 before uplifting this to beta. We should try and get this tested as early in preparation for uplift on our next beta which is going to build on Monday, if no issues are found.
Verified on on Win 8.1 and Mac 10.9 with latest Nightly and latest UX
Status: RESOLVED → VERIFIED
(In reply to [:tracy] Tracy Walker - QA Mentor from comment #36)
> Verified on on Win 8.1 and Mac 10.9 with latest Nightly and latest UX

Thanks Tracy!
Comment on attachment 8355518 [details] [diff] [review]
Patch for beta uplift

Given QA verification approving this, please make sure to land before our Monday so this can get into our next beta, which will be going to build on that day.
Attachment #8355518 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Did some exploratory testing on Firefox 27 beta 8 and did not encountered anything broken around Customize Toolbar.
You need to log in before you can comment on or make changes to this bug.