Closed Bug 1321509 Opened 8 years ago Closed 8 years ago

Land inspector html using webpack / devtools-local-toolbox

Categories

(DevTools :: Inspector, defect, P1)

Product:
DevTools
Component:
Inspector
Version:
52 Branch
Type:
defect

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Iteration:
53.2 - Dec 12
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

(Whiteboard: [devtools-html] )

Attachments

(5 files)

Bug 1321509 - move client/package.json to sourceeditor;
(deleted), text/x-review-board-request
bgrins
: review+
Details
Bug 1321509 - use devtools-local-toolbox to load the inspector in content;
(deleted), text/x-review-board-request
bgrins
: review+
tromey
: review+
Details
Bug 1321509 - allow non devtools prefs to be loaded in webpack prefs loader;
(deleted), text/x-review-board-request
tromey
: review+
Details
Bug 1321509 - remove references to Array.slice in addon-sdk util/object;
(deleted), text/x-review-board-request
zer0
: review+
Details
Bug 1321509 - Remove unused parameters from node-highlight event;
(deleted), text/x-review-board-request
pbro
: review+
Details
Let's continue the review from Bug 1291049, to get a clean mozreview board.
Comment on attachment 8816074 [details] Bug 1321509 - move client/package.json to sourceeditor; Forwarding r+ from bug 1291049
Attachment #8816074 - Flags: review?(bgrinstead) → review+
Comment on attachment 8816076 [details] Bug 1321509 - allow non devtools prefs to be loaded in webpack prefs loader; Forwarding r+ for bug 1291049.
Attachment #8816076 - Flags: review?(ttromey) → review+
Blocks: devtools-html-phase2
Iteration: --- → 53.2 - Dec 12
Flags: qe-verify?
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 :)
Attachment #8816076 - Flags: review?(ttromey) → review+
Comment on attachment 8816074 [details] Bug 1321509 - move client/package.json to sourceeditor; https://reviewboard.mozilla.org/r/96866/#review97176
Attachment #8816074 - Flags: review+
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"
Attachment #8816075 - Flags: review?(ttromey) → review+
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.
Attachment #8816075 - Flags: review?(bgrinstead) → review+
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.
Attachment #8816090 - Flags: review?(pbrosset) → review+
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 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 on attachment 8816089 [details] Bug 1321509 - remove references to Array.slice in addon-sdk util/object; https://reviewboard.mozilla.org/r/96888/#review97820
Attachment #8816089 - Flags: review?(zer0) → review+
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 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.
Pushed by jdescottes@mozilla.com: 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
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!
Flags: needinfo?(jdescottes)
No need for QA on this one, thanks!
Flags: qe-verify?
Flags: qe-verify-
Flags: needinfo?(jdescottes)
Attachment #8816075 - Flags: review?(jlaster)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: