When resetting the defaults, we should use clearUserPref

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Downloads API
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: sdwilsh, Assigned: mkohler)

Tracking

Trunk
mozilla1.9.2a1
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

9 years ago
Right now we manually set what we think are the defaults in a few tests, and we should be using clearUserPref.
Product: Firefox → Toolkit
(Reporter)

Updated

9 years ago
Assignee: sdwilsh → nobody
Status: ASSIGNED → NEW
Whiteboard: [good first bug]
(Assignee)

Comment 1

8 years ago
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?
Assignee: nobody → michaelkohler
Status: NEW → ASSIGNED
Attachment #366674 - Flags: review?(sdwilsh)
(Reporter)

Comment 2

8 years ago
There are a bunch of tests here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/
(Reporter)

Comment 3

8 years ago
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)
(Assignee)

Comment 4

8 years ago
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?
(Reporter)

Comment 5

8 years ago
(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.
(Assignee)

Comment 6

8 years ago
oh, now I got it. Gonna do that right know.
(Assignee)

Comment 7

8 years ago
Created attachment 367041 [details] [diff] [review]
Patch v2.0
Attachment #366674 - Attachment is obsolete: true
Attachment #367041 - Flags: review?(sdwilsh)
(Assignee)

Updated

8 years ago
Whiteboard: [good first bug]
(Reporter)

Comment 8

8 years ago
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+
(Assignee)

Comment 9

8 years ago
Created attachment 367842 [details] [diff] [review]
Patch v2.1 (sdwilsh's change)
Attachment #367041 - Attachment is obsolete: true
Attachment #367842 - Flags: review?(sdwilsh)
(Reporter)

Updated

8 years ago
Attachment #367842 - Flags: review?(sdwilsh) → review+
(Reporter)

Comment 10

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

r=sdwilsh
(Assignee)

Updated

8 years ago
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?
(Reporter)

Comment 12

8 years ago
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.
(Reporter)

Comment 14

8 years ago
(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
(Assignee)

Comment 15

8 years ago
Created attachment 368296 [details] [diff] [review]
Patch v3.0

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.
(Assignee)

Comment 17

8 years ago
Created attachment 368323 [details] [diff] [review]
Patch v3.1
Attachment #368296 - Attachment is obsolete: true
Attachment #368323 - Flags: review?(sdwilsh)
Attachment #368296 - Flags: review?(sdwilsh)
(Reporter)

Comment 18

8 years ago
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 19

8 years ago
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 ;)
(Assignee)

Comment 21

8 years ago
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.
Attachment #368323 - Attachment is obsolete: true
Attachment #368360 - Flags: review?(sdwilsh)
(Reporter)

Comment 22

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

r=sdwilsh
Attachment #368360 - Flags: review?(sdwilsh) → review+
(Assignee)

Updated

8 years ago
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?
(Reporter)

Comment 24

8 years ago
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-
(Reporter)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 25

8 years ago
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)
Attachment #368360 - Attachment is obsolete: true
Attachment #368788 - Flags: review?(sdwilsh)
(Assignee)

Updated

8 years ago
Attachment #368788 - Attachment is obsolete: true
Attachment #368788 - Flags: review?(sdwilsh)
(Assignee)

Comment 26

8 years ago
Created attachment 368789 [details] [diff] [review]
Patch v3.4

sdwilsh: you mean like this?
Attachment #368789 - Flags: review?(sdwilsh)
(Reporter)

Comment 27

8 years ago
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-
(Assignee)

Comment 28

8 years ago
Created attachment 369147 [details] [diff] [review]
Patch v3.5
[Checkin: Comment 30]
Attachment #368789 - Attachment is obsolete: true
Attachment #369147 - Flags: review?(sdwilsh)
(Reporter)

Comment 29

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

r=sdwilsh
Attachment #369147 - Flags: review?(sdwilsh) → review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Attachment #369147 - Attachment description: Patch v3.5 → Patch v3.5 [Checkin: Comment 30]
Comment on attachment 369147 [details] [diff] [review]
Patch v3.5
[Checkin: Comment 30]


http://hg.mozilla.org/mozilla-central/rev/665f9854ded3
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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.