Closed Bug 1265808 Opened 4 years ago Closed 3 years ago

replace Services.prefs

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
49.3 - Jun 6
Tracking Status
firefox49 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file)

Replace uses of Services.prefs for devtools de-chrome-ification project.
Flags: qe-verify-
Priority: -- → P1
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
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
Iteration: --- → 49.1 - May 9
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.
Iteration: 49.1 - May 9 → 49.2 - May 23
Blocks: 1272098
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
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.
Iteration: 49.2 - May 23 → 49.3 - Jun 6
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
Attachment #8751431 - Flags: review?(bgrinstead)
(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 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/
Attachment #8751431 - Flags: review?(bgrinstead)
Comment on attachment 8751431 [details]
MozReview Request: Bug 1265808 - add Services.prefs shim; r?bgrins

https://reviewboard.mozilla.org/r/52017/#review52320
Attachment #8751431 - Flags: review?(bgrinstead) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/26ff2bb48c6d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.