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

RESOLVED FIXED in Firefox 51

Status

enhancement
P1
normal
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

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

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [reserve-html])

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

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

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

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

Comment 7

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

Updated

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

Comment 10

3 years ago
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 hidden (mozreview-request)

Comment 12

3 years ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 14

3 years ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c2e88b8bdcf
load default prefs into Services shim; r=bgrins

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2c2e88b8bdcf
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

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