Closed Bug 442099 Opened 13 years ago Closed 12 years ago

When resetting the defaults, we should use clearUserPref

Categories

(Toolkit :: Downloads API, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: sdwilsh, Assigned: mkohler)

Details

Attachments

(1 file, 8 obsolete files)

Right now we manually set what we think are the defaults in a few tests, and we should be using clearUserPref.
Product: Firefox → Toolkit
Assignee: sdwilsh → nobody
Status: ASSIGNED → NEW
Whiteboard: [good first bug]
Attached patch Patch v1.0 (obsolete) — Splinter Review
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?
Assignee: nobody → michaelkohler
Status: NEW → ASSIGNED
Attachment #366674 - Flags: review?(sdwilsh)
Comment on attachment 366674 [details] [diff] [review]
Patch v1.0

comment 2 needs to be addressed before I review this
Attachment #366674 - Flags: review?(sdwilsh)
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?
(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.
oh, now I got it. Gonna do that right know.
Attached patch Patch v2.0 (obsolete) — Splinter Review
Attachment #366674 - Attachment is obsolete: true
Attachment #367041 - Flags: review?(sdwilsh)
Whiteboard: [good first bug]
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
Attachment #367041 - Flags: review?(sdwilsh) → review+
Attached patch Patch v2.1 (sdwilsh's change) (obsolete) — Splinter Review
Attachment #367041 - Attachment is obsolete: true
Attachment #367842 - Flags: review?(sdwilsh)
Attachment #367842 - Flags: review?(sdwilsh) → review+
Comment on attachment 367842 [details] [diff] [review]
Patch v2.1 (sdwilsh's change)

r=sdwilsh
Keywords: checkin-needed
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?
Yes, those tests should fail, which is why we set the prefs to a certain value.
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.
(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).
Keywords: checkin-needed
Attached patch Patch v3.0 (obsolete) — Splinter Review
okay like that?
Attachment #367842 - Attachment is obsolete: true
Attachment #368296 - Flags: review?(sdwilsh)
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.
Attached patch Patch v3.1 (obsolete) — Splinter Review
Attachment #368296 - Attachment is obsolete: true
Attachment #368323 - Flags: review?(sdwilsh)
Attachment #368296 - Flags: review?(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) { }
Attachment #368323 - Flags: review?(sdwilsh) → review+
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?
heh, I know why I dislike that pattern with the dot at the end of the previous line ;)
Attached patch Patch v3.2 (obsolete) — Splinter Review
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.
Attachment #368323 - Attachment is obsolete: true
Attachment #368360 - Flags: review?(sdwilsh)
Comment on attachment 368360 [details] [diff] [review]
Patch v3.2

r=sdwilsh
Attachment #368360 - Flags: review?(sdwilsh) → review+
Keywords: checkin-needed
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 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
Attachment #368360 - Flags: review+ → review-
Keywords: checkin-needed
Attached patch Patch v3.3 (obsolete) — Splinter Review
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)
Attachment #368360 - Attachment is obsolete: true
Attachment #368788 - Flags: review?(sdwilsh)
Attachment #368788 - Attachment is obsolete: true
Attachment #368788 - Flags: review?(sdwilsh)
Attached patch Patch v3.4 (obsolete) — Splinter Review
sdwilsh: you mean like this?
Attachment #368789 - Flags: review?(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
Attachment #368789 - Flags: review?(sdwilsh) → review-
Attachment #368789 - Attachment is obsolete: true
Attachment #369147 - Flags: review?(sdwilsh)
Comment on attachment 369147 [details] [diff] [review]
Patch v3.5
[Checkin: Comment 30]

r=sdwilsh
Attachment #369147 - Flags: review?(sdwilsh) → review+
Keywords: checkin-needed
Attachment #369147 - Attachment description: Patch v3.5 → Patch v3.5 [Checkin: Comment 30]
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.