Summary: | StyleBench: Attribute selectors and other improvements | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cdumez, ews-watchlist, joepeck, lforschler, rniwa, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Antti Koivisto
2018-02-01 07:07:25 PST
Created attachment 332879 [details]
patch
Created attachment 333279 [details]
patch
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. |