Closed
Bug 791569
Opened 12 years ago
Closed 12 years ago
Downloads panel and download manager show up on first download, then only download manager (download button missing)
Categories
(Firefox :: Downloads Panel, defect)
Tracking
()
VERIFIED
FIXED
Firefox 19
People
(Reporter: aryx, Assigned: mconley)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → mconley
Assignee | ||
Comment 1•12 years ago
|
||
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.
Assignee | ||
Comment 2•12 years ago
|
||
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.
Attachment #669718 -
Attachment is obsolete: true
Attachment #669727 -
Flags: review?(mak77)
Assignee | ||
Updated•12 years ago
|
Blocks: ReleaseDownloadsPane
Comment 3•12 years ago
|
||
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()
})
}
Attachment #669727 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 4•12 years ago
|
||
Thanks Mak! Changes made.
Attachment #669727 -
Attachment is obsolete: true
Assignee | ||
Comment 5•12 years ago
|
||
Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/816bba27680f
OS: Windows XP → All
Comment 6•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Comment 8•12 years ago
|
||
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
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•