1321509 - Land inspector html using webpack / devtools-local-toolbox

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Julian Descottes [:jdescottes]]

Let's continue the review from Bug 1291049, to get a clean mozreview board.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1321509 - move client/package.json to sourceeditor;

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

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1321509 - use devtools-local-toolbox to load the inspector in content;

Add package.json to devtools/client/inspector
Integration with devtools-local-toolbox:
- provides development server
- default webpack config
- landing page to select a tab

In this patch:
- bin/dev-server-js contains the inspector specific routes
- webpack/*-sham.js : inspector dependencies that had to be mocked
  ideally, decoupling work should continue and the inspector client
  should only ever require files that require no chrome priviledged
  APIs.
- toolbox.js contains the interface with devtools-local-toolbox bootstrap
  and the inspector panel initialization
- configs/development.json is the inspector config for the devtools-local-toolbox

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

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1321509 - allow non devtools prefs to be loaded in webpack prefs loader;

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

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8816074 [details]
Bug 1321509 - move client/package.json to sourceeditor;

Forwarding r+ from bug 1291049

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8816076 [details]
Bug 1321509 - allow non devtools prefs to be loaded in webpack prefs loader;

Forwarding r+ for bug 1291049.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1321509 - remove references to Array.slice in addon-sdk util/object;

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

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1321509 - Remove unused parameters from node-highlight event;

The last parameter sent with the node-highlight event is never used.
Also toSource is a Firefox only API, in the context of devtools-html it
would be nice to get rid of it.

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

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8816075 [details]
Bug 1321509 - use devtools-local-toolbox to load the inspector in content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96868/diff/1-2/

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8816076 [details]
Bug 1321509 - allow non devtools prefs to be loaded in webpack prefs loader;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96870/diff/1-2/

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8816075 [details]
Bug 1321509 - use devtools-local-toolbox to load the inspector in content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96868/diff/2-3/

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8816076 [details]
Bug 1321509 - allow non devtools prefs to be loaded in webpack prefs loader;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96870/diff/2-3/

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8816090 [details]
Bug 1321509 - Remove unused parameters from node-highlight event;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96890/diff/1-2/

Comment 13

[Tom Tromey :tromey]

Comment on attachment 8816076 [details]
Bug 1321509 - allow non devtools prefs to be loaded in webpack prefs loader;

https://reviewboard.mozilla.org/r/96870/#review97172

Thanks.  This is still ok :)

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8816074 [details]
Bug 1321509 - move client/package.json to sourceeditor;

https://reviewboard.mozilla.org/r/96866/#review97176

Comment 15

[Julian Descottes [:jdescottes]]

Talos comparison ongoing: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=7172f400caf7baa0f5c5e872715c8fbf56c757a6&newProject=try&newRevision=4caeed724dee4ed439ebe038b3e57a8e4e30b9ca&framework=1&filter=damp&showOnlyImportant=0
(looks samey so far, but only a few runs completed)

Try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cf8d4f0326d4ca14633ca4786c868c47452a65f
(looks good)

Comment 16

[Tom Tromey :tromey]

Comment on attachment 8816075 [details]
Bug 1321509 - use devtools-local-toolbox to load the inspector in content;

https://reviewboard.mozilla.org/r/96868/#review97270

Thank you.  I think this addressed all my comments from earlier.  I found one more little nit, a typo.

::: devtools/client/inspector/webpack/prefs-loader.js:14
(Diff revision 3)
> +const PREF_RX = new RegExp("^ *pref\\(\"devtools");
> +
> +module.exports = function (content) {
> +  this.cacheable && this.cacheable();
> +
> +  // If we're reading devtools.js we have to do some reprocessing.

Typo, "preprocessing"

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8816075 [details]
Bug 1321509 - use devtools-local-toolbox to load the inspector in content;

https://reviewboard.mozilla.org/r/96868/#review97276

::: devtools/client/inspector/local-toolbox.js:74
(Diff revision 3)
> +
> +/**
> + * Called each time a childList mutation is received on the main document.
> + * Check the iframes currently loaded in the document and call fixStylesheets if needed.
> + */
> +function fixStylesheetsOnMutation() {

Thanks.  This will work for now and hopefully we can get rid of this step by making theme-switching work in content (or somehow changing the way themes are loaded to make it easier).

I was also thinking that we could build a shim for theme-switching that does run in content and then rewrite paths to point to that, but let's not rush into that.

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8816090 [details]
Bug 1321509 - Remove unused parameters from node-highlight event;

https://reviewboard.mozilla.org/r/96890/#review97766

Looks good. I didn't even know we had that extra parameter, and it doesn't seem to be used anyway.

Comment 19

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8816089 [details]
Bug 1321509 - remove references to Array.slice in addon-sdk util/object;

https://reviewboard.mozilla.org/r/96888/#review97812

::: addon-sdk/source/lib/sdk/util/object.js:53
(Diff revision 1)
>   * properties from all following arguments.
>   * `extend(source1, source2, source3)` is equivalent of
>   * `merge(Object.create(source1), source2, source3)`.
>   */
>  function extend(source) {
>    let rest = Array.slice(arguments, 1);

You want probably to refactor also this `Array.slice`, I guess.

Comment 20

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8816089 [details]
Bug 1321509 - remove references to Array.slice in addon-sdk util/object;

https://reviewboard.mozilla.org/r/96888/#review97812

r+ with the comment below addressed.
Also, since you're creating a new array instance everytime you call `slice`, in three different function, it could be worthy using this approach instead: https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/core/heritage.js#16-22 : you could either add in the same file (`object` module), or just to the `./array` module – since is already `require`d.

Comment 21

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8816089 [details]
Bug 1321509 - remove references to Array.slice in addon-sdk util/object;

https://reviewboard.mozilla.org/r/96888/#review97820

Comment 22

[Julian Descottes [:jdescottes]]

Comment on attachment 8816074 [details]
Bug 1321509 - move client/package.json to sourceeditor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96866/diff/1-2/

Comment 23

[Julian Descottes [:jdescottes]]

Comment on attachment 8816089 [details]
Bug 1321509 - remove references to Array.slice in addon-sdk util/object;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96888/diff/1-2/

Comment 24

[Julian Descottes [:jdescottes]]

Comment on attachment 8816075 [details]
Bug 1321509 - use devtools-local-toolbox to load the inspector in content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96868/diff/3-4/

Comment 25

[Julian Descottes [:jdescottes]]

Comment on attachment 8816076 [details]
Bug 1321509 - allow non devtools prefs to be loaded in webpack prefs loader;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96870/diff/3-4/

Comment 26

[Julian Descottes [:jdescottes]]

Comment on attachment 8816090 [details]
Bug 1321509 - Remove unused parameters from node-highlight event;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96890/diff/2-3/

Comment 27

[Julian Descottes [:jdescottes]]

Comment on attachment 8816089 [details]
Bug 1321509 - remove references to Array.slice in addon-sdk util/object;

https://reviewboard.mozilla.org/r/96888/#review97812

Thanks for the review Matteo! Added a similar approach to the one in heritage.js. Try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1309f3a296a32a51d89bddd25e5c5f9a7ba2b33

Comment 28

[Julian Descottes [:jdescottes]]

Comment on attachment 8816075 [details]
Bug 1321509 - use devtools-local-toolbox to load the inspector in content;

https://reviewboard.mozilla.org/r/96868/#review97276

> Thanks.  This will work for now and hopefully we can get rid of this step by making theme-switching work in content (or somehow changing the way themes are loaded to make it easier).
> 
> I was also thinking that we could build a shim for theme-switching that does run in content and then rewrite paths to point to that, but let's not rush into that.

I guess we should have a follow up to figure out how we handle themes in content. As we are in mc our approach differs a bit from the one used by debugger.html for now.

Comment 29

[Julian Descottes [:jdescottes]]

Comment on attachment 8816075 [details]
Bug 1321509 - use devtools-local-toolbox to load the inspector in content;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96868/diff/4-5/

Comment 30

[Julian Descottes [:jdescottes]]

Comment on attachment 8816076 [details]
Bug 1321509 - allow non devtools prefs to be loaded in webpack prefs loader;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96870/diff/4-5/

Comment 31

[Julian Descottes [:jdescottes]]

Comment on attachment 8816090 [details]
Bug 1321509 - Remove unused parameters from node-highlight event;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96890/diff/3-4/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61d6fe7c6390
move client/package.json to sourceeditor;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/d72c318a4f42
remove references to Array.slice in addon-sdk util/object;r=zer0
https://hg.mozilla.org/integration/autoland/rev/b9b90a3362ea
use devtools-local-toolbox to load the inspector in content;r=bgrins,tromey
https://hg.mozilla.org/integration/autoland/rev/e858457cd353
allow non devtools prefs to be loaded in webpack prefs loader;r=tromey
https://hg.mozilla.org/integration/autoland/rev/91e1d8b0cdeb
Remove unused parameters from node-highlight event;r=pbro

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/61d6fe7c6390
https://hg.mozilla.org/mozilla-central/rev/d72c318a4f42
https://hg.mozilla.org/mozilla-central/rev/b9b90a3362ea
https://hg.mozilla.org/mozilla-central/rev/e858457cd353
https://hg.mozilla.org/mozilla-central/rev/91e1d8b0cdeb

Comment 36

[Petruta Horea [:phorea], Desktop Test Engineering]

Hi Julien,

This issue is marked with qe-verify? flag. Does it need manual QA? If it does, can you please provide some guidelines? Thank you!

Comment 37

[Julian Descottes [:jdescottes]]

No need for QA on this one, thanks!