[webkit-reviews] review granted: [Bug 177993] Add UI for A/B testing on owned commits. : [Attachment 330083] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 177993: Add UI for A/B testing on owned commits.
https://bugs.webkit.org/show_bug.cgi?id=177993

Attachment 330083: Patch

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




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

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

> Websites/perf.webkit.org/public/v3/components/button-base.js:7
> +    constructor(name, disabled = false)
> +    {
> +	   super(name);
> +	   this._disabled = disabled;

Let's not do this. Just initialize this._disabled = false for now.

In the current custom elements API, constructors aren't really supposed to take
any arguments.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:42
> +		   const candidate = this._candidateNameForCurrentIndex();

Just inline _candidateNameForCurrentIndex here instead of adding the helper
since it's only used once.
Also, what happens when candidate is null?
Also, shouldn't we run the same logic as the one in
_autoCompleteIfOnlyOneMatchingItem()
when there is no currently selected item but there is exactly one item?

> Websites/perf.webkit.org/public/v3/components/combo-box.js:53
> +	   this._renderCandidateListLazily.evaluate(this._candidates,
this.content('text-field').value);
> +	  
this._updateCandidateListLazily.evaluate(this._candidateElementForCurrentIndex(
), this._showCandidateList);

Just inline _candidateElementList here instead of adding the helper.
Also, _renderCandidateList should simply return the candidateElementList
instead of storing it as an instance variable.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:57
> +    _candidateNameForCurrentIndex() { return this._currentCandidateIndex ===
null ? null : this._candidateList[this._currentCandidateIndex]; }
> +    _candidateElementForCurrentIndex() { return this._currentCandidateIndex
=== null ? null : this._candidateElementList[this._currentCandidateIndex]; }

Remove these helper functions.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:88
> +	       this._currentCandidateIndex += forward ? 1 : -1;
> +	       if (this._currentCandidateIndex >= this._candidateList.length)
> +		   this._currentCandidateIndex = this._candidateList.length -
1;
> +	       if (this._currentCandidateIndex < 0)
> +		   this._currentCandidateIndex = 0;

Instead of keep referring to this._currentCandidateIndex,
I think it's cleaner to have a local variable like "newIndex" and assign to it
at the end.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:117
> +	       if (key && searchResult === -1)

A more canonical way to check for the return value of indexOf is searchResult <
0.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:125
> +	   this._candidateElementList = this._candidateList.map((candidate) =>
{

Declare this as a local variable.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:137
> +    }

And return it here.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:4
4
> +		   if (this._fetchingCommitPromises.length)
> +		       return;

I don't think we should exit early here.
In the case the user had clicked on customize multiple times,
we want to use the latest commit sets to override the commit sets
for which we're currently fetching results.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
55
> +		   tableBodyList.push(element('tbody', rows));

Why are we adding an empty tbody here??

> Websites/perf.webkit.org/public/v3/components/minus-button.js:5
> +    constructor(disabled=false)
> +    {
> +	   super('minus-button', disabled);

Please get rid of this.

> Websites/perf.webkit.org/public/v3/components/plus-button.js:6
> +    constructor(disabled=false)
> +    {
> +	   super('plus-button', disabled);
> +    }

Ditto.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:104
> +	       this._ownedCommits.forEach((ownedCommit) =>
ownedCommit.setOwnerCommits(this));

We do that the first time we fetched owned commits, right?
Why do we need to do it here again?

> Websites/perf.webkit.org/public/v3/models/commit-set.js:314
> +	   console.assert(repository instanceof Repository);

You should also remove the entry from this._fetchingPromiseByRepository.
Please add a test where we'd start fetching commit logs but a repository is
removed before the fetching completes.