If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 20

Status

()

Firefox
Downloads Panel
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

Trunk
Firefox 20
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 693002 [details] [diff] [review]
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.
Assignee: nobody → mconley
Attachment #693002 - Flags: review?(mak77)
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)
Attachment #693002 - Flags: review?(mak77) → review+
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
(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
(Assignee)

Updated

5 years ago
Attachment #693002 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 693401 [details] [diff] [review]
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?
Attachment #693401 - Flags: feedback?(mak77)
(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.
(Assignee)

Comment 8

5 years ago
Created attachment 693426 [details] [diff] [review]
Patch v3

Ok, added the comment. I hope I understood the problem correctly - let me know if I should alter it any.
Attachment #693401 - Attachment is obsolete: true
Attachment #693401 - Flags: feedback?(mak77)
Attachment #693426 - Flags: review?(mak77)
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
Attachment #693426 - Flags: review?(mak77)

Updated

5 years ago
Priority: -- → P1
(Assignee)

Comment 10

5 years ago
Created attachment 693879 [details] [diff] [review]
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.
Attachment #693426 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #693879 - Flags: review?(mak77)
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
Attachment #693879 - Flags: review?(mak77) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 694372 [details] [diff] [review]
Patch v5 (r+'d by mak)

Thanks Marco! Comments addressed - here's the final patch.
Attachment #693879 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Summary: Hook up Downloads Panel to use the new Downloads view in Places if preffed on → Switch the browser UI to use the Downloads View in Places if it's preffed on
(Assignee)

Updated

5 years ago
Summary: Switch the browser UI to use the Downloads View in Places if it's preffed on → Use the Downloads View in Places if it's preffed on

Updated

5 years ago
Blocks: 815352
(Assignee)

Comment 13

5 years ago
Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/6bcdc46945a6
https://hg.mozilla.org/mozilla-central/rev/6bcdc46945a6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Depends on: 824345
Depends on: 825040
You need to log in before you can comment on or make changes to this bug.