Matt Brubeck (:mbrubeck) Reporter
	Description • 11 years ago

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

	Jim Mathies [:jimm]
	Comment 1 • 11 years ago

Potentially a dupe of bug 928202?

	Matt Brubeck (:mbrubeck) Reporter
	Comment 2 • 11 years ago

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.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 3 • 11 years ago

I'll take a look into this

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 4 • 11 years ago

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?

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 5 • 11 years ago

attachment 830493 [details] [diff] [review] does also seem to fix bug 928202.

	Matt Brubeck (:mbrubeck) Reporter
	Comment 6 • 11 years ago

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

	Matt Brubeck (:mbrubeck) Reporter
	Comment 7 • 11 years ago

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.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 8 • 11 years ago

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.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 9 • 11 years ago

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.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 10 • 11 years ago

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.

	Matt Brubeck (:mbrubeck) Reporter
	Comment 11 • 11 years ago

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");

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 12 • 11 years ago

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.

	Matt Brubeck (:mbrubeck) Reporter
	Comment 13 • 11 years ago

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.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 14 • 11 years ago

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.

	Matt Brubeck (:mbrubeck) Reporter
	Comment 15 • 11 years ago

Comment on attachment 8344112 [details] [diff] [review]
Move download-progress notifications before a tab closes

Awesome!

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 16 • 11 years ago

Landed on fx-team, I hope I made the uplift cutoff: 
https://hg.mozilla.org/integration/fx-team/rev/846cd3814810

	Phil Ringnalda (:philor)
	Comment 17 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/846cd3814810

	Marco Mucci [:MarcoM]
	Comment 18 • 11 years ago

Temporarily reopening to add to ScrumBug.  Will mark as resolved right after.

	Cornel Ionce [:noni] [Hubs QA]
	Comment 19 • 11 years ago

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.