Closed Bug 825852 Opened 7 years ago Closed 7 years ago

Add a further migration UI to move the downloads button in place.

Categories

(Firefox :: Downloads Panel, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 21
Tracking Status
firefox20 --- verified

People

(Reporter: mak, Assigned: mconley)

References

Details

Attachments

(1 file, 4 obsolete files)

The old migration UI was for Nightly, users in Aurora/Beta/Release skipped that cause the feature was disabled.
So once we have the confirmation the feature is being released we must push to all branches a new migrationUI version (see nsBrowserGlue.js) to put the button in place on the navigation toolbar.
Assignee: nobody → mconley
Attached patch Patch v1 (obsolete) — Splinter Review
This patch adds a new migration, which is essentially the same as migration #7.
Attachment #697452 - Flags: review?(mak77)
since it's the same, may we move the code to an helper function and invoke that one in both steps?
Whiteboard: [do not land until 100% sure the feature will be shipped]
Attachment #697452 - Flags: review?(mak77) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
(In reply to Marco Bonardo [:mak] from comment #2)
> since it's the same, may we move the code to an helper function and invoke
> that one in both steps?

Sounds OK to me - I've added an inline function used by both UIVersion 7 and 9.
Attachment #697452 - Attachment is obsolete: true
Attachment #697906 - Flags: review?(mak77)
Comment on attachment 697906 [details] [diff] [review]
Patch v2

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

r=me with the following concern addressed/answered

::: browser/components/nsBrowserGlue.js
@@ +1449,5 @@
> +    // the toolkit download UI in about:config.
> +    if (currentUIVersion < 9 &&
> +        !Services.prefs.getBoolPref("browser.download.useToolkitUI")) {
> +      addDownloadsButton();
> +    }

My only concern here, is whether we should ignore browser.download.useToolkitUI, and also clearUserPref on it.
Many users may have disabled the feature cause it was not ready and thus killing their productivity, for example due to the long hang when opening the Library view or the missing management options in that view. This would be a way to notify the feature is ready, and then the "disable" choice would be conscious based on the actual status of the feature.

I'll defer this decision to the module owner, I'm not sure how we want to handle these cases.
Attachment #697906 - Flags: review?(mak77)
Attachment #697906 - Flags: review+
Attachment #697906 - Flags: feedback?(gavin.sharp)
Whiteboard: [do not land until 100% sure the feature will be shipped] → [do not land until all feature blockers but this are fixed]
When upgrading from profiles with UI versions prior to 7, I think we have no
specific reason to perform the migration step for the download button before
we execute the migration step for version 8 (reset the homepage preference).

For version 9, I tend to agree with Marco that we should not check the preference
that enables the panel, and just reset it, for the mentioned reasons.
(In reply to Paolo Amadini [:paolo] from comment #5)
> When upgrading from profiles with UI versions prior to 7, I think we have no
> specific reason to perform the migration step for the download button before
> we execute the migration step for version 8 (reset the homepage preference).

are you suggesting to just get rid of version 7 and move code to version 9?  Well that may indeed be fine and sounds like a good suggestion.
Attached patch Patch v3 (r+'d by mak) (obsolete) — Splinter Review
Removes old migration at version 7, and unconditionally adds downloads button at migration version 9.

Re-requesting feedback from gavin, since we want to make sure that migrating even if useToolkitUI is true is OK to do.
Attachment #697906 - Attachment is obsolete: true
Attachment #697906 - Flags: feedback?(gavin.sharp)
Attachment #698033 - Flags: feedback?(gavin.sharp)
(In reply to Mike Conley (:mconley) from comment #7)
> Re-requesting feedback from gavin, since we want to make sure that migrating
> even if useToolkitUI is true is OK to do.

yes, provided you also clearUserPref useToolkitUI, otherwise we'd enforce a useless button to users...
Attached patch Patch v4 (r+'d by mak) (obsolete) — Splinter Review
Yep, good call - thanks!
Attachment #698033 - Attachment is obsolete: true
Attachment #698033 - Flags: feedback?(gavin.sharp)
Attachment #698050 - Flags: feedback?(gavin.sharp)
Comment on attachment 698050 [details] [diff] [review]
Patch v4 (r+'d by mak)

A "reset" of any custom values for useToolkitUI seems reasonable to me.
Attachment #698050 - Flags: feedback?(gavin.sharp) → feedback+
Comment on attachment 698050 [details] [diff] [review]
Patch v4 (r+'d by mak)

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

::: browser/components/nsBrowserGlue.js
@@ +1434,2 @@
>  
> +      Services.prefs.clearUserPref("browser.download.useToolkitUI");

While here, may you please also clearUserPref the useNewDownloadsView (or whatever was called that temporary pref), so we clean it up...
(In reply to Marco Bonardo [:mak] from comment #11)
> Comment on attachment 698050 [details] [diff] [review]
> Patch v4 (r+'d by mak)
> 
> Review of attachment 698050 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/nsBrowserGlue.js
> @@ +1434,2 @@
> >  
> > +      Services.prefs.clearUserPref("browser.download.useToolkitUI");
> 
> While here, may you please also clearUserPref the useNewDownloadsView (or
> whatever was called that temporary pref), so we clean it up...

Done.
Attachment #698050 - Attachment is obsolete: true
I think once the dependencies here are done we can proceed, other blockers are minor problems
Depends on: 819428, 826991
Whiteboard: [do not land until all feature blockers but this are fixed] → [do not land until all dependencies are fixed]
Bug 819428 and bug 826991 have both landed on inbound, so I'll land this one too.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7c6feb01670
Comment on attachment 700061 [details] [diff] [review]
Patch v5 (r+'d by mak, feedback+ from gavin)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): downloads panel feature
User impact if declined: incomplete feature
Testing completed (on m-c, etc.): m-i (pending merge)
Risk to taking this patch (and alternatives if risky): limited, reusing existing code already used in the past
String or UUID changes made by this patch: none
Attachment #700061 - Flags: approval-mozilla-aurora?
Attachment #700061 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/630691b60f25
Whiteboard: [do not land until all dependencies are fixed]
Target Milestone: --- → Firefox 21
https://hg.mozilla.org/mozilla-central/rev/a7c6feb01670
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Verified as fixed on Firefox 20 beta 1 - the downloads button is placed on the navigation toolbar after updating  Firefox 19 beta 6 (the feature was disabled on this version) to Firefox 20 beta 1. 

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.8:
Build ID: 20130220104816
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
You need to log in before you can comment on or make changes to this bug.