341270 - preferences;1 is a deprecated contract ID

Status: RESOLVED FIXED

(Core Graveyard :: Profile: Roaming, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

"@mozilla.org/preferences;1" refers to nsPref, the class for the deprecated nsIPref interface. callers should use preferences-service when using the newer nsIPrefService/nsIPrefBranch interfaces.

This affects:
/extensions/sroaming/resources/content/prefs/files.js, line 80 -- var prefs = Components.classes["@mozilla.org/preferences;1"]
/extensions/sroaming/resources/content/prefs/all.js, line 147 -- var pref = Components.classes["@mozilla.org/preferences;1"]
/extensions/sroaming/resources/content/prefs/top.js, line 134 -- var prefBranch = Components.classes["@mozilla.org/preferences;1"]

Comment 1

[Joel Nackman]

These classes seem to use the nsIPrefBranch as they are supposed to.  It's not the 
Components.classes["@mozilla.org/preferences;1"] part that matters, "@mozilla.org/preferences;1" implements both the old nsIPref and the newer versions (http://www.xulplanet.com/references/xpcomref/comps/c_preferences1.html)

I'm not completely sure (I got this off the "Good First Bugs" page), but I think that there aren't any problems here.

Comment 2

[Joel Nackman]

Sorry for the double post, I should have read the report more carefully.  I did not see the note about the preferences-service contract id.  Whoops.

Comment 3

[Joel Nackman]

Attachment: Patch

Sorry again about my confusion.

Comment 4

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch Part 1 v1

Patch Part 1 v1

This patches the following:

* /toolkit/content/customizeCharset.js
* /extensions/sroaming/resources/content/prefs/top.js
* /extensions/sroaming/resources/content/prefs/files.js
* /extensions/sroaming/resources/content/prefs/all.js

Patch for the tests coming up.

Comment 5

[Ben Bucksch (:BenB)]

Comment on attachment 242794 [details] [diff] [review]
Patch Part 1 v1

assuming this works, r=BenB

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 242794 [details] [diff] [review]
Patch Part 1 v1

the customizeCharset change is wrong, but sr=biesi on the rest

Comment 7

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Part 1 for checkin. (checked in)

Removed the toolkit changes as apparently it should still use nsIPref for now. Thanks for pointing that out Christian :)

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #1)
> I'm not completely sure (I got this off the "Good First Bugs" page), but I
> think that there aren't any problems here.

The problem is that "@mozilla.org/preferences;1" will go away once nsIPref is killed. There is therefore a problem here.

Comment 9

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch Part 2 v1 (checked in)

Patch v1.

Changing instances of |@mozilla.org/preferences;1| in the js tests too |@mozilla.org/preferences-service;1|. Not sure if this is correct but here is a patch in case its needed.

Comment 10

[Bob Clary [:bc] (inactive)]

Comment on attachment 242802 [details] [diff] [review]
Patch Part 2 v1 (checked in)

sr isn't necessary for the js tests unless you want to ask brendan or mrbkap or igor. This looks good. Thanks. Let me know if you want me to check it in for you.

Comment 11

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #10)
> (From update of attachment 242802 [details] [diff] [review] [edit])
> sr isn't necessary for the js tests unless you want to ask brendan or mrbkap or
> igor. This looks good. Thanks. Let me know if you want me to check it in for
> you.
> 

Thanks and please do. I _don't_ think I missed any but I'll check to be sure later.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 242796 [details] [diff] [review]
Part 1 for checkin. (checked in)

out of curiosity, how did you generate this patch? it doesn't end in a newline, leading to a warning from "patch".

anyway, checked in.

Checking in extensions/sroaming/resources/content/prefs/all.js;
/cvsroot/mozilla/extensions/sroaming/resources/content/prefs/all.js,v  <--  all.js
new revision: 1.15; previous revision: 1.14
done
Checking in extensions/sroaming/resources/content/prefs/files.js;
/cvsroot/mozilla/extensions/sroaming/resources/content/prefs/files.js,v  <--  files.js
new revision: 1.4; previous revision: 1.3
done
Checking in extensions/sroaming/resources/content/prefs/top.js;
/cvsroot/mozilla/extensions/sroaming/resources/content/prefs/top.js,v  <--  top.js
new revision: 1.5; previous revision: 1.4
done

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 242802 [details] [diff] [review]
Patch Part 2 v1 (checked in)

hm, this patch, on the other hand, is fine (and with unix linebreaks)...

checked in too