1265808 - replace Services.prefs

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

Replace uses of Services.prefs for devtools de-chrome-ification project.

Comment 1

[Tom Tromey :tromey]

We discussed this on irc and came to the following conclusions:

* When running in chrome we want to continue using Services.prefs
* When running in content, a wrapper around Web Storage should be ok
* There's no need to try to unify the two stores, or to try to extract
  any current settings from prefs when running in content
* We need some way to extract defaults from devtools.js
* While some prefs don't make sense in content, we're leaving that up to the
  various bits of UI, and not this bug

Comment 2

[Tom Tromey :tromey]

I see a few uses of Services.prefs.getComplexValue to get "intl.ellipsis",.
I think this can be part of bug 1266075.
There are some other uses in webide, but they aren't part of this project yet.

Comment 3

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1265808 - add Services.prefs shim; r?bgrins

Review commit: https://reviewboard.mozilla.org/r/52017/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52017/

Comment 4

[Brian Grinstead [:bgrins]]

I filed an issue on ff-devtools-libs to try and get some consensus on how we can test this out with the debugger.html project (which is currently using the 'sham' prefs module that was made for the initial devtools.html project).  Here's the issue: https://github.com/jlongster/ff-devtools-libs/issues/4#issuecomment-218580566

Comment 5

[Brian Grinstead [:bgrins]]

I'm going to be delayed on this review as I'm going to be unable to get to it next week - sorry!  I do hope we can have some discussion on the github issue about how to sync this over to ff-devtools-libs in the meantime.

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8751431 [details]
MozReview Request: Bug 1265808 - add Services.prefs shim; r?bgrins

https://reviewboard.mozilla.org/r/52017/#review51576

This all seems really sensible.  it's hard to review without a consumer other than the test, but one advantage of that is that we can handle updates as follow ups.

::: devtools/client/shared/shim/Services.js:96
(Diff revision 1)
> +
> +  /**
> +   * Helper function to write the preference's value to local storage
> +   * and then emit a change notification.
> +   */
> +  notify: function () {

This name could be something like persist() or save() or saveAndNotify(), since it does more than just emit the notification

::: devtools/client/shared/shim/Services.js:485
(Diff revision 1)
> +  thePref.setDefault(value);
> +}
> +
> +exports.Services = Services;
> +// Exported for testing purposes.
> +exports._pref = pref;

Says this is exported for testing but I don't see it used anywhere.  Should it be?  Or should this be removed?

::: devtools/client/shared/shim/test/test_service_prefs.html:17
(Diff revision 1)
> +
> +<script type="application/javascript;version=1.8">
> +"use strict";
> +var exports = {};
> +
> + // Add some starter prefs.

Could these defaults be set down next to the call to SpecialPowers.pushPrefEnv so it's all in one place?

::: devtools/client/shared/shim/test/test_service_prefs.html:218
(Diff revision 1)
> +  pref("devtools.branch1.someotherstring", "lazuli bunting");
> +  isDeeply(notifications, {
> +    "someotherstring": true
> +  }, "pref worked");
> +
> +

Guessing this should also clean out localStorage so we don't leak to other tests

Comment 7

[Tom Tromey :tromey]

(In reply to Brian Grinstead [:bgrins] from comment #6)

> > +// Exported for testing purposes.
> > +exports._pref = pref;
> 
> Says this is exported for testing but I don't see it used anywhere.  Should
> it be?  Or should this be removed?

The function is needed for a later bug, when we load devtools.js to install
the default prefs.  I'm not sure whether we'll actually need to export this
at that time, but in the meanwhile the export prevents an eslint complaint.
I've updated the comment to reflect this; however, I could just as easily remove
this function and add it back later on.

> > + // Add some starter prefs.
> 
> Could these defaults be set down next to the call to
> SpecialPowers.pushPrefEnv so it's all in one place?

They have to be there before Services.js is loaded.

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8751431 [details]
MozReview Request: Bug 1265808 - add Services.prefs shim; r?bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52017/diff/1-2/

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8751431 [details]
MozReview Request: Bug 1265808 - add Services.prefs shim; r?bgrins

https://reviewboard.mozilla.org/r/52017/#review52320

Comment 10

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4f6fba0bfca

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/26ff2bb48c6d

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26ff2bb48c6d