[webkit-reviews] review granted: [Bug 233628] [Performance test][css-contain] Add test large-grid.html : [Attachment 445404] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rbuis at igalia.com> has granted cathiechen <cathiechen at igalia.com>'s
request for review:
Bug 233628: [Performance test][css-contain] Add test large-grid.html
https://bugs.webkit.org/show_bug.cgi?id=233628

Attachment 445404: Patch

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




--- Comment #6 from Rob Buis <rbuis at igalia.com> ---
Comment on attachment 445404
  --> https://bugs.webkit.org/attachment.cgi?id=445404
Patch

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

> PerformanceTests/ChangeLog:14
> +	   relayout. Then it can schedule the relayout requirement of substree
asynchronously.

I think the last sentence may be confusing or not relevant, so probably better
to remove it.

>>> PerformanceTests/Layout/large-grid.html:148
>>> +		 description: "Measures performance of getting offsetHeight of
a large grid container.",
>> 
>> I think the focus is more on performance of relayout rather than
offsetHeight. Also getting offsetHeight from document element rather than grid
container should be about the same time right? So maybe it is better to use
that?
> 
> You mean `document.body.offsetHeight`? It's same to
gridContainer.offsetHeight, doesn't relayout, if the element is outside the
relayout boundary.
> The following interfaces are same: offsetWidth / offsetHeight, scrollWidth /
scrollHeight, clientWidth / clientHeight of element and innerWidth /
innerHeight of window.
> 
> As to the performance of relayout(like getBoundingClientRect), containment
doesn't change the performance much in this scenario. Layout containment
generates a relayout boundary. It is very efficient to changes of a small part
inside a complex document.
> But in this scenario, it changes a large part of the page, so the performance
is not improved. On the other way, for interfaces like offsetHeight, the
performance is improved.
> The relayout boundary makes it's possible to perform the relayout requirement
asynchronously.

Ok thanks, that clears things up.