Closed Bug 1448077 Opened 2 years ago Closed 2 years ago

Move DevTools specific preferences from /modules/libpref/init/all.js to devtools/

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

Details

Attachments

(4 files)

Follow up to RFC https://github.com/devtools-html/rfcs/issues/43

Preferences that should be moved are https://searchfox.org/mozilla-central/search?q=pref(%22devtools&case=false&regexp=false&path=modules%2Flibpref%2Finit%2Fall.js

The goal of this bug is to create a new preferences file either under devtools/preferences or under devtools/server/preferences

We will also fix the consistency of our preferences files by adding /preferences subfolders when missing (namely for /client/webide/webide-prefs.js and /startup/devtools-startup-prefs.js)
Comment on attachment 8965634 [details]
Bug 1448077 - move DevTools prefs from libpref/init/all.js to devtools/shared;

https://reviewboard.mozilla.org/r/234498/#review240082


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/preferences/devtools.js:14
(Diff revision 1)
> +pref("devtools.enabled", true);
> +
> +// Enable deprecation warnings.
> +pref("devtools.errorconsole.deprecation_warnings", true);
> +
> +#ifdef NIGHTLY_BUILD

Error: Parsing error: unexpected character '#' [eslint]
Comment on attachment 8965631 [details]
Bug 1448077 - Move webide preferences file to dedicated /preferences folder;

https://reviewboard.mozilla.org/r/234492/#review240208

Thanks, looks good! :)
Attachment #8965631 - Flags: review?(jryans) → review+
Comment on attachment 8965632 [details]
Bug 1448077 - Move devtools-startup preferences file to dedicated /preferences folder;

https://reviewboard.mozilla.org/r/234494/#review240210
Attachment #8965632 - Flags: review?(jryans) → review+
Comment on attachment 8965634 [details]
Bug 1448077 - move DevTools prefs from libpref/init/all.js to devtools/shared;

https://reviewboard.mozilla.org/r/234498/#review240212

Overall, this seems like it's on the right track; thanks for looking at this! :) I'd like to work out exactly the name and directory we want first, though.

::: commit-message-ebbc5:1
(Diff revision 2)
> +Bug 1448077 - move DevTools prefs from libpref/init/all.js to devtools/.js;r=jryans

Looks like something removed the new file path in this message.

::: devtools/docs/preferences.md:73
(Diff revision 2)
>  
>  ## Create a new preference
>  
>  To create a new preference, it should be assigned a default value. Default preferences are
>  defined in preferences files such as:
> +- devtools/preferences/devtools.js

Calling the file just `devtools.js` worries me a bit that people will end up placing client prefs there on accident.

Could we use a name like `devtools-server.js` or `devtools-shared.js`?

Similarly, would it be better to place this at `devtools/server/preferences` or `devtools/shared/preferences`?  This may also help to highlight its purpose.

::: modules/libpref/init/all.js
(Diff revision 2)
> -// Disable debugging chrome
> -pref("devtools.chrome.enabled", false, sticky);
> -// Disable remote debugging connections
> -pref("devtools.debugger.remote-enabled", false, sticky);
> -// enable JS dump() function.
> -pref("browser.dom.window.dump.enabled", false, sticky);

Do we consider `dump` part of DevTools?  (Looks like I reviewed the patch in bug 1395711 that put this here...)

I would say leave this pref back here in this file with its two versions based on `MOZILLA_OFFICIAL`.

::: modules/libpref/init/all.js
(Diff revision 2)
> -
> -// Used for devtools debugging
> -pref("devtools.dump.emit", false);
> -
> -// Controls whether EventEmitter module throws dump message on each emit
> -pref("toolkit.dump.emit", false);

Looks like someone snuck this toolkit pref in the middle here...  We should likely leave this where it is since it affects a toolkit module, not DevTools.
Attachment #8965634 - Flags: review?(jryans) → review-
Comment on attachment 8965633 [details]
Bug 1448077 - Rename client/preferences/devtools.js to devtools-client.js;

https://reviewboard.mozilla.org/r/234496/#review240216

Thanks, looks reasonable! :)
Attachment #8965633 - Flags: review?(jryans) → review+
Ok to move to server, and thanks for spotting the non-devtools prefs! Can't believe I didn't see those.
Comment on attachment 8965634 [details]
Bug 1448077 - move DevTools prefs from libpref/init/all.js to devtools/shared;

https://reviewboard.mozilla.org/r/234498/#review240236


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/shared/preferences/devtools-shared.js:14
(Diff revision 4)
> +pref("devtools.enabled", true);
> +
> +// Enable deprecation warnings.
> +pref("devtools.errorconsole.deprecation_warnings", true);
> +
> +#ifdef NIGHTLY_BUILD

Error: Parsing error: unexpected character '#' [eslint]
Comment on attachment 8965634 [details]
Bug 1448077 - move DevTools prefs from libpref/init/all.js to devtools/shared;

https://reviewboard.mozilla.org/r/234498/#review240240


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/shared/preferences/devtools-shared.js:14
(Diff revision 3)
> +pref("devtools.enabled", true);
> +
> +// Enable deprecation warnings.
> +pref("devtools.errorconsole.deprecation_warnings", true);
> +
> +#ifdef NIGHTLY_BUILD

Error: Parsing error: unexpected character '#' [eslint]
Comment on attachment 8965634 [details]
Bug 1448077 - move DevTools prefs from libpref/init/all.js to devtools/shared;

https://reviewboard.mozilla.org/r/234498/#review240242

Great, thanks for working on this! :)

::: .eslintignore:158
(Diff revision 4)
>  devtools/shared/qrcode/tests/mochitest/test_decode.html
>  devtools/shared/tests/mochitest/*.html
>  devtools/shared/webconsole/test/test_*.html
>  
>  # Ignore devtools preferences files
> +devtools/preferences/**

I think this should be `devtools/shared` now

::: devtools/docs/preferences.md:73
(Diff revision 4)
>  
>  ## Create a new preference
>  
>  To create a new preference, it should be assigned a default value. Default preferences are
>  defined in preferences files such as:
> +- devtools/shared/preferences/devtools-shared.js

Maybe move this to position 3 in the list, since that matches alphabetical order and also the order its described below?
Attachment #8965634 - Flags: review?(jryans) → review+
Comment on attachment 8965634 [details]
Bug 1448077 - move DevTools prefs from libpref/init/all.js to devtools/shared;

https://reviewboard.mozilla.org/r/234498/#review240242

Thanks for the reviews! Sorry for re-pushing to review a bit too quickly :)

All issues should be fixed now, pushed to try again to make sure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c42ca03089ee0306bc1d36fc9a271874338724d2 I didn't forget anything this time!
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c61caf7df6c7
Move webide preferences file to dedicated /preferences folder;r=jryans
https://hg.mozilla.org/integration/autoland/rev/42b4ccfcb9c4
Move devtools-startup preferences file to dedicated /preferences folder;r=jryans
https://hg.mozilla.org/integration/autoland/rev/a826e8871e9f
Rename client/preferences/devtools.js to devtools-client.js;r=jryans
https://hg.mozilla.org/integration/autoland/rev/456166433c71
move DevTools prefs from libpref/init/all.js to devtools/shared;r=jryans
Looks like init/all.js is loaded using a specific mechanism that ensures its availability on all versions of FF, including the xpcshell binary. See https://searchfox.org/mozilla-central/source/modules/libpref/greprefs.js

Here is a try run loading devtools-shared.js in the same way as init/all.js:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=709656712036f6bbe965a277880d97ba0045b8a7
Flags: needinfo?(jdescottes)
Comment on attachment 8965634 [details]
Bug 1448077 - move DevTools prefs from libpref/init/all.js to devtools/shared;

Implementation changed a bit, so re-flagging for review!
Attachment #8965634 - Flags: review+ → review?(jryans)
Comment on attachment 8965634 [details]
Bug 1448077 - move DevTools prefs from libpref/init/all.js to devtools/shared;

https://reviewboard.mozilla.org/r/234498/#review240784

Ah interesting!  Seems like a fine approach to me. :)
Attachment #8965634 - Flags: review?(jryans) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3473058aec14
Move webide preferences file to dedicated /preferences folder;r=jryans
https://hg.mozilla.org/integration/autoland/rev/9d30a4658d70
Move devtools-startup preferences file to dedicated /preferences folder;r=jryans
https://hg.mozilla.org/integration/autoland/rev/e6fc76e02e4c
Rename client/preferences/devtools.js to devtools-client.js;r=jryans
https://hg.mozilla.org/integration/autoland/rev/c99761c34811
move DevTools prefs from libpref/init/all.js to devtools/shared;r=jryans
Pushed by richard.marti@gmail.com:
https://hg.mozilla.org/comm-central/rev/802aaf131d72
Port bug 1448077 to TB/IB/SM: Rename some preferences files. rs=bustage-fix
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.