Closed Bug 815352 Opened 7 years ago Closed 7 years ago

Make it possible to open the downloads view in the Library window to be opened up inside a tab

Categories

(Firefox :: Downloads Panel, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 20

People

(Reporter: ehsan, Assigned: andreshm)

References

(Blocks 1 open bug)

Details

(Whiteboard: [appcoast])

Attachments

(3 files, 4 obsolete files)

We need this in order to be able to support opening this tab when the user clicks Show All Downloads in a private window in per-window private browsing builds.

This is a small subset of the work involved in bug 697359, and does not involve making the other panes in the Library window to work at all.  In fact we don't even want those panes to be visible for the purposes of this bug.
So fryn tells me that the places download manager uses the browsing history, which is not what we want here, since we need a UI which talks directly to the download manager.  Should we just put the toolkit UI (downloads.xul) in a tab?
Whiteboard: [appcoast]
OS: Mac OS X → All
Hardware: x86 → All
we need bug 675902.
The Library view will have to handle PB still. I still don't get why we hide past history in PB mode, but whatever, the interim solution to use the old toolkit ui in a tab could be fine to remove dependency on bug 675902.
Depends on: 675902
Fwiw, we can reconfigure the new view as we prefer, the current Library downloads view is just a placeholder.
(In reply to comment #2)
> we need bug 675902.
> The Library view will have to handle PB still. I still don't get why we hide
> past history in PB mode, but whatever, the interim solution to use the old
> toolkit ui in a tab could be fine to remove dependency on bug 675902.

Yeah, I'm looking for a solution which we can ship in Firefox 20, so let's go with embedding downloads.xul here for now.
Assignee: nobody → andres
Status: NEW → ASSIGNED
Note that over in bug 801232, I have a set of patches to add support for per-window PB to the download manager, and you might find them useful.
the panel and the manager are totally different things (and code), if we use toolkit ui here there's nothing in bug 801232 that helps, afaict.
(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> (In reply to comment #2)
> > we need bug 675902.
> > The Library view will have to handle PB still. I still don't get why we hide
> > past history in PB mode, but whatever, the interim solution to use the old
> > toolkit ui in a tab could be fine to remove dependency on bug 675902.
> 
> Yeah, I'm looking for a solution which we can ship in Firefox 20, so let's
> go with embedding downloads.xul here for now.

This should be evaluated carefully. Even if the changes that are required to make
the old downloads.xul view compatible with per-window private browsing are not too
many (I don't know), this way we're ending up with two entirely different views
to maintain at the same time, with possibly inconsistent CSS styling, user
interaction, and a different set of bugs to investigate.

The old view is also still using main-thread database access.

We're likely to see progress on the Library view in a few days from now, at that
point it's worth revisiting whether to use the old view, or just use the new view
without hooking it to the Places back-end.
Yeah, I also prefer if we don't end up with two different UIs, but I have no idea how much work is involved in making the Library view shippable, but the amount of work needed to make the toolkit UI shippable is very tractable.  But sure, we can hold on until you guys take a look and have a better estimate on what the work there looks like.
Status: ASSIGNED → NEW
So, the current view has some performance and theming issues we are still working on, but I think it's complete enough to start evaluating private browsing issues with it and try to put it into a tab. Waiting for it to be "perfect" may just delay this excessively.
Mano, could you please explain brefly what needs to be done to put just the new view into a tab and what may be the pitfalls?
Flags: needinfo?(mano)
What do you mean by "just the view"? Without the Library UI?

The new view has ver very little interaction with the code in places.js, so, in theory, you could easily create a new xul file with soem bits from places.xul and it shoud. just work.
Flags: needinfo?(mano)
yes, without the Library UI.
I can write a lthe stub xul file for that if it helps. You'd be able to load it in the browser by its chrome url. Would that help?
Flags: needinfo?(andres)
Sure, thanks!
Flags: needinfo?(andres)
Priority: -- → P1
Not sure it's a good idea to split this in two separate UIs. Implement in-content library direcly would be better.
Comment on attachment 693978 [details] [diff] [review]
prepare the ground

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

>+++ b/browser/components/downloads/content/allDownloadsViewOverlay.css

missing license header


>+richlistitem.download button {
>+  -moz-user-focus: none;
>+}

comment above please.


>diff --git a/browser/components/downloads/content/allDownloadsViewOverlay.xul b/browser/components/downloads/content/allDownloadsViewOverlay.xul
>+<?xml version="1.0"?>

pretty please comment about what this overlay is for and where it's used, since the name is confusing at least let's document it.


>+  <script type="application/javascript"
>+          src="chrome://global/content/contentAreaUtils.js"/>

I don't recall, may this be problematic when in content, since already has it?


>diff --git a/browser/components/downloads/content/downloads.css b/browser/components/downloads/content/downloads.css

>-richlistitem[type="download"]:not([selected]) button {
>-  /* Only focus buttons in the selected item. */
>+/* Only focus buttons in the selected item. */
>+richlistitem[type="download"]:not([selected]) button
>+{

this change looks unrelated and unneeded, please avoid cleanup here


> /*** Visibility of controls inside download items ***/
> 
>-.download-state:-moz-any(     [state="6"], /* Blocked (parental) */
>-                              [state="8"], /* Blocked (dirty)    */
>-                              [state="9"]) /* Blocked (policy)   */
>-                                           .downloadTypeIcon:not(.blockedIcon),
>-
>-.download-state:not(:-moz-any([state="6"], /* Blocked (parental) */
>-                              [state="8"], /* Blocked (dirty)    */
>-                              [state="9"]) /* Blocked (policy)   */)
>-                                           .downloadTypeIcon.blockedIcon,
>-
>-.download-state:not(:-moz-any([state="-1"],/* Starting (initial) */
>-                              [state="5"], /* Starting (queued)  */
>-                              [state="0"], /* Downloading        */
>-                              [state="4"], /* Paused             */
>-                              [state="7"]) /* Scanning           */)
>-                                           .downloadProgress,
>-

> 
> /*** Visibility of download buttons and indicator controls. ***/
> 
>-.download-state:not(:-moz-any([state="-1"],/* Starting (initial) */
>-                              [state="5"], /* Starting (queued)  */
>-                              [state="0"], /* Downloading        */
>-                              [state="4"]) /* Paused             */)
>-                                           .downloadCancel,
>-
>-.download-state:not(:-moz-any([state="2"], /* Failed             */
>-                              [state="3"]) /* Canceled           */)
>-                                           .downloadRetry,
>-
>-.download-state:not(          [state="1"]  /* Finished           */)
>-                                           .downloadShow,
>-

Unless I'm reading it wrongly, these changes look scary and probably some are wrong...
For example these 3 should use visibility hidden and not be collapsed in the panel, using display:none on them would regress a bug. That bug doesn't exist anywhere else (panels are special).
May we avoid touching the panel code at all in this bug?
We spent lot of time on polishing hard to find bugs and these changes may easily cause regressions.


>diff --git a/browser/components/downloads/jar.mn b/browser/components/downloads/jar.mn

>-        content/browser/downloads/download.xml           (content/download.xml)
>-        content/browser/downloads/downloads.css          (content/downloads.css)
>-*       content/browser/downloads/downloads.js           (content/downloads.js)
>-*       content/browser/downloads/downloadsOverlay.xul   (content/downloadsOverlay.xul)
>-        content/browser/downloads/indicator.js           (content/indicator.js)
>-        content/browser/downloads/indicatorOverlay.xul   (content/indicatorOverlay.xul)
>+        content/browser/downloads/download.xml                (content/download.xml)
>+        content/browser/downloads/download.css                (content/download.css)
>+        content/browser/downloads/downloads.css               (content/downloads.css)
>+*       content/browser/downloads/downloads.js                (content/downloads.js)
>+*       content/browser/downloads/downloadsOverlay.xul        (content/downloadsOverlay.xul)
>+        content/browser/downloads/indicator.js                (content/indicator.js)
>+        content/browser/downloads/indicatorOverlay.xul        (content/indicatorOverlay.xul)

These changes are unneeded, let's avoid cleanup for now please, the patch is big enough.


>diff --git a/browser/themes/gnomestripe/jar.mn b/browser/themes/gnomestripe/jar.mn
>--- a/browser/themes/gnomestripe/jar.mn
>+++ b/browser/themes/gnomestripe/jar.mn
>@@ -45,17 +45,18 @@
>   skin/classic/browser/Toolbar-small.png
>   skin/classic/browser/urlbar-arrow.png
>   skin/classic/browser/webRTC-shareDevice-16.png
>   skin/classic/browser/webRTC-shareDevice-64.png
>   skin/classic/browser/downloads/buttons.png          (downloads/buttons.png)
>   skin/classic/browser/downloads/download-glow.png    (downloads/download-glow.png)
>   skin/classic/browser/downloads/download-glow-small.png (downloads/download-glow-small.png)
>   skin/classic/browser/downloads/download-notification.png (downloads/download-notification.png)
>-  skin/classic/browser/downloads/downloads.css        (downloads/downloads.css)
>+  skin/classic/browser/downloads/downloads.css                 (downloads/downloads.css)
>+  skin/classic/browser/downloads/allDownloadsViewOverlay.css   (downloads/allDownloadsViewOverlay.css)

the first change is unneeded
Attachment #693978 - Flags: review?(mak77)
Mano is traveling, so I'll update and push the patch for him.
this just fixes the aero theme jar.mn and some trailing spaces.
Attachment #694257 - Attachment is obsolete: true
Attachment #694257 - Flags: review?(mak77)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eec367668f2
Whiteboard: [appcoast] → [appcoast][leave open]
Attachment #694340 - Attachment is patch: true
Depends on: 822244
Comment on attachment 694340 [details] [diff] [review]
Part 2 - the stub view

>diff -r d08057e095a2 browser/components/downloads/content/allDownloadsViewOverlay.js
>--- a/browser/components/downloads/content/allDownloadsViewOverlay.js	Thu Dec 20 20:48:03 2012 +0900
>+++ b/browser/components/downloads/content/allDownloadsViewOverlay.js	Thu Dec 20 12:32:08 2012 +0000
>@@ -11,16 +11,17 @@
> let Cu = Components.utils;
> let Ci = Components.interfaces;
> let Cc = Components.classes;
> 
> Cu.import("resource://gre/modules/Services.jsm");
> Cu.import("resource://gre/modules/NetUtil.jsm");
> Cu.import("resource://gre/modules/DownloadUtils.jsm");
> Cu.import("resource:///modules/DownloadsCommon.jsm");
>+Cu.import("resource://gre/modules/PlacesUtils.jsm");
> 
> const nsIDM = Ci.nsIDownloadManager;
> 
> const DESTINATION_FILE_URI_ANNO  = "downloads/destinationFileURI";
> const DESTINATION_FILE_NAME_ANNO = "downloads/destinationFileName";
> const DOWNLOAD_STATE_ANNO        = "downloads/state";
> 
> const DOWNLOAD_VIEW_SUPPORTED_COMMANDS =
>diff -r d08057e095a2 browser/components/downloads/content/allDownloadsViewOverlay.xul
>--- a/browser/components/downloads/content/allDownloadsViewOverlay.xul	Thu Dec 20 20:48:03 2012 +0900
>+++ b/browser/components/downloads/content/allDownloadsViewOverlay.xul	Thu Dec 20 12:32:08 2012 +0000
>@@ -16,23 +16,24 @@
>      using the DownloadsView API, and history downloads, using places queries.
>      The view also implements a command controller and a context menu for
>      managing the downloads list.  In order to use this view:
>      1. Apply this overlay to your window.
>      2. Insert in all the overlay entry-points, namely:
>         <richlistbox id="downloadsRichListBox"/>
>         <commandset id="downloadCommands"/>
>         <menupopup id="downloadsContextMenu"/>
>-    3. Make sure your window also has the editMenuOverlay overlay applied,
>+    3. Make sure your window has the editMenuOverlay overlay applied, 
>        because the view implements cmd_copy and cmd_delete.
>-    4. To initialize the view
>+    4. Make sure your window has the globalOverlay.js script loaded.
>+    5. To initialize the view
>         let view = new DownloadsPlacesView(document.getElementById("downloadsRichListBox"));
>         // This is what the Places Library uses. It could be tweaked a bit as long as the
>         // transition-type is set correctly
>-        view.places = "place:transition=7&sort=4";
>+        view.place = "place:transition=7&sort=4";
> -->
> <overlay id="downloadsViewOverlay"
>          xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>          xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> 
>   <script type="application/javascript"
>           src="chrome://browser/content/downloads/allDownloadsViewOverlay.js"/>
>   <script type="application/javascript"
>diff -r d08057e095a2 browser/components/downloads/content/contentAreaDownloadsView.js
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/browser/components/downloads/content/contentAreaDownloadsView.js	Thu Dec 20 12:32:08 2012 +0000
>@@ -0,0 +1,10 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+let ContentAreaDownloadsView = {
>+  init: function CADV_init() {
>+    let view = new DownloadsPlacesView(document.getElementById("downloadsRichListBox"));
>+    view.place = "place:transition=7&sort=4";
>+  }
>+};
>diff -r d08057e095a2 browser/components/downloads/content/contentAreaDownloadsView.xul
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/browser/components/downloads/content/contentAreaDownloadsView.xul	Thu Dec 20 12:32:08 2012 +0000
>@@ -0,0 +1,32 @@
>+<?xml version="1.0"?>
>+
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+<?xml-stylesheet href="chrome://global/skin/"?>
>+<?xul-overlay href="chrome://browser/content/downloads/allDownloadsViewOverlay.xul"?>
>+
>+<?xul-overlay href="chrome://global/content/editMenuOverlay.xul"?>
>+
>+<window id="contentAreaDownloadsView"
>+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+        onload="ContentAreaDownloadsView.init();">
>+
>+  <script type="application/javascript"
>+          src="chrome://global/content/globalOverlay.js"/>
>+  <script type="application/javascript"
>+          src="chrome://browser/content/downloads/contentAreaDownloadsView.js"/>
>+
>+  <commandset id="editMenuCommands"/>
>+
>+  <keyset id="editMenuKeys">
>+#ifdef XP_MACOSX
>+    <key id="key_delete2" keycode="VK_BACK" command="cmd_delete"/>
>+#endif
>+  </keyset>
>+
>+  <richlistbox id="downloadsRichListBox"/>
>+  <commandset id="downloadCommands"/>
>+  <menupopup id="downloadsContextMenu"/>
>+</window>
>diff -r d08057e095a2 browser/components/downloads/jar.mn
>--- a/browser/components/downloads/jar.mn	Thu Dec 20 20:48:03 2012 +0900
>+++ b/browser/components/downloads/jar.mn	Thu Dec 20 12:32:08 2012 +0000
>@@ -8,8 +8,10 @@
>         content/browser/downloads/downloads.css          (content/downloads.css)
> *       content/browser/downloads/downloads.js           (content/downloads.js)
> *       content/browser/downloads/downloadsOverlay.xul   (content/downloadsOverlay.xul)
>         content/browser/downloads/indicator.js           (content/indicator.js)
>         content/browser/downloads/indicatorOverlay.xul   (content/indicatorOverlay.xul)
> *       content/browser/downloads/allDownloadsViewOverlay.xul (content/allDownloadsViewOverlay.xul)
>         content/browser/downloads/allDownloadsViewOverlay.js  (content/allDownloadsViewOverlay.js)
>         content/browser/downloads/allDownloadsViewOverlay.css (content/allDownloadsViewOverlay.css)
>+*       content/browser/downloads/contentAreaDownloadsView.xul (content/contentAreaDownloadsView.xul)
>+        content/browser/downloads/contentAreaDownloadsView.js  (content/contentAreaDownloadsView.js)
Attachment #694340 - Flags: review?(mak77) → review+
Sorry, didn't mean to post the whole patch as comment :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/8881a95c8b2d
Attachment #694311 - Flags: review+
Attachment #694311 - Flags: checkin+
Attachment #694340 - Flags: checkin+
All of the base code (included bug 822244) should be in inbound, so should be possible to start working on this using inbound as code base.
Just in case, I'll be off until next Wednesday, in case the bug is urgent and someone wants to pick it up.
So, just we're on the same page, I think I've done my part in this bug. To test this, load chrome://browser/content/downloads/contentAreaDownloadsView.xul in the browser.

Please let me know if there's anything else I could help with.
Attached patch Part 3 - Open view in a tab (obsolete) — Splinter Review
Opening view in a tab for private windows, not sure if this should be the same for all windows.
Attachment #695848 - Flags: review?(mak77)
Comment on attachment 695848 [details] [diff] [review]
Part 3 - Open view in a tab

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

The opening code looks almost fine.

is the theme fine already for the in-content view? do we need further adjustements to make it look nice?
Is the functionality working properly and properly showing only private downloads in private windows?

::: browser/components/downloads/src/DownloadsUI.js
@@ +138,5 @@
> +        "chrome://browser/content/downloads/contentAreaDownloadsView.xul";
> +      aWindowContext.gBrowser.selectedTab =
> +        aWindowContext.gBrowser.addTab(DOWNLOAD_VIEW_URL);
> +      return;
> +    }

It's possible this is invoked from a non-browser window, so just in case you should check if gBrowser is defined, otherwise get the most recent browser window and open the tab there (use RecentWindow.getMostRecentBrowserWindow)
Attachment #695848 - Flags: review?(mak77)
(In reply to Marco Bonardo [:mak] (intermittently avail. until 3 Jan) from comment #30)
> ::: browser/components/downloads/src/DownloadsUI.js
> @@ +138,5 @@
> > +        "chrome://browser/content/downloads/contentAreaDownloadsView.xul";
> > +      aWindowContext.gBrowser.selectedTab =
> > +        aWindowContext.gBrowser.addTab(DOWNLOAD_VIEW_URL);
> > +      return;
> > +    }
> 
> It's possible this is invoked from a non-browser window, so just in case you
> should check if gBrowser is defined, otherwise get the most recent browser
> window and open the tab there (use RecentWindow.getMostRecentBrowserWindow)

Actually, use openUILinkIn from utilityOverlay.js.
Attached patch Part 3 - Open view in a tab (obsolete) — Splinter Review
> is the theme fine already for the in-content view? do we need further
> adjustements to make it look nice?
It looks good on Mac

> Is the functionality working properly and properly showing only private
> downloads in private windows?
I think the idea is that in private windows, it shows the public downloads plus the private ones. But in public windows, it shows only the public downloads.
Attachment #695848 - Attachment is obsolete: true
Attachment #696148 - Flags: feedback?(mak77)
Comment on attachment 696148 [details] [diff] [review]
Part 3 - Open view in a tab

the panel works differently and I think we wanted to move this in content to show only private downloads in private windows... let's ask Ehsan for clarifying any doubt on the wanted behavior.
Attachment #696148 - Flags: feedback?(ehsan)
Ehsan's out for the rest of the year, but our plans were to have the in-content view show only the private downloads in private windows.
Please let me know the exact spec and I'll fix the view to support that.

Particularly, there are *three* types of downloads:
(a) Public "session" downloads
(b) Private "session" downloads
(c) History downloads

So, do you suggest that in a private-browsing window only (b) downloads should be visible?
FWIW Google Chrome shows both public and private downloads in private browsing, but with a clear indication about what is private.
(In reply to Mano from comment #35)
> Please let me know the exact spec and I'll fix the view to support that.
> 
> Particularly, there are *three* types of downloads:
> (a) Public "session" downloads
> (b) Private "session" downloads
> (c) History downloads
> 
> So, do you suggest that in a private-browsing window only (b) downloads
> should be visible?

Yes, (b) is the desired behavior.
(I'll try to look at this patch later today...)
In case I'm not around and Mano is, feel free to ask him. I'm traveling in the next days.
Blocks: 825295
Comment on attachment 696148 [details] [diff] [review]
Part 3 - Open view in a tab

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

This is basically all we need in this bug.  Any future theme changes and such can be addressed in follow-up bugs.  f=me with Marco's and Dao's comments.

I filed bug 825295 to make this view show only private downloads when opened in a private window, but let's land this patch first and address that issue in bug 825295.
Attachment #696148 - Flags: feedback?(ehsan) → feedback+
Whiteboard: [appcoast][leave open] → [appcoast]
With review comments addressed.  Asking for review from mak, mano and mconley to see who can get to this first.
Attachment #696148 - Attachment is obsolete: true
Attachment #696148 - Flags: feedback?(mak77)
Attachment #696528 - Flags: review?(mconley)
Attachment #696528 - Flags: review?(mano)
Attachment #696528 - Flags: review?(mak77)
Also filed bug 825454 in order to make this pane a proper in-content UI.
Attachment #696528 - Flags: review?(dao)
So in non-private windows we'll still open the organizer?
Comment on attachment 696528 [details] [diff] [review]
Part 3 - Open view in a tab

>+    // If we weren't given a window context, try to find a browser window
>+    // to use as our parent - and if that doesn't work, error out and give up.
>+    let parentWindow = aWindowContext;
>+    if (!parentWindow) {
>+      parentWindow = RecentWindow.getMostRecentBrowserWindow();

>+    // If window is private then show it in a tab.
>+    if (PrivateBrowsingUtils.isWindowPrivate(parentWindow)) {
>+      const DOWNLOAD_VIEW_URL =
>+        "chrome://browser/content/downloads/contentAreaDownloadsView.xul";
>+      parentWindow.openUILinkIn(DOWNLOAD_VIEW_URL, "tab");
>+      return;

In what cases is aWindowContext missing? Making this behavior depend semi-randomly on some browser window that you happen to have focused most recently doesn't seem ideal.
Into what windows DownloadsUI.js is loaded? Isn't it just browser.js?
(In reply to Mano from comment #43)
> So in non-private windows we'll still open the organizer?

Correct.

(In reply to Dão Gottwald [:dao] from comment #44)
> Comment on attachment 696528 [details] [diff] [review]
> Part 3 - Open view in a tab
> 
> >+    // If we weren't given a window context, try to find a browser window
> >+    // to use as our parent - and if that doesn't work, error out and give up.
> >+    let parentWindow = aWindowContext;
> >+    if (!parentWindow) {
> >+      parentWindow = RecentWindow.getMostRecentBrowserWindow();
> 
> >+    // If window is private then show it in a tab.
> >+    if (PrivateBrowsingUtils.isWindowPrivate(parentWindow)) {
> >+      const DOWNLOAD_VIEW_URL =
> >+        "chrome://browser/content/downloads/contentAreaDownloadsView.xul";
> >+      parentWindow.openUILinkIn(DOWNLOAD_VIEW_URL, "tab");
> >+      return;
> 
> In what cases is aWindowContext missing? Making this behavior depend
> semi-randomly on some browser window that you happen to have focused most
> recently doesn't seem ideal.

nsIDownloadManagerUI::Show can be called with a null window argument.  This is supposed to signal the implementation that the caller does not care which window would be the parent.  The nsDownloadManager code passes null, among other things: <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#2487>
(In reply to Ehsan Akhgari [:ehsan] (Mostly away until Jan 2) from comment #47)
> nsIDownloadManagerUI::Show can be called with a null window argument.  This
> is supposed to signal the implementation that the caller does not care which
> window would be the parent.  The nsDownloadManager code passes null, among
> other things:
> <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/
> nsDownloadManager.cpp#2487>

Which is alright if there's just one UI, but not in the new world when we show a different UI based on the parent window's privacy status.
(In reply to Dão Gottwald [:dao] from comment #48)
> (In reply to Ehsan Akhgari [:ehsan] (Mostly away until Jan 2) from comment
> #47)
> > nsIDownloadManagerUI::Show can be called with a null window argument.  This
> > is supposed to signal the implementation that the caller does not care which
> > window would be the parent.  The nsDownloadManager code passes null, among
> > other things:
> > <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/
> > nsDownloadManager.cpp#2487>
> 
> Which is alright if there's just one UI, but not in the new world when we
> show a different UI based on the parent window's privacy status.

My patch in bug 810208 makes sure that we open the correct UI based on the privacy status of the download in question.  Note that we'll need both of these bugs to land.
Comment on attachment 696528 [details] [diff] [review]
Part 3 - Open view in a tab

>+    // If we weren't given a window context, try to find a browser window
>+    // to use as our parent - and if that doesn't work, error out and give up.
>+    let parentWindow = aWindowContext;
>+    if (!parentWindow) {
>+      parentWindow = RecentWindow.getMostRecentBrowserWindow();
>       if (!parentWindow) {
>+        Components.utils.reportError(
>+          "Couldn't find a browser window to open the Places Downloads View " +
>+          "from.");
>+        return;

Seems like we should just open a new window in that case. openUILinkIn does this automatically, but the script it's part of (utilityOverlay.js) needs to be loaded in a window (not necessarily a browser window).
Attachment #696528 - Flags: review?(dao) → review+
Comment on attachment 696528 [details] [diff] [review]
Part 3 - Open view in a tab

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

Looks good to me. Thanks Ehsan!
Attachment #696528 - Flags: review?(mconley) → review+
(In reply to Dão Gottwald [:dao] from comment #50)
> Comment on attachment 696528 [details] [diff] [review]
> Part 3 - Open view in a tab
> 
> >+    // If we weren't given a window context, try to find a browser window
> >+    // to use as our parent - and if that doesn't work, error out and give up.
> >+    let parentWindow = aWindowContext;
> >+    if (!parentWindow) {
> >+      parentWindow = RecentWindow.getMostRecentBrowserWindow();
> >       if (!parentWindow) {
> >+        Components.utils.reportError(
> >+          "Couldn't find a browser window to open the Places Downloads View " +
> >+          "from.");
> >+        return;
> 
> Seems like we should just open a new window in that case. openUILinkIn does
> this automatically, but the script it's part of (utilityOverlay.js) needs to
> be loaded in a window (not necessarily a browser window).

I'll do that in my patch to bug 810208.
Attachment #696528 - Flags: review?(mano)
Attachment #696528 - Flags: review?(mak77)
And also a follow-up to use about:downloads:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e79a86b4c1c
https://hg.mozilla.org/mozilla-central/rev/9aa424d5a92f
https://hg.mozilla.org/mozilla-central/rev/8e79a86b4c1c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Verified as fixed on the latest Nighlty - if Firefox is in Private Browsing mode, the downloads view from the Library window is opened inside a new tab (named Downloads).

Verified on Windows 7, Ubuntu 12.10 and Mac OS X 10.8.2:
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20130102 Firefox/20.0 Build ID: 20130102030907
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20130103 Firefox/20.0 Build ID: 20130103030946
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20130103 Firefox/20.0 Build ID: 20130103030946
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.