Bug 156273 - Update master benchmark with SVG test
Summary: Update master benchmark with SVG test
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:
Depends on:
Blocks:
 
Reported: 2016-04-05 19:22 PDT by Jon Lee
Modified: 2016-04-06 18:48 PDT (History)
6 users (show)

See Also:


Attachments
Patch (19.72 KB, patch)
2016-04-06 02:13 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (19.65 KB, patch)
2016-04-06 15:07 PDT, Jon Lee
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2016-04-05 19:22:37 PDT
Replace the masks test with an SVG test.
Comment 1 Jon Lee 2016-04-06 02:13:03 PDT
Created attachment 275762 [details]
Patch
Comment 2 Said Abou-Hallawa 2016-04-06 12:58:42 PDT
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.
Comment 3 Jon Lee 2016-04-06 14:16:13 PDT
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.
Comment 4 Jon Lee 2016-04-06 15:07:01 PDT
Created attachment 275821 [details]
Patch
Comment 5 Said Abou-Hallawa 2016-04-06 15:13:42 PDT
Comment on attachment 275821 [details]
Patch

Looks good to me. Unofficial r=me.
Comment 6 Dean Jackson 2016-04-06 15:26:16 PDT
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?
Comment 7 Jon Lee 2016-04-06 18:46:21 PDT
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.
Comment 8 Jon Lee 2016-04-06 18:48:03 PDT
Committed r199134: <http://trac.webkit.org/changeset/199134>