[webkit-reviews] review denied: [Bug 184419] Write a script that detects chart changes by using v3 API. : [Attachment 339165] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied dewei_zhu at apple.com's request for
review:
Bug 184419: Write a script that detects chart changes by using v3 API.
https://bugs.webkit.org/show_bug.cgi?id=184419

Attachment 339165: Patch

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




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

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

We still need tests.

> Websites/perf.webkit.org/public/v3/async-task.js:34
> +	   return typeof module === 'undefined';

We should just check directly typeof Worker.

> Websites/perf.webkit.org/public/v3/models/bug.js:33
> +
> +

Reduce this to one blank line?

> Websites/perf.webkit.org/tools/run-analysis.js:32
> +	   global.RemoteAPI = new RemoteAPI(serverConfig.server);

I don't think this makes sense. We should just construct a local instance.

> Websites/perf.webkit.org/tools/run-analysis.js:43
> +    analyzer.showAnalysisStatistics();

I don't think we need this logging because we're logging whenever we analyze a
new configuration.

> Websites/perf.webkit.org/tools/run-analysis.js:105
> +    async analyzeMeasurementSet(platform, metric)

I think it's still better for this function to take in measurementSet.
Finding platform & metric is cheap enough.
We should just assert that they're not null right away.

> Websites/perf.webkit.org/tools/run-analysis.js:108
> +	   this._logger.info(`\n==== "${metric.fullName()}" on
"${platform.name()}" ====\n`);

I don't think we need to create blank lines before/after this.

> Websites/perf.webkit.org/tools/run-analysis.js:128
> +		   description: `series index range: [${range.startIndex} -
${range.endIndex}]`

Remove this.

> Websites/perf.webkit.org/tools/run-analysis.js:147
> +	   }).sort((a, b) => {
> +	       if (a.valueChangeSummary.changeType ===
b.valueChangeSummary.changeType)
> +		   return Math.abs(a.valueChangeSummary.relativeChange) -
Math.abs(b.valueChangeSummary.relativeChange);
> +	       return a.valueChangeSummary.changeType === progressionString ?
-1 : 1;
> +	   });

I don't think we should be sorting ranges like this.
Just find the one with the biggest value instead.

> Websites/perf.webkit.org/tools/run-analysis.js:157
> +	       this._logger.log('Detected:', summary);

I don't think we should be making logging like this.