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

RESOLVED FIXED in mozilla23

Status

()

Toolkit
General
--
enhancement
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: gps, Assigned: nalexander)

Tracking

({dev-doc-needed})

22 Branch
mozilla23
dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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)
Try push running at https://tbpl.mozilla.org/?tree=Try&rev=325391616620
https://tbpl.mozilla.org/?tree=Try&rev=5b3ca2886c83

This time, with proper gre/modules!
Gah, missed one:

http://tbpl.mozilla.org/?tree=Try&rev=43d3c4cec944
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).
Created attachment 736585 [details] [diff] [review]
Patch against m-c

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)
Created attachment 736586 [details] [diff] [review]
Patch against m-c, v2

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+
Blocks: 861546
> 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff628a483c47
https://hg.mozilla.org/integration/mozilla-inbound/rev/05cf87d0435b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b681cdd0bc5
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/ff628a483c47
https://hg.mozilla.org/mozilla-central/rev/05cf87d0435b
https://hg.mozilla.org/mozilla-central/rev/3b681cdd0bc5
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
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.