[webkit-reviews] review granted: [Bug 186260] Add sub-tests based on Suits : [Attachment 341894] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Jon Lee
<jonlee at apple.com>'s request for review:
Bug 186260: Add sub-tests based on Suits
https://bugs.webkit.org/show_bug.cgi?id=186260

Attachment 341894: Patch

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




--- Comment #4 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 341894
  --> https://bugs.webkit.org/attachment.cgi?id=341894
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.