Closed Bug 1458767 Opened 6 years ago Closed 6 years ago

Lazy require the HighlightersOverlay in the Inspector

Categories

(DevTools :: Inspector, enhancement, P3)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: gl, Assigned: gl)

References

Details

Attachments

(1 file)

Bug 1458767 - Lazy require the HighlightersOverlay in the Inspector.
59 bytes, text/x-review-board-request
pbro
: review+
Details 
- Lazy require the HighlightersOverlay in the Inspector and only add it to the view when we mousemove over the rule or computed view.
Attachment #8972761 - Flags: review?(poirot.alex)
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]
Attachment #8972761 - Flags: review?(poirot.alex)
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`?
Attachment #8972761 - Flags: review?(pbrosset)
Blocks: 1462648
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.
Attachment #8972761 - Flags: review?(pbrosset) → review+
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
Attachment #8972761 - Flags: review+ → review?(pbrosset)
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.
Attachment #8972761 - Flags: review?(pbrosset) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/607663065b17
Lazy require the HighlightersOverlay in the Inspector. r=pbro
https://hg.mozilla.org/mozilla-central/rev/607663065b17
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
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: