Closed Bug 1272098 Opened 9 years ago Closed 8 years ago

Services.js replacement should load devtools/client/preferences/devtools.js

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Iteration:
51.2 - Aug 29
Tracking Status
firefox51 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Whiteboard: [reserve-html])

Attachments

(1 file)

The implementation of Services.js in bug 1265808 doesn't read the default prefs from devtools/client/preferences/devtools.js. Doing this isn't difficult, but it depends on how exactly in-content loading will work. There are two issues: one, the URL to use for the resource; and two, getting a suitable definition of "prefs" in scope when reading that file. For #2 various options exist, e.g. the webpack import loader: https://github.com/webpack/imports-loader
Also the content loader should make it so require("Services") loads the replacement Services.js.
Whiteboard: [devtools-html] [triage]
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
Priority: P2 → P3
Whiteboard: [devtools-html] → [reserve-html]
Priority: P3 → P2
Maybe it could be done using the "raw!" loader support (see bug 1287915) plus an "eval". There's still the issue of what URL to use though.
The preferences file can be preprocessed and installed using something like this in moz.build: EXTRA_PP_JS_MODULES.devtools.client.preferences += [ 'devtools.js', ] This will stick it in a sort of funny spot; but of course in the content case everything may move around anyhow.
Experimenting with the webpack config revealed a few things: * If we do the webpacking in the build tree, we could just refer directly to the devtools.js we want * We'll need to define |sticky_pref| as well * I hacked Services.js to do this: let defaults = require("raw!devtools/client/shared/shim/devtools.js"); /* eslint-disable no-eval */ eval(defaults); ... however, it would be better for testing not to do this, and instead have some sort of "inspector-main.js", then use the import-loader there to read devtools.js with pref and sticky_pref defined. See https://github.com/webpack/imports-loader
Note that we'll also need to read at least part of all.js, because devtools.js doesn't define all the devtools preferences.
(In reply to Tom Tromey :tromey from comment #5) > Note that we'll also need to read at least part of all.js, because > devtools.js doesn't > define all the devtools preferences. I wonder if we moved all of the "devtools.*" prefs out of all.js and into devtools.js, how many other usages would be remaining. It would be great if we didn't have to process a file out of our devtools/ folder, but if we have to at least for now then that's OK. For the case of 'intl.ellipsis' we should be able to put a localized key in a properties file and fetch it that way instead of using a pref.
(In reply to Brian Grinstead [:bgrins] from comment #6) > (In reply to Tom Tromey :tromey from comment #5) > > Note that we'll also need to read at least part of all.js, because > > devtools.js doesn't > > define all the devtools preferences. > > I wonder if we moved all of the "devtools.*" prefs out of all.js and into > devtools.js, how many other usages would be remaining. It would be great if > we didn't have to process a file out of our devtools/ folder, but if we have > to at least for now then that's OK. I looked using: pokyo. git grep 'Services.prefs.*("' -- devtools/client/| grep -v /test | sed -n -e 's/^.*Services.prefs[^"]*"\([^"]*\)".*$/\1/p'|sort -u|grep -v '^devtools\.' browser.cache.disk.enable browser.dom.window.dump.enabled intl.ellipsis webgl.force-enabled Aside from intl.ellipsis these don't seem to be used by the inspector. > For the case of 'intl.ellipsis' we should be able to put a localized key in > a properties file and fetch it that way instead of using a pref. I'll mention this in the other bug.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 51.2 - Aug 29
Priority: P2 → P1
(In reply to Brian Grinstead [:bgrins] from comment #6) > (In reply to Tom Tromey :tromey from comment #5) > > Note that we'll also need to read at least part of all.js, because > > devtools.js doesn't > > define all the devtools preferences. > > I wonder if we moved all of the "devtools.*" prefs out of all.js and into > devtools.js, how many other usages would be remaining. It would be great if > we didn't have to process a file out of our devtools/ folder, but if we have > to at least for now then that's OK. The devtools.* prefs in all.js are meant to be ones used by the server, while devtools.js is client only. (The client might currently access some of the all.js prefs as well.) If we want to collect all DevTools prefs in devtools.js, that would be okay, as long as we work out a way for the server-only runtimes (Fennec) to load that file.
(In reply to Tom Tromey :tromey from comment #7) > (In reply to Brian Grinstead [:bgrins] from comment #6) > > (In reply to Tom Tromey :tromey from comment #5) > > > Note that we'll also need to read at least part of all.js, because > > > devtools.js doesn't > > > define all the devtools preferences. > > > > I wonder if we moved all of the "devtools.*" prefs out of all.js and into > > devtools.js, how many other usages would be remaining. It would be great if > > we didn't have to process a file out of our devtools/ folder, but if we have > > to at least for now then that's OK. > > I looked using: > > pokyo. git grep 'Services.prefs.*("' -- devtools/client/| grep -v /test | > sed -n -e 's/^.*Services.prefs[^"]*"\([^"]*\)".*$/\1/p'|sort -u|grep -v > '^devtools\.' > browser.cache.disk.enable > browser.dom.window.dump.enabled > intl.ellipsis > webgl.force-enabled In the general case for non-devtools prefs that the client is reading / writing to know things about the target runtime, we should probably be using the preference actor instead, so that they apply to the runtime being debugged. (I haven't actually checked what each of these are used for...)
I finally realized that since the Services shim is only used in content, the URL doesn't matter -- instead we can use a webpack loader to make it do the right thing. So, I wrote a simple "prefs" loader that does the minimal preprocessing necessary; for the time being it just assumes all conditions in devtools.js are false (and just ignores preprocessing in all.js entirely). The resulting use in Services looks like: /* eslint-disable no-eval */ let devtools = require("raw!prefs!devtools/client/preferences/devtools"); eval(devtools); let all = require("raw!prefs!modules/libpref/init/all"); eval(all); /* eslint-enable no-eval */
Comment on attachment 8783555 [details] Bug 1272098 - load default prefs into Services shim; https://reviewboard.mozilla.org/r/73336/#review71090 Looks good to me - just a nit on how to disable default loading ::: devtools/client/shared/shim/Services.js:493 (Diff revision 1) > + > + /** > + * A function for use by tests. If called before Services.prefs is > + * used, this will disable the reading of the default prefs. > */ > - prefs: new PrefBranch(null, "", ""), > + _disableDefaults: function () { Would like these names to be more specific to prefs since there's some other shims here (defaultPrefsEnabled / disableDefaultPrefs) Alternatively, we could just stick a property onto the Services object and get rid of the setter altogether: Services = { defaultPrefsEnabled: true, ... } Then the test could do Services.defaultPrefsEnabled = false.
Attachment #8783555 - Flags: review?(bgrinstead) → review+
Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2c2e88b8bdcf load default prefs into Services shim; r=bgrins
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: