791569 - Downloads panel and download manager show up on first download, then only download manager (download button missing)

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: screenshot both download modes active

Firefox Aurora 20120916, Windows XP SP3 32 bit

The downloads panel and the download manager show up on first download, then only the download manager (the download button is always missing).

Steps to reproduce:
1. Create a new profile.
2. Dismiss the check for default browser and 'Know your rights' yada yada.
3. Open about:config and set browser.download.useToolkitUI to false.
4. Go to http://www.7-zip.org and download the .exe for 32 bit.

Actual result:
The 'old' downloads manager window pops up and the downloads panel is also shown.

5. Go to http://www.heise.de/download/wsus-offline-update-ct-offline-update-348c5f757089469c57791ccd3ed6bc64-1347818738-2638170.html
6. Click 'Download direkt von heise.de'.
7. Download the file offered.

Actual result:
Only the downloads manager is shown downloading the file, the 'Download complete' will slide in after the download has been finished.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch 1

I believe this patch addresses the first problem mentioned in comment 0.

I think the bug is caused because sometimes the indicatorOverlay.xul has not finished loading by the time that DownloadsUI.show fires. This means that the button fails the "isVisible" test, and we default to the toolkit downloads manager.

This patch adds an asynchronous function to replace isVisible, to ensure that the overlay has finished loading before checking for visibility.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

I've polished the patch. I think I'm ready for review.

Just to reiterate - this patch only deals with the bug where we sometimes open up the old toolkit downloads manager on the first download. The other bug mentioned in comment 0 has not yet been investigated.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 669727 [details] [diff] [review]
Patch v2

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

The second bug in comment 0 should be moved to a separate report please, we can't handle more than 1 bug per report.

::: browser/components/downloads/content/indicator.js
@@ +180,5 @@
> +   * Checks whether the indicator is, or will soon be visible in the browser
> +   * window.
> +   *
> +   * @param aCallback
> +   *        Called once the indicator overlay has loaded, passing true if the

nit: s/, passing.*/. Gets a boolean argument representing the indicator visibility./

@@ +186,2 @@
>     */
> +  checkEventualVisibility: function DB_checkEventualVisibility(aCallback)

nit: I find the name a bit too verbose, just checkIsVisible may be enough and avoid some fancy indentation when used

@@ +190,5 @@
> +      if (!this._placeholder) {
> +        return aCallback(false);
> +      }
> +      let element = DownloadsIndicatorView.indicator || this._placeholder;
> +      return aCallback(isElementVisible(element.parentNode));

Please avoid the return(s) and just use an if/else, since the return value is unused but this makes it look like it matters.

::: browser/components/downloads/src/DownloadsUI.js
@@ +83,5 @@
>          return;
>        }
>      }
>  
>      this._toolkitUI.show(aWindowContext, aID, aReason);

I think the logic of the code is starting becoming unreadable with the early return, what about:

if (!browserwin || minimized) {
  _toolkitUI.show()
}
else {
  DownloadsButton.checkIsVisible(function() {
    if (!visible)
      _toolkitUI.show()
  })
}

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3 (r+'d by mak)

Thanks Mak! Changes made.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/816bba27680f

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/816bba27680f

Comment 8

[Simona Badau, Desktop QA]

Verified as fixed on the latest Nightly- the Download Manager does not show up on the first download - only the Download Panel does. Verified by following the first 4 steps from the Description.

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

Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/19.0 Firefox/19.0        
Build ID: 20121112030753
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/19.0 Firefox/19.0        
Build ID: 20121113030658
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:19.0) Gecko/19.0 Firefox/19.0        
Build ID: 20121113030658