Closed
Bug 1365891
Opened 7 years ago
Closed 7 years ago
Avoid using preprocessing in devtools preference files
Categories
(DevTools :: General, enhancement, P3)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 55
People
(Reporter: ochameau, Assigned: jdescottes)
References
Details
Attachments
(1 file)
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
Updated•7 years ago
|
Summary: Avoir using preprocessing in devtools preference files → Avoid using preprocessing in devtools preference files
Comment 1•7 years ago
|
||
Ok, I'm going to work through this all in this doc: https://docs.google.com/document/d/1c24FW5PgdU7F1mDFqk73nLT3SsTFL7XpV6hRjdWOajE/edit?usp=sharing
Assignee | ||
Comment 2•7 years ago
|
||
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.
Reporter | ||
Comment 3•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8873473 -
Flags: review?(poirot.alex)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
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)
Reporter | ||
Comment 9•7 years ago
|
||
(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.
Reporter | ||
Comment 10•7 years ago
|
||
mozreview-review |
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 11•7 years ago
|
||
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)
Reporter | ||
Comment 12•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
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+
Comment 15•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6175f32bed68 interpret preprocessing instructions in DevTools addon bootstrap;r=bgrins
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6175f32bed68
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
Assignee: nobody → jdescottes
status-firefox55:
fixed → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•