Closed
Bug 1386535
Opened 7 years ago
Closed 7 years ago
Remove all add-on compatiblity shims
Categories
(DevTools :: General, enhancement, P3)
DevTools
General
Tracking
(firefox57 fix-optional, firefox58 fixed)
RESOLVED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox57 | --- | fix-optional |
firefox58 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
As we start working against Firefox 57, which prevents running non-web-extension add-ons, we should be able to remove all the compatiblity shims we kept around like: http://searchfox.org/mozilla-central/source/devtools/client/shims http://searchfox.org/mozilla-central/source/devtools/client/themes/shims http://searchfox.org/mozilla-central/source/devtools/server/shims http://searchfox.org/mozilla-central/source/devtools/shared/shims As well as in-code shims from /browser/: http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#96-97 http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#96-97 http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#8514-8521 http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#8579-8601 These symbols were kept for add-ons. Browser codebase should no longer use these symbols and, instead use DevToolsShim.
Updated•7 years ago
|
Blocks: devtools-debtools
Assignee | ||
Comment 1•7 years ago
|
||
Andy, we have a bunch of compatiblity shim that helps old bootstrapped addons which are using devtools API/modules via URLs/APIs that changed over time. Do you think FF57 is the right release to drop all of that, or should we wait for 58 or some later step? I also opened bug 1397452 which is specific to SDK features implemented in DevTools. I would have the same question about that.
Flags: needinfo?(amckay)
Comment 2•7 years ago
|
||
57 is fine, because all add-ons will need to be WebExtensions by then anyway. Likely the only add-ons that might be affected are ones looked after by the devtools team. So, do whatever is the most convenient for your team.
Flags: needinfo?(amckay)
Updated•7 years ago
|
status-firefox57:
--- → fix-optional
Updated•7 years ago
|
Blocks: dt-polish-debt
Updated•7 years ago
|
No longer blocks: devtools-debtools
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8911804 [details] Bug 1386535 - Remove all DevTools shims used to support legacy add-ons. https://reviewboard.mozilla.org/r/183228/#review188474 Thanks, this looks good to me! :)
Attachment #8911804 -
Flags: review?(jryans) → review+
Comment 5•7 years ago
|
||
The netmonitor and webconsole use a couple of these for Launchpad support [0] "devtools/client/framework/devtools": path.join(__dirname, "../../client/shims/devtools"), "devtools/shared/fronts/timeline": path.join(__dirname, "../../shared/shims/fronts/timeline"), It seems kind of shim for these will be required to build those with webpack (I tested with the patch applied and `cd devtools/client/netmonitor/ && npm install && npm start` fails). I'm not sure if this is something that could move into the launchpad repo or not. Consumers of launchpad are providing their own webpack config but it does look like there may be some kind of shared configuration in https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-launchpad/webpack.config.js. Julian, would it be possible / a good idea to move a couple of these shims into launchpad and make it work with all consumers? [0]: https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools+%22%2Fshims%2F%22&redirect=true
Flags: needinfo?(jdescottes)
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #5) > The netmonitor and webconsole use a couple of these for Launchpad support [0] > > "devtools/client/framework/devtools": path.join(__dirname, > "../../client/shims/devtools"), > "devtools/shared/fronts/timeline": path.join(__dirname, > "../../shared/shims/fronts/timeline"), That's so weird. Can we map to the actual modules?!
Comment 7•7 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #6) > (In reply to Brian Grinstead [:bgrins] from comment #5) > > The netmonitor and webconsole use a couple of these for Launchpad support [0] > > > > "devtools/client/framework/devtools": path.join(__dirname, > > "../../client/shims/devtools"), > > "devtools/shared/fronts/timeline": path.join(__dirname, > > "../../shared/shims/fronts/timeline"), > > That's so weird. > Can we map to the actual modules?! I'm not sure the details off hand, but this was probably done because the actual modules weren't de-chromed. And at least in the case of devtools.js there's not really a way to de-chrome it without going in and shimming/de-chroming a bunch of its imports. Since the require path is used in the tools, if there's no stub then webpack will go through and require the world and not build. There may be options available in the webpack config for this other than shims, I'm not sure.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
I didn't realized client/shims/devtools and shared/shims/fronts/timeline were mock files just for webpack. I no longer remove them in my last patch, it should prevent breaking webpack. Also, referencing them in moz.build was a mistake, it shouldn't be included in firefox package! Someone with more webpack expertize should followup on that to get rid of these shims folder completely, as we should keep them just for webpack. Also note that npm start doesn't work on console: ERROR in ../shared/components/reps/reps.js Module not found: Error: Can't resolve 'devtools/client/shared/vendor/lodash' in '/mnt/desktop/gecko/devtools/client/shared/components/reps'
Comment 10•7 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #9) > Also note that npm start doesn't work on console: > ERROR in ../shared/components/reps/reps.js > Module not found: Error: Can't resolve > 'devtools/client/shared/vendor/lodash' in > '/mnt/desktop/gecko/devtools/client/shared/components/reps' Nicolas just fixed that in bug 1403106 Honza
Comment 11•7 years ago
|
||
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6cc2df751ca9 Remove all DevTools shims used to support legacy add-ons. r=jryans
Comment 12•7 years ago
|
||
Just logged Bug 1403175 to cleanup & regroup our current webpack shims. (In reply to Brian Grinstead [:bgrins] from comment #5) > The netmonitor and webconsole use a couple of these for Launchpad support [0] > > "devtools/client/framework/devtools": path.join(__dirname, > "../../client/shims/devtools"), > "devtools/shared/fronts/timeline": path.join(__dirname, > "../../shared/shims/fronts/timeline"), > > It seems kind of shim for these will be required to build those with webpack > (I tested with the patch applied and `cd devtools/client/netmonitor/ && npm > install && npm start` fails). > > I'm not sure if this is something that could move into the launchpad repo or > not. Consumers of launchpad are providing their own webpack config but it > does look like there may be some kind of shared configuration in > https://github.com/devtools-html/devtools-core/blob/master/packages/devtools- > launchpad/webpack.config.js. Julian, would it be possible / a good idea to > move a couple of these shims into launchpad and make it work with all > consumers? > > [0]: > https://dxr.mozilla.org/mozilla-central/ > search?q=path%3Adevtools+%22%2Fshims%2F%22&redirect=true Re: moving the shims to launchpad. I think we'll have a better picture once all the shims are in the same place but I tend to think that the shims should rather stay next to the code that consumes them. Imagine a console file uses a chrome privileged module. We have a shim for this module. for methods a() b() and c(). If the console file starts using a new method from the module, d(), it will break in launchpad. If the shim is in m-c it can easily be fixed. If the shim is provided by Launchpad then it means a new launchpad release is needed.
Flags: needinfo?(jdescottes)
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6cc2df751ca9
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•