[Webkit-unassigned] [Bug 184419] Write a script that detects chart changes by using v3 API.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=184419

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #339165|review?                     |review-
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180501/ee92cf03/attachment.html>