replace Services.prefs

RESOLVED FIXED in Firefox 49

Status

P1
enhancement
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

unspecified
Firefox 49
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

2 years ago
Flags: qe-verify-
Priority: -- → P1
(Assignee)

Updated

2 years ago
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
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

Updated

2 years ago
Iteration: --- → 49.1 - May 9
(Assignee)

Comment 2

2 years ago
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.

Updated

2 years ago
Iteration: 49.1 - May 9 → 49.2 - May 23
(Assignee)

Comment 3

2 years ago
Created attachment 8751431 [details]
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/
Attachment #8751431 - Flags: review?(bgrinstead)
(Assignee)

Updated

2 years ago
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.

Updated

2 years ago
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)
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Comment 8

2 years ago
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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/26ff2bb48c6d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.