Closed Bug 790272 Opened 8 years ago Closed 8 years ago

Unnecessary repaints of the Layout View

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 18

People

(Reporter: jaws, Assigned: Optimizer)

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
OS: Windows 7 → All
Hardware: x86_64 → All
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 :)
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
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
Paul: that method is only for OOP build of Firefox, for remote connections. Sad, but true.
Going forward with text content approach only.
Attached patch Patch v0.1 (obsolete) — Splinter Review
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.
Attachment #667143 - Flags: review?(paul)
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") ?
Attached patch Patch v0.2 (obsolete) — Splinter Review
switching to getElementById and removing unnecessary checks.
Attachment #667143 - Attachment is obsolete: true
Attachment #667143 - Flags: review?(paul)
Attachment #667324 - Flags: review?(paul)
Attached patch Patch v0.2 - corrected (obsolete) — Splinter Review
Accidentally missed one line in 0.2
Attachment #667324 - Attachment is obsolete: true
Attachment #667324 - Flags: review?(paul)
Attachment #667325 - Flags: review?(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?
Attachment #667325 - Flags: review?(paul)
Attachment #667325 - Flags: review?(jaws)
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;
Attachment #667325 - Flags: review?(jaws) → review+
Attached patch Patch v0.3Splinter Review
Carry forward Jared's r+
Attachment #667325 - Attachment is obsolete: true
Attachment #667325 - Flags: review?(paul)
Attachment #667804 - Flags: review?(paul)
Optimizer, can you push that to try?
try is taking like forever....

though results are grassy :)
Attachment #667804 - Flags: review?(paul) → review+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e5d8ddb1e35f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.