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

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

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

Comment 1

[Tom Tromey :tromey]

Also the content loader should make it so require("Services") loads the replacement
Services.js.

Comment 2

[Tom Tromey :tromey]

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.

Comment 3

[Tom Tromey :tromey]

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.

Comment 4

[Tom Tromey :tromey]

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

Comment 5

[Tom Tromey :tromey]

Note that we'll also need to read at least part of all.js, because devtools.js doesn't
define all the devtools preferences.

Comment 6

[Brian Grinstead [:bgrins]]

(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.

Comment 7

[Tom Tromey :tromey]

(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.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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...)

Comment 10

[Tom Tromey :tromey]

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 11

[Tom Tromey :tromey]

Attachment: Bug 1272098 - load default prefs into Services shim;

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

Comment 12

[Brian Grinstead [:bgrins]]

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.

Comment 13

[Tom Tromey :tromey]

Comment on attachment 8783555 [details]
Bug 1272098 - load default prefs into Services shim;

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c2e88b8bdcf
load default prefs into Services shim; r=bgrins

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2c2e88b8bdcf