Bug 188532

Summary: [MotionMark] Update Multiply test
Product: WebKit Reporter: Jon Lee <jonlee>
Component: AnimationsAssignee: Jon Lee <jonlee>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dino, ews-watchlist, rniwa, sabouhallawa, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Jon Lee 2018-08-13 14:27:36 PDT
Update Multiply test
Comment 1 Radar WebKit Bug Importer 2018-08-13 14:27:55 PDT
<rdar://problem/43252125>
Comment 2 Radar WebKit Bug Importer 2018-08-13 14:28:28 PDT
<rdar://problem/43252151>
Comment 3 Jon Lee 2018-08-13 14:57:16 PDT
Created attachment 347044 [details]
Patch
Comment 4 Said Abou-Hallawa 2018-08-13 16:14:33 PDT
Comment on attachment 347044 [details]
Patch

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

> PerformanceTests/MotionMark/tests/dom/resources/multiply.js:33
> +            options.visibleCSS = [["opacity", 0, 1]];

I would suggest writing this as an structures:
    options.visibleCSS = [{ property: "visibility", initialValue: "hidden", animateValue: "visible" }];

> PerformanceTests/MotionMark/tests/master/resources/multiply.js:39
> +    visibleCSS: [
> +        ["visibility", "hidden", "visible"],
> +        ["opacity", 0, 1],
> +        ["display", "none", "block"]
> +    ],

Ditto:

visibleCSS: [
    { property: "visibility", initialValue: "hidden", animateValue: "visible" },
    { property: "opacity", initialValue: 0, animateValue: 1 },
    { property: "display", initialValue: "none", animateValue: "block" }
],

> PerformanceTests/MotionMark/tests/master/resources/multiply.js:45
> -        var tileSize = Math.round(this.size.height / 25);
> +        var tileSize = Math.round(this.size.height / this.totalRows);

Shouldn't we use the screen.height instead of the stage height to control how many tiles can be shown in the page?

> PerformanceTests/MotionMark/tests/master/resources/multiply.js:96
> +        tile.style[visibleCSS[0]] = visibleCSS[1];

tile.style[visibleCSS.property] = visibleCSS.initialValue;

> PerformanceTests/MotionMark/tests/master/resources/multiply.js:130
> +            tile.element.style[tile.visibleCSS[0]] = tile.visibleCSS[2];

tile.style[tile.visibleCSS.property] = tile.visibleCSS.animateValue;

> PerformanceTests/MotionMark/tests/master/resources/multiply.js:141
> +            tile.element.style[tile.visibleCSS[0]] = tile.visibleCSS[1];

tile.style[tile.visibleCSS.property] = tile.visibleCSS.initialValue;
Comment 5 Jon Lee 2018-08-13 17:31:05 PDT
Comment on attachment 347044 [details]
Patch

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

>> PerformanceTests/MotionMark/tests/master/resources/multiply.js:39
>> +    ],
> 
> Ditto:
> 
> visibleCSS: [
>     { property: "visibility", initialValue: "hidden", animateValue: "visible" },
>     { property: "opacity", initialValue: 0, animateValue: 1 },
>     { property: "display", initialValue: "none", animateValue: "block" }
> ],

I'll do this update in a later patch.

>> PerformanceTests/MotionMark/tests/master/resources/multiply.js:45
>> +        var tileSize = Math.round(this.size.height / this.totalRows);
> 
> Shouldn't we use the screen.height instead of the stage height to control how many tiles can be shown in the page?

I don't think so. It does mean literally equal work in terms of point area among all device classes, but if we did this then desktop spaces would have high 10,000s max # of particles, which might be detrimental to getting high-enough scores given the # of renderers that need to be iterated.
Comment 6 WebKit Commit Bot 2018-08-13 17:58:46 PDT
Comment on attachment 347044 [details]
Patch

Clearing flags on attachment: 347044

Committed r234832: <https://trac.webkit.org/changeset/234832>
Comment 7 WebKit Commit Bot 2018-08-13 17:58:48 PDT
All reviewed patches have been landed.  Closing bug.