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

Tue Dec 19 21:35:10 PST 2017

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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #329750|review?                     |review-
              Flags|                            |

--- 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: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.

-- 
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/20171220/271053d2/attachment-0001.html>