Closed Bug 1365891 Opened 7 years ago Closed 7 years ago

Avoid using preprocessing in devtools preference files

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 55

People

(Reporter: ochameau, Assigned: jdescottes)

References

Details

Attachments

(1 file)

Bug 1365891 - interpret preprocessing instructions in DevTools addon bootstrap;
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
Preference files still use preprocessing
But this end up with unexpected when devtools run as an add-on. With should fine an alternative to that:
http://searchfox.org/mozilla-central/source/devtools/client/preferences
Summary: Avoir using preprocessing in devtools preference files → Avoid using preprocessing in devtools preference files
Ok, I'm going to work through this all in this doc: https://docs.google.com/document/d/1c24FW5PgdU7F1mDFqk73nLT3SsTFL7XpV6hRjdWOajE/edit?usp=sharing
What about setting all those prefs to their "release" value and loading an additional preferences file depending on the update channel in bootstrap.js :

`function setPrefs() {
  processPrefFile(resourceURI.spec + "./client/preferences/devtools.js");
  processPrefFile(resourceURI.spec + "./client/preferences/devtools-" + system.constants.MOZ_UPDATE_CHANNEL + ".js");
  processPrefFile(resourceURI.spec + "./client/preferences/debugger.js");
  processPrefFile(resourceURI.spec + "./client/webide/webide-prefs.js");
}`

We could either wait for the switch day to do that, or start preparing by loading those different files with the moz.build.
(In reply to Julian Descottes [:jdescottes] from comment #2)
> What about setting all those prefs to their "release" value and loading an
> additional preferences file depending on the update channel in bootstrap.js :

Looks good, we have to be careful about the pref files order.

> We could either wait for the switch day to do that, or start preparing by
> loading those different files with the moz.build.

It is better to limit the number of changes we have to land at once.
So anything that doesn't disturb/regress/slow down current devtools setup should be landed.
Attachment #8873473 - Flags: review?(poirot.alex)
Comment on attachment 8873473 [details]
Bug 1365891 - interpret preprocessing instructions in DevTools addon bootstrap;

Brian, could you tell me what you think about this patch? 

Some context first: we want to remove preprocessing instructions from all our preferences files. Long term we will rethink how we flag features for our release channels (some might make sense for a given firefox channel, some might make sense for a devtools channel). But for now the goal is to be functionally equivalent to what we are doing today.

The prefs behind preprocessing are:
- devtools.debugger.new-debugger-frontend
- devtools.webconsole.new-frontend-enabled
- devtools.devedition.promo.enabled
- devtools.performance.ui.experimental
- devtools.theme

I removed them from devtools.js/debugger.js and created 4 "channel" preferences files:
- devtools-default
- devtools-nightly
- devtools-devedition
- devtools-beta

(I could have left the default values in the initial files, but I felt it would be clearer to maintain if they were only defined in those dedicated preferences files).

I would like to make sure the conditions I use to check the channels make sense and that I'm not missing obscure channels that should be considered as nightly/beta/devedition (worst case scenario they would get the "default" pref).

Let me know what you think,
Thanks!
Attachment #8873473 - Flags: review?(poirot.alex) → feedback?(bgrinstead)
(In reply to Julian Descottes [:jdescottes] from comment #8)
> Comment on attachment 8873473 [details]
> Bug 1365891 - remove preprocessing from DevTools preferences files
>
> (I could have left the default values in the initial files, but I felt it
> would be clearer to maintain if they were only defined in those dedicated
> preferences files).

My reaction by looking at patch first, before reading this comment was to
suggest getting rid of devtools-default.js and consider all prefs to be release/default values
and see the devtools-*.js pref only as overrides.
The advantage of them being overrides is that it is less prone to error of having undefined pref.
If you happen to add a pref only to devtools-default.js for example, it wouldn't be set on all other channels.

Then I would name these files overrides-$(channel-name). Or channel-$(channel-name), or build-$(channel-name), or ...

Otherwise the patch looks good.
Comment on attachment 8873473 [details]
Bug 1365891 - interpret preprocessing instructions in DevTools addon bootstrap;

https://reviewboard.mozilla.org/r/144846/#review149210

::: browser/installer/package-manifest.in:645
(Diff revision 4)
>  @RESPATH@/browser/chrome/devtools.manifest
>  @RESPATH@/browser/@PREF_DIR@/devtools.js
>  @RESPATH@/browser/@PREF_DIR@/debugger.js
> +#if defined(NIGHTLY_BUILD)
> +@RESPATH@/browser/@PREF_DIR@/devtools-nightly.js
> +#elif defined(MOZ_DEV_EDITION)

I'm not an expert in preprocessing,
but I don't see any occurence of this form, instead there is planty of:
#ifdef MOZ_DEV_EDITION
May be NIGTLY_BUILD could be using this form as there is planty of it as well:
#ifdef NIGHTLY_BUILD
Comment on attachment 8873473 [details]
Bug 1365891 - interpret preprocessing instructions in DevTools addon bootstrap;

We discussed this in person and the takeaway was to modify processPrefFile to be aware of the conditionals, similarly to how we are processing pref files with webpack
Attachment #8873473 - Flags: feedback?(bgrinstead)
Note that if that can help, you can mode this code to some module, i.e. outside of bootstrap.js
For example, early in /devtools/client/framework/devtools.js or devtools-browser.js.
You would only have to ensure kicking of this code only when loading it from the add-on for now.
Comment on attachment 8873473 [details]
Bug 1365891 - interpret preprocessing instructions in DevTools addon bootstrap;

https://reviewboard.mozilla.org/r/144846/#review150260

This works for me. It seems low risk because this code will be removeable once we are shipping the addon.  Do we have existing addon tests that would catch the thrown error if a new conditional is added?
Attachment #8873473 - Flags: review?(bgrinstead) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6175f32bed68
interpret preprocessing instructions in DevTools addon bootstrap;r=bgrins
https://hg.mozilla.org/mozilla-central/rev/6175f32bed68
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
Assignee: nobody → jdescottes
status-firefox55: fixed → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: