Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1254463002:
media: Always enable trace event for "media" category group. (Closed)
|
Created:
5 years, 5 months ago by xhwang Modified:
5 years, 5 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, erikwright+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Always enable trace event for "media" category group.
BUG=510201
TEST=Maually tested with a clean user-data-dir, and "media" is in the manual
pick list in about://tracing.
Committed: https://crrev.com/1e8feadcda949653f061e5733eb548e10ce42b73
Cr-Commit-Position: refs/heads/master@{#340321}
Patch Set 1 #
Total comments: 2
Patch Set 2 : comments addressed #
Total comments: 2
Patch Set 3 : comments addressed #
Messages
Total messages: 25 (8 generated)
xhwang@chromium.org changed reviewers: + nduca@chromium.org
Please check whether this is what you meant :) PTAL!
nduca: Kindly ping?
lgtm nomnomq https://codereview.chromium.org/1254463002/diff/1/base/trace_event/trace_event.h File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1254463002/diff/1/base/trace_event/trace_even... base/trace_event/trace_event.h:920: // Macro to explicitly enable a given category group. Can you add some explanation text on when you have to use this versus when you don't? I think some folks could misread this and think you always have to init categories. wdyt about TRACE_EVENT_WARMUP_CATEGORY as a name so its usage is clearer.
comments addressed
https://codereview.chromium.org/1254463002/diff/1/base/trace_event/trace_event.h File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1254463002/diff/1/base/trace_event/trace_even... base/trace_event/trace_event.h:920: // Macro to explicitly enable a given category group. On 2015/07/23 17:00:53, nduca wrote: > Can you add some explanation text on when you have to use this versus when you > don't? I think some folks could misread this and think you always have to init > categories. > > wdyt about TRACE_EVENT_WARMUP_CATEGORY as a name so its usage is clearer. Done.
nduca: I updated the comment. Do you want to take another look?
xhwang@chromium.org changed reviewers: + dalecurtis@chromium.org
dalecurtis: Please OWNERS review media/ changes.
lgtm w/ future modification q. https://codereview.chromium.org/1254463002/diff/20001/media/base/media.cc File media/base/media.cc (right): https://codereview.chromium.org/1254463002/diff/20001/media/base/media.cc#new... media/base/media.cc:27: TRACE_EVENT_WARMUP_CATEGORY("media"); Should we warm up "audio" too ?
comments addressed
https://codereview.chromium.org/1254463002/diff/20001/media/base/media.cc File media/base/media.cc (right): https://codereview.chromium.org/1254463002/diff/20001/media/base/media.cc#new... media/base/media.cc:27: TRACE_EVENT_WARMUP_CATEGORY("media"); On 2015/07/24 16:22:31, DaleCurtis wrote: > Should we warm up "audio" too ? Good point. Done!
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1254463002/#ps40001 (title: "comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1254463002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1254463002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1254463002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1254463002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1254463002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1254463002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1e8feadcda949653f061e5733eb548e10ce42b73 Cr-Commit-Position: refs/heads/master@{#340321} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
