[webkit-reviews] review granted: [Bug 222897] [perf dashboard] Add commit revision label support : [Attachment 424437] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Mar 26 21:42:14 PDT 2021


Ryosuke Niwa <rniwa at webkit.org> has granted Zhifei Fang
<zhifei_fang at apple.com>'s request for review:
Bug 222897: [perf dashboard] Add commit revision label support
https://bugs.webkit.org/show_bug.cgi?id=222897

Attachment 424437: Patch

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




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

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

> Websites/perf.webkit.org/public/v3/models/commit-log.js:61
>      }

Like I pointed out earlier, we should return null explicitly here.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:65
> +	   let formattedRevision =  (() => {

Nit: use const. Two spaces after =

> Websites/perf.webkit.org/public/v3/models/commit-log.js:105
> +	       return `${previousCommit.label()}${separator}${this.label()}`;

Actually, this is basically same as ${formattedFrom}${separator}${formattedTo}
above.
So maybe it's better to do something like this:
	const label = ((fromMatch, toMatch) => {
	    const separator = repositoryType == 'git' ? '..' : (repositoryType
== 'svn' ? '-' : ' - ');
	    const revisionRange =
`${CommitLog._formatttedRevision(fromRevision)}${separator}${CommitLog._formatt
tedRevision(toRevision);}`;
	    if (fromMatch && toMatch) {
		console.assert(fromMatch.groups.branch ==
toMatch.groups.branch);
		return
`${fromMatch.groups.number}-${toMatch.groups.number}@${fromMatch.groups.branch}
(${revisionRange})`;
	    }
	    if (fromMatch || toMatch)
		return `${previousCommit.label()} - ${this.label()}`;
	    return revisionRange;
	})(identifierPattern.exec(previousCommit.revisionIdentifier()),
identifierPattern.exec(this.revisionIdentifier()));


More information about the webkit-reviews mailing list