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

Mon Nov 20 17:44:47 PST 2017

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 327035: Patch

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

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

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

New patch looks much better!

> Websites/perf.webkit.org/public/v3/components/combo-box.js:21
> +	   candidates.forEach((candidate) => {

Why don't we just use for (const candidate of candidates) instead?

> Websites/perf.webkit.org/public/v3/components/combo-box.js:46
> +	   if (this._currentCandidateIndex  >=
this._currentCandidateList.length)

Nit: Two spaces before >=.
It's very strange to make this range enforcement inside a function named
_candidateElementForCurrentIndexIndex.
I think it's cleaner to do this inside _didArrowUpOrDown since that's where
these values are incremented / decremented.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:67
> +    _didArrowUpOrDown(isArrowDown)

I think it's better to name this function based on what it does
e.g. _moveCandidate(forward)
Instead of calling this based on when they're called like didArrowUpOrDown.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:71
> +	       this._currentCandidateIndex = (isArrowDown ? 0 :
this._currentCandidateList.length - 1);

There is no need for parenthesis here.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:74
> +	       this._currentCandidateIndex += (isArrowDown ? 1 : -1);

Ditto.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:92
> +	   textField.addEventListener('input', () =>
this.renderReplace(candidateList,
this._createCandidateListLazily.evaluate(this._candidates, textField.value)));

You shouldn't be calling this.renderReplace without outside (i.e. render() is
not in the call stack) like this.
Use this.enqueueToRender() instead. r- because of this.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:94
> +	   textField.addEventListener('focusout', () => {

Use "blur" instead of "focusout" if you're using "focus" event.
"focusout" pairs with "focusin".

> Websites/perf.webkit.org/public/v3/components/combo-box.js:95
> +	       this.content('candidate-list').style.display = 'none';

Don't set the style directly like this. Again, in our components mode,
you should be able to call render() function anytime, and get to exactly the
same state.
So this visibility of the candidate list should be stored as a separate state,
and this should be only calling this.enqueueToRender().

> Websites/perf.webkit.org/public/v3/components/combo-box.js:100
> +	       candidateList.style.display = null;
> +	       this.renderReplace(candidateList,
this._createCandidateListLazily.evaluate(this._candidates, textField.value))

Again, don't directly call this.renderReplace outside render() function.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:105
> +	       if (event.key === 'ArrowDown' || event.key === 'ArrowUp')
> +		   this._didArrowUpOrDown(event.key === 'ArrowDown');
> +	       if(event.key === 'Tab' || event.key === 'Enter')

I'm a bit surprised that event.key contains these human-readable names
since I often end up writing code which compares key code.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:114
> +		   <input id='text-field'/>

Nit: You're indenting twice here.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:131
> +		   transition: background 250ms ease-in;

Nice.

>>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:8
>> +class IntermediateCommitSet {
> 
> I would prefer to move this to the commit-set.js if there is a place to share
common used helper function like 'ensureSetForMap'

Moving this to commit-set.js makes a lot of sense to me.
Since ensureSetForMap is only used once in this class, and it's such a simple
function,
I think it's fine to just duplicate the code there.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:3
4
> +	   if (!ownerCommit)
> +	       return;
> +	   const repositorySet =
ensureSetForMap(this._ownedRepositoriesByOwnerRepository,
ownerCommit.repository());
> +	   repositorySet.add(repository);

Since having an owner comment isn't always expected, it's more semantically
clear to use "if" without an early return as in:
if (ownerCommit) {
    ~
}

We prefer early exists over nested if's but only if follows the normal logic.
i.e. the last statement of the function should be what would "normally" happen.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:4
6
> +	   }
> +	   else if (this._ownedRepositoriesByOwnerRepository.has(repository)) {

Nit: } and else if should be on the same line.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:4
8
> +	       ownedRepositories.forEach((ownedRepository) =>
this._commitByRepository.delete(ownedRepository));

It's usually cleaner to use "for (const X of Y)" loop in cases like this.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:6
2
> +	       return commit.fetchOwnedCommits();

I think it's cleaner to nest promises here as in:
commit.fetchOwnedCommits().then(~).
Because it's confusing that the early return above also ends up running "then"
below.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:7
4
> +    topLevelRepositories() { return
Repository.sortByNamePreferringOnesWithURL(this.repositories().filter((reposito
ry) => !this.ownerCommitForRepository(repository))); }

These aren't all top-level, right? They're owners repositories in this commit
set.
Why don't we call it like highestLevelRepositories or owernessRepositories?

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:9
5
> +	   this._ownerRevisionMap = {};

Why don't we use Map instead? A lot of code does uses {~} but that's mostly for
historical reasons.
In new code, we should use Map whenever we want a hash map.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
95
> +	       const ownsCommits = Object.values(commitSetMap).map((commitSet)
=> commitSet.commitForRepository(repository).ownsCommits()).every((value) =>
value);

Can we add some helper function to Repository or CommitSet
(probably a better place per layering) to do this check?

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
96
> +	       const countForRowsWithUndeterminedRepository =
this._incompletedRowForOwnerRepository.get(repository) || 0;

What does "undetermined repository" mean? Surely we know the repository???
Are we talking about the repository for which all revision values are not known
yet?
If so, something like "incomplete revisions" is probably a better name.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:2
01
> +	       [...ownedRepositories].forEach((ownedRepository, index) => {

What's the point of using spread here? Isn't ownedRepositories just an array?
If it's a set, etc... you can use "for (const repository of ownedRepositories)"
instead,
which probably reads better anyway.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:2
06
> +	       if (countForRowsWithUndeterminedRepository === 0 )

Nit: a space after 0.

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

This would read much saner if we used a Map for commitSetMap:
commitSetMap.values().

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:2
41
> +	   const actionName = `owned-commit-${repository.label()}`;

This is not right.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:2
47
> +	   minusButton.listenToAction(actionName, () => {

Since you're calling listenToAction on each minus button,
you should be able to listen to "activate" (button-base provides this) here.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:2
60
> +	   const actionName =
`incompleted-row-${ownerRepository.label()}-${index}`;

Again, we shouldn't be run-time configuring action name.
Think of action name like Objective-C selector name, or a DOM event name.
They're never runtime configured like this, and there is no need to do that.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:2
71
> +	   const cells = [element('td', {class: 'owner-repository-label'}),
element('th', new ComboBox('', (repositoryName) => {
> +	       const targetRepository = changedRepositories.find((repository)
=> repositoryName === repository.name());
> +	       const ownedCommitsIterator =
commitDiff.get(targetRepository)[Symbol.iterator]();
> +	       Object.values(commitSetMap).forEach((commitSet) => {
> +		   commitSet.setCommitForRepository(targetRepository,
ownedCommitsIterator.next().value);
> +	       });
> +	       const value =
this._incompletedRowForOwnerRepository.get(ownerRepository);
> +	       this._incompletedRowForOwnerRepository.set(ownerRepository,
value - 1);
> +	       this.enqueueToRender();
> +	   }, changedRepositories.map((repository) => repository.name()))),
element('td', {colspan: labelCount * (labelCount + 1)})];

Please split this callback as a separate helper function. It's way too long to
be included in here.

> Websites/perf.webkit.org/public/v3/components/minus-button.js:7
> +    constructor(eventName)
> +    {
> +	   super('minus-button');
> +	   this._eventName = eventName || 'minus-button-event'
> +    }

As I stated where this is used. This code is wrong. Action name should never be
runtime configurable.
There should be no need to differentiate an action dispatched in one button to
another by its name.
There are components which must communicate which item within a single
component is picked,
but they communicate it via an argument to dispatchAction.
However, since each minus button does exactly one thing (removes that one
particular item),
the button shouldn't have to communicate anything to the embedder. r- because
of this.

> Websites/perf.webkit.org/public/v3/components/minus-button.js:27
> +	   a {
> +	       padding-top: 0.12rem;
> +	   }`;

We also shouldn't have to add this.
The CSS used to include this button has a problem instead.

> Websites/perf.webkit.org/public/v3/components/plus-button.js:6
> +	   super('plus-button');
> +	   this._eventName = eventName || 'plus-button-event';

Ditto about not making this runtime configurable.

> Websites/perf.webkit.org/public/v3/components/plus-button.js:28
> +	   a {
> +	       padding-top: 0.12rem;
> +	   }`;

Ditto about this CSS.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:-44
> -	   var revision = this.revision();
> -	   if (parseInt(revision) == revision) // e.g. r12345
> -	       return 'r' + revision;

Again, we really need to come up with a better workaround than simply removing
this feature.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:125
> -    static diffOwnedCommits(previousCommit, currentCommit)
> +    static diffMultipleOwnedCommits(...commits)

What are diff'ing? Are these commits owned commits or owner commits?
The semantics of this function is very unclear from the name or its arguments.
Also, the result of this function appears to be a single map which contains a
single value.
What is the value of that map relative to this function's argument?
All of that should be clear from the function name.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:131
> +	   for (let commit of commits) {
> +	       console.assert(commit);
> +	       console.assert(commit._ownedCommits);
> +	   }

This is rather expensive assertion. Can't we do it as we traverse through
commits?

> Websites/perf.webkit.org/public/v3/models/commit-log.js:134
> +	   const ownedCommitRepositories = new
Set(ownedCommitMapList.map((ownedCommitMap) =>
Array.from(ownedCommitMap.keys())).reduce((a, b) => a.concat(b)));

I think a more canonical way to write would be:
[].concat(...ownedCommitMapList.map(~)).