|
|
DescriptionPort a media metric to TBMv2
Add time_to_play media metric.
BUG=chromium:700160
Review-Url: https://chromiumcodereview.appspot.com/2996233004
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/0328138ea253631ba091567f37f893fdb1427d23
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 #
Messages
Total messages: 33 (11 generated)
johnchen@chromium.org changed reviewers: + nednguyen@google.com
Description was changed from ========== Port a media metric to TBMv2 Add time_to_play media metric. BUG=chromium:700160 ========== to ========== Port a media metric to TBMv2 Add time_to_play media metric. BUG=chromium:700160 ==========
crouleau@chromium.org changed reviewers: + crouleau@chromium.org
Thanks for sending this out! https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:5: found in the LICENSE file. There should be overall description of what mediaMetric is trying to do and what is already done and what is work in progress. something like: "media_metrics uses Chrome trace events to calculate metrics about video playback. It is meant to be used for pages with a <video> element. It is used by videostack-eng@google.com team for regression testing." Also, is there some way to make it clear that we are the owners for this file so people will have us review changes to it? If the framework doesn't provide a formal way then perhaps we should just add a comment saying that crouleau@ and johnchen@ are owners. https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:22: if (type === 'willPlay') { Discussed offline: maybe we should either use "willPlay" or use "play", but not both. Also, can Ned/someone else from speed infra team tell us if willPlay event (which is generated from JavaScript) is the wrong wway to do this? I suspect that we need to add trace events directly into Chrome. https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:37: addTimeValue(histograms, 'time_to_play', timeToPlay); Is there at place where time_to_play is documented in a descriptive way? If there is not, it would be nice to describe it somewhere. Something like: "time_to_play calculates how long after a video is requested to start playing before the video actually starts. If time_to_play regresses, then users will click to play videos and then have to wait longer before the videos start actually playing."
https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:30: timeToPlay = event.start - playStart; How can we be sure that these two events belong to a same video? Does this work correctly if: 1) THere are multiple videos 2) There are multiple videos and some videos are being corrupted & only have "willPlay" but not "playing"?
charliea@chromium.org changed reviewers: + benjhayden@chromium.org, charliea@chromium.org - nednguyen@google.com
Removing nednguyen@, who doesn't do much Javascript work Adding benjhayden@, a tracing/ OWNER who's also an expert in the metrics/histogram world. https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:5: found in the LICENSE file. On 2017/08/18 22:10:03, CalebRouleau wrote: > There should be overall description of what mediaMetric is trying to do and what > is already done and what is work in progress. something like: > > "media_metrics uses Chrome trace events to calculate metrics about video > playback. It is meant to be used for pages with a <video> element. It is used by > mailto:videostack-eng@google.com team for regression testing." > > Also, is there some way to make it clear that we are the owners for this file so > people will have us review changes to it? If the framework doesn't provide a > formal way then perhaps we should just add a comment saying that crouleau@ and > johnchen@ are owners. There's definitely a way to add an OWNERS file and list this file as having explicitly different owners, but I see two problems with that: 1) tracing/ owners won't be able to make small refactoring changes, which happen relatively often in TBMv2, without your approval. 2) You'll be able to submit changes to the file without review from any tracing/ owners, which makes it likely that the stil in this codebase is likely to diverge from that in the rest of the codebase Neither of these is good. Realistically, I think that doing nothing is probably the right thing: if someone wants to make substantive changes to this file, the tracing/ owner should look at the blame lines for this file, see who's responsible for the bulk of it, and make sure to loop you into the review. Understanding the necessity of what reviews to delegate is as important as anything else in the process of becoming a reviewer. +benjhayden@chromium.org, eakuefner@chromium.org, would you agree with this? https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:37: addTimeValue(histograms, 'time_to_play', timeToPlay); On 2017/08/18 22:10:03, CalebRouleau wrote: > Is there at place where time_to_play is documented in a descriptive way? If > there is not, it would be nice to describe it somewhere. Something like: > > "time_to_play calculates how long after a video is requested to start playing > before the video actually starts. If time_to_play regresses, then users will > click to play videos and then have to wait longer before the videos start > actually playing." Yea - I agree with you in this, but I'm not sure there is any great place to describe this right now, beyond just a comment above the mediaMetric comment, which is where we typically do it. It'd be great to do it in a way that surfaced in the dashboard to help users there. If that happens, though, then we need to get into what database we store that string in, etc., which is very likely why it hasn't happened.
Please also write a test for your metric so we can avoid breaking it when we need to refactor. https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:5: found in the LICENSE file. On 2017/08/25 at 19:35:09, charliea wrote: > On 2017/08/18 22:10:03, CalebRouleau wrote: > > There should be overall description of what mediaMetric is trying to do and what > > is already done and what is work in progress. something like: > > > > "media_metrics uses Chrome trace events to calculate metrics about video > > playback. It is meant to be used for pages with a <video> element. It is used by > > mailto:videostack-eng@google.com team for regression testing." > > > > Also, is there some way to make it clear that we are the owners for this file so > > people will have us review changes to it? If the framework doesn't provide a > > formal way then perhaps we should just add a comment saying that crouleau@ and > > johnchen@ are owners. > > There's definitely a way to add an OWNERS file and list this file as having explicitly different owners, but I see two problems with that: > > 1) tracing/ owners won't be able to make small refactoring changes, which happen relatively often in TBMv2, without your approval. > 2) You'll be able to submit changes to the file without review from any tracing/ owners, which makes it likely that the stil in this codebase is likely to diverge from that in the rest of the codebase > > Neither of these is good. Realistically, I think that doing nothing is probably the right thing: if someone wants to make substantive changes to this file, the tracing/ owner should look at the blame lines for this file, see who's responsible for the bulk of it, and make sure to loop you into the review. Understanding the necessity of what reviews to delegate is as important as anything else in the process of becoming a reviewer. > > +benjhayden@chromium.org, eakuefner@chromium.org, would you agree with this? +100, thank you! https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:19: for (const event of model.getDescendantEvents()) { Metric performance doesn't matter terribly much when running benchmarks on the waterfall, but users can and do run metrics interactively in trace viewer and telemetry directly. It's also nice to be good stewards of the bots when possible. Instead of iterating over many thousands of events, please consider the process[es], thread[s], and event container[s] that could contain your events. We provide the model helper system to make this a bit easier. The powerMetric demonstrates finding the browser and renderer processes and their main threads: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr... Please let me know if you have any questions about finding your events efficiently. https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:22: if (type === 'willPlay') { On 2017/08/18 at 22:10:03, CalebRouleau wrote: > Discussed offline: maybe we should either use "willPlay" or use "play", but not both. Also, can Ned/someone else from speed infra team tell us if willPlay event (which is generated from JavaScript) is the wrong wway to do this? I suspect that we need to add trace events directly into Chrome. This is a metric design question, which is mostly up to you to experiment with, but we can help somewhat. I recommend considering two factors: 1. which events more closely reflect the user's experience, and 2. which events are less noisy. Often, the noisier event is more representative of the user's experience, and the less noisy event is easier to bisect when the noisier metric regresses, so we recommend measuring both and defining the less noisy metric as a diagnostic metric of the more user-centric metric. https://codesearch.chromium.org/chromium/src/docs/speed/diagnostic_metrics.md https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:41: function addTimeValue(histograms, name, value) { Instead of defining this helper function, please use histograms.createHistogram(name, unit, samples, opt_options) https://github.com/catapult-project/catapult/blob/master/tracing/tracing/valu...
Updated the CL based on review suggestions, and added tests. Please take another look. Thanks! https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:5: found in the LICENSE file. On 2017/08/28 17:40:15, benjhayden wrote: > On 2017/08/25 at 19:35:09, charliea wrote: > > On 2017/08/18 22:10:03, CalebRouleau wrote: > > > There should be overall description of what mediaMetric is trying to do and > what > > > is already done and what is work in progress. something like: > > > > > > "media_metrics uses Chrome trace events to calculate metrics about video > > > playback. It is meant to be used for pages with a <video> element. It is > used by > > > mailto:videostack-eng@google.com team for regression testing." > > > > > > Also, is there some way to make it clear that we are the owners for this > file so > > > people will have us review changes to it? If the framework doesn't provide a > > > formal way then perhaps we should just add a comment saying that crouleau@ > and > > > johnchen@ are owners. > > > > There's definitely a way to add an OWNERS file and list this file as having > explicitly different owners, but I see two problems with that: > > > > 1) tracing/ owners won't be able to make small refactoring changes, which > happen relatively often in TBMv2, without your approval. > > 2) You'll be able to submit changes to the file without review from any > tracing/ owners, which makes it likely that the stil in this codebase is likely > to diverge from that in the rest of the codebase > > > > Neither of these is good. Realistically, I think that doing nothing is > probably the right thing: if someone wants to make substantive changes to this > file, the tracing/ owner should look at the blame lines for this file, see who's > responsible for the bulk of it, and make sure to loop you into the review. > Understanding the necessity of what reviews to delegate is as important as > anything else in the process of becoming a reviewer. > > > > mailto:+benjhayden@chromium.org, mailto:eakuefner@chromium.org, would you agree with this? > > +100, thank you! Added comments about the purpose of this metric, with emails of people responsible. https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:19: for (const event of model.getDescendantEvents()) { On 2017/08/28 17:40:15, benjhayden wrote: > Metric performance doesn't matter terribly much when running benchmarks on the > waterfall, but users can and do run metrics interactively in trace viewer and > telemetry directly. It's also nice to be good stewards of the bots when > possible. > Instead of iterating over many thousands of events, please consider the > process[es], thread[s], and event container[s] that could contain your events. > We provide the model helper system to make this a bit easier. > The powerMetric demonstrates finding the browser and renderer processes and > their main threads: > https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr... > > Please let me know if you have any questions about finding your events > efficiently. Modified the code to look for events in the renderer processes only. Can't restrict to a particular thread, since the two events we're looking for are raised from different threads. Please let me know if there are any more ways to improve the performance. https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:30: timeToPlay = event.start - playStart; On 2017/08/18 23:50:55, nednguyen (ooo til 08-30) wrote: > How can we be sure that these two events belong to a same video? Does this work > correctly if: > 1) THere are multiple videos > 2) There are multiple videos and some videos are being corrupted & only have > "willPlay" but not "playing"? For now we only support one video on the page, though this would be an area for future improvements. https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:37: addTimeValue(histograms, 'time_to_play', timeToPlay); Added comments in the file header. https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:41: function addTimeValue(histograms, name, value) { On 2017/08/28 17:40:15, benjhayden wrote: > Instead of defining this helper function, please use > histograms.createHistogram(name, unit, samples, opt_options) > https://github.com/catapult-project/catapult/blob/master/tracing/tracing/valu... Done.
https://codereview.chromium.org/2996233004/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/2996233004/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:45: for (const event of rendererHelper.process.getDescendantEvents()) { Thanks, this is good, keep going. Which thread[s] do these events appear on? Are they ThreadSlices or AsyncSlices or object snapshots or samples or FlowEvents? https://codereview.chromium.org/2996233004/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:54: break; We try to make metrics as general as possible in order to support traces taken manually. Can you create the histogram before running this state machine, and add as many samples to it as there are Render events?
johnchen@chromium.org changed reviewers: + dalecurtis@chromium.org
Ben: I've optimized the code to look for media events on specific threads. PTAL. Dale: I'm not certain if it is reliable to look for events on specific threads only. Right now, the code does the following: * Look for WebMediaPlayerImpl::DoLoad on main renderer thread * Look for VideoRendererImpl::Render on compositor thread * Look for AudioRendererImpl::Render on AudioOutputDevice thread My test shows that this works with current Chrome, but is this subject to change? Thanks, John https://codereview.chromium.org/2996233004/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/2996233004/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:45: for (const event of rendererHelper.process.getDescendantEvents()) { On 2017/08/29 23:17:25, benjhayden wrote: > Thanks, this is good, keep going. > Which thread[s] do these events appear on? > Are they ThreadSlices or AsyncSlices or object snapshots or samples or > FlowEvents? The code now looks for the events on specific threads. https://codereview.chromium.org/2996233004/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:54: break; On 2017/08/29 23:17:25, benjhayden wrote: > We try to make metrics as general as possible in order to support traces taken > manually. > Can you create the histogram before running this state machine, and add as many > samples to it as there are Render events? The Render events are raised for each video frame. However, for the purpose of time_to_play, only the first Render event is meaningful. Until we add more metrics that make the additional Render events useful, we can save some time by ignoring the additional events.
On 2017/08/29 22:45:47, johnchen wrote: > Updated the CL based on review suggestions, and added tests. Please take another > look. Thanks! > > https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... > File tracing/tracing/metrics/media_metric.html (right): > > https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... > tracing/tracing/metrics/media_metric.html:5: found in the LICENSE file. > On 2017/08/28 17:40:15, benjhayden wrote: > > On 2017/08/25 at 19:35:09, charliea wrote: > > > On 2017/08/18 22:10:03, CalebRouleau wrote: > > > > There should be overall description of what mediaMetric is trying to do > and > > what > > > > is already done and what is work in progress. something like: > > > > > > > > "media_metrics uses Chrome trace events to calculate metrics about video > > > > playback. It is meant to be used for pages with a <video> element. It is > > used by > > > > mailto:videostack-eng@google.com team for regression testing." > > > > > > > > Also, is there some way to make it clear that we are the owners for this > > file so > > > > people will have us review changes to it? If the framework doesn't provide > a > > > > formal way then perhaps we should just add a comment saying that crouleau@ > > and > > > > johnchen@ are owners. > > > > > > There's definitely a way to add an OWNERS file and list this file as having > > explicitly different owners, but I see two problems with that: > > > > > > 1) tracing/ owners won't be able to make small refactoring changes, which > > happen relatively often in TBMv2, without your approval. > > > 2) You'll be able to submit changes to the file without review from any > > tracing/ owners, which makes it likely that the stil in this codebase is > likely > > to diverge from that in the rest of the codebase > > > > > > Neither of these is good. Realistically, I think that doing nothing is > > probably the right thing: if someone wants to make substantive changes to this > > file, the tracing/ owner should look at the blame lines for this file, see > who's > > responsible for the bulk of it, and make sure to loop you into the review. > > Understanding the necessity of what reviews to delegate is as important as > > anything else in the process of becoming a reviewer. > > > > > > mailto:+benjhayden@chromium.org, mailto:eakuefner@chromium.org, would you > agree with this? > > > > +100, thank you! > > Added comments about the purpose of this metric, with emails of people > responsible. > > https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... > tracing/tracing/metrics/media_metric.html:19: for (const event of > model.getDescendantEvents()) { > On 2017/08/28 17:40:15, benjhayden wrote: > > Metric performance doesn't matter terribly much when running benchmarks on the > > waterfall, but users can and do run metrics interactively in trace viewer and > > telemetry directly. It's also nice to be good stewards of the bots when > > possible. > > Instead of iterating over many thousands of events, please consider the > > process[es], thread[s], and event container[s] that could contain your events. > > We provide the model helper system to make this a bit easier. > > The powerMetric demonstrates finding the browser and renderer processes and > > their main threads: > > > https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr... > > > > Please let me know if you have any questions about finding your events > > efficiently. > > Modified the code to look for events in the renderer processes only. Can't > restrict to a particular thread, since the two events we're looking for are > raised from different threads. Please let me know if there are any more ways to > improve the performance. > > https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... > tracing/tracing/metrics/media_metric.html:30: timeToPlay = event.start - > playStart; > On 2017/08/18 23:50:55, nednguyen (ooo til 08-30) wrote: > > How can we be sure that these two events belong to a same video? Does this > work > > correctly if: > > 1) THere are multiple videos > > 2) There are multiple videos and some videos are being corrupted & only have > > "willPlay" but not "playing"? > > For now we only support one video on the page, though this would be an area for > future improvements. That's fine, Though can you add TODO & assertion that this metric only supports single video on a single page. If people in the future somehow enable this metric on a Youtube video with a video ad on the site, hopefully this will give them a clear error message "this is not supported" instead of outputting flawed metrics. > > https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... > tracing/tracing/metrics/media_metric.html:37: addTimeValue(histograms, > 'time_to_play', timeToPlay); > Added comments in the file header. > > https://codereview.chromium.org/2996233004/diff/1/tracing/tracing/metrics/med... > tracing/tracing/metrics/media_metric.html:41: function addTimeValue(histograms, > name, value) { > On 2017/08/28 17:40:15, benjhayden wrote: > > Instead of defining this helper function, please use > > histograms.createHistogram(name, unit, samples, opt_options) > > > https://github.com/catapult-project/catapult/blob/master/tracing/tracing/valu... > > Done.
dalecurtis@google.com changed reviewers: + dalecurtis@google.com
Want to move this to Gerrit? I thought we turned off submissions via rietveld now?
On 2017/08/30 15:54:23, Dale Curtis wrote: > Want to move this to Gerrit? I thought we turned off submissions via rietveld > now? Catapult still appears to be Rietveld only. I got an error when tried to create the CL on Gerrit.
https://codereview.chromium.org/2996233004/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/2996233004/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:53: if (event.title === 'WebMediaPlayerImpl::DoLoad') { Keep in mind this only works if you ensure that the audio and video tags in our page sets are marked autoplay before setting the src= value -- thus ensuring the fastest possible play time. https://codereview.chromium.org/2996233004/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:64: timeToPlay = event.start - playStart; Probably should be timeToVideoPlay and timeToAudioPlay, and if you want to distill to one metric just record the max of those two as timeToPlay.
benjhayden@chromium.org changed reviewers: - dalecurtis@google.com
What happens when these metrics regress? Do you want to add any Diagnostics like Breakdowns or RelatedEventSets to help diagnose regressions? I can help design diagnostics if you have questions. https://github.com/catapult-project/catapult/blob/master/docs/how-to-write-me... Otherwise, last nit from me, lgtm, thanks! https://codereview.chromium.org/2996233004/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/2996233004/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:49: if (mainThread !== undefined && There's a lot of indentation here that is making it a bit difficult to read. We encourage using early-returns to reduce indentation: if (mainThread === undefined) continue; if (compositorThread === undefined && audioThread === undefined) continue; Also, factoring out these loops into helper functions might help, up to you.
On 2017/08/30 17:25:19, benjhayden wrote: > What happens when these metrics regress? Do you want to add any Diagnostics like > Breakdowns or RelatedEventSets to help diagnose regressions? I can help design > diagnostics if you have questions. > https://github.com/catapult-project/catapult/blob/master/docs/how-to-write-me... This will take some time to design, probably more suitable as an add-on feature. > https://codereview.chromium.org/2996233004/diff/60001/tracing/tracing/metrics... > File tracing/tracing/metrics/media_metric.html (right): > > https://codereview.chromium.org/2996233004/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/media_metric.html:49: if (mainThread !== undefined && > There's a lot of indentation here that is making it a bit difficult to read. > We encourage using early-returns to reduce indentation: > if (mainThread === undefined) continue; > if (compositorThread === undefined && audioThread === undefined) continue; Done. > Also, factoring out these loops into helper functions might help, up to you. More metrics will be added in the near future, and it's probably easier to split into helper functions when this happens (e.g., one function for each group of metrics).
On 2017/08/30 01:42:37, nednguyen wrote: > That's fine, Though can you add TODO & assertion that this metric only supports > single video on a single page. If people in the future somehow enable this > metric on a Youtube video with a video ad on the site, hopefully this will give > them a clear error message "this is not supported" instead of outputting flawed > metrics. I've added a TODO about adding support for multiple video, however I wasn't able to figure out the right way to add an assertion. Is there an example for this?
On 2017/09/06 at 21:54:35, johnchen wrote: > On 2017/08/30 01:42:37, nednguyen wrote: > > That's fine, Though can you add TODO & assertion that this metric only supports > > single video on a single page. If people in the future somehow enable this > > metric on a Youtube video with a video ad on the site, hopefully this will give > > them a clear error message "this is not supported" instead of outputting flawed > > metrics. > > I've added a TODO about adding support for multiple video, however I wasn't able to figure out the right way to add an assertion. Is there an example for this? throw new Error('...'); https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr...
On 2017/09/06 22:37:46, benjhayden wrote: > throw new Error('...'); > https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr... Thanks for the suggestion. The CL has been updated accordingly.
benjhayden: Do you have any additional concerns about this CL, or is it ready for commit? Thanks.
On 2017/09/08 at 14:39:10, johnchen wrote: > benjhayden: Do you have any additional concerns about this CL, or is it ready for commit? Thanks. It looks ready to commit to me.
The CQ bit was checked by johnchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benjhayden@chromium.org Link to the patchset: https://codereview.chromium.org/2996233004/#ps120001 (title: "Throw error if multiple media elements")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1504893556486300, "parent_rev": "c254090dae4641f4610c33a9fb0ad68d8a913bb7", "commit_rev": "0328138ea253631ba091567f37f893fdb1427d23"}
Message was sent while issue was closed.
Description was changed from ========== Port a media metric to TBMv2 Add time_to_play media metric. BUG=chromium:700160 ========== to ========== Port a media metric to TBMv2 Add time_to_play media metric. BUG=chromium:700160 Review-Url: https://chromiumcodereview.appspot.com/2996233004 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
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? |