- Lazy require the HighlightersOverlay in the Inspector and only add it to the view when we mousemove over the rule or computed view.

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=71206e1d74d76a1eb0e09ac5b6b97f7a00c042ea damp https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=ba8693e301e500eea7352086820cbf2418ea3ede

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=0804ebee0bd019b30798c3542e7cf7198335a54f damp https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=3d29306373400dabedc4fae0746bc1101da410fc

Comment on attachment 8972761 [details] Bug 1458767 - Lazy require the HighlightersOverlay in the Inspector. https://reviewboard.mozilla.org/r/241320/#review247170 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/inspector/test/shared-head.js:164 (Diff revision 3) > - return inspector.getPanel("ruleview").view; > + const view = inspector.getPanel("ruleview").view; > + > + // Add the highlighters overlay to the rule view. > + EventUtils.synthesizeMouseAtCenter(view.element, { type: "mousemove"}, > + view.styleWindow); > + Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=abcb6a9142cfed6750f40ea67c3ec53eede6cb01

baseline try https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=94cbef8707d7babc549d23ee2dcba70be4f4e9f3 talos mochitest-dt https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=431a0189d00ab43d3969c62e02bced0cdd1dffae

Comment on attachment 8972761 [details] Bug 1458767 - Lazy require the HighlightersOverlay in the Inspector. https://reviewboard.mozilla.org/r/241320/#review251698 ::: devtools/client/inspector/computed/computed.js:186 (Diff revision 5) > this.shortcuts.on("Escape", event => this._onShortcut("Escape", event)); > this.styleDocument.addEventListener("copy", this._onCopy); > this.styleDocument.addEventListener("mousedown", this.focusWindow); > this.element.addEventListener("click", this._onClick); > this.element.addEventListener("contextmenu", this._onContextMenu); > + this.element.addEventListener("mousemove", this._onMouseMove, { once: true }); Because it's a `{once:true}` handler, I think I'd prefer to have the function inline: ``` this.element.addEventListener("mousemove", () => { this.addHighlightersToView(); }, { once: true }); ``` This way, it's easier to understand that this is called just once. Otherwise when you see the `_onMouseMove` function later in the you don't really know whether it's supposed to be called over and over again, or just once. This way, you can also remove the `bind(this)` above, and the `_onMouseMove` function altogether. ::: devtools/client/inspector/computed/computed.js:773 (Diff revision 5) > // Remove bound listeners > - this.styleDocument.removeEventListener("mousedown", this.focusWindow); > this.element.removeEventListener("click", this._onClick); > - this.styleDocument.removeEventListener("copy", this._onCopy); > this.element.removeEventListener("contextmenu", this._onContextMenu); > + this.element.removeEventListener("mousemove", this._onMouseMove); No need to remove it since it's a `{once:true}` handler. ::: devtools/client/inspector/rules/rules.js:157 (Diff revision 5) > this.shortcuts.on("Return", event => this._onShortcut("Return", event)); > this.shortcuts.on("Space", event => this._onShortcut("Space", event)); > this.shortcuts.on("CmdOrCtrl+F", event => this._onShortcut("CmdOrCtrl+F", event)); > this.element.addEventListener("copy", this._onCopy); > this.element.addEventListener("contextmenu", this._onContextMenu); > + this.element.addEventListener("mousemove", this._onMouseMove, { once: true }); Same comments for this file. ::: devtools/client/inspector/test/shared-head.js:89 (Diff revision 5) > // Replace the view to use a custom debounce function that can be triggered manually > // through an additional ".flush()" property. > - data.inspector.getPanel("ruleview").view.debounce = manualDebounce(); > + view.debounce = manualDebounce(); > + > + // Adds the highlighters overlay in the rule view. > + view.addHighlightersToView(); Shouldn't we also do the same thing `openComputedView`?

Comment on attachment 8972761 [details] Bug 1458767 - Lazy require the HighlightersOverlay in the Inspector. https://reviewboard.mozilla.org/r/241320/#review252346 ::: devtools/client/inspector/inspector.js:1326 (Diff revision 7) > + if (this._highlighters) { > + this._highlighters.destroy(); > + this._highlighters = null; > + } There's a change of destroy sequence here. Before we used to store the promise returned by `this._highlighters.destroy()` and would wait for it to resolve at in the `this._panelDestroyer` part further down. Now we don't anymore. I think it's risky to change this. So you should change to: ``` let highlighterDestroyer = Promise.resolve(); if (this._highlighters) { highlighterDestroyer = this._highlighers.destroy(); this._highlighters = null; } ``` And keep `highlighterDestroyer` in the promise.all below. ::: devtools/client/inspector/shared/highlighters-overlay.js:975 (Diff revision 7) > > /** > * Destroy this overlay instance, removing it from the view and destroying > * all initialized highlighters. > */ > - async destroy() { > + destroy() { Oh I see, this now isn't an async function anymore. Nevermind my previous comment then.

Comment on attachment 8972761 [details] Bug 1458767 - Lazy require the HighlightersOverlay in the Inspector. Gonna ask for a re-review since I found some more instances where it might trigger the initialization of the HighlightersOverlay

Comment on attachment 8972761 [details] Bug 1458767 - Lazy require the HighlightersOverlay in the Inspector. https://reviewboard.mozilla.org/r/241320/#review252510 ::: devtools/client/inspector/boxmodel/box-model.js:70 (Diff revision 8) > > + get highlighters() { > + if (!this._highlighters) { > + // highlighters is a lazy getter in the inspector. > + this._highlighters = this.inspector.highlighters; > + } missing return statement from this getter.

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/607663065b17 Lazy require the HighlightersOverlay in the Inspector. r=pbro