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

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Marco Bonardo [:mak]]

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

Comment 1

[Marco Bonardo [:mak]]

This bug has been found on Mozillazine forums, patrickjdempsey found this regression-range:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=b3cce81fef1a

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.0

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.

Comment 3

[:Paolo Amadini]

Comment on attachment 678729 [details] [diff] [review]
patch v1.0

Regression indeed.

Comment 4

[:Paolo Amadini]

(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.

Comment 5

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b3e98b9f918

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

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?

Comment 7

[Marco Bonardo [:mak]]

(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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b3e98b9f918

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

(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.

Comment 10

[Marco Bonardo [:mak]]

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

Comment 11

[Simona Badau, Desktop QA]

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.

Comment 12

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34b009e46fd1
https://hg.mozilla.org/releases/mozilla-beta/rev/25b230afe821

Comment 13

[[:Aleksej]]

Reproducible in 17.0b5 Linux-x86_64.

Comment 14

[Marco Bonardo [:mak]]

(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.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

(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.

Comment 16

[[:Aleksej]]

Verified fixed with https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-linux/1352721280/firefox-17.0.en-US.linux-i686.tar.bz2

Comment 17

[Simona Badau, Desktop QA]

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

Comment 18

[Mihai Morar, (:MihaiMorar)]

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.

Comment 19

[Mihai Morar, (:MihaiMorar)]

(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