Bug 155232 – Add a new image test

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

[Jon Lee]

Add a new image test for Animometer

[Jon Lee]

Created attachment 273435 [details]
Patch

[Jon Lee]

Created attachment 273436 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 273436 [details]
Patch

Those images are really small. I think the test would be a bit more realistic if it used larger images with some downscaling.

[Jon Lee]

Created attachment 273469 [details]
Patch

[Dean Jackson]

Comment on attachment 273469 [details]
Patch

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

> PerformanceTests/Animometer/tests/master/resources/leaves.js:17
> +    }, {
> +
> +    sizeMinimum: 25,
> +    sizeRange: 0,

The indentation is a bit weird here.

[Said Abou-Hallawa]

Comment on attachment 273469 [details]
Patch

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

> PerformanceTests/Animometer/tests/master/resources/leaves.js:7
> +        this.element.setAttribute("src", stage.images[Stage.randomInt(0, stage.images.length - 1)].src);

Since the images are different, should not we round among the stages.images instead of selecting a random one? Just to guarantee the fairness of the test.

> PerformanceTests/Animometer/tests/master/resources/leaves.js:35
> +        if (!this._life || this._position.y > this.stage.size.y)

Why do we reset the particle after a certain time of animations? Why don't we keep its position and velocity as is as long as it is animating?

> PerformanceTests/Animometer/tests/master/resources/leaves.js:39
> +            this._position.x = (this._position.x + this.maxPosition.x) % this.maxPosition.x;

Why do we flip the position.x when the particle goes out of the stage? Should not we change the velocity x-direction instead?

> PerformanceTests/Animometer/tests/master/resources/leaves.js:46
> +    }

Should not this be a utility function since it is now repeated in leaves.js and don-particle.js? Also why we are using Math.round() here and not using it in DOMParticle.move()?

> PerformanceTests/Animometer/tests/master/resources/leaves.js:73
> +        var images = this.images;

Do we really need the "images" variable?

> PerformanceTests/Animometer/tests/master/resources/leaves.js:87
> +            images.push(img);

Since we are binding to this, I think this can be replaced by this.images.push(img);. Otherwise I do not see a reason for this binding since images and benchmark are in the scope of this function.

> PerformanceTests/Animometer/tests/master/resources/leaves.js:96
> +        img.addEventListener('load', function onImageLoad(e) {

What is the difference between adding and removing load event listener and just using img.onload = function () { promise.resolve(omg); }?

> PerformanceTests/Animometer/tests/master/resources/leaves.js:97
> +            img.removeEventListener('load', onImageLoad);

Do we need to removeEventListener() after addEventListener()?

> PerformanceTests/ChangeLog:10
> +        the particle goes offscreen, it resets itself somewhere on the stage.

The test is mimicking leaves falling down. So I would expect a leaf stays moving till it reaches the bottom of the stage or it is not needed. This life time makes it hard to track the movement of a specific particle.

[Said Abou-Hallawa]

Sorry I did not intend to set the review flag again. It did happen when resolving the review conflict

[Jon Lee]

Committed r197907: <http://trac.webkit.org/changeset/197907>

[Jon Lee]

Comment on attachment 273469 [details]
Patch

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

>> PerformanceTests/Animometer/tests/master/resources/leaves.js:7
>> +        this.element.setAttribute("src", stage.images[Stage.randomInt(0, stage.images.length - 1)].src);
> 
> Since the images are different, should not we round among the stages.images instead of selecting a random one? Just to guarantee the fairness of the test.

Doing that doesn't reduce the noise of the test.

>> PerformanceTests/Animometer/tests/master/resources/leaves.js:17
>> +    sizeRange: 0,
> 
> The indentation is a bit weird here.

I did this on purpose everywhere, to make it a little easier to read the class definitions. Otherwise most of the code ends up indented 8 spaces.

>> PerformanceTests/Animometer/tests/master/resources/leaves.js:35
>> +        if (!this._life || this._position.y > this.stage.size.y)
> 
> Why do we reset the particle after a certain time of animations? Why don't we keep its position and velocity as is as long as it is animating?

It mitigates some of the visual impact of the ramp removing groups of particles.

>> PerformanceTests/Animometer/tests/master/resources/leaves.js:39
>> +            this._position.x = (this._position.x + this.maxPosition.x) % this.maxPosition.x;
> 
> Why do we flip the position.x when the particle goes out of the stage? Should not we change the velocity x-direction instead?

No bouncing in this animation.

>> PerformanceTests/Animometer/tests/master/resources/leaves.js:46
>> +    }
> 
> Should not this be a utility function since it is now repeated in leaves.js and don-particle.js? Also why we are using Math.round() here and not using it in DOMParticle.move()?

It doesn't need to. Removed.

>> PerformanceTests/Animometer/tests/master/resources/leaves.js:73
>> +        var images = this.images;
> 
> Do we really need the "images" variable?

If I remove the bind below it'd be easier to do. It's also used in line 80.

>> PerformanceTests/Animometer/tests/master/resources/leaves.js:87
>> +            images.push(img);
> 
> Since we are binding to this, I think this can be replaced by this.images.push(img);. Otherwise I do not see a reason for this binding since images and benchmark are in the scope of this function.

Actually, I'm not sure why the bind is there.

>> PerformanceTests/Animometer/tests/master/resources/leaves.js:96
>> +        img.addEventListener('load', function onImageLoad(e) {
> 
> What is the difference between adding and removing load event listener and just using img.onload = function () { promise.resolve(omg); }?

you're right.

>> PerformanceTests/Animometer/tests/master/resources/leaves.js:97
>> +            img.removeEventListener('load', onImageLoad);
> 
> Do we need to removeEventListener() after addEventListener()?

Removed with the alternate proposal.