[webkit-reviews] review granted: [Bug 182387] StyleBench: Attribute selectors and other improvements : [Attachment 333279] patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Antti Koivisto
<koivisto at iki.fi>'s request for review:
Bug 182387: StyleBench: Attribute selectors and other improvements
https://bugs.webkit.org/show_bug.cgi?id=182387

Attachment 333279: patch

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




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 333279
  --> https://bugs.webkit.org/attachment.cgi?id=333279
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.