[webkit-reviews] review denied: [Bug 177993] Add UI for A/B testing on owned commits. : [Attachment 328791] 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 328791: Patch

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




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

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

I think we need to split the changes to CommitLog and the addition of
IntermediateCommitSet into a separate patch to make this patch manageable.

> Websites/perf.webkit.org/ChangeLog:8
> +	   Customizable test group form should support specifying and A/B
testing owned commits.

You should add a high description of what you're adding & what you're changing.

> Websites/perf.webkit.org/ChangeLog:23
> +	   (CustomizableTestGroupForm):

You should really explain why you're replacing dictionaries with map.
In theory, there should be a change log description for each code change you're
making.

> Websites/perf.webkit.org/ChangeLog:61
> +	   (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.

You should explain the difference between IntermediateCommitSet and
CustomCommitSet.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:3
> +    constructor(defaultValue, didUpdateValue, candidates)

This argument order seems backwards. It should take the list of candidates
first and then the default value.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:7
> +	   this._didUpdateValue = didUpdateValue;

Don't use a callback in new code. Use dispatchAction & listenToAction combo
instead.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:13
> +	   this._selectedItemValue = '';

Why isn't this initialized to null instead?

> Websites/perf.webkit.org/public/v3/components/combo-box.js:18
> +    _createCandidateList(candidates, keyword)

I would call "keyword" just "key".

> Websites/perf.webkit.org/public/v3/components/combo-box.js:22
> +	   const startsWithKeywordList = [];

"startsWithKeyword" list is an awkward name. How about
candidatesStartingWithKey?

> Websites/perf.webkit.org/public/v3/components/combo-box.js:23
> +	   const containsWithKeywordList = [];

candidatesContainingKey? contains with keyword doesn't make much sense as a
phrase.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:24
> +	   for (const candidate of candidates) {

You should add some sort of a cap so that we don't end up showing too many
results (might be slow to build a large DOM like that).

> Websites/perf.webkit.org/public/v3/components/combo-box.js:29
> +	       if (keyword &&
!candidateInLowerCase.includes(keyword.toLowerCase()))
> +		   continue;
> +
> +	       if (candidateInLowerCase.startsWith(keyword.toLowerCase()))

Instead of calling includes & startsWith and thereby searching each string
twice, just call indexOf.
If the result is -1, it doesn't contain the keyword. If it's 0, then it starts
with the keyword.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:38
> +	       item.addEventListener('mousedown', () => {

Why are you creating link and then manually attaching an event listener?
Just use link(candidate, () => ~).

> Websites/perf.webkit.org/public/v3/components/combo-box.js:43
> +	       this._elementByCandidateName.set(candidate, item);

Why do we need a map from the candidate to its item like this?
Since each candidate generates exactly one element in your design,
you can just create an array and use _currentCandidateIndex to access the right
element.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:73
> +	       if(event.key === 'Tab' || event.key === 'Enter') {

Nit: Need a space between if and (. Why tab? Why is that special?

> Websites/perf.webkit.org/public/v3/components/combo-box.js:95
> +	   if (!this._inputValue.length)

This function should check the value of _currentCandidateIndex.
If it's not null, then the user had already picked a candidate so there is no
need to do the work here.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:97
> +	   const matchingCandidates = this._candidates.filter((candidate) =>
candidate.toLowerCase().includes(this._inputValue.toLowerCase()));

Why don't we find the first two matching candidates instead of finding all of
them?

> Websites/perf.webkit.org/public/v3/components/combo-box.js:119
> +	   if (candidateElement) {
> +	       if (previousElement)
> +		   previousElement.className = null;
> +	       candidateElement.className = 'selected';

This should be a enqueueToRender call instead.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:130
> +	   textField.value = this._inputValue;

You shouldn't always re-set the input element's value like this.
That would reset the caret position each time, which would be annoying user
experience.

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

Don't use self-closing syntax in HTML5.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:153
> +	       }
> +	       ul:empty {

Please add a blank line between each CSS rule.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:161
> +		   padding-left: 0.3rem;
> +		   padding-right: 0.3rem;
> +		   padding-bottom: 0.2rem;

Use the short hand: padding: 0.1rem 0.3rem.
This would expand to padding-left/right: 0.3rem and padding-top/bottom: 0.1rem.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:168
> +		   margin-top: -0.1rem;

Use 0.1rem padding for top-bottom instead of using a negative margin like this.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:185
> +	       li a:hover, a.selected {

You're missing li for the second selector.
li a:hover, a.selected expands to "li a:hover" and "a.selected", not "li
a.selected".

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
> +function ensureSetForMap(map, key) {

I don't think we need this helper. It's only used once.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:2
3
> +	   for (const label in map)

Do: for (const label of map.keys()) instead. It's more canonical & descriptive
way of iterating over keys of a map.

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:7
4
> -	   this._renderCustomRevisionTableLazily.evaluate(this._commitSetMap,
this._isCustomized);
> +	   this._renderCustomRevisionTable(this._commitSetMap,
this._isCustomized);

This would force the reconstruction of table in each render call. That's not
okay.

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

Why isn't this just a call to every? map(f).every((x) => x) is equivalent to
every(f).

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
25
> +		   const lastRow = index === ownedRepositories.size - 1 &&
incompleteRowCount === 0;

Don't compare against 0:
https://webkit.org/code-style-guidelines/#zero-comparison

>
Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:1
29
> +	       if (incompleteRowCount === 0)

Ditto.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:59
> -	   if (this == previousCommit)
> +	   if (this === previousCommit)

Why ===? This change should be explained in the change log.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:71
> +	   else if (to.length === 40) // e.g. git hash

What's the point of using ===? There's no difference.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:113
> +	       this._ownedCommits.forEach((ownedCommit) => {
> +		   ownedCommit.setOwnerCommits(this);
> +		  
this._ownedCommitByOwnedRepository.set(ownedCommit.repository(), ownedCommit);
> +	       });

We should add tests to verify these changes are working as intended.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:136
> +	   const ownedCommitRepositories = new Set([].concat(
> +	       ...ownedCommitMapList.map((ownedCommitMap) =>
Array.from(ownedCommitMap.keys()))));

All these constructs to concat arrays with spread just to flatten nested arrays
is really hard to read.
Just write two nested loops.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:80
> +    

Nit: Trailing whitespace.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:256
> +function ensureSetForMap(map, key) {

I don't think we need this helper. It's only used once and it's a simple code.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:267
> +class IntermediateCommitSet {
> +
> +    constructor(commitSet)
> +    {
> +	   this._commitByRepository = new Map;
> +	   this._ownedRepositoriesByOwnerRepository = new Map;

Not only do IntermediateCommitSet store commits instead of revision number, it
also stores owned repositories for each commit?
That's a significant difference worth mentioning in the change log.
Also, IntermediateCommitSet is too vague of a name.
Someone who isn't familiar with this code can't tell the difference between
CustomCommitSet and IntermediateCommitSet.