[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
bugzilla-daemon at webkit.org
Tue Jan 15 19:18:15 PST 2019
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?
More information about the webkit-reviews
mailing list