Last Comment Bug 848519 - Move services' preferences.js into Toolkit as Preferences.jsm
: Move services' preferences.js into Toolkit as Preferences.jsm
Status: RESOLVED FIXED
: dev-doc-needed
Product: Toolkit
Classification: Components
Component: General (show other bugs)
: 22 Branch
: All All
: -- enhancement (vote)
: mozilla23
Assigned To: Nick Alexander :nalexander
:
Mentors:
Depends on:
Blocks: 861546
  Show dependency treegraph
 
Reported: 2013-03-06 12:51 PST by Gregory Szorc [:gps]
Modified: 2016-01-22 00:58 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch against m-c (29.63 KB, patch)
2013-04-11 17:53 PDT, Nick Alexander :nalexander
no flags Details | Diff | Splinter Review
Patch against m-c, v2 (29.63 KB, patch)
2013-04-11 17:56 PDT, Nick Alexander :nalexander
gavin.sharp: review+
Details | Diff | Splinter Review

Description Gregory Szorc [:gps] 2013-03-06 12:51:00 PST
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: gavin@gavinsharp.com] 2013-04-01 10:26:10 PDT
(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] 2013-04-09 21:16:23 PDT
Nick might be tackling this as part of FHR on Android.
Comment 3 Nick Alexander :nalexander 2013-04-10 09:27:04 PDT
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: gavin@gavinsharp.com] 2013-04-10 09:44:42 PDT
Sounds good to me!
Comment 5 Nick Alexander :nalexander 2013-04-10 17:23:45 PDT
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: gavin@gavinsharp.com] 2013-04-10 17:39:15 PDT
(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] 2013-04-10 17:54:02 PDT
(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…
Comment 8 Nick Alexander :nalexander 2013-04-10 21:16:32 PDT
Try push running at https://tbpl.mozilla.org/?tree=Try&rev=325391616620
Comment 9 Nick Alexander :nalexander 2013-04-11 11:41:13 PDT
https://tbpl.mozilla.org/?tree=Try&rev=5b3ca2886c83

This time, with proper gre/modules!
Comment 10 Nick Alexander :nalexander 2013-04-11 13:40:51 PDT
Gah, missed one:

http://tbpl.mozilla.org/?tree=Try&rev=43d3c4cec944
Comment 11 Nick Alexander :nalexander 2013-04-11 17:49:49 PDT
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 2013-04-11 17:53:16 PDT
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.
Comment 13 Nick Alexander :nalexander 2013-04-11 17:56:00 PDT
Created attachment 736586 [details] [diff] [review]
Patch against m-c, v2

A tab slipped into toolkit/modules/Makefile.in.  This version removes it.
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-04-12 14:15:26 PDT
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 2013-04-15 11:42:59 PDT
> 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 18 Henrik Skupin (:whimboo) 2016-01-22 00:58:35 PST
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.

Note You need to log in before you can comment on or make changes to this bug.