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

[bugzilla-daemon at webkit.org]

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 424433: Patch

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




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

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

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:11
> -	   $commit_rows = $db->query_and_fetch_all('SELECT commit_id FROM
commits WHERE commit_repository = $1 AND commit_revision = $2',
array($repository_id, $revision));
> +	   $column_name =
CommitLogFetcher::is_commit_revision_identifier($revision) ?
'commit_revision_identifier' : 'commit_revision';

I'm actually having second thoughts that maybe we shouldn't detect revision
identifier like this in the backend.
But we can address that sometime in the future.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:63
> +    static _formatttedRevision(revision, revisionIdentifier=null) 

Please address the review comment I just posted.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:95
> +		   console.assert(fromMatch.groups.branch ==
toMatch.groups.branch);

Ditto.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:103
> +	   return {repository: repository, label: label, url:
repository.urlForRevisionRange(fromRevision, toRevision)};

Ditto.