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

RESOLVED DUPLICATE of bug 1291049

Status

defect
RESOLVED DUPLICATE of bug 1291049
3 years ago
a year ago

People

(Reporter: jdescottes, Unassigned)

Tracking

Trunk
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 affected)

Details

Attachments

(1 attachment)

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.)
Flags: qe-verify?
Priority: -- → P2
Reporter

Comment 2

3 years ago
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.
Attachment #8809055 - Flags: feedback?(bgrinstead)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Iteration: --- → 52.3 - Nov 14
Priority: P2 → P1
(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.)
Flags: qe-verify? → qe-verify-
Comment on attachment 8809055 [details]
Bug 1310417 - use shams for devtools.js dependencies unable to run in content

f+ as we discussed earlier
Attachment #8809055 - Flags: feedback?(bgrinstead) → feedback+

Updated

3 years ago
Depends on: 1316630
Reporter

Comment 5

3 years ago
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).
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1291049
Assignee: jdescottes → nobody
No longer blocks: devtools-html-phase2
Iteration: 52.3 - Nov 14 → ---
Priority: P1 → --
Whiteboard: [devtools-html]

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.