[webkit-reviews] review granted: [Bug 230810] Summary page should support calculating summary using weighted mean. : [Attachment 439318] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 230810: Summary page should support calculating summary using weighted
mean.
https://bugs.webkit.org/show_bug.cgi?id=230810

Attachment 439318: Patch

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




--- Comment #5 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 439318
  --> https://bugs.webkit.org/attachment.cgi?id=439318
Patch

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

We really need tests for summary page...

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:17
> +	   this._weightConfigurations = summarySettings.weightConfigurations;

This sounds like the configuration of weights. So I think it should be
weightedConfigurations.
Like excluded configurations, we're listing a list of configurations whose
weight isn't 1 along with its weight.

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:354
> +	   const isNumber = (object) => ['number', 'string'].includes(typeof
object) && !isNaN(+object);
> +	   if (isNumber(weightForPlatform))
> +	       return +weightForPlatform;

This isn't quite right. This will throw an exception if weightForPlatform is
null or true/false as well.
I think what we want to check instead is simply isNaN(+object). i.e.
if (!isNaN(weightForPlatform))
    return weightForPlatform;

> Websites/perf.webkit.org/public/v3/pages/summary-page.js:358
> +	   const weight = weightForPlatform[metricId]
> +	   if (isNumber(weight))
> +	       return +weight;

Here, we probably want to do instead:
if (typeof weightForPlatform != 'object' ||
isNaN(+weightForPlatform[metricId]))
    return 1;