Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(146)

Issue 2996233004: Port a media metric to TBMv2 (Closed)

Created:
3 years, 4 months ago by johnchen
Modified:
3 years, 3 months ago
CC:
catapult-reviews_chromium.org, tracing-review_chromium.org
Target Ref:
refs/heads/master
Project:
catapult
Visibility:
Public.

Description

Patch Set 1 #

Total comments: 15

Patch Set 2 : Update to use new media-specific traces from Chrome #

Patch Set 3 : mediaMetric: Look in renderer only; add tests #

Total comments: 4

Patch Set 4 : Look for media events on specific threads #

Total comments: 3

Patch Set 5 : Updates based on review feedback #

Patch Set 6 : Update media_metric tests #

Patch Set 7 : Throw error if multiple media elements #

Unified diffs Side-by-side diffs Delta from patch set Stats (+194 lines, -0 lines) Patch
M tracing/trace_viewer.gypi View 1 chunk +1 line, -0 lines 0 comments Download
M tracing/tracing/metrics/all_metrics.html View 1 chunk +1 line, -0 lines 0 comments Download
A tracing/tracing/metrics/media_metric.html View 1 2 3 4 5 6 1 chunk +115 lines, -0 lines 0 comments Download
A tracing/tracing/metrics/media_metric_test.html View 1 2 3 4 5 1 chunk +77 lines, -0 lines 0 comments Download

Messages

Total messages: 33 (11 generated)
johnchen
3 years, 4 months ago (2017-08-18 21:26:12 UTC) #2
CalebRouleau
Thanks for sending this out! https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/media_metric.html File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/media_metric.html#newcode5 tracing/tracing/metrics/media_metric.html:5: found in the LICENSE ...
3 years, 4 months ago (2017-08-18 22:10:03 UTC) #5
nednguyen
https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/media_metric.html File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/media_metric.html#newcode30 tracing/tracing/metrics/media_metric.html:30: timeToPlay = event.start - playStart; How can we be ...
3 years, 4 months ago (2017-08-18 23:50:56 UTC) #6
charliea (OOO until 10-5)
Removing nednguyen@, who doesn't do much Javascript work Adding benjhayden@, a tracing/ OWNER who's also ...
3 years, 3 months ago (2017-08-25 19:35:10 UTC) #8
benjhayden
Please also write a test for your metric so we can avoid breaking it when ...
3 years, 3 months ago (2017-08-28 17:40:15 UTC) #9
johnchen
Updated the CL based on review suggestions, and added tests. Please take another look. Thanks! ...
3 years, 3 months ago (2017-08-29 22:45:47 UTC) #10
benjhayden
https://codereview.chromium.org/2996233004/diff/40001/tracing/tracing/metrics/media_metric.html File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/2996233004/diff/40001/tracing/tracing/metrics/media_metric.html#newcode45 tracing/tracing/metrics/media_metric.html:45: for (const event of rendererHelper.process.getDescendantEvents()) { Thanks, this is ...
3 years, 3 months ago (2017-08-29 23:17:25 UTC) #11
johnchen
Ben: I've optimized the code to look for media events on specific threads. PTAL. Dale: ...
3 years, 3 months ago (2017-08-30 01:39:19 UTC) #13
nednguyen
On 2017/08/29 22:45:47, johnchen wrote: > Updated the CL based on review suggestions, and added ...
3 years, 3 months ago (2017-08-30 01:42:37 UTC) #14
Dale Curtis
Want to move this to Gerrit? I thought we turned off submissions via rietveld now?
3 years, 3 months ago (2017-08-30 15:54:23 UTC) #16
johnchen
On 2017/08/30 15:54:23, Dale Curtis wrote: > Want to move this to Gerrit? I thought ...
3 years, 3 months ago (2017-08-30 15:56:51 UTC) #17
Dale Curtis
https://codereview.chromium.org/2996233004/diff/60001/tracing/tracing/metrics/media_metric.html File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/2996233004/diff/60001/tracing/tracing/metrics/media_metric.html#newcode53 tracing/tracing/metrics/media_metric.html:53: if (event.title === 'WebMediaPlayerImpl::DoLoad') { Keep in mind this ...
3 years, 3 months ago (2017-08-30 16:04:55 UTC) #18
benjhayden
What happens when these metrics regress? Do you want to add any Diagnostics like Breakdowns ...
3 years, 3 months ago (2017-08-30 17:25:19 UTC) #20
johnchen
On 2017/08/30 17:25:19, benjhayden wrote: > What happens when these metrics regress? Do you want ...
3 years, 3 months ago (2017-09-06 21:52:05 UTC) #21
johnchen
On 2017/08/30 01:42:37, nednguyen wrote: > That's fine, Though can you add TODO & assertion ...
3 years, 3 months ago (2017-09-06 21:54:35 UTC) #22
benjhayden
On 2017/09/06 at 21:54:35, johnchen wrote: > On 2017/08/30 01:42:37, nednguyen wrote: > > That's ...
3 years, 3 months ago (2017-09-06 22:37:46 UTC) #23
johnchen
On 2017/09/06 22:37:46, benjhayden wrote: > throw new Error('...'); > https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metrics/system_health/memory_metric.html#L1036 Thanks for the suggestion. ...
3 years, 3 months ago (2017-09-06 23:13:47 UTC) #24
johnchen
benjhayden: Do you have any additional concerns about this CL, or is it ready for ...
3 years, 3 months ago (2017-09-08 14:39:10 UTC) #25
benjhayden
On 2017/09/08 at 14:39:10, johnchen wrote: > benjhayden: Do you have any additional concerns about ...
3 years, 3 months ago (2017-09-08 17:53:38 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2996233004/120001
3 years, 3 months ago (2017-09-08 17:59:23 UTC) #29
commit-bot: I haz the power
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/0328138ea253631ba091567f37f893fdb1427d23
3 years, 3 months ago (2017-09-08 18:31:00 UTC) #32
johnchen
3 years, 3 months ago (2017-09-08 18:37:32 UTC) #33
Message was sent while issue was closed.
The change is now committed. Thanks for everyone's help.

Do I need to do anything for the change to be picked up by Chromium repo?

Powered by Google App Engine
This is Rietveld 408576698