766263 - Downloads Panel should not pop up automatically once per Firefox session

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Jesse Ruderman]

Once I've told the Downloads panel to go away, it should stay away.  Popping up once per process is arbitrary and weird.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

it's once per session exactly, but I see your point, we just need a way to educate users to it without annoying them, a global counter may work better.

Comment 2

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

Attachment: Patch v1

This patch adds a pref with which we track whether or not the user saw the panel open automatically on the first download. If so, we just show the notification.

As this is a pref, it will persist from session to session.

Comment 3

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

Attachment: Smoke test

Smoke test for this fix.

I'm not 100% happy with it, since it pokes into a private method in order to trigger the notification behaviour. I figured, however, that this was preferable over trying to mock an nsIDownload, load it into the database, etc.

Please let me know if there's a better way of doing that, or anything else I should do to make this test stronger.

Comment 4

[:Paolo Amadini]

Comment on attachment 671943 [details] [diff] [review]
Smoke test

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

(In reply to Mike Conley (:mconley) from comment #3)
> I'm not 100% happy with it, since it pokes into a private method in order to
> trigger the notification behaviour. I figured, however, that this was
> preferable over trying to mock an nsIDownload, load it into the database,
> etc.

I agree that this works fine until we come up with a more complete test suite (probably by mocking the DownloadsData JavaScript object, rather than the back-end, as much as possible).

::: browser/components/downloads/test/browser/browser_first_download_panel.js
@@ +9,5 @@
> + * not open the panel automatically.
> + */
> +function gen_test()
> +{
> +  Components.utils.import("resource:///modules/DownloadsCommon.jsm");

We should either load this in the head file, or load it into a local scope. I tend to prefer the former, since other tests might use the module.

@@ +10,5 @@
> + */
> +function gen_test()
> +{
> +  Components.utils.import("resource:///modules/DownloadsCommon.jsm");
> +  const kFirstPanelPref = "browser.download.firstPanelShown";

General naming suggestion: browser.download.panel.shownOnce ? Or something else in the "panel" namespace, since this doesn't apply to downloads in general but to the panel UI only.

@@ +34,5 @@
> +
> +  } finally {
> +    // Clean up when the test finishes.
> +    for (let yy in gen_resetState()) yield;
> +    Services.prefs.setBoolPref(kFirstPanelPref, kOldFirstPanel);

Let's just clearBoolPref in gen_resetState, so that all tests are consistent across the suite.

Comment 5

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

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

Thanks Paolo!

Withdrawing review request to respond to Paolo's feedback.

Comment 6

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

Attachment: Patch v2

Updated after Paolo's feedback.

Comment 7

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

Attachment: Smoke test

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 671984 [details] [diff] [review]
Patch v2

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

it's fine, but we should brainstorm better naming for the var, I included some options but I'm not extremely happy with them yet.

::: browser/app/profile/firefox.js
@@ +328,5 @@
>  pref("browser.download.panel.removeFinishedDownloads", false);
>  
> +// This records whether or not the panel has been shown for the first download
> +// yet.
> +pref("browser.download.panel.shownOnce", false);

nit: I'd go just for .shown, see for example browser.rights.3.shown for user's rights. It's clear enough, "once" doesn't add anything interesting to its definition.

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +653,5 @@
>    /**
>     * Set to true after the first download in the session caused the downloads
>     * panel to be displayed.
>     */
> +  get firstPanelShown() {

I honestly find this inverted logic puzzling...
when I read "if (firstPanelShown)" I have to think twice what it means, I originally thought was "this is the first time the panel shows", but it's the opposite instead. basically this is notFirstPanelShown :)
Maybe panelFirstShownDone? panelFirstRun (and invert the boolean)? panelNeverShown (inverted)?

@@ +661,5 @@
> +    return false;
> +  },
> +
> +  set firstPanelShown(aValue) {
> +    Services.prefs.setBoolPref("browser.download.panel.shownOnce", aValue);

please always return aValue from setters

@@ +686,2 @@
>        // For new downloads after the first one in the session, don't show the
>        // panel automatically, but provide a visible notification in the topmost

Looks like this comment needs an update, it's no more the session.
likely just s/in the session//

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 671985 [details] [diff] [review]
Smoke test

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

::: browser/components/downloads/test/browser/browser_first_download_panel.js
@@ +27,5 @@
> +    // If we got here, that means the panel opened.
> +    DownloadsPanel.hidePanel();
> +
> +    is(Services.prefs.getBoolPref(kFirstPanelPref), true,
> +       "Should have recorded that the panel was opened on a download.")

is(something, true, message) => ok(something, message);

::: browser/components/downloads/test/browser/head.js
@@ +235,5 @@
> +function waitFor(aMs)
> +{
> +  setTimeout(function() {
> +    testRunner.continueTest();
> +  }, aMs);

I'm not that happy with this, cause in the past timers have demonstrated to be extremely flaky... Though likely there's no other way. considered milliseconds are usually broken (some platform has bad timers resolution, and regardless tinderboxes take seconds to do what we do in milliseconds) I'd say to take seconds as argument instead of ms.

@@ +273,5 @@
> +  DownloadsPanel.onViewLoadCompleted = function () {
> +    DownloadsPanel.onViewLoadCompleted = originalOnViewLoadCompleted;
> +    ok(false, "Should not have opened the downloads panel.");
> +  };
> +}

This looks really specific to the test you are adding, doesn't look fitting into head, I'd probably move it to the test.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

see bug 746756 too, I feel like we somehow need to unify these prefs.

Comment 11

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

Attachment: Patch v3

We're using "shown" instead of "shownOnce". I've also changed the accessors in DownloadsCommon from "firstPanelShown" (which I agree, was ambiguous) to "panelHasShownBefore", which is hopefully clearer.

Comment 12

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

Attachment: Smoke test v2

Updated test per review comments. Also, changed prepareForPanelOpen to wrap the onPopupShown function instead of the onViewLoadCompleted function, since it shouldn't necessarily depend on downloads data being loaded.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

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

::: browser/app/profile/firefox.js
@@ +329,5 @@
>  // This controls retention behavior in the Downloads Panel only.
>  pref("browser.download.panel.removeFinishedDownloads", false);
>  
> +// This records whether or not the panel has been shown for the first download
> +// yet.

let's oneline this as s/for the first download yet./at least once./
just cause this file is big enough to parse visually, so simpler comments are a win.

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +655,1 @@
>     * panel to be displayed.

s/in the session// since this is now global

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 675362 [details] [diff] [review]
Smoke test v2

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

once you land it, please have a look to bc for some runs, the other test in the harness is disabled due to random failures and I'd not want to give more headaches to inbound vikings :)

::: browser/components/downloads/test/browser/browser_first_download_panel.js
@@ +17,5 @@
> +    for (let yy in gen_resetState()) yield;
> +
> +    // With this pref set to false, we should automatically open the panel
> +    // the first time a download is started.
> +    Services.prefs.setBoolPref(kFirstPanelPref, false);

Couldn't you directly call DownloadsCommon.panelHasShownBefore = false; ?

@@ +27,5 @@
> +    // If we got here, that means the panel opened.
> +    DownloadsPanel.hidePanel();
> +
> +    ok(Services.prefs.getBoolPref(kFirstPanelPref),
> +       "Should have recorded that the panel was opened on a download.")

and here as well ok(DownloadsCommon.panelHasShownBefore, ...
If you do so, remember to remove the pref const above

::: browser/components/downloads/test/browser/head.js
@@ +237,5 @@
> + * Spin the event loop for aSeconds seconds, and then signal the test to
> + * continue.
> + *
> + * @param aSeconds the number of seconds to wait.
> + */

just add a @note: this helper should be used _only_ when there's no valid event to listen to and one can't be made.

Comment 15

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

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

Thanks - those last fixes are in now.

Comment 16

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

Attachment: Smoke test v3 (r+'d by mak)

Comment 17

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

Landed on inbound:

Fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/194dcfbc328d
Test: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a1be51f0a56

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/194dcfbc328d
https://hg.mozilla.org/mozilla-central/rev/9a1be51f0a56

Comment 19

[Simona Badau, Desktop QA]

Verified on the latest Nightly that after the first download per Firefox session the Downloads Panel does not pop out automatically (after that the completion of the first download is made through a notification). 
Also, after the first download the preferences browser.download.panel.shown and browser.download.panel.firstSessionCompleted are set to true in about:config. 

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.7:

Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/19.0 Firefox/19.0        
Build ID: 20121112030753
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/19.0 Firefox/19.0        
Build ID: 20121113030658
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:19.0) Gecko/19.0 Firefox/19.0        
Build ID: 20121113030658