Bug 182387 - StyleBench: Attribute selectors and other improvements
Summary: StyleBench: Attribute selectors and other improvements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-01 07:07 PST by Antti Koivisto
Modified: 2018-02-07 11:36 PST (History)
6 users (show)

See Also:


Attachments
patch (12.74 KB, patch)
2018-02-01 08:14 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (11.02 KB, patch)
2018-02-07 05:53 PST, Antti Koivisto
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2018-02-01 07:07:25 PST
We should test attribute selectors too.
Comment 1 Antti Koivisto 2018-02-01 08:14:23 PST
Created attachment 332879 [details]
patch
Comment 2 Antti Koivisto 2018-02-07 05:53:20 PST
Created attachment 333279 [details]
patch
Comment 3 Joseph Pecoraro 2018-02-07 11:14:13 PST
Comment on attachment 333279 [details]
patch

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

r=me

> PerformanceTests/StyleBench/resources/style-bench.js:203
> +        if (valueNum==0)
> +            return "";
> +        if (valueNum==1)
> +            return "val";

Normal style would be:

    if (valueNum === 0)
        return "";
    if (valueNum === 1)
        return "val";

Up to you if you want to converge on `==` or `===`.

> PerformanceTests/StyleBench/resources/style-bench.js:204
> +        return `val${ valueNum }`;

Style: Normally you don't use spaces inside the ${...}.

> PerformanceTests/StyleBench/resources/style-bench.js:233
> +        if (operator=='')

Ditto on style. Or here you could even just:

    if (!operator)

> PerformanceTests/StyleBench/resources/style-bench.js:266
> +            result +=  this.randomAttributeSelector();

Style: Accidental double space after the operator.

> PerformanceTests/StyleBench/resources/style-bench.js:493
> +                const count = this.random.number(this.configuration.elementMaximumAttributes) + 1;
> +                for (let i = 0; i < count; ++i)
> +                    element.setAttribute(this.randomAttributeName(), this.randomAttributeValue());

It is bad style to shadow both `i` and `count`. Had you used `var` you'd have run into issues! I'd recommend using different names just for clarity.
Comment 4 Antti Koivisto 2018-02-07 11:34:10 PST
https://trac.webkit.org/r228237
Comment 5 Radar WebKit Bug Importer 2018-02-07 11:36:10 PST
<rdar://problem/37321177>