[webkit-reviews] review granted: [Bug 227948] [Performance test][css-contain] Add test to contain: size layout : [Attachment 434157] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted cathiechen
<cathiechen at igalia.com>'s request for review:
Bug 227948: [Performance test][css-contain] Add test to contain: size layout
https://bugs.webkit.org/show_bug.cgi?id=227948

Attachment 434157: Patch

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




--- Comment #13 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 434157
  --> https://bugs.webkit.org/attachment.cgi?id=434157
Patch

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

> PerformanceTests/Layout/css-contain-change-size.html:26
> +    let flexibleNodes = [];

Since we're not assigning a new value to this variable, this should also be
const.
JavaScript const semantics ins't like that of C++.
It doesn't mean that the object referenced by the variable isn't mutable.
So even if we declared this const variable, we can still push more contents to
the array.
Also, we should call this widthChangingElements or resizingElements.
"flexibleNodes" seems rather vague.

> PerformanceTests/Layout/css-contain-change-size.html:38
> +	       row.style.top = top + "px";

We should probably use single quotation marks throughout the script for
consistency.

> PerformanceTests/Layout/css-contain-change-size.html:44
> +		   flexibleNode.style.width = (100 *
PerfTestRunner.random()).toFixed(0) + "px";
> +		   flexibleNode.style.height = "100px";

Ditto.

> PerformanceTests/Layout/css-contain-change-size.html:46
> +		   cell.style.left = left + "px";

Ditto.

> PerformanceTests/Layout/css-contain-change-size.html:63
> +	       description: "Measures performance of changing widths of
nodes.",

Ditto.