Closed Bug 1195780 Opened 9 years ago Closed 4 years ago

Remove #ifdefs from browser/devtools JS/XUL files

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox43 affected)

RESOLVED FIXED
Tracking Status
firefox43 --- affected

People

(Reporter: Gijs, Unassigned)

References

(Blocks 2 open bugs)

Details

Remaining lines starting with "#" in xul/js: browser/devtools/framework/toolbox-window.xul:#ifdef XP_MACOSX browser/devtools/framework/toolbox-window.xul:#else browser/devtools/framework/toolbox-window.xul:#endif browser/devtools/scratchpad/scratchpad.xul:#ifdef XP_WIN browser/devtools/scratchpad/scratchpad.xul:#else browser/devtools/scratchpad/scratchpad.xul:#endif browser/devtools/framework/toolbox-process-window.js:#ifdef XP_MACOSX browser/devtools/framework/toolbox-process-window.js:#endif browser/devtools/webide/webide-prefs.js:# -*- indent-tabs-mode: nil; js-indent-level: 2 -*- browser/devtools/webide/webide-prefs.js:# This Source Code Form is subject to the terms of the Mozilla Public browser/devtools/webide/webide-prefs.js:# License, v. 2.0. If a copy of the MPL was not distributed with this browser/devtools/webide/webide-prefs.js:# file, You can obtain one at http://mozilla.org/MPL/2.0/. browser/devtools/webide/webide-prefs.js:#ifdef MOZ_DEV_EDITION browser/devtools/webide/webide-prefs.js:#else browser/devtools/webide/webide-prefs.js:#endif The license headers can just be converted to comments. Everything else can use AppConstants.jsm - we should add MOZ_DEV_EDITION there. Patrick, can you sanity-check I'm not missing any particular reasons why this can't go ahead?
> Patrick, can you sanity-check I'm not missing any particular reasons why > this can't go ahead?
Flags: needinfo?(pbrosset)
DevTools also wants preprocessing gone to ease reloading files. Gijs, are we able to use AppConstants.jsm from a prefs file for webide-prefs.js? I assumed the prefs files had a limited syntax available.
Flags: needinfo?(pbrosset) → needinfo?(gijskruitbosch+bugs)
Thanks Gijs from filing this and listing the files. Getting rid of preprocessor directives in JS files also means these files become ESLint-able (and should therefore be removed from the .eslintignore list here: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/.eslintignore ) That list seems good, and I don't see any reason why we shouldn't be able to get rid of these #ifdefs. A couple questions though, mainly because I've never used AppConstants.jsm: - How can #ifdefs in XUL be replaced with checks from AppConstants? For example the <key> modifiers in toolbox-window.xul? Does it need to be dynamically set from JS instead? - I wonder how webide-prefs.js is loaded, will we have access to AppConstants from this file?
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #3) > Thanks Gijs from filing this and listing the files. > Getting rid of preprocessor directives in JS files also means these files > become ESLint-able (and should therefore be removed from the .eslintignore > list here: > https://dxr.mozilla.org/mozilla-central/source/browser/devtools/. > eslintignore ) > > That list seems good, and I don't see any reason why we shouldn't be able to > get rid of these #ifdefs. > > A couple questions though, mainly because I've never used AppConstants.jsm: > - How can #ifdefs in XUL be replaced with checks from AppConstants? For > example the <key> modifiers in toolbox-window.xul? Does it need to be > dynamically set from JS instead? It's possible I moved too quickly here. You'd need to check that it continued to work if you did that. Key elements are finnicky, and they might need to be created completely dynamically for that to work. > - I wonder how webide-prefs.js is loaded, will we have access to > AppConstants from this file? I didn't realize that was a prefs file. Any particular reason not to just fold that into firefox.js ?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(pbrosset)
Forwarding this question to Ryan.
Flags: needinfo?(pbrosset) → needinfo?(jryans)
(In reply to :Gijs Kruitbosch from comment #4) > > - I wonder how webide-prefs.js is loaded, will we have access to > > AppConstants from this file? > > I didn't realize that was a prefs file. Any particular reason not to just > fold that into firefox.js ? I can't recall the precise reason Paul made a separate prefs file for WebIDE. But, I am about to move all of DevTools to /devtools, so a natural next step could be to move all DevTools prefs to their own prefs file for Firefox to include. Either way, it would seem we still have the preprocessing issue for the file contents.
Flags: needinfo?(jryans)
You can have platform specific XUL overlays the way SeaMonkey does it. You will still need #ifdef's in the jar.mn file to map/package the right overlay depending on the target OS.
Product: Firefox → DevTools

There is no more ifdef in DevTools for a while now.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.