822244 - Use the Downloads View in Places if it's preffed on

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

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

The "Show All Downloads", and "+ X other downloads" items in the Download Panel currently open the toolkit downloads window.

If the Places Downloads view is preffed on, we should open it instead.

Comment 1

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

Attachment: Patch v1

This changes BrowserDownloadsUI to try opening the Places view if browser.library.useNewDownloadsView is set to true. If false, or if all else fails, reverts to the old toolkit Downloads window.

Comment 2

[Asaf Romano (gone)]

Comment on attachment 693002 [details] [diff] [review]
Patch v1

r=mano for the browser.js changes only (I think the gnomestipe bits belong to bug 822257)

Comment 3

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

(In reply to Mano from comment #2)
> Comment on attachment 693002 [details] [diff] [review]
> Patch v1
> 
> r=mano for the browser.js changes only (I think the gnomestipe bits belong
> to bug 822257)

Wow, yep, that stuff really shouldn't be in there. Thanks, I'll remove before landing.

Comment 4

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

Hm - I seem to have missed a spot:

http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/downloads/src/DownloadsUI.js#62

I'll come up with a new patch.

Comment 5

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

(In reply to Mike Conley (:mconley) from comment #4)
> Hm - I seem to have missed a spot:
> 
> http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/
> downloads/src/DownloadsUI.js#62
> 
> I'll come up with a new patch.

Whoops - linked to the wrong lines:

http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/downloads/src/DownloadsUI.js#75

and

http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/downloads/src/DownloadsUI.js#87

Comment 6

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

Attachment: Patch v2

Instead of patching BrowserDownloadsUI like before, we put the switching logic inside DownloadsUI.js.

Marco - for the Places UI, do you know if I should handle getAttention and visible for nsIDownloadManagerUI as well?

Comment 7

[Marco Bonardo [:mak]]

(In reply to Mike Conley (:mconley) from comment #6)
> Marco - for the Places UI, do you know if I should handle getAttention and
> visible for nsIDownloadManagerUI as well?

I'm not sure if we documented somewhere how and if the Library should get attention compared to the main window that contains the panel. The current code just makes me think we didn't want to get attention with the new ui. This is likely something for a separate bug to evaluate that. I'd be fine with the lack attention on first impl.

visible is used to show the downloads UI when a new download starts:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadProxy.h#63
we don't want this, so I think for now it's ok to just always return true (if useToolkitUI is false), with a nice comment. Then we'll see in future. The problem is whether we want to open the Library or bring on a browser window with the panel.

Comment 8

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

Attachment: Patch v3

Ok, added the comment. I hope I understood the problem correctly - let me know if I should alter it any.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 693426 [details] [diff] [review]
Patch v3

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

::: browser/components/downloads/src/DownloadsUI.js
@@ +108,5 @@
> +   * Helper function that tries to open the Downloads view in the Places
> +   * Organizer. If the Downloads View in Places is preffed off, we fallback
> +   * to showing the toolkit downloads manager.
> +   */
> +  _showPlacesUI: function(aWindowContext, aID, aReason)

So, if useToolkitUI is true, we should always trust that, regardless the downloads view pref.

Thus I think this should be renamed to _showDownloadManagerUI() and will indeed open the right manager. And the javadoc should be updated according.

@@ +115,5 @@
> +      if (Services.prefs.getBoolPref("browser.library.useNewDownloadsView")) {
> +        let organizer = Services.wm.getMostRecentWindow("Places:Organizer");
> +        if (!organizer) {
> +          Services.ww
> +                  .activeWindow

So, I wonder if we should open the Library window if there is no Browser window around... I'm prone to say no, so maybe here we should just use RecentWindow.jsm

@@ +128,5 @@
> +        return;
> +      }
> +    } catch(e) {}
> +
> +    this._toolkitUI.show(aWindowContext, aID, aReason);

As stated, we should show the toolkitUI if:
- useToolkitUI is true
- useToolkitUI is false but we are not using the new view

Comment 10

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

Attachment: Patch v4

(In reply to Marco Bonardo [:mak] from comment #9)
> Comment on attachment 693426 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 693426 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/downloads/src/DownloadsUI.js
> @@ +108,5 @@
> > +   * Helper function that tries to open the Downloads view in the Places
> > +   * Organizer. If the Downloads View in Places is preffed off, we fallback
> > +   * to showing the toolkit downloads manager.
> > +   */
> > +  _showPlacesUI: function(aWindowContext, aID, aReason)
> 
> So, if useToolkitUI is true, we should always trust that, regardless the
> downloads view pref.

Ok, fixed.

> 
> Thus I think this should be renamed to _showDownloadManagerUI() and will
> indeed open the right manager. And the javadoc should be updated according.
> 

Done.

> @@ +115,5 @@
> > +      if (Services.prefs.getBoolPref("browser.library.useNewDownloadsView")) {
> > +        let organizer = Services.wm.getMostRecentWindow("Places:Organizer");
> > +        if (!organizer) {
> > +          Services.ww
> > +                  .activeWindow
> 
> So, I wonder if we should open the Library window if there is no Browser
> window around... I'm prone to say no, so maybe here we should just use
> RecentWindow.jsm
> 

Done - though I'm a little worried about downloads that were initiated from private browsing windows... though I guess we can sort that out once we have the downloads view for private browsing windows displaying in tabs.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 693879 [details] [diff] [review]
Patch v4

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

::: browser/components/downloads/src/DownloadsUI.js
@@ +63,3 @@
>      if (!aReason) {
>        aReason = Ci.nsIDownloadManagerUI.REASON_USER_INTERACTED;
>      }

I think I confused you with my previous comments, and now I see you were right. Sorry!

The global check:

if (DownloadsCommon.useToolkitUI) {		
  this._toolkitUI.show(aWindowContext, aID, aReason);		
  return;		
}

should stay where it was, at the beginning of show() and not be moved to _showDownloadManagerUI.
This way we reach _showDownloadManagerUI only if DownloadsCommon.useToolkitUI is false, that is exactly what we want.
Doing differently may change the default behavior of the old UI.

@@ +87,5 @@
>  
>    get visible()
>    {
> +    // If we're still using the toolkit downloads manager, delegate the call
> +    // to it. Otherwise, return true for now until we decide on how we want

comma after "now"

@@ +106,5 @@
> +   *
> +   *  1) useToolkitUI is true
> +   *  2) useToolkitUI is false, but the Places Downloads View is disabled.
> +   *
> +   * Otherwise, it attempts to open the Places Downloads View.

this becomes: "Helper function that opens the right download manager UI: the new Downloads View if it's enabled, the old toolkit UI otherwise." (feel free to fix my broken English)

@@ +114,5 @@
> +  {
> +    if (DownloadsCommon.useToolkitUI) {
> +      this._toolkitUI.show(aWindowContext, aID, aReason);
> +      return;
> +    }

This should be moved back, as I said

@@ +152,5 @@
> +      // If we got here, then the browser.library.useNewDownloadsView pref
> +      // either didn't exist or was false, so just show the toolkit downloads
> +      // manager.
> +      this._toolkitUI.show(aWindowContext, aID, aReason);
> +    }

MAybe we can reduce indentation a bit here, using some early return (pseudocode):

if (!usePlacesView) {
  show_toolkit
  return
}

let organizer...
if (organizer) {
  show organizer
  return
}

all-the-remaining

Comment 12

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

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

Thanks Marco! Comments addressed - here's the final patch.

Comment 13

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

Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/6bcdc46945a6

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/6bcdc46945a6