Bug 186260 - Add sub-tests based on Suits
Summary: Add sub-tests based on Suits
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on: 186259
Blocks: 186447
  Show dependency treegraph
 
Reported: 2018-06-03 23:03 PDT by Jon Lee
Modified: 2018-06-08 16:14 PDT (History)
7 users (show)

See Also:


Attachments
Patch (21.38 KB, patch)
2018-06-03 23:11 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.74 MB, application/zip)
2018-06-04 06:29 PDT, EWS Watchlist
no flags Details
Patch (16.41 KB, patch)
2018-06-08 14:23 PDT, Jon Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2018-06-03 23:03:35 PDT
ssia
Comment 1 Jon Lee 2018-06-03 23:11:57 PDT
Created attachment 341894 [details]
Patch
Comment 2 EWS Watchlist 2018-06-04 06:29:14 PDT
Comment on attachment 341894 [details]
Patch

Attachment 341894 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7977141

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html
Comment 3 EWS Watchlist 2018-06-04 06:29:26 PDT
Created attachment 341900 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 4 Said Abou-Hallawa 2018-06-06 12:20:57 PDT
Comment on attachment 341894 [details]
Patch

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

> PerformanceTests/MotionMark/tests/master/resources/svg-particles.js:2
> - * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
> + * Copyright (C) 2015-2018 Apple Inc. All rights reserved.

Maybe we can change the file name form svg-particles.js to suits.js or something similar. Similarly tests/master/svg-particles.html can be renamed to tests/master/suits.html

> PerformanceTests/MotionMark/tests/svg/suits.js:52
> +        this.position = Stage.randomElementInArray(this.stage.emitLocation);
> +
> +        var velocityMagnitude = Stage.random(.5, 2.5);
> +        var angle = Stage.randomInt(0, this.stage.emitSteps) / this.stage.emitSteps * Math.PI * 2 + Stage.dateCounterValue(1000) * this.stage.emissionSpin + velocityMagnitude;
> +        this.velocity = new Point(Math.sin(angle), Math.cos(angle))
> +            .multiply(velocityMagnitude);

The calculation of the position and velocity is repeated exactly the same five times in this file. Can we move it to a global function? Or can we have the four classes inherit from a base class which has this calculation in a single function?

> PerformanceTests/MotionMark/tests/svg/suits.js:90
> +        this.transformSuffix = " scale(" + this.size.x + ") translate(-.5,-.5)";

Can you please add a comment explaining what this transform is doing? It took me a while to realize that the path element has size (1, 1) and we want to center it around this.position and scale it by this.size.

> PerformanceTests/MotionMark/tests/svg/suits.js:111
> +            this.element = Utilities.createSVGElement("rect", {
> +                x: 0,
> +                y: 0,
> +                "clip-path": "url(" + shapeId + ")"
> +            }, {}, stage.element);

This code is repeated four times in this file.

> PerformanceTests/MotionMark/tests/svg/suits.js:115
> +            var shapePath = document.querySelector(shapeId + " path");
> +            this.element = shapePath.cloneNode();
> +            stage.element.appendChild(this.element);

And this part also.

> PerformanceTests/MotionMark/tests/svg/suits.js:261
> +        if (this.isClipPath) {
> +            this.element.setAttribute("width", this.size.x);
> +            this.element.setAttribute("height", this.size.y);
> +            this.transformSuffix += " translate(-" + this.size.center.x + ",-" + this.size.center.y + ")";
> +        } else
> +            this.transformSuffix += " scale(" + this.size.x + ") translate(-.5,-.5)";

It might also be worthy if we have the calculation of resizing and moving the particle in one function for all the classes based on two arguments: isClipPath, isStatic.
Comment 5 Jon Lee 2018-06-08 14:23:44 PDT
Created attachment 342328 [details]
Patch
Comment 6 Jon Lee 2018-06-08 14:25:08 PDT
Comment on attachment 341894 [details]
Patch

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

>> PerformanceTests/MotionMark/tests/master/resources/svg-particles.js:2
>> + * Copyright (C) 2015-2018 Apple Inc. All rights reserved.
> 
> Maybe we can change the file name form svg-particles.js to suits.js or something similar. Similarly tests/master/svg-particles.html can be renamed to tests/master/suits.html

I'll do that in a follow-up.

>> PerformanceTests/MotionMark/tests/svg/suits.js:52
>> +            .multiply(velocityMagnitude);
> 
> The calculation of the position and velocity is repeated exactly the same five times in this file. Can we move it to a global function? Or can we have the four classes inherit from a base class which has this calculation in a single function?

Done.
Comment 7 Said Abou-Hallawa 2018-06-08 15:28:13 PDT
Comment on attachment 342328 [details]
Patch

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

> PerformanceTests/MotionMark/tests/svg/suits.js:36
> +    isClipPath: true,
> +    hasGradient: false,

Nit: The concrete Suits classes could pass isClipPath and hasGradient to the SuperSuitsParticle. These two data members are only used by SuperSuitsParticle.
Comment 8 Jon Lee 2018-06-08 15:45:48 PDT
Comment on attachment 342328 [details]
Patch

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

>> PerformanceTests/MotionMark/tests/svg/suits.js:36
>> +    hasGradient: false,
> 
> Nit: The concrete Suits classes could pass isClipPath and hasGradient to the SuperSuitsParticle. These two data members are only used by SuperSuitsParticle.

Both of these are set on the prototype, not the particle instance. In the case where the test combines both clip and shapes, them isClipPath becomes a data member on the instance. But when it's all of one type, there's no need to have all of the particles contain their own data member, so I put it on the prototype.
Comment 9 WebKit Commit Bot 2018-06-08 16:12:28 PDT
Comment on attachment 342328 [details]
Patch

Clearing flags on attachment: 342328

Committed r232645: <https://trac.webkit.org/changeset/232645>
Comment 10 WebKit Commit Bot 2018-06-08 16:12:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-06-08 16:14:07 PDT
<rdar://problem/40954896>