[webkit-reviews] review denied: [Bug 192972] Add UI in analysis task page to show commit testability infomation. : [Attachment 358048] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied dewei_zhu at apple.com's request for
review:
Bug 192972: Add UI in analysis task page to show commit testability infomation.
https://bugs.webkit.org/show_bug.cgi?id=192972

Attachment 358048: Patch

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




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

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

The patch mostly looks good but there are two subtle but major problems so r-
for now.

> Websites/perf.webkit.org/ChangeLog:9
> +	   Add UI in custom analysis task configuration and customizable test
group form to
> +	   show testability information.

It seems like this would fit in a single line?

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:61
> +	   this._specifiedRevisions = {'Baseline': new Map, 'Comparison': new
Map};
> +	   this._fetchedCommits = {'Baseline': new Map, 'Comparison': new Map};
> +	   this._invalidRevisionForRepositoryByConfiguration = {'Baseline': new
Map, 'Comparison': new Map};

I specifically didn't do this because people had complained that changing the
platform
(e.g. from MBA to MBP) shouldn't reset which revision they had selected for,
say, WebKit.
So we shouldn't do this. r- because of this. Not sure why I didn't catch this
earlier.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:200
> +	   if (this._showComparison)
> +	       this.renderReplace(this.content('comparison-testability'),
this._buildTestabilityList(this._commitSetMap['Comparison'], 'Comparison',
> +		  
this._invalidRevisionForRepositoryByConfiguration['Comparison']));

Nit: Need { ~ } around this two line statement.
In WebKit coding guideline the number of lines refer to the number of physical
lines, not the number of statements.
Because the coding guideline also prohibits having multiple statements per
line,
each physical line almost always happens to end up having single statement.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:403
> +	       fetchedCommits.delete(repository);

This seems too late. It's problematic that we'd have a stale commit in
this._fetchedCommits until we're done fetching.
That is, the user picks revision A, we'd fetch A.
Then the user picks revision B, but until we fetch A, we'd have B in
this._fetchedCommits.
I think this bug exists before this patch but since we're modifying so much of
the commit fetching logic
that we might as well as fix it while we're at it.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:508
> +	   for (const repository of commitSet ? commitSet.repositories() : [])
{

Why don't we just return early when commitSet is undefined / null?

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:509
> +	       const commit =
this._fetchedCommits[configurationName].get(repository);

This isn't right. There is no guarantee that the newly set commit had already
been fetched.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:516
> +	   if (!entries.length)
> +	       return [];

What the heck is the point of this early return??