Closed Bug 810208 Opened 7 years ago Closed 7 years ago

Clicking the Downloads Finished alert box always brings up public download list

Categories

(Toolkit :: Downloads API, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: jdm, Assigned: ehsan)

References

Details

Attachments

(1 file, 1 obsolete file)

In the per-window PB world, this behaviour is not always correct.
We should pass a "cookie" of "public" or "private" to the showAlertNotification call in nsDownloadManager so that the alert listener can properly act on it.
Blocks: PBnGen
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #696564 - Flags: review?(mak77)
Comment on attachment 696564 [details] [diff] [review]
Patch (v1)

>     //TODO: This doens't make sense when clicking a notification related to
>     //      private downloads when per-window mode is enabled. (bug 810208)

Scrap this comment while you're in here.

>+++ b/toolkit/components/downloads/nsDownloadProxy.h
>@@ -65,17 +65,17 @@ public:
> 
>       bool focusWhenStarting = true;
>       if (branch)
>         (void)branch->GetBoolPref(PREF_BDM_FOCUSWHENSTARTING, &focusWhenStarting);
> 
>       if (visible && !focusWhenStarting)
>         return NS_OK;
> 
>-      return dmui->Show(nullptr, id, nsIDownloadManagerUI::REASON_NEW_DOWNLOAD);
>+      return dmui->Show(nullptr, id, nsIDownloadManagerUI::REASON_NEW_DOWNLOAD, false);

This should pass aIsPrivate.
Right, will address your comments when landing.
Priority: -- → P1
Comment on attachment 696564 [details] [diff] [review]
Patch (v1)

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

Please get a SR for the API change.

::: browser/components/downloads/src/DownloadsUI.js
@@ +117,5 @@
>      // 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({private: aPrivateDownloadUI});

please whitespace onlined objects like { key: value }

Also, since it's optional I'd like an explicit boolean cast like !!aPrivateDownloadUI

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +2485,5 @@
>      nsCOMPtr<nsIDownloadManagerUI> dmui =
>        do_GetService("@mozilla.org/download-manager-ui;1", &rv);
>      NS_ENSURE_SUCCESS(rv, rv);
> +    return dmui->Show(nullptr, 0, nsIDownloadManagerUI::REASON_USER_INTERACTED,
> +                      nsCRT::strcmp(aData, NS_LITERAL_STRING("private").get()) == 0);

bug 791546 and bug 798840 make me think this is not fine, please use NS_strcmp (and remove the nsCTR.h include)

::: toolkit/components/downloads/nsIDownloadManagerUI.idl
@@ +31,5 @@
>    *        REASON_USER_INTERACTED, and should be one of the previously listed
>    *        constants.
> +  * @param [optional] aPrivateDownloadUI
> +  *        True if the UI specific to private downloads should be shown.
> +  *        This parameter will be ignored if aWindowContext is not null.

The comment may be clearer by specifying that in such a case the private browsing status is gathered from the window context.
I'd also probably call this just aUsePrivateUI (please search & replace in the patch).

It's a pity we can't coalesce the pb status in a single argument, but I don't see an escape from this solution.
Attachment #696564 - Flags: review?(mak77) → review+
Attachment #696564 - Flags: superreview?(gavin.sharp)
Comment on attachment 696564 [details] [diff] [review]
Patch (v1)

>diff --git a/toolkit/components/downloads/nsIDownloadManagerUI.idl b/toolkit/components/downloads/nsIDownloadManagerUI.idl

>+  * @param [optional] aPrivateDownloadUI
>+  *        True if the UI specific to private downloads should be shown.

"True if the download intended to be revealed is a private download."? "the UI specific to private downloads" is not very well defined, and seems likely to be very app-specific. Makes me wonder whether this should just be an nsIDownload reference instead, so that show() can do what it likes depending on the download's state and all the PB-specific behavior can just live there.

>+  *        This parameter will be ignored if aWindowContext is not null.

This seems like an odd restriction to bake in to the interface (very specific to Firefox's current implementation).
Attached patch Patch (v2)Splinter Review
Attachment #696564 - Attachment is obsolete: true
Attachment #696564 - Flags: superreview?(gavin.sharp)
Attachment #697131 - Flags: superreview?(gavin.sharp)
Attachment #697131 - Flags: superreview?(gavin.sharp) → superreview+
https://hg.mozilla.org/mozilla-central/rev/7ff599b28e11
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Blocks: 833015
You need to log in before you can comment on or make changes to this bug.