The downloads button opens the panel even if it's disabled

VERIFIED FIXED in Firefox 17

Status

()

defect
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({regression, verifyme})

unspecified
Firefox 19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16 unaffected, firefox17+ verified, firefox18+ verified)

Details

()

Attachments

(1 attachment)

bug 748160 regressed this, the placeholder was observing Tools:Downloads, the patch made it use the command handler, plus on another patch landed at the same time we started replacing the button with the placeholder regardless (we were keeping the placeholder forever previously).

STR:
1. set browser.downloads.useToolkitUI = true (or just use beta/aurora where it's already set)
2. put the downloads button on the toolbar 
3. click on it

Expected:
The old DM window opens

Currently:
The new panel opens
This bug has been found on Mozillazine forums, patrickjdempsey found this regression-range:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=b3cce81fef1a
Posted patch patch v1.0Splinter Review
this is a simple patch that may be used.
My concern is whether we should do more, like avoiding replacing the placeholder or such, but actually that could be just a waste of time for this simple fix.

The fix for this bug will have to be backported to Aurora and Beta, since feature disable is broken currently.
Attachment #678729 - Flags: review?(paolo.mozmail)
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Comment on attachment 678729 [details] [diff] [review]
patch v1.0

Regression indeed.
Attachment #678729 - Flags: review?(paolo.mozmail) → review+
(In reply to Marco Bonardo [:mak] from comment #2)
> My concern is whether we should do more, like avoiding replacing the
> placeholder or such, but actually that could be just a waste of time for
> this simple fix.

I think like you here. Let's see how difficult it is to avoid replacing the
placeholder with the indicator, but this fix should be done regardless.
At this stage in the 17 beta cycle, we need to look at backing out the regressing bug and fixing this properly on Aurora instead.  Can someone prepare a backout patch for bug 748160 that returns us to a known good state?
Flags: needinfo?(mak77)
(In reply to Lukas Blakk [:lsblakk] from comment #6)
> At this stage in the 17 beta cycle, we need to look at backing out the
> regressing bug and fixing this properly on Aurora instead.  Can someone
> prepare a backout patch for bug 748160 that returns us to a known good state?

the backout is too risky imo, we coded on top of it, this patch is 10 times less risky than the backout.
Flags: needinfo?(mak77)
https://hg.mozilla.org/mozilla-central/rev/9b3e98b9f918
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to Marco Bonardo [:mak] from comment #7)
> (In reply to Lukas Blakk [:lsblakk] from comment #6)
> > At this stage in the 17 beta cycle, we need to look at backing out the
> > regressing bug and fixing this properly on Aurora instead.  Can someone
> > prepare a backout patch for bug 748160 that returns us to a known good state?
> 
> the backout is too risky imo, we coded on top of it, this patch is 10 times
> less risky than the backout.

Sounds like we need an uplift nomination here then, once qa has done verification on trunk, so this can get into our final beta 17.
Keywords: qawanted, verifyme
Comment on attachment 678729 [details] [diff] [review]
patch v1.0

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 748160
User impact if declined: The downloads button opens the panel even if this feature is disabled, it should keep opening the old DM
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): It's basically a oneliner, so risk is limited. Alternative is to find all the downloads panel bugs that landed in the meanwhile and backout all of them, there's risk to miss something though, making things worse.
String or UUID changes made by this patch: none
Attachment #678729 - Flags: approval-mozilla-beta?
Attachment #678729 - Flags: approval-mozilla-aurora?
Verified on Windows 7 64-bit, Ubuntu 12.04 and Mac OS X 10.8:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/19.0 Firefox/19.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/19.0 Firefox/19.0
Build ID: 20121107030713

Verified that the the downloads button doesn't open the panel if it's disabled on the latest Nightly.
QA Contact: simona.marcu
Attachment #678729 - Flags: approval-mozilla-beta?
Attachment #678729 - Flags: approval-mozilla-beta+
Attachment #678729 - Flags: approval-mozilla-aurora?
Attachment #678729 - Flags: approval-mozilla-aurora+
Keywords: qawanted
Reproducible in 17.0b5 Linux-x86_64.
(In reply to Aleksej [:Aleksej] from comment #13)
> Reproducible in 17.0b5 Linux-x86_64.

b05 has been released just before this fix, so that's expected.
(In reply to Aleksej [:Aleksej] from comment #13)
> Reproducible in 17.0b5 Linux-x86_64.

Please try the most recent beta build from http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-linux/  to see if the current code that is about to become beta 6 is fixed.
Verified as fixed on Firefox 17 beta 6 - the downloads button doesn't open the downloads panel if is disabled.

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.8:

Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Firefox/17.0
Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:17.0) Gecko/17.0 Firefox/17.0
Verified as fixed on Firefox 18 beta 2. This issue in no more reproducing.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 (20121128060531)
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/20100101 Firefox/18.0 (20121128060531)
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:18.0) Gecko/20100101 Firefox/18.0 (20121128060531)

Please someone with permission set status for firefox18 as verified.
Status: RESOLVED → VERIFIED
(In reply to MarioMi from comment #18)
> Verified as fixed on Firefox 18 beta 2. This issue in no more reproducing.

Same results for FF 19b2
You need to log in before you can comment on or make changes to this bug.