Closed Bug 848519 Opened 7 years ago Closed 7 years ago

Move services' preferences.js into Toolkit as Preferences.jsm

Categories

(Toolkit :: General, enhancement)

22 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: gps, Assigned: nalexander)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

I was using preferences.js today (https://hg.mozilla.org/mozilla-central/file/default/services/common/preferences.js) and figured it might make a good addition to Toolkit.

If Toolkit is interested, I'll code up a patch to land it there.

We may do this in two phases: 1) Add to Toolkit with minor changes 2) Port existing consumers to use the Toolkit version. I haven't looked at the source in a while - maybe the API is already good enough to land!
(In reply to Gregory Szorc [:gps] from comment #0)
> If Toolkit is interested, I'll code up a patch to land it there.

Yes, please.

> I haven't looked at the source in a while - maybe the API is already good enough to
> land!

It's already being used all over the place (including e.g. DOM code: http://hg.mozilla.org/integration/mozilla-inbound/rev/2aaf82b852e7#l14.23), so it's absolutely "good enough to land" as-is.
Nick might be tackling this as part of FHR on Android.
I think it makes sense to move

services/common/preferences.js

into

toolkit/modules/Preferences.jsm

The capital-P and jsm suffix is consistent with toolkit/modules.  Sound good?
Flags: needinfo?(dtownsend+bugmail)
Sounds good to me!
As part of Android FHR, we are likely to ship services-common on Android.  That removes my immediate desire to do this.  I wonder if we should ship services' log4moz.js as well?  I see only one use outside of services/, namely toolkit/modules/Sqlite.jsm.
(In reply to Nick Alexander :nalexander from comment #5)
> As part of Android FHR, we are likely to ship services-common on Android. 
> That removes my immediate desire to do this.

Why not just fix this bug instead? Solves the problem for everyone, rather than extending it.

Log4moz is bug 451283, and I think that one should be trivially fixed as well.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #6)

> Log4moz is bug 451283, and I think that one should be trivially fixed as
> well.

You beat me to it.

Also, I think between Gavin and myself, we can save Mossop another bug comment…
Flags: needinfo?(dtownsend+bugmail)
http://tbpl.mozilla.org/?tree=Try&rev=66574239569d

Final try push to all architectures (previous one was just to Fedora to make sure I unbusted M bc correctly).
Attached patch Patch against m-c (obsolete) — Splinter Review
I removed preferences.js from removed-files.in, but I didn't add Preferences.jsm to any such list because I saw no other toolkit/modules files in such a list.
Assignee: nobody → nalexander
Attachment #736585 - Flags: review?(gavin.sharp)
A tab slipped into toolkit/modules/Makefile.in.  This version removes it.
Attachment #736585 - Attachment is obsolete: true
Attachment #736585 - Flags: review?(gavin.sharp)
Attachment #736586 - Flags: review?(gavin.sharp)
Comment on attachment 736586 [details] [diff] [review]
Patch against m-c, v2

>diff --git a/browser/installer/removed-files.in b/browser/installer/removed-files.in

>-  modules/services-common/preferences.js

removed-files is a list of files that used to be present in the appdir but were since removed - you should essentially never remove entries from it, so don't make this change. (Eventually we can clean this up once we don't care about upgrades from versions where these files existed.)

>diff --git a/services/common/tests/unit/test_preferences.js b/toolkit/modules/tests/xpcshell/test_Preferences.js

Do you know why some tests are commented out? It would be good to reference bugs on file to re-enabled them.
Attachment #736586 - Flags: review?(gavin.sharp) → review+
> Do you know why some tests are commented out? It would be good to reference
> bugs on file to re-enabled them.

I have no idea, and gps doesn't recall.  It's part of a general refactor:

https://bugzilla.mozilla.org/show_bug.cgi?id=731494

I'm going to commit without touching this.
Keywords: dev-doc-needed
Summary: Move services' preferences.js into Toolkit → Move services' preferences.js into Toolkit as Preferences.jsm
It would actually be nice to get this module documented on MDN. Eric, do you think that this can be done somehow in the next time? Looks like the request for it is waiting for more than 2 years now. Thanks.
You need to log in before you can comment on or make changes to this bug.