Bug 156273 – Update master benchmark with SVG test

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jon Lee]

Replace the masks test with an SVG test.

[Jon Lee]

Created attachment 275762 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 275762 [details]
Patch

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

> PerformanceTests/Animometer/tests/dom/resources/dom-particles.js:17
> +        this.position = new Point(emitLocation.x, emitLocation.y);

I think "var emitLocation" is not needed.

> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:7
> +        this.isClipPath = Stage.randomBool();

Why do not we always clip? Why do we need to test two different things: clipping using path and drawing a path?

> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:31
> +        this.position = new Point(emitLocation.x, emitLocation.y);

I think "var emitLocation" is not needed.

> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:49
> +            this.gradient.remove();

I am not sure about this one. Shouldn't the parent remove the child instead? i.e. this.stage.gradientsDefs.removeChild(this.gradient);

> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:50
> +        var gradientId = "gradient-" + this.stage.gradientsCounter++;

Why do we have to have a new incremented id every time we create the gradient? Didn't we remove the old one so we can reuse the same id?

> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:62
> +        }, {}, this.gradient);

Is it easier/faster to create a new gradient rather than changing the existing one?  I would suggest doing the following:

In svg-particles.html:
    <defs id="gradients">
        <linearGradient id="default-gradient">
           <stop offset="0%"/>
           <stop offset="100%"/>
       </linearGradient>
    </defs>

In the 3SVGParticle constructor:
    var defaultGradient = document.querySelector("#default-gradient");
    this.gradient = defaultGradient.cloneNode(true);
    this.gradient.id = "gradient-" + stage.gradientsCounter++;
    stage.gradientsDefs.appendChild(this.gradient);
    this.element.setAttribute("fill", "url(#" + this.gradient.id + ")");

And in this function:
    var transform = this.stage.element.createSVGTransform();
    transform.setRotate(Stage.randomInt(0, 359), 0, 0);
    this.gradient.gradientTransform.baseVal.initialize(transform);

    var stops = this.gradient.querySelectorAll("stop");
    stops[0].setAttribute("stop-color", "hsl(" + this.stage.colorOffset + ", 70%, 45%)");
    stops[1].setAttribute("stop-color", "hsl(" + ((this.stage.colorOffset + Stage.randomInt(50,100)) % 360) + ", 70%, 65%)");

> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:96
> +        var particleTypes = document.getElementById("shapes").childNodes;
> +        this.particleTypeCount = 0;
> +        for (var i = 0; i < particleTypes.length; ++i) {
> +            if (particleTypes[i].nodeType == Node.ELEMENT_NODE)
> +                this.particleTypeCount++;
> +        }

Here you are looping through all the children nodes to know the number of <clipPath> inside <defs> element. But above when you were trying to get a random one of them you used the id: var shapeId = "#shape-" + ... I think this is inconsistent. So I would suggest: 

    this.particleTypeCount = document.querySelectorAll("[id^='shape-']").length; 
Or
    this.particleTypeCount = document.querySelectorAll(".shape").length; // assuming you add class="shape" to the <clip-path> elements.

> PerformanceTests/Animometer/tests/master/svg-particles.html:12
> +            <clipPath id="shape-1" clipPathUnits="objectBoundingBox">

Using ID's like "shape-?" makes it difficult to know what shape will be displayed. Can't we use real names like "spades", hearts", etc.? Alternatively, adding a comment before each shape will make things clearer.

[Jon Lee]

Comment on attachment 275762 [details]
Patch

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

>> PerformanceTests/Animometer/tests/dom/resources/dom-particles.js:17
>> +        this.position = new Point(emitLocation.x, emitLocation.y);
> 
> I think "var emitLocation" is not needed.

Removed.

>> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:7
>> +        this.isClipPath = Stage.randomBool();
> 
> Why do not we always clip? Why do we need to test two different things: clipping using path and drawing a path?

The intent is to combine a couple techniques into the same test.

>> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:31
>> +        this.position = new Point(emitLocation.x, emitLocation.y);
> 
> I think "var emitLocation" is not needed.

Removed.

>> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:49
>> +            this.gradient.remove();
> 
> I am not sure about this one. Shouldn't the parent remove the child instead? i.e. this.stage.gradientsDefs.removeChild(this.gradient);

It could, but not necessary.

>> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:50
>> +        var gradientId = "gradient-" + this.stage.gradientsCounter++;
> 
> Why do we have to have a new incremented id every time we create the gradient? Didn't we remove the old one so we can reuse the same id?

Other tests do not do this (look at bouncing-svn-shapes.js). I don't think it's that big of a deal.

>> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:62
>> +        }, {}, this.gradient);
> 
> Is it easier/faster to create a new gradient rather than changing the existing one?  I would suggest doing the following:
> 
> In svg-particles.html:
>     <defs id="gradients">
>         <linearGradient id="default-gradient">
>            <stop offset="0%"/>
>            <stop offset="100%"/>
>        </linearGradient>
>     </defs>
> 
> In the 3SVGParticle constructor:
>     var defaultGradient = document.querySelector("#default-gradient");
>     this.gradient = defaultGradient.cloneNode(true);
>     this.gradient.id = "gradient-" + stage.gradientsCounter++;
>     stage.gradientsDefs.appendChild(this.gradient);
>     this.element.setAttribute("fill", "url(#" + this.gradient.id + ")");
> 
> And in this function:
>     var transform = this.stage.element.createSVGTransform();
>     transform.setRotate(Stage.randomInt(0, 359), 0, 0);
>     this.gradient.gradientTransform.baseVal.initialize(transform);
> 
>     var stops = this.gradient.querySelectorAll("stop");
>     stops[0].setAttribute("stop-color", "hsl(" + this.stage.colorOffset + ", 70%, 45%)");
>     stops[1].setAttribute("stop-color", "hsl(" + ((this.stage.colorOffset + Stage.randomInt(50,100)) % 360) + ", 70%, 65%)");

Done.

>> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:96
>> +        }
> 
> Here you are looping through all the children nodes to know the number of <clipPath> inside <defs> element. But above when you were trying to get a random one of them you used the id: var shapeId = "#shape-" + ... I think this is inconsistent. So I would suggest: 
> 
>     this.particleTypeCount = document.querySelectorAll("[id^='shape-']").length; 
> Or
>     this.particleTypeCount = document.querySelectorAll(".shape").length; // assuming you add class="shape" to the <clip-path> elements.

Done.

>> PerformanceTests/Animometer/tests/master/svg-particles.html:12
>> +            <clipPath id="shape-1" clipPathUnits="objectBoundingBox">
> 
> Using ID's like "shape-?" makes it difficult to know what shape will be displayed. Can't we use real names like "spades", hearts", etc.? Alternatively, adding a comment before each shape will make things clearer.

I didn't think this was an important detail, but I can add it as a comment.

[Jon Lee]

Created attachment 275821 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 275821 [details]
Patch

Looks good to me. Unofficial r=me.

[Dean Jackson]

Comment on attachment 275821 [details]
Patch

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

> PerformanceTests/Animometer/resources/runner/tests.js:49
> +            name: "Suits"

I don't understand this name.

> PerformanceTests/Animometer/tests/dom/particles.html:11
> +            -webkit-mask-image: url(../resources/star.svg);
> +            mask: url(../resources/star.svg#star-mask);
> +        }

Is the 2nd mask to make it work in other browsers?

I was just asking smfr if we should unprefix.

> PerformanceTests/Animometer/tests/dom/resources/dom-particles.js:30
> +        this.element.style.transform = "translate(" + this.position.x + "px, " + this.position.y + "px)" + this.rotater.rotateZ();

Does this.rotator.rotateZ() always prefix a space character?

> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:48
> +        } else {
> +            this.transformSuffix = "scale(" + this.size.x + ")translate(-.5px,-.5px)";
> +        }

Nit: single line block.

Also, I guess we do handle the case where there is no whitespace between functions!

> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:63
> +        this.element.style.transform = "translate(" + this.position.x + "px," + this.position.y + "px)" + this.rotater.rotateZ() + this.transformSuffix;

And here?

[Jon Lee]

Comment on attachment 275821 [details]
Patch

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

>> PerformanceTests/Animometer/resources/runner/tests.js:49
>> +            name: "Suits"
> 
> I don't understand this name.

Particle shapes are card suits.

>> PerformanceTests/Animometer/tests/dom/particles.html:11
>> +        }
> 
> Is the 2nd mask to make it work in other browsers?
> 
> I was just asking smfr if we should unprefix.

Yes, although one references a <mask> in <defs> and the other references a <path> outside of <defs>

>> PerformanceTests/Animometer/tests/dom/resources/dom-particles.js:30
>> +        this.element.style.transform = "translate(" + this.position.x + "px, " + this.position.y + "px)" + this.rotater.rotateZ();
> 
> Does this.rotator.rotateZ() always prefix a space character?

No, but it doesn't seem to matter.

>> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:48
>> +        }
> 
> Nit: single line block.
> 
> Also, I guess we do handle the case where there is no whitespace between functions!

Removed. And yes.

>> PerformanceTests/Animometer/tests/master/resources/svg-particles.js:63
>> +        this.element.style.transform = "translate(" + this.position.x + "px," + this.position.y + "px)" + this.rotater.rotateZ() + this.transformSuffix;
> 
> And here?

Same.

[Jon Lee]

Committed r199134: <http://trac.webkit.org/changeset/199134>