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

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied 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 329750: Patch

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




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

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

> Websites/perf.webkit.org/ChangeLog:57
> +	   (CommitLog.prototype.label):

You should explain what kind of change you're making and why the change is
needed.

> Websites/perf.webkit.org/ChangeLog:62
> +	   (CommitLog.ownedCommitDifferenceForOwnerCommits): A more generic
version of which compares multiple owned commits.

How generic is new version? A change log comment should clarify that as well as
why this generalization is needed.

> Websites/perf.webkit.org/ChangeLog:69
> +	   (IntermediateCommitSet): In order to support owned commits, commit
set should support mutation as we may add/remove commits for a repository.
> +	   This variant of commit set supports remove/update commits for
repositories. IntermediateCommitSet provides a super set of functionalities of
CustomCommitSet.
> +	   It stores CommitLog object rather than the revision, allows updating
and removing a commit as well as fetching owned commits for a commit.

Why can't we just make CustomCommitSet support all the features of
IntermediateCommitSet then?

> Websites/perf.webkit.org/public/v3/components/combo-box.js:35
> +	       const item = link(candidate, () => null);

What's up with the empty arrow function?

> Websites/perf.webkit.org/public/v3/components/combo-box.js:41
> +	       item.className = candidate == selectedCandidate ? 'selected' :
null;

We shouldn't be re-constructing the entire list each time the current candidate
moves.
Instead, simply update the className in a separate lazily evaluated function.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:75
> +		   this._moveCandidate(event.key === 'ArrowDown');
> +		   this.enqueueToRender();

_moveCandidate should be the one to call enqueueToRender.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:83
> +    _candidateElementForCurrentIndexIndex() { return
this._elementByCandidateName.get(this._candidateNameForCurrentIndex()); }

This function is only used once. It's better to inline the code in where it's
used.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:122
> +    _renderCandidateList(candidates, inputValue, selectedCandidate,
showCandidateList) {

Nit: { should be on a new line.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:125
> +	   this.renderReplace(candidateList,
this._createCandidateListLazily.evaluate(candidates, inputValue,
selectedCandidate));

Again, _createCandidateList should be the one calling renderReplace.
Otherwise, we would mutate the entire tree each time.
Also, please order functions in the order they're used top-down.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:136
> +	   this._constructTextFieldLazily.evaluate();
> +	   this._renderCandidateListLazily.evaluate(this._candidates,
this._inputValue, this._candidateNameForCurrentIndex(),
this._showCandidateList);

Please move _constructTextField and _renderCandidateList after this function.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:8
3
> +	   for (const label of commitSetMap.keys()) {
> +	       for (const repository of
commitSetMap.get(label).highestLevelRepositories()) {

If you're immediately getting the commit set out of it, then you can do 
for (const [label, commitSet] of commitSetMap)
instead.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
14
> +	       const ownsCommits = [...commitSetMap.values()].every((commitSet)
=> commitSet.ownsCommitsForRepository(repository));

Use Array.from(commitSetMap.values()) instead.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
20
> +	       for (const [index, ownedRepository] of
[...ownedRepositories].entries()) {

What the heck is the point of converting Set to an array and then calling
entries on it?
That's a lot of temporary objects being created.
It's better to have a separate local variable for count instead.
e.g.
let index = 0;
for (const ownedRepository of ownedRepositories) {
    ...
    index++;
}
But this whole lastRow thing is completely unnecessary. See my comment below
for CSS.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
28
> +	       const commits = [...commitSetMap.values()].map((commitSet) =>
commitSet.commitForRepository(repository));

Use Array.from(commitSetMap.values()) instead.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
30
> +	       for (let i = 0; i < incompleteRowCount; i++)

It doesn't make much sense to have more than one incomplete row. We should just
forbid that in UI instead.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
58
> +    _constructTableRowForCommitsWithOwner(commitSetMap, repository,
ownerRepository, lastRow) {

{ should be on a new line.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
74
>      }
> +    _constructRowWithoutRepositorySpecified(commitSetMap, ownerRepository,
commitDiff, lastRow)

Need a blank line between functions.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:2
62
> +	       #custom-table .last-owned-row td.owner-repository-label {
> +		   border-bottom: solid 1px #ddd;
> +	       }

Use "#custom-table tr:last-child td.owner-repository-label" instead

> Websites/perf.webkit.org/public/v3/models/commit-log.js:69
> -	   if (parseInt(from) == from) { // e.g. r12345.
> -	       fromRevisionForURL = (parseInt(from) + 1).toString;
> +	   if (parseInt(from) == from)// e.g. r12345.

Why are you removing the logic to add +1? This should clearly needs to be
explained in the change log.
Also, presumably you're only doing this for owned commits so we should be able
to check the repository type,
and not add "r" or add 1 just for owned commits only for owned commits.

In the long term, we should store a new field to the repository table in the
database indicating
the repository type such as SVN, Git, etc...

> Websites/perf.webkit.org/public/v3/models/commit-set.js:259
> +	       CommitLog.fetchForSingleRevision(repository,
commit.revision()).then(() => {

WTF!? Constructor of an object shouldn't have a side-effect like this.
It should be the responsible of the users of this object to fetch data at an
appropriate time.
r- because of this.