[webkit-reviews] review granted: [Bug 184368] Added a helper function in CommitSet which will be shared between multiple independent incoming changes. : [Attachment 337412] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 184368: Added a helper function in CommitSet which will be shared between
multiple independent incoming changes.
https://bugs.webkit.org/show_bug.cgi?id=184368

Attachment 337412: Patch

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




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

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

r=me assuming the following comments are addressed.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:149
> +    static createNameWithoutCollision(name, hasDuplicateTestGroupName)

Why don't we make this function take a set of names instead of a closure
since most of other call sites you're adding will be using a set.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:204
> +		   nameParts.push(`${repository.name()}:
Patch(${nameForFirstPatch}) - Patch(${nameForSecondPath})`);

We shouldn't make up a new syntax like Patch(a.patch).
Just say: "WebKit: a.patch - b.patch"
Filenames and revision/git hashes look sufficiently different that the
confusion
between the two isn't really a serious practical concern as far as I can tell.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:207
> +		   nameParts.push(`${repository.name()}: ${firstCommit.label()}
with Patch(${nameForFirstPatch}) - ${secondCommit.label()} with
Patch(${nameForSecondPath})`);

Ditto. We shouldn't be using unnecessarily verbose syntax like Patch(a.patch).
Just say: WebKit: r12345 with a.patch - r12346 with b.patch.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:217
> +	       const leftRootFileDescription =
uniqueInFirstCommitSet.map((rootFile) =>
nameGenerator(rootFile.filename())).join(', ');
> +	       const rightRootFileDescription =
uniqueInSecondCommitSet.map((rootFile) =>
nameGenerator(rootFile.filename())).join(', ');

These should firstDescription and secondDescription.
Left & right are very LTR centric way of looking at things.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:218
> +	       nameParts.push(`Roots: ${leftRootFileDescription.length ?
leftRootFileDescription : 'none'} - ${rightRootFileDescription.length ?
rightRootFileDescription : 'none'}`);

"leftRootFileDescription || 'none'" would be sufficient here since '' is
falsey.

> Websites/perf.webkit.org/unit-tests/commit-set-tests.js:334
> +	   });

You need a test case where two repositories' commits differ.