Bug 186260 – Add sub-tests based on Suits

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jon Lee]

ssia

[Jon Lee]

Created attachment 341894 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

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.

[Jon Lee]

Created attachment 342328 [details]
Patch

[Jon Lee]

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.

[Said Abou-Hallawa]

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.

[Jon Lee]

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.

[WebKit Commit Bot]

Comment on attachment 342328 [details]
Patch

Clearing flags on attachment: 342328

Committed r232645: <https://trac.webkit.org/changeset/232645>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40954896>