Closed Bug 936897 Opened 6 years ago Closed 6 years ago

Defect - Download progress bar not appearing after first tab is closed (this._notificationBox.appendNotification is not a function)

Categories

(Firefox for Metro Graveyard :: Downloads, defect, P2)

28 Branch
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 28

People

(Reporter: mbrubeck, Assigned: sfoster)

References

Details

(Whiteboard: [beta28] feature=defect c=Info_app_bar u=metro_firefox_user p=2)

Attachments

(1 file, 5 obsolete files)

Steps to reproduce:
1) Click on a download link.
2) A notification bar appears asking whether to open or save.  Choose either.

Expected results: A download progress notification bar appears.  Pressing the download button on the navbar toggles the notification bar.

Actual results: No notification bar appears.  Pressing the download button does nothing.  There are errors messages in the JavaScript console:

TypeError: this._notificationBox.appendNotification is not a function
chrome://browser/content/downloads.js:199
Potentially a dupe of bug 928202?
The steps in comment 0 were incomplete.  This happens only if you close the first tab.  downloads.jsm caches the selected tab's notificationbox on startup, and always tries to add notifications to that notificationbox, even if it no longer exists.

Correct steps to reproduce:
1) Open Metro Firefox.
2) Close the initially-selected tab.
3) Go to a web page like http://nighty.mozilla.org and click on a download link.
4) Choose whether to open or save the file.

The correct solution might be a global notificationbox for the download progress notification bar.  In the short-term, we should at least use the current selected tab's notificationbox.
Keywords: regression
OS: Windows 8.1 → All
Hardware: x86_64 → All
Summary: Defect - Download progress bar not appearing (this._notificationBox.appendNotification is not a function) → Defect - Download progress bar not appearing after first tab is closed (this._notificationBox.appendNotification is not a function)
I'll take a look into this
Assignee: nobody → sfoster
Whiteboard: [triage] feature=defect c=Info_app_bar u=metro_firefox_user p=0 → [block28] feature=defect c=Info_app_bar u=metro_firefox_user p=0
Blocks: metrov1it19
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [block28] feature=defect c=Info_app_bar u=metro_firefox_user p=0 → [block28] feature=defect c=Info_app_bar u=metro_firefox_user p=2
I made _notificationBox a getter, which calls Browser.getNotificationBox, which already defaults to using the selectedBrowser. Maybe _notificationBox should lose its underscore as a getter?
Attachment #830493 - Flags: review?(mbrubeck)
Comment on attachment 830493 [details] [diff] [review]
Make MetroDownloadsView._notificationBox a getter to avoid caching a bad reference

Unfortunately this isn't sufficient, because downloads.js calls removeNotification in several places, and this needs to be called on the same notificationbox where the notification was originally added.  (The same goes for places where we check to see if a notification already exists.)
Attachment #830493 - Flags: review?(mbrubeck) → review-
Attached patch WIP: global notification box (obsolete) — Splinter Review
This is a patch I started to add a global notificationbox.  It has some layout issues, but might be a useful reference or starting point.
Quick status update on this. Currently each browser has its own notificationbox, and we show/hide the selected browser/tab's notifications, so UI-wise the download is tied to the tab it began in. Actually though it continues in the background if you switch tabs, or close that tab. 

So, we need a global notificationbox for notifications that should be associated with the session as a whole, as well as the per-browser ones. We expect popupblockers and similar messages to go away when we close a given tab. But a download may not finish for hours, and when it completes we want to know whichever tab we happen to be using, so we expect its status and notification to be associated with the global notificationbox. 

To accomplish this we'll need to create a new notificationbox, probably in browser.xul, probably in or near #navbar. It will need to play nice with the layout and ContentAreaObserver needs to know about it. Most - possibly all - places where we add download notifications will need to use this global notificationbox. 

There is the possibility of both global and tab-specific notifications being on the screen at the same time, so we'll need to accommodate that too. 

I have a not yet useful for feedback WIP building on Matt's patch to try and get this working. Its fighting me every step of the way :) I may be down a rat hole, if anyone has a shorter route to the goal let me know.
I've refactored a lot of downloads.js / MetroDownloadsView to use the DownloadList and remove a lot of the book-keeping. There's a shim "Download" class that stands in for the proper all-async one from DownloadCore in DownloadWrapper. All download management is still down by the niIDownloadManager. 

Its a big diff, sorry. This is /mostly-working/. The smoke-tests I've got in there pass, downloads kinda work but the notifications logic isn't right yet (and/or there's bugs I've not tracked down yet)
To address the tab/notification issue, the DownloadNotificationView knows how to poke around the open tabs' linkedBrowsers to find and move existing notifications. Also, it handles TabClose to rescue notifications before they die with that browser. 

I'm on PTO next weeek, so if someone wants to take this and run it across the finish line, be my guest. As-is, or you could probably just extract the tab/notification stuff and come up with a smaller/lower-risk patch. Otherwise I'll pick it back up when I return.
Attachment #830493 - Attachment is obsolete: true
Attachment #8337206 - Flags: feedback?(mbrubeck)
No longer blocks: metrov1it19
Assignee: sfoster → mbrubeck
Assignee: mbrubeck → sfoster
Blocks: 928202
This is pretty much working by my testing. The particular closed-tab/missing notification bug is fixed and I /think/ I've not regressed any other downloads/notifications behavior but will continue testing. 

This new patch includes changes to move most of the download event listening to the DownloadProgressListener, which does the byGuid lookup and DownloadList updates, and delivers Download instances back to methods on MetroDownloadsView where appropriate. The result is a passable separation of concerns and hopefully gets us closer to wholesale Downloads.jsm adoption down the road. 

Note the _downloadCount we discussed, which counts the number of completed downloads not yet acknowledges or opened by the user, is preserved by the completed() listing. downloads are removed from the downloadList once run/opened.

(I ran into strange bugs when trying to make 'inProgress' and 'completed' getter properties so they are methods for now) 

Any feedback you have on the organization, approach, bugs/regressions all appreciated.
Attachment #8337206 - Attachment is obsolete: true
Attachment #8337206 - Flags: feedback?(mbrubeck)
Attachment #8342587 - Flags: feedback?(mbrubeck)
Whiteboard: [block28] feature=defect c=Info_app_bar u=metro_firefox_user p=2 → [beta28] feature=defect c=Info_app_bar u=metro_firefox_user p=2
Comment on attachment 8342587 [details] [diff] [review]
WIP: downloads-notifications + DownloadList

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

This looks pretty good, and seems to solve the notificationbox problem in a reasonable way.  I'm worried about the amount of code going on here... but alternatives aren't that pretty either.  Still, what do you think about landing the notification-finding-and-moving parts for v1 and saving the rest for v2?  Is it feasible to disentangle them?

I haven't done anything close to a detailed review of this code, so I have only a couple of specific random notes:

::: browser/metro/modules/DownloadWrapper.jsm
@@ +11,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource:///modules/ContentUtil.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "DownloadListBase",
> +                                  "resource://gre/modules/DownloadList.jsm", "DownloadList");

Maybe we should call our subclass "MetroDownloadList" and keep the original name for "DownloadList", to avoid possible future confusion.

@@ +15,5 @@
> +                                  "resource://gre/modules/DownloadList.jsm", "DownloadList");
> +
> +
> +function assert(thing, check, msg) {
> +  let checkFn = ('function' == typeof check) ?

Does this take a function so you can avoid running the check when asserts are disabled?

I think it would be less wordy to either take just a boolean, or at least to eliminate the "thing" argument and just take a function:

  assert(download instanceof Download, "message");

or 


  assert(() => download instanceof Download, "message");
Attachment #8342587 - Flags: feedback?(mbrubeck) → feedback+
Blocks: metrov1it21
No longer blocks: metrov1it20
Whiteboard: [beta28] feature=defect c=Info_app_bar u=metro_firefox_user p=2 → [beta28] feature=defect c=Info_app_bar u=metro_firefox_user p=2
This is just the closeTab listener and associated changes to how we track the progressNotification across browsers/notificationboxes. I'll rework the full DownloadList patch to apply on top of this another time.
Attachment #830973 - Attachment is obsolete: true
Attachment #8342587 - Attachment is obsolete: true
Attachment #8344002 - Flags: review?(mbrubeck)
Comment on attachment 8344002 [details] [diff] [review]
Move download-progress notifications before a tab closes

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

Thanks!  One little thing I think you missed:

::: browser/metro/base/content/downloads.js
@@ +115,5 @@
> +
> +  _getNotificationWithValue: function(aValue) {
> +    let notn;
> +    let allNotificationBoxes = this._notificationBoxes;
> +    for(let box of allNotificationBoxes) {

You should also make _removeNotification call this function, rather than assume that the notification is in the selected tab's notificationbox.

onDownloadButton could also stand to be a little smarter (in the case where the notification exists but is in a different tab), but we could leave that until we get around to rewriting more of this code for v2.

@@ +557,5 @@
> +          let box = Browser.getNotificationBox(nextTab.linkedBrowser);
> +          if (box.childElementCount) {
> +            box.insertBefore(notn, box.firstChild);
> +          } else {
> +            box.appendChild(notn);

I think you can just call "box.insertBefore(notn, box.firstChild);" unconditionally, since it'll still work if the second argument is null.
Attachment #8344002 - Flags: review?(mbrubeck) → review+
I addressed previous review comments and came up with something for the onDownloadButton so can you sanity-check before I push this? 

On thing I noticed while testing this was that if you hit the 'X' on the download complete notification, the progress-ring isn't cleared and clicking it again (on any tab now) brings back the same download complete notification. This is unchanged by this patch only that that ring button works for all tabs now.
Attachment #8344002 - Attachment is obsolete: true
Attachment #8344112 - Flags: review?(mbrubeck)
Comment on attachment 8344112 [details] [diff] [review]
Move download-progress notifications before a tab closes

Awesome!
Attachment #8344112 - Flags: review?(mbrubeck) → review+
Landed on fx-team, I hope I made the uplift cutoff: 
https://hg.mozilla.org/integration/fx-team/rev/846cd3814810
https://hg.mozilla.org/mozilla-central/rev/846cd3814810
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Temporarily reopening to add to ScrumBug.  Will mark as resolved right after.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Verified using latest aurora (build ID: 20140108004006) on Windows 8 and Windows 8.1 (using both 32bit and 64bit architecture) using the STR from comment 2.
The download progress notification bar is shown; Pressing the download button will toggle the notification bar.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.