[Webkit-unassigned] [Bug 177993] Add UI for A/B testing on owned commits.

Thu Dec 21 00:00:03 PST 2017

https://bugs.webkit.org/show_bug.cgi?id=177993

--- Comment #15 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 330009
  --> https://bugs.webkit.org/attachment.cgi?id=330009
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171221/9d23cb7e/attachment.html>