Last Comment Bug 442099 - When resetting the defaults, we should use clearUserPref
: When resetting the defaults, we should use clearUserPref
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Download Manager (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla1.9.2a1
Assigned To: Michael Kohler [:mkohler]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-06-26 12:38 PDT by Shawn Wilsher :sdwilsh
Modified: 2009-03-27 20:45 PDT (History)
1 user (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (1.02 KB, patch)
2009-03-10 15:13 PDT, Michael Kohler [:mkohler]
no flags Details | Diff | Splinter Review
Patch v2.0 (2.90 KB, patch)
2009-03-12 07:35 PDT, Michael Kohler [:mkohler]
sdwilsh: review+
Details | Diff | Splinter Review
Patch v2.1 (sdwilsh's change) (2.90 KB, patch)
2009-03-17 13:23 PDT, Michael Kohler [:mkohler]
sdwilsh: review+
Details | Diff | Splinter Review
Patch v3.0 (3.03 KB, patch)
2009-03-19 11:05 PDT, Michael Kohler [:mkohler]
no flags Details | Diff | Splinter Review
Patch v3.1 (3.07 KB, patch)
2009-03-19 12:41 PDT, Michael Kohler [:mkohler]
sdwilsh: review+
Details | Diff | Splinter Review
Patch v3.2 (3.13 KB, patch)
2009-03-19 15:20 PDT, Michael Kohler [:mkohler]
sdwilsh: review-
Details | Diff | Splinter Review
Patch v3.3 (3.11 KB, patch)
2009-03-22 10:02 PDT, Michael Kohler [:mkohler]
no flags Details | Diff | Splinter Review
Patch v3.4 (3.33 KB, patch)
2009-03-22 10:22 PDT, Michael Kohler [:mkohler]
sdwilsh: review-
Details | Diff | Splinter Review
Patch v3.5 [Checkin: Comment 30] (3.47 KB, patch)
2009-03-24 14:07 PDT, Michael Kohler [:mkohler]
sdwilsh: review+
Details | Diff | Splinter Review

Description Shawn Wilsher :sdwilsh 2008-06-26 12:38:19 PDT
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] 2009-03-10 15:13:19 PDT
Created attachment 366674 [details] [diff] [review]
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 2009-03-10 16:20:16 PDT
There are a bunch of tests here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/
Comment 3 Shawn Wilsher :sdwilsh 2009-03-10 22:05:02 PDT
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] 2009-03-11 11:01:49 PDT
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 2009-03-11 13:09:52 PDT
(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] 2009-03-11 13:11:16 PDT
oh, now I got it. Gonna do that right know.
Comment 7 Michael Kohler [:mkohler] 2009-03-12 07:35:23 PDT
Created attachment 367041 [details] [diff] [review]
Patch v2.0
Comment 8 Shawn Wilsher :sdwilsh 2009-03-17 11:06:07 PDT
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] 2009-03-17 13:23:39 PDT
Created attachment 367842 [details] [diff] [review]
Patch v2.1 (sdwilsh's change)
Comment 10 Shawn Wilsher :sdwilsh 2009-03-17 13:27:44 PDT
Comment on attachment 367842 [details] [diff] [review]
Patch v2.1 (sdwilsh's change)

r=sdwilsh
Comment 11 Dão Gottwald [:dao] 2009-03-19 07:15:15 PDT
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 2009-03-19 07:19:40 PDT
Yes, those tests should fail, which is why we set the prefs to a certain value.
Comment 13 Dão Gottwald [:dao] 2009-03-19 07:23:55 PDT
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 2009-03-19 07:34:28 PDT
(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] 2009-03-19 11:05:48 PDT
Created attachment 368296 [details] [diff] [review]
Patch v3.0

okay like that?
Comment 16 Dão Gottwald [:dao] 2009-03-19 12:13:23 PDT
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] 2009-03-19 12:41:42 PDT
Created attachment 368323 [details] [diff] [review]
Patch v3.1
Comment 18 Shawn Wilsher :sdwilsh 2009-03-19 15:06:26 PDT
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 2009-03-19 15:10:52 PDT
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] 2009-03-19 15:15:15 PDT
heh, I know why I dislike that pattern with the dot at the end of the previous line ;)
Comment 21 Michael Kohler [:mkohler] 2009-03-19 15:20:59 PDT
Created attachment 368360 [details] [diff] [review]
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 2009-03-20 14:38:15 PDT
Comment on attachment 368360 [details] [diff] [review]
Patch v3.2

r=sdwilsh
Comment 23 Dão Gottwald [:dao] 2009-03-22 01:54:33 PDT
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 2009-03-22 09:52:14 PDT
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] 2009-03-22 10:02:46 PDT
Created attachment 368788 [details] [diff] [review]
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] 2009-03-22 10:22:36 PDT
Created attachment 368789 [details] [diff] [review]
Patch v3.4

sdwilsh: you mean like this?
Comment 27 Shawn Wilsher :sdwilsh 2009-03-24 13:34:51 PDT
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] 2009-03-24 14:07:52 PDT
Created attachment 369147 [details] [diff] [review]
Patch v3.5
[Checkin: Comment 30]
Comment 29 Shawn Wilsher :sdwilsh 2009-03-24 17:12:49 PDT
Comment on attachment 369147 [details] [diff] [review]
Patch v3.5
[Checkin: Comment 30]

r=sdwilsh
Comment 30 Serge Gautherie (:sgautherie) 2009-03-27 20:44:59 PDT
Comment on attachment 369147 [details] [diff] [review]
Patch v3.5
[Checkin: Comment 30]


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

Note You need to log in before you can comment on or make changes to this bug.