Bug 224496 - Support Commit revision identifier in the Chart
Summary: Support Commit revision identifier in the Chart
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zhifei Fang
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-13 11:27 PDT by Zhifei Fang
Modified: 2021-04-14 20:07 PDT (History)
5 users (show)

See Also:


Attachments
Patch (31.24 KB, patch)
2021-04-13 11:39 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (31.23 KB, patch)
2021-04-13 13:59 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (31.22 KB, patch)
2021-04-13 17:55 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (42.96 KB, patch)
2021-04-13 20:56 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (42.96 KB, patch)
2021-04-13 20:59 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (42.87 KB, patch)
2021-04-14 16:05 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch for landing (42.87 KB, patch)
2021-04-14 18:54 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhifei Fang 2021-04-13 11:27:52 PDT
Support Commit revision identifiter in the Chart
Comment 1 Zhifei Fang 2021-04-13 11:39:41 PDT
Created attachment 425889 [details]
Patch
Comment 2 dewei_zhu 2021-04-13 13:57:12 PDT
Comment on attachment 425889 [details]
Patch

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

r=me

> Websites/perf.webkit.org/ChangeLog:7
> +        In the measurement-set, we include the commit info but not the commit revision identifier column.
> +        Make the API returen the commit revision identifier as part of the commit data.

Nit: this should below `Reviewed by section`
Typo returen -> return

> Websites/perf.webkit.org/server-tests/api-measurement-set-tests.js:501
> +         assert.deepStrictEqual(format(response['formatMap'], currentRows[1]), {

Nit: this indentation doesn't look right.
Comment 3 Zhifei Fang 2021-04-13 13:59:58 PDT
Created attachment 425910 [details]
Patch
Comment 4 Zhifei Fang 2021-04-13 17:55:56 PDT
Created attachment 425938 [details]
Patch
Comment 5 Zhifei Fang 2021-04-13 20:56:13 PDT
Created attachment 425946 [details]
Patch
Comment 6 Chris Dumez 2021-04-13 20:57:11 PDT
Comment on attachment 425946 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:3
> +        Support Commit revision identifiter in the Chart

typo: identifiter
Comment 7 Zhifei Fang 2021-04-13 20:59:04 PDT
Created attachment 425947 [details]
Patch
Comment 8 dewei_zhu 2021-04-14 13:28:23 PDT
Comment on attachment 425947 [details]
Patch

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

r=me

> Websites/perf.webkit.org/browser-tests/customizable-test-group-form-tests.js:96
> +        // click Radio button will enqueue to render, we will get a fresh input element

Lower case R for `Radio`?

> Websites/perf.webkit.org/browser-tests/customizable-test-group-form-tests.js:109
> +        // customizableTestGroupForm will use commitSet.updateRevisionForOwnerRepository to update the revision, need to await this to see the input value get updated to 210948

need to await this to see the input value get updated to 210948 -> need to await this to see the input value *gets* updated to 210948
Comment 9 Ryosuke Niwa 2021-04-14 14:55:26 PDT
Comment on attachment 425947 [details]
Patch

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

>> Websites/perf.webkit.org/browser-tests/customizable-test-group-form-tests.js:96
>> +        // click Radio button will enqueue to render, we will get a fresh input element
> 
> Lower case R for `Radio`?

I don't think this comment is needed.

> Websites/perf.webkit.org/public/api/measurement-set.php:211
> +        // e.g. {"(<commit-id>,<repository-id>,<revision>,<revision_identifier>,<order>,\"2012-10-16 14:53:00\")","(<commit-id>,<repository-id>,<revision>,<revision_identifier>,<order>,)",
> +        // "(<commit-id>,<repository-id>,<revision>,<revision_identifier>,,)", "(<commit-id>,<repository-id>,<revision>,<revision_identifier>,,\"2012-10-16 14:53:00\")"}

Use - instead of _.
Comment 10 Zhifei Fang 2021-04-14 16:05:48 PDT
Created attachment 426056 [details]
Patch
Comment 11 Zhifei Fang 2021-04-14 18:54:45 PDT
Created attachment 426067 [details]
Patch for landing
Comment 12 EWS 2021-04-14 20:06:46 PDT
Committed r275993 (236545@main): <https://commits.webkit.org/236545@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426067 [details].
Comment 13 Radar WebKit Bug Importer 2021-04-14 20:07:16 PDT
<rdar://problem/76681144>