790272 - Unnecessary repaints of the Layout View

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Enable paint flashing (debug.ng_layout.paint_flashing)
Inspect an element and show the Layout View
Mouse around the page to generate events (or type in a text field)

Expected:
No extra flashing after the initial paint

Actual:
Nonstop painting of the layout view.

Comment 1

[Paul Rouget [:paul]]

Here http://mxr.mozilla.org/mozilla-central/source/browser/devtools/layoutview/LayoutView.jsm#326

and here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/layoutview/LayoutView.jsm#338

We should make sure that the new textContent is actually different.

Comment 2

[Girish Sharma [:Optimizer]]

Tha might do some improvement, but the actual problem here is that update() function is being called on chrome's MozAfterPaint, upon which it displays the value in the layout view, which in turn causes MozAfterPaint event, and the cycle continues.

In theory, update should be listening to content's MozAfterPaint, but for that a pref "dom.send_after_paint_to_content" has to be turned on and off while layout view is showing and closing resp. Which brings us to a practical approach of what paul said above :)

Comment 3

[Paul Rouget [:paul]]

I don't know if that works. Really. But maybe this will let you activate MozAfterPaint events for a specific window: http://mxr.mozilla.org/mozilla-central/source/dom/ipc/test.xul#54

Comment 4

[Girish Sharma [:Optimizer]]

Paul: that method is only for OOP build of Firefox, for remote connections. Sad, but true.
Going forward with text content approach only.

Comment 5

[Girish Sharma [:Optimizer]]

Attachment: Patch v0.1

Added a check before updating the node thus avoiding repaint and in turn MozAfterPaint event.

Also made reference to the two test labels displaying the |width|x|height| avoiding unnecessary querySelection. (There are still 8 query selectors going on, but holding reference to 8 more nodes didn't sound like a good idea to me).

Also, one line was totally unneeded : let selector = this.maps[i].selector , which I removed.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 667143 [details] [diff] [review]
Patch v0.1

Review of attachment 667143 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/layoutview/LayoutView.jsm
@@ +164,5 @@
> +    if (!this.sizeLabel) {
> +      this.sizeLabel = this.doc.querySelector(".size > span");
> +    }
> +    if (!this.sizeHeadingLabel) {
> +      this.sizeHeadingLabel = this.doc.querySelector("#element-size");

Since the query here is only for an ID, can you use this.doc.getElementById("element-size") ?

Comment 7

[Girish Sharma [:Optimizer]]

Attachment: Patch v0.2

switching to getElementById and removing unnecessary checks.

Comment 8

[Girish Sharma [:Optimizer]]

Attachment: Patch v0.2 - corrected

Accidentally missed one line in 0.2

Comment 9

[Paul Rouget [:paul]]

Comment on attachment 667325 [details] [diff] [review]
Patch v0.2 - corrected

Jared, can I ask you to review this and confirm that it works as intended?

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 667325 [details] [diff] [review]
Patch v0.2 - corrected

Review of attachment 667325 [details] [diff] [review]:
-----------------------------------------------------------------

I tested this out on this bug's page as well as https://developer.mozilla.org/samples/cssref/animations/cssanim4.html and it works as intended. Thanks Girish!

I'm not sure if Paul also wants to take a look at this, so I'll leave his request on the bug.

::: browser/devtools/layoutview/LayoutView.jsm
@@ +128,5 @@
>      this.browser.removeEventListener("MozAfterPaint", this.update, true);
>      this.iframe.removeEventListener("keypress", this.bound_handleKeypress, true);
>      this.inspector.chromeWindow.removeEventListener("message", this.onMessage, true);
>      this.close();
> +    this.sizeHeadingLabel = this.sizeLabel = null;

I'd rather this just be:
this.sizeHeadingLabel = null;
this.sizeLabel = null;

Comment 11

[Girish Sharma [:Optimizer]]

Attachment: Patch v0.3

Carry forward Jared's r+

Comment 12

[Paul Rouget [:paul]]

Optimizer, can you push that to try?

Comment 13

[Girish Sharma [:Optimizer]]

https://tbpl.mozilla.org/?tree=Try&rev=95096bb438bf

Comment 14

[Girish Sharma [:Optimizer]]

try is taking like forever....

though results are grassy :)

Comment 15

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=e5d8ddb1e35f

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/e5d8ddb1e35f