[webkit-reviews] review granted: [Bug 201735] Web Inspector: Improve auto completion typing performance by avoiding global forced layouts : [Attachment 378746] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 201735: Web Inspector: Improve auto completion typing performance by
avoiding global forced layouts
https://bugs.webkit.org/show_bug.cgi?id=201735

Attachment 378746: [PATCH] Proposed Fix

https://bugs.webkit.org/attachment.cgi?id=378746&action=review




--- Comment #8 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 378746
  --> https://bugs.webkit.org/attachment.cgi?id=378746
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=378746&action=review

r=me, with some quick changes

> Source/WebInspectorUI/ChangeLog:12
> +	   Provide a helper for measuring an element in a hidden containment

You could just say “container” instead of “containment div”.

> Source/WebInspectorUI/UserInterface/Base/Main.js:300
> +    WI.layoutMeasurementContainer =
document.body.appendChild(document.createElement("div"));

Can we lazily create this element, or at the very least wait to add it to the
DOM till the first invocation? Or is the reason you’re doing this to also avoid
an “expensive” layout on the first call too?

> Source/WebInspectorUI/UserInterface/Base/Main.js:2789
> +WI.measureElement = function(element)

This is awesome!!!

> Source/WebInspectorUI/UserInterface/Base/Main.js:2791
> +    WI.layoutMeasurementContainer.className = element.className;

I don’t think we want to do this, especially since `cloneNode` will copy any
CSS classes on the child for us, so we’d potentially have a duplicate selector
on BOTH the parent and child. That could also cause issues if any class
selectors have any `!important` and overrode our hiding styles.

> Source/WebInspectorUI/UserInterface/Base/Main.js:2797
> +    WI.layoutMeasurementContainer.className = "";

Ditto (2791)