[webkit-reviews] review denied: [Bug 191557] Perf dashboard should show testability warning when scheduling an A/B task. : [Attachment 354587] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied dewei_zhu at apple.com's request for
review:
Bug 191557: Perf dashboard should show testability warning when scheduling an
A/B task.
https://bugs.webkit.org/show_bug.cgi?id=191557

Attachment 354587: Patch

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




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

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

> Websites/perf.webkit.org/init-database.sql:104
> +    commit_testability_warning text DEFAULT NULL,

text is a heavy handed approach. varchar(256) should do it.

> Websites/perf.webkit.org/public/api/update-commit-testability.php:23
> +	   if (!array_key_exists('testabilityWarning', $commit_info))
> +	       exit_with_error('MissingTestabilityWarning', array('commit' =>
$commit_info));

Rather than adding a new API, why don't we just update /api/report-commits?

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:193
> +	   this._renderRepositoryPanesLazily.evaluate(triggerable, error,
this._selectedPlatform,this._repositoryGroupByConfiguration,

Nit: Need a space after ",".

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

What's up with the insertion of a new line here?

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:310
> +	   const currentComparison = this._commitSetMap['Comparison'];

Is this some kind of a bug fix? If so, can we add a test?
At minimum, please add a description as to what why we're changing this
function.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:312
> +	   const sameBaselineConfig = currentBaseline == newBaseline ||
(currentBaseline && newBaseline && currentBaseline.equals(newBaseline));
> +	   const sameComparisionConfig = currentComparison == newComparison ||
(currentComparison && newComparison &&
currentComparison.equals(newComparison));

Rather than repeating the same logic twice, why don't we create a lambda?
e.g. areCommitSetsEqual = (a, b) = a == b || (a && a.equals(b));

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:352
> +	       this._fetchCommitByRevision(repository, revision);

We shouldn't be fetching revisions as a side effect of rendering them.
r- because of this. It should be done when a commit set is set or when the user
starts mutating them.
Otherwise, we'd delay fetching of commit sets by up to 16ms while we wait for
requestAnimationFrame.
In general, render() functions shouldn't have side-effects.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:364
> +	   if (!this._commitByRepositoryAndRevision.has(repository))
> +	       this._commitByRepositoryAndRevision.set(repository, new Map);

This kind of cache should really exist as a static method on CommitLog instead.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:370
> +	   CommitLog.fetchForSingleRevision(repository,
revision).then((commits) => {

For example, fetchForSingleRevision could have such a cache internally, or we
can wrap it in a helper function.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:495
> +	       !commitsWithTestabilityWarning.length ? [] : element('tbody',
{id: 'testability-warning-table'}, commitsWithTestabilityWarning.map((commit)
=>
> +		   element('tr', element('td', {colspan: 2},
`${commit.repository().name()} - ${commit.label()}:
${commit.testabilityWarning()}`))))];

This code is way too much to inline like this.
Why don't we add a new method which loops over repositories and generates this
tbody instead?

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:528
> -	   const input = element('input', {value: revision, oninput: () => {
> +	   const input = element('input', {value: revision, onchange: () => {

Why is this change made?
change event wouldn't fire until the input element loses focus. That seems like
a regression to me. 
Again, at minimum, this needs a change log description.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:722
> +	       .revision-table #testability-warning-table,
> +	       .revision-table #testabilty-warning-table tr,
> +	       .revision-table #testability-warning-table td {

Why do these selectors need .revision-table? We don't use
testability-warning-table anywhere else.
Also, it's super weird to call a tbody testability-warning-*table*.
Furthermore, none of the CSS properties specified here are needed for the tbody
itself.

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:725
> +		   font-weight: inherit;

Why do we need this?

>
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator
.js:727
> +		   border: 0px;
> +		   padding: 0px;

Or these?

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
42
> +    _constructTestabilityWarningTable(commitSetMap)

This function doesn't return a table, it returns tbody. It should probably
called like _constructTestabilityWarningRows.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
49
> +	       commitSet.commits().forEach(async (commit) => await
CommitLog.fetchForSingleRevision(commit.repository(), commit.revision()));

Again, constructing a view of something shouldn't randomly fetch things.
We need to be doing this right when we get the commit set.
r- because of this.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
51
> +	       hasCommitWithWarning |=
!!commitSet.commitsWithTestabilityWarnings();
> +	   }

Once we've moved the above line out of here, we can do:
const hasCommitWithWarning = commitSets.some(() =>
!!commitSet.commitsWithTestabilityWarnings());

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
55
> +
> +

Nit: Two blank lines.

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

Looks like this code to generate a human readable label is duplicated here and
custom-analysis-task-configurator.js
Why don't we add a helper function to CommitLog to share code?

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:2
84
> +		       () => this.enqueueToRender(),

Why is this needed? We need an inline change log description here.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:3
06
> +		       this.enqueueToRender();

And here.