Bug 159242 – Update focus test

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jon Lee]

Update focus test

[Radar WebKit Bug Importer]

<rdar://problem/27070007>

[Jon Lee]

Created attachment 282309 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 282309 [details]
Patch

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

Looks good to me. Unofficial r=me.

> PerformanceTests/Animometer/tests/master/resources/focus.js:12
> +var movementDuration = 2500;

Should not these be const?

> PerformanceTests/Animometer/tests/master/resources/focus.js:27
> +        this.particle = document.createElement('div');

Don't we need to append this.particle the childList of some parent? stage.element.appendChild(obj.particle);

> PerformanceTests/Animometer/tests/master/resources/focus.js:38
> +        this.animate(stage, 0, 0);

Why do we need this call here? The FocusElement.animate() function will be called anyway from FocusStage.animate(). It is strange also we call this function before appending this.particle to any parent.

> PerformanceTests/Animometer/tests/master/resources/focus.js:-97
> -        Utilities.setElementPrefixedProperty(this._centerElement, "filter", "blur(" + blur + "px)");

Why did we remove the center element? Is this because we want more accurate results?

[Jon Lee]

Comment on attachment 282309 [details]
Patch

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

>> PerformanceTests/Animometer/tests/master/resources/focus.js:12
>> +var movementDuration = 2500;
> 
> Should not these be const?

I avoided it to keep things simple.

>> PerformanceTests/Animometer/tests/master/resources/focus.js:27
>> +        this.particle = document.createElement('div');
> 
> Don't we need to append this.particle the childList of some parent? stage.element.appendChild(obj.particle);

That's done in tune().

>> PerformanceTests/Animometer/tests/master/resources/focus.js:38
>> +        this.animate(stage, 0, 0);
> 
> Why do we need this call here? The FocusElement.animate() function will be called anyway from FocusStage.animate(). It is strange also we call this function before appending this.particle to any parent.

The order is that we create the particle, add it, and then style it. It is possible that before we style it in animate() we paint after adding, in which case you get a flash of opaque circles.

>> PerformanceTests/Animometer/tests/master/resources/focus.js:-97
>> -        Utilities.setElementPrefixedProperty(this._centerElement, "filter", "blur(" + blur + "px)");
> 
> Why did we remove the center element? Is this because we want more accurate results?

Remove some variability.

[Dean Jackson]

Comment on attachment 282309 [details]
Patch

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

> PerformanceTests/Animometer/tests/dom/resources/focus.js:16
> +        // size and blurring are a function of depth

Nit: We do comments with sentence case and periods.

> PerformanceTests/Animometer/tests/dom/resources/focus.js:137
> +        // update center element before loop

Here too.

>>> PerformanceTests/Animometer/tests/master/resources/focus.js:12
>>> +var movementDuration = 2500;
>> 
>> Should not these be const?
> 
> I avoided it to keep things simple.

I'm not sure if all other browsers support const.

>>> PerformanceTests/Animometer/tests/master/resources/focus.js:27
>>> +        this.particle = document.createElement('div');
>> 
>> Don't we need to append this.particle the childList of some parent? stage.element.appendChild(obj.particle);
> 
> That's done in tune().

Nit: Use " not '

[Jon Lee]

Comment on attachment 282309 [details]
Patch

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

>> PerformanceTests/Animometer/tests/dom/resources/focus.js:16
>> +        // size and blurring are a function of depth
> 
> Nit: We do comments with sentence case and periods.

Done.

>> PerformanceTests/Animometer/tests/dom/resources/focus.js:137
>> +        // update center element before loop
> 
> Here too.

Removed this instead.

>>>> PerformanceTests/Animometer/tests/master/resources/focus.js:27
>>>> +        this.particle = document.createElement('div');
>>> 
>>> Don't we need to append this.particle the childList of some parent? stage.element.appendChild(obj.particle);
>> 
>> That's done in tune().
> 
> Nit: Use " not '

Done.

[Jon Lee]

Committed r202601: <http://trac.webkit.org/changeset/202601>