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

Status: RESOLVED FIXED

(Toolkit :: General, enhancement)

Description

[Gregory Szorc [:gps]]

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!

Comment 1

[:Gavin Sharp [email: [e-mail]]]

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

Comment 2

[Richard Newman [:rnewman]]

Nick might be tackling this as part of FHR on Android.

Comment 3

[Nick Alexander :nalexander [he/him]]

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?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Sounds good to me!

Comment 5

[Nick Alexander :nalexander [he/him]]

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.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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

Comment 7

[Richard Newman [:rnewman]]

(In reply to :Gavin Sharp (use [e-mail] 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…

Comment 8

[Nick Alexander :nalexander [he/him]]

Try push running at https://tbpl.mozilla.org/?tree=Try&rev=325391616620

Comment 9

[Nick Alexander :nalexander [he/him]]

https://tbpl.mozilla.org/?tree=Try&rev=5b3ca2886c83

This time, with proper gre/modules!

Comment 10

[Nick Alexander :nalexander [he/him]]

Gah, missed one:

http://tbpl.mozilla.org/?tree=Try&rev=43d3c4cec944

Comment 11

[Nick Alexander :nalexander [he/him]]

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

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: 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.

Comment 13

[Nick Alexander :nalexander [he/him]]

Attachment: Patch against m-c, v2

A tab slipped into toolkit/modules/Makefile.in.  This version removes it.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 15

[Nick Alexander :nalexander [he/him]]

> 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.

Comment 16

[Nick Alexander :nalexander [he/him]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff628a483c47
https://hg.mozilla.org/mozilla-central/rev/05cf87d0435b
https://hg.mozilla.org/mozilla-central/rev/3b681cdd0bc5

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.