Add UI for A/B testing on owned commits.

• Component: New Bugs
• Status: RESOLVED FIXED

Add UI for A/B testing on owned commits.

Created attachment 322989 [details]
Patch

Created attachment 323075 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 323075 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:70
> +        (CommitLog.prototype.label): Remove the logic that adds 'r' prefix to svn revision since some revisions from a repository sometimes are labeled with 'r' sometimes are not.

Why? That sounds like a bug somewhere. revision() should never return a value with 'r' prefix.

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

"RevisionInfo" doesn't convey the fact this is a table row.
I think it's better call it CustomRevisionTableRow or something.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:29
> +class RevisionInfoRow extends RevisionInfoRowBase {

We should call it CustomRevisionTableRevisionTopLevelRow or something.
CustomRevisionTableRevisionOwnerRow? but that's weird because not all top-level repositories are owners.
CustomRevisionTableRevisionMajorRow? that's kind of vague.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:49
> +    addOwnedRevisionInfoRowAsync() { return this.loadOwnedCommits().then(() => this.addOwnedRevisionInfoRow(new OwnedRevisionInfoRow(this))); }
> +
> +    removeOwnedRevisionInfoRow(ownedRevisionRow) { this._ownedRevisionInfoRows.splice(this._ownedRevisionInfoRows.indexOf(ownedRevisionRow), 1); }
> +
> +    changedOwnedRepositoryNames() { return this._changedOwnedRepositoryNames; }
> +
> +    ownedRevisionInfoRows() { return this._ownedRevisionInfoRows; }
> +
> +    addOwnedRevisionInfoRow(ownedRevisionInfoRow) { this._ownedRevisionInfoRows.push(ownedRevisionInfoRow); }

This is a lot of methods only defined in this subclass.
It's usually a good indication that we've got the abstraction wrong.
In general, if you find yourself adding lots of method to expose different kinds of internal objects,
then we've got a wrong abstraction / interface for the job.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:87
> +        return this.labels().reduce((chainedPromise, label) => {
> +            const revision = this._revisionSectionsByLabel[label].revision();
> +            return chainedPromise.then(() => {
> +                return CommitLog.fetchForSingleRevision(this._repository, revision);
> +            }).then((fullCommit) => {
> +                console.assert(fullCommit.length === 1);
> +                const commit = fullCommit[0];
> +                fullCommitList.push(commit);
> +                indexToLabel.push(label);
> +                return commit.fetchOwnedCommits();
> +            });
> +        }, Promise.resolve()).then(() => {

Why are we fetching these values in serial? We should do it in parallel.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:88
> +            CommitLog.diffMultipleOwnedCommits.apply(CommitLog, fullCommitList).forEach((commits, repository) => {

Do CommitLog.diffMultipleOwnedCommits(...fullCommitList).

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:93
> +            this._changedOwnedRepositoryNames = [...this._ownedCommitDifferences.keys()].map((repository) => repository.name()).sort();

Why are we sorting this?

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:100
> +    constructor(rowOwner, repository=null, commitSetMap=null)

Nit: Need a space around =.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:109
> +        const repository = this._owner.repository().findOwnedRepositoryByName(repositoryName);
> +        if (!repository || !this._owner.changedOwnedRepositoryNames().includes(repositoryName))

It's very strange to ask another row about which repositories are owned, etc...
I think this code should really be living in Repository class itself.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:125
> +class RevisionSection {

We shouldn't give this class such a generic name since it could collide with some other class name.
Call it CustomRevisionTableRevision?

Comment on attachment 323075 [details]
Patch

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

>> Websites/perf.webkit.org/ChangeLog:70
>> +        (CommitLog.prototype.label): Remove the logic that adds 'r' prefix to svn revision since some revisions from a repository sometimes are labeled with 'r' sometimes are not.
> 
> Why? That sounds like a bug somewhere. revision() should never return a value with 'r' prefix.

Revisions from same repository could be like '123' and '123.1'. For 123, label will add 'r' as prefix. However, for '123.1', it will not be prefixed with 'r'.
Another way is we specify certain type of characters are allowed to occur in a revision (e.g. (. , -)).

[Ryosuke Niwa]

(In reply to dewei_zhu from comment #4)
> Comment on attachment 323075 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323075&action=review
> 
> >> Websites/perf.webkit.org/ChangeLog:70
> >> +        (CommitLog.prototype.label): Remove the logic that adds 'r' prefix to svn revision since some revisions from a repository sometimes are labeled with 'r' sometimes are not.
> > 
> > Why? That sounds like a bug somewhere. revision() should never return a value with 'r' prefix.
> 
> Revisions from same repository could be like '123' and '123.1'. For 123,
> label will add 'r' as prefix. However, for '123.1', it will not be prefixed
> with 'r'.
> Another way is we specify certain type of characters are allowed to occur in
> a revision (e.g. (. , -)).

I see. This really stems from the fact we don't know whether a given repository uses a number like revision, hash like revision, or a version number. We should probably add a new field to repositories table (e.g. repository_type where type is an enum of {number, hash, version}) so that we can explicitly specify this. We should do it in a separate patch though.

Created attachment 327035 [details]
Patch

Comment on attachment 327035 [details]
Patch

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

> 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'

[Ryosuke Niwa]

Comment on attachment 327035 [details]
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:34
> +        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:46
> +        }
> +        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:48
> +            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:62
> +            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:74
> +    topLevelRepositories() { return Repository.sortByNamePreferringOnesWithURL(this.repositories().filter((repository) => !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:95
> +        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:195
> +            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:196
> +            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:201
> +            [...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:206
> +            if (countForRowsWithUndeterminedRepository === 0 )

Nit: a space after 0.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:209
> +            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:241
> +        const actionName = `owned-commit-${repository.label()}`;

This is not right.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:247
> +        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:260
> +        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:271
> +        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(~)).

Created attachment 328791 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 328791 [details]
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:23
> +        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:74
> -        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:118
> +            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:125
> +                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:129
> +            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.

Created attachment 329750 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 329750 [details]
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:83
> +        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:114
> +            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:120
> +            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:128
> +            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:130
> +            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:158
> +    _constructTableRowForCommitsWithOwner(commitSetMap, repository, ownerRepository, lastRow) {

{ should be on a new line.

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

Need a blank line between functions.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:262
> +            #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.

Comment on attachment 329750 [details]
Patch

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

>> Websites/perf.webkit.org/public/v3/components/combo-box.js:35
>> +            const item = link(candidate, () => null);
> 
> What's up with the empty arrow function?

createLink assumes at least 2 arguments are supplied when called.
Otherwise, onclick event will be registered undefined and an error will be raised at 'callback.call(this, event);' of ComponentBase.createEventHandler.
Another alternative is change ComponentBase.createEventHandler to not call callback when it is undefined.

>> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:262
>> +            }
> 
> Use "#custom-table tr:last-child td.owner-repository-label" instead

This may not work for some cases.
This class should really be renamed to 'last-row-for-owned-repository'. It is not granted the last row of owned repository is the last row of the table.

>> Websites/perf.webkit.org/public/v3/models/commit-log.js:69
>> +        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...

Actually, 'fromRevisionForURL' is an unused variable. Do we actually want 
"label = `r${fromRevisionForURL}-r${this.revision()}`;"?

Created attachment 330009 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 330009 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:13
> +        Introduce 'IntermediateCommitSet' to achieve the goal of specifying owned commits for A/B test.
> +        In order to support configure A/B testing that may need to add/remove owned commits, CommitSet may be the closest thing we can get. However, it is a subclass of DataModelObject,
> +        which means CommitSet is a representation of 'commit_sets' table and can only be updated from server data. Thus, we want something like CustomCommitSet that is not a representation
> +        of database table, but unlike CommitSet, it should store information about commits rather than a revision. As a result, IntermediateCommitSet is introduced. For a longer term, we may replace
> +        CustomCommitSet with IntermediateCommitSet as it carries more information and could potentially simplify some CustomCommitSet related APIs by using commit id instead of commit revision.

Please wrap these long lines after ~120 characters.

> Websites/perf.webkit.org/public/v3/components/button-base.js:10
> +    disable()

Instead of adding two methods like this, just add setDisabled instead. "disabled" is a well understood concept in HTML at large.

> Websites/perf.webkit.org/public/v3/components/button-base.js:32
> +        this.content('button').className = this._enabled ? null : 'disabled';

Instead of adding a class, add a content attribute "disabled"; e.g.
if (this._enabled)
    this.content('button').removeAttribute('disabled')
else
    this.content('button').setAttribute('disabled', '')

> Websites/perf.webkit.org/public/v3/components/button-base.js:63
> +            a.disabled {

And use a[disabled] to select an "a" element with disabled attribute.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:29
> +    _constructTextField()

Rename this to _renderTextField.

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

Nit: No curly brackets around a single line statement.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:104
> +    _renderCandidateList(candidates, inputValue, previouslySelectedCandidateName, selectedCandidate, showCandidateList)
> +    {
> +        this._createCandidateListLazily.evaluate(candidates, inputValue);

I don't think this nesting of function makes sense.
What we need is _renderCandidateList which renders the list as currently done in _createCandidateList,
and then a separate function which updates the currently selected candidate as done in this function.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:108
> +        const previouslySelectedCandidateElement = this._elementByCandidateName.get(previouslySelectedCandidateName);

As I've repeatedly stated, there is no point in creating a map like this.
Just create an array of elements and address them by an index.
In fact, _renderCandidateList should return an array of elements and this function should take it as an arugment
so that this function would get involved whenever the list changes as well.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:112
> +        const selectedCandidateElement = this._elementByCandidateName.get(selectedCandidate);

Ditto. We shouldn't be converting an index to name and back to an element
when we can simply convert an index to its corresponding element.
You could even do: this.content('candidate-list').children[currentCandidateIndex]
but I think an explicit array would be better.

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

This should be renamed to _renderCandidateList.

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

Add a FIXME to use link()'s callback instead, and state why we're doing this instead.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:52
> +                    this._isCustomized = true;
> +                    this._fetchingCommitPromises = [];

You should check that this function hadn't been called multiple times since we've started fetching data.
i.e. you should check that _commitSetMap and _fetchingCommitPromises are the ones we set here.
Otherwise, when the user reacts faster than the network (e.g. network is stable),
we run the risk of overriding what the user did (because e.g. the fetching of data scheduled earlier finishes later).

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

Why does this nee to be every? Who owns commit?
We should use a more descriptive variable name here.
e.g. allCommitSetSpecifiesOwnerCommit.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:141
> +            const hasIncompleteRow = this._hasIncompletedRowForOwnerRepository.get(repository);

We should rename variable to _hasIncompletedOwnedRepository and pass it as an argument instead of implicitly depending on it here.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:175
> +                this._hasIncompletedRowForOwnerRepository.set(repository, true);

Rendering functions shouldn't be depending on instance variable like this.
It happens to work now because _hasIncompletedRowForOwnerRepository's value only gets updated when the list of repositories
but an implicit dependency like that are bad.

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

We should clarify that this is for an owned repository/commit in its name.
e.g. _constructTableRowForIncompleteOwnedCommits

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:209
> +        const changedRepositories = [...commitDiff.keys()];

Use Array.from.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:216
> +            const ownedCommitsIterator = commitDiff.get(targetRepository).values();
> +            for (const commitSet of commitSetMap.values())

Just do: for (let i = 0; i < commitSetList.length; i++) commitSet.setCommitForRepository(targetRepository, ownedCommitList[I])
after converting iterables to arrays.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:222
> +        const cells = [element('td', {class: 'owner-repository-label'}), element('th', comboBox), element('td', {colspan: labelCount * (labelCount + 1)})];

You might wanna define a local variable like numberOfCellsPerConfiguration = labelCount + 1 to make this math clear.
Also, call it as configurationCount instead of labelCount.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:257
> +        this._ownedRepositoriesByOwnerRepository = new Map;

Why not just this._ownerToOwnedRepositories?

> Websites/perf.webkit.org/public/v3/models/commit-set.js:263
> +    fetchFullCommits()

I'd call this fetchCommitLogs instead.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:267
> +            fetchingPromises.push(this.updateCommitForRepositoryWithRevision(repository, commit.revision()));

I think we should extract the logic to fetch commit logs out of updateCommitForRepositoryWithRevision and avoid calling setCommitForRepository.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:271
> +    setCommitForRepository(repository, commit, ownerCommit = null)

Please move this to after updateCommitForRepositoryWithRevision.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:304
> +    updateCommitForRepositoryWithRevision(repository, revision)

This should be renamed to updateRevisionForOwnerRepository or something to clarify the relationship with other commits.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:306
> +        let commit = null;

Move this to the callback.

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

Since this fetch can re-order things, we need to check that the current revision for which we're fetching commit is the one we fetched.
So at minimum, we need to store what the current repository to revision mappings are.

[Ryosuke Niwa]

Comment on attachment 330009 [details]
Patch

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

>> Websites/perf.webkit.org/public/v3/components/combo-box.js:29
>> +    _constructTextField()
> 
> Rename this to _renderTextField.

Actually, do this in didConstructShadowTree instead.

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

We should access this.content('text-field').value instead.

> Websites/perf.webkit.org/public/v3/components/combo-box.js:109
> +        if (previouslySelectedCandidateElement)

We could just do: this.content('candidate-list').querySelector('.selected').classList.remove('selected');

Created attachment 330082 [details]
Patch

Created attachment 330083 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 330083 [details]
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:44
> +                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:155
> +                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.

[Joseph Pecoraro]

Did this land? Should the bug be closed?

Yes, this has been landed in r226259. Sorry, I should have updated this.