442099 - When resetting the defaults, we should use clearUserPref

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Shawn Wilsher :sdwilsh]

Right now we manually set what we think are the defaults in a few tests, and we should be using clearUserPref.

Comment 1

[Michael Kohler [:mkohler]]

Attachment: Patch v1.0

I've just found 1 test that is wrong in http://hg.mozilla.org/mozilla-central/file/42cc5b6b32e8/toolkit/components/downloads/test/ .

Are there more?

Comment 2

[Shawn Wilsher :sdwilsh]

There are a bunch of tests here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 366674 [details] [diff] [review]
Patch v1.0

comment 2 needs to be addressed before I review this

Comment 4

[Michael Kohler [:mkohler]]

I don't find any set*Pref(..) where the value get set back to default. All these are used for a specific action. Any hint?

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> I don't find any set*Pref(..) where the value get set back to default. All
> these are used for a specific action. Any hint?
There's a bunch of tests that set prefs and then don't set things back to how they were AFAIK.  Perhaps I'm wrong, but that's why this bug was filed in the first place because a bunch of tests did this.

Comment 6

[Michael Kohler [:mkohler]]

oh, now I got it. Gonna do that right know.

Comment 7

[Michael Kohler [:mkohler]]

Attachment: Patch v2.0

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 367041 [details] [diff] [review]
Patch v2.0

>+  Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch).
>+    clearUserPref("browser.download.manager.closeWhenDone");
nit: newline for getService, and do not indent the two spaces on the following lines

r=sdwilsh

Comment 9

[Michael Kohler [:mkohler]]

Attachment: Patch v2.1 (sdwilsh's change)

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 367842 [details] [diff] [review]
Patch v2.1 (sdwilsh's change)

r=sdwilsh

Comment 11

[Dão Gottwald [:dao]]

Are these tests expected to fail in the case that browser.download.manager.retention or browser.download.manager.closeWhenDone should ever default to a different value?

Comment 12

[Shawn Wilsher :sdwilsh]

Yes, those tests should fail, which is why we set the prefs to a certain value.

Comment 13

[Dão Gottwald [:dao]]

Well, so the actual tests won't fail, since the prefs are set to a certain value, but clearUserPref will throw if that certain value happens to be the default value.

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> Well, so the actual tests won't fail, since the prefs are set to a certain
> value, but clearUserPref will throw if that certain value happens to be the
> default value.
That's right, I forgot that our preferences API was not sane.  Michael, can you wrap the calls to clearUserPref in a try catch block please (silently ignoring the failure).

Comment 15

[Michael Kohler [:mkohler]]

Attachment: Patch v3.0

okay like that?

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 368296 [details] [diff] [review]
Patch v3.0

>+  try {
>+    prefs.clearUserPref("browser.download.manager.retention");
>+    prefs.clearUserPref("browser.download.manager.closeWhenDone");
>+  }
>+  catch (err) {
>+  }

You need to try and catch them separately, otherwise the second user pref won't be cleared if the first fails.

Comment 17

[Michael Kohler [:mkohler]]

Attachment: Patch v3.1

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 368323 [details] [diff] [review]
Patch v3.1

r=sdwilsh, but please just have the catch and it's braces on one line:
} catch(e) { }

Comment 19

[Simon Bünzli]

Comment on attachment 368323 [details] [diff] [review]
Patch v3.1

>     Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch).
>-    setBoolPref(PREF_BDM_CLOSEWHENDONE, false);
>+    try {    
>+      clearUserPref(PREF_BDM_CLOSEWHENDONE);

Does that test really pass?

Comment 20

[Dão Gottwald [:dao]]

heh, I know why I dislike that pattern with the dot at the end of the previous line ;)

Comment 21

[Michael Kohler [:mkohler]]

Attachment: Patch v3.2

Corrected version. Now there should not be any further mistakes.

@Simon: huh, that wouldn't have run I guess.. currently this test is disabled anyway: http://hg.mozilla.org/mozilla-central/file/5e53b13462c4/toolkit/components/downloads/test/browser/Makefile.in#l48

@Dao: indeed.

Comment 22

[Shawn Wilsher :sdwilsh]

Comment on attachment 368360 [details] [diff] [review]
Patch v3.2

r=sdwilsh

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 368360 [details] [diff] [review]
Patch v3.2

>--- a/toolkit/mozapps/downloads/tests/chrome/test_retention_is_0_closes.xul
>+++ b/toolkit/mozapps/downloads/tests/chrome/test_retention_is_0_closes.xul
>@@ -190,16 +190,29 @@ function test()
>   // Start the test when the download manager window loads
>   let ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
>            getService(Ci.nsIWindowWatcher);
>   ww.registerNotification(obs);
> 
>   addDownload();
> 
>   SimpleTest.waitForExplicitFinish();
>+
>+  let prefs = Cc["@mozilla.org/preferences-service;1"].
>+              getService(Ci.nsIPrefService);
>+  try {
>+    prefs.clearUserPref("browser.download.manager.retention");
>+  }
>+  catch (err) { }
>+  try {
>+    prefs.clearUserPref("browser.download.manager.closeWhenDone");
>+  }
>+  catch (err) { }
>+
>+  SimpleTest.finish();
> }

Err, what's the finish() doing there?

Comment 24

[Shawn Wilsher :sdwilsh]

Comment on attachment 368360 [details] [diff] [review]
Patch v3.2

(In reply to comment #23)
> Err, what's the finish() doing there?
Shame on me for missing that, and missing the point that that actually makes the test useless.

That bit of code should be here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_retention_is_0_closes.xul#109

Comment 25

[Michael Kohler [:mkohler]]

Attachment: Patch v3.3

No, shame on me.

(haven't tested the latest patch 'cause I can't build a m-c build right now. I get this error on a clean m-c repository with the latest changesets: http://pastebin.mozilla.org/636108)

Comment 26

[Michael Kohler [:mkohler]]

Attachment: Patch v3.4

sdwilsh: you mean like this?

Comment 27

[Shawn Wilsher :sdwilsh]

Comment on attachment 368789 [details] [diff] [review]
Patch v3.4

>+++ b/toolkit/mozapps/downloads/tests/chrome/test_ui_stays_open_on_alert_clickback.xul
This file has the same problem

Comment 28

[Michael Kohler [:mkohler]]

Attachment: Patch v3.5 [Checkin: Comment 30]

Comment 29

[Shawn Wilsher :sdwilsh]

Comment on attachment 369147 [details] [diff] [review]
Patch v3.5
[Checkin: Comment 30]

r=sdwilsh

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 369147 [details] [diff] [review]
Patch v3.5
[Checkin: Comment 30]


http://hg.mozilla.org/mozilla-central/rev/665f9854ded3