1310417 - inspector in content tab: devtools.js requires files that are still using chrome-priv APIs

Status: RESOLVED DUPLICATE of bug 1291049

(DevTools :: Inspector, defect)

Description

[Julian Descottes [:jdescottes]]

Blocking bug 1291049.

devtools.js requires three files that currently can't run in a content tab:

> devtools/client/jsonview/main
> devtools/client/framework/about-devtools-toolbox 
> sdk/system/unload

In Bug 1291049, for now I swapped the require used to get those files to lazyRequireGetter. Since this is aliased to ()=>{} in our inspector webpack config, the dependencies don't get loaded when using webpack, and we just have to protect the code using the dependencies to check that they have been loaded before using them. (if (typeof JSONView != "undefined") {...}).

This works nicely but I think it's kind of obscure and hard to understand. Several options here:
- remove all dependencies on devtools.js from the inspector
- provide a content-friendly devtools shim
- add a "chromeOnlyRequire" helper on our loader (that would map to a simple require but aliased to () => {} in webpack). Would make it clearer that the dependency might not be available in all contexts

For the record the current usage devtools.js in the inspector is:
- listen to pref changes
- open the style editor when clicking on a stylesheet link
- listen to theme changes
- open the inspector when trigger the element picker or eyedropper (but this is from inspector-commands and can probably be ignored here.)

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1310417 - use shams for devtools.js dependencies unable to run in content

Review commit: https://reviewboard.mozilla.org/r/91718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91718/

Comment 2

[Julian Descottes [:jdescottes]]

Comment on attachment 8809055 [details]
Bug 1310417 - use shams for devtools.js dependencies unable to run in content

Brian, can I get your opinion on this bug ? I presented 3 options in the summary and implemented a 4th one in this patch. Here I simply use empty mocks for all the files required by devtools.js that can't run in a content tab. 

The nice part about this approach is that it has no impact on the original devtools.js code.

(if you want to apply it, you'll need to first apply the patches from Bug 1291049.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Julian Descottes [:jdescottes] from comment #0)
> This works nicely but I think it's kind of obscure and hard to understand.
> Several options here:
> - remove all dependencies on devtools.js from the inspector

This is my favorite option for long term if it's possible.  I know at least some can (and should) be removed now, but I think we'll still need some kind of shim like your patch until they are all gone. See comments below:

> For the record the current usage devtools.js in the inspector is:
> - listen to pref changes

We should switch this to use a normal pref observer (or pull away a lighter weight helper like styleeditor/utils.js into a shared folder and use that).  We should actually completely get rid of the pref-changed event on gDevTools I believe - it's buggy in that it only fires when the pref is changed from the options panel. 

> - open the style editor when clicking on a stylesheet link

I don't get this indirection - instead of `let toolbox = gDevTools.getToolbox(this.inspector.target)` couldn't we do `let toolbox = this.inspector.toolbox`?  I can't think of a case where that's expected to be different.

> - listen to theme changes

I'm not sure yet how to handle theme-switching without loading gDevTools, will need to think about that more.

> - open the inspector when trigger the element picker or eyedropper (but this
> is from inspector-commands and can probably be ignored here.)

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8809055 [details]
Bug 1310417 - use shams for devtools.js dependencies unable to run in content

f+ as we discussed earlier

Comment 5

[Julian Descottes [:jdescottes]]

I am closing this one as duplicate of Bug 1291049. Since it depends on the webpack configuration, it will make more sense as a patch in Bug 1291049 than as a separate bug (won't be able to land it earlier anyway).