Bug 46745 – Fix perf/class-list-remove test

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Erik Arvidsson]

Fix perf/class-list-remove test

[Erik Arvidsson]

Created attachment 69083 [details]
Patch

[Darin Adler]

Comment on attachment 69083 [details]
Patch

For next time, it would be good to explain what was wrong in the change log and why your fix should work. This is especially helpful if the fix is ineffective and someone else is trying to figure out what you meant to do.

[Ojan Vafai]

Comment on attachment 69083 [details]
Patch

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

> LayoutTests/perf/class-list-remove.html:22
>  function test1(magnitude)
>  {
> +    var s = 'b';
> +    element.className = Array(magnitude).join('a ') + s;
>      element.classList.remove(s);

I worry that this is testing Array.join's perf more than classList.remove. Shouldn't the Array.join be in the setup function? Same with test2.

[Ojan Vafai]

Comment on attachment 69083 [details]
Patch

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

>> LayoutTests/perf/class-list-remove.html:22
>>      element.classList.remove(s);
> 
> I worry that this is testing Array.join's perf more than classList.remove. Shouldn't the Array.join be in the setup function? Same with test2.

What I mean is that you should do something like the following. In the setup function:
classToUse = Array(magnitude).join('a ') + 'b';

Then in the test function:
element.className = classToUse;

[Erik Arvidsson]

Created attachment 69112 [details]
Patch

[Ojan Vafai]

Comment on attachment 69112 [details]
Patch

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

> LayoutTests/perf/class-list-remove.html:4
> +var element, className, s;

Nit: would be nice to have a more descriptive name than s. classToRemove perhaps?

[Erik Arvidsson]

Comment on attachment 69112 [details]
Patch

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

>> LayoutTests/perf/class-list-remove.html:4
>> +var element, className, s;
> 
> Nit: would be nice to have a more descriptive name than s. classToRemove perhaps?

Done.

[Erik Arvidsson]

Created attachment 69115 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 69115 [details]
Patch for landing

Clearing flags on attachment: 69115

Committed r68596: <http://trac.webkit.org/changeset/68596>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.