Downloads Panel should not pop up automatically once per Firefox session

VERIFIED FIXED in Firefox 19

Status

()

Firefox
Downloads Panel
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: mconley)

Tracking

Trunk
Firefox 19
x86_64
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
Once I've told the Downloads panel to go away, it should stay away.  Popping up once per process is arbitrary and weird.
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.

Updated

5 years ago
Blocks: 760733

Updated

5 years ago
Blocks: 747422
No longer blocks: 760733

Updated

5 years ago
Blocks: 760733
Summary: Downloads Panel should not pop up automatically once per Firefox process → Downloads Panel should not pop up automatically once per Firefox session
Created attachment 671938 [details] [diff] [review]
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.
Assignee: nobody → mconley
Created attachment 671943 [details] [diff] [review]
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.
Attachment #671943 - Flags: feedback?(paolo.mozmail)
Attachment #671938 - Flags: review?(mak77)

Comment 4

5 years ago
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.
Attachment #671943 - Flags: feedback?(paolo.mozmail) → feedback+
Comment on attachment 671938 [details] [diff] [review]
Patch v1

Thanks Paolo!

Withdrawing review request to respond to Paolo's feedback.
Attachment #671938 - Flags: review?(mak77)
Created attachment 671984 [details] [diff] [review]
Patch v2

Updated after Paolo's feedback.
Attachment #671938 - Attachment is obsolete: true
Attachment #671984 - Flags: review?(mak77)
Created attachment 671985 [details] [diff] [review]
Smoke test
Attachment #671943 - Attachment is obsolete: true
Attachment #671985 - Flags: review?(mak77)
Blocks: 746756
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//
Attachment #671984 - Flags: review?(mak77) → review+
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.
Attachment #671985 - Flags: review?(mak77) → review+
see bug 746756 too, I feel like we somehow need to unify these prefs.
Created attachment 675356 [details] [diff] [review]
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.
Attachment #671984 - Attachment is obsolete: true
Created attachment 675362 [details] [diff] [review]
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.
Attachment #671985 - Attachment is obsolete: true
Attachment #675356 - Flags: review?(mak77)
Attachment #675362 - Flags: review?(mak77)
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
Attachment #675356 - Flags: review?(mak77) → review+
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.
Attachment #675362 - Flags: review?(mak77) → review+
Created attachment 675590 [details] [diff] [review]
Patch v4 (r+'d by mak)

Thanks - those last fixes are in now.
Attachment #675356 - Attachment is obsolete: true
Created attachment 675591 [details] [diff] [review]
Smoke test v3 (r+'d by mak)
Attachment #675362 - Attachment is obsolete: true
Landed on inbound:

Fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/194dcfbc328d
Test: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a1be51f0a56
https://hg.mozilla.org/mozilla-central/rev/194dcfbc328d
https://hg.mozilla.org/mozilla-central/rev/9a1be51f0a56
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.