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

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect, P1)

Description

[Josh Matthews [:jdm]]

In the per-window PB world, this behaviour is not always correct.

Comment 1

[Josh Matthews [:jdm]]

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.

Comment 2

[(no longer active)]

Attachment: Patch (v1)

Comment 3

[Josh Matthews [:jdm]]

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.

Comment 4

[(no longer active)]

Right, will address your comments when landing.

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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

Comment 7

[(no longer active)]

Attachment: Patch (v2)

Comment 8

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6af3c44e395

Comment 9

[Ed Morley [:emorley]]

Backed out for crashes on most/all platforms:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f6af3c44e395

https://hg.mozilla.org/integration/mozilla-inbound/rev/888109be06e1

Comment 10

[(no longer active)]

Relanded with the null check: https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff599b28e11

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7ff599b28e11