[webkit-reviews] review granted: [Bug 192972] Add UI in analysis task page to show commit testability information. : [Attachment 358878] Patch

[bugzilla-daemon at webkit.org]

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

Attachment 358878: Patch

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




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

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

r=me assuming the following comments are addressed.

>
Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.
js:56
> +	   await sleep(100);

What is this? Why are we waiting for 100ms?
We should avoid waiting for a fixed amount like this as much as possible.
If there is some kind of timeout in the UI code itself,
then we should be periodically polling to get to a good state instead of
hard-coding a timeout like this.

>
Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.
js:65
> +	   await sleep(100);

Ditto.

>
Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.
js:143
> +	   await sleep(100);

Ditto.

>
Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.
js:152
> +	   await sleep(100);

Ditto.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:17
> -	   this._updateTriggerableLazily = new
LazilyEvaluatedFunction(this._updateTriggerable.bind(this));
> +	   this._invalidRevisionForRepositoryByConfiguration = {'Baseline': new
Map, 'Comparison': new Map};

This variable name is pretty long. I think we can get away by simply
_invalidRevisionsByConfiguration

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:62
> +	       for (const [repository, commit] of
this._fetchedCommits[configuration].entries()) {

We should probably extract this code to create a new Map based on
this._specifiedRevisions[configuration]
as a helper function instead of repeating the same code twice.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:67
> +	       this._fetchedCommits[configuration] =
newFetchedCommitsForConfiguration;

We should avoid seeing it to the new map when there was no change so that the
lazily evaluated function can avoid the work.

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

Why are we rapping the line right after the first argument?? There is a lot of
space after the first argument.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:217
> +	   }

We need to clear this.content('comparison-testability') when
this._showComparison is false.
Note that a single analysis task configurator could be used to show multiple
analysis tasks
when navigating from one analysis task to another.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:409
> +	   try {

We should only wrap the call to CommitLog.fetchForSingleRevision with this try
& catch.
Also, we should assert that the error we got is UnknownCommit.
Otherwise can suppress random exceptions being thrown elsewhere.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:586
> +	       }, 100);

We should probably expose this 100ms delay in some constant like
CustomAnalysisTaskConfigurator.commitFetchInterval
so that the tests can use it instead of hard-coding 100ms there again.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
49
> +	   for (const commitSet of commitSets)
> +	       hasCommitWithTestability |=
!!commitSet.commitsWithTestability();

Should be commitSets.some(() => !!commitSet.commitsWithTestability()).

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
56
> +		   element('tr', {class: 'testability'},
`${commit.repository().name()} - ${commit.label()}: ${commit.testability()}`));

Use commit.label() instead of getting repository name & label manually?