Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814843005:
media: Add incognito EME playback UMA (Closed)
|
Created:
3 years, 8 months ago by xhwang Modified:
3 years, 4 months ago CC:
apacible+watch_chromium.org, asvitkine+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, erickung+watch_chromium.org, feature-media-reviews_chromium.org, jam, miu+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Add incognito EME playback UMA
BUG=707866
TEST=Manually tested with EME playback in normal and incognito window.
Patch Set 1 #
Total comments: 7
Patch Set 2 : comments #
Total comments: 2
Patch Set 3 : rebase only #Patch Set 4 : fix test #
Total comments: 2
Patch Set 5 : enum #
Total comments: 1
Messages
Total messages: 34 (21 generated)
xhwang@chromium.org changed reviewers: + dalecurtis@chromium.org, ddorwin@chromium.org, msramek@chromium.org
ddorwin: PTAL from EME's perspective dalecurtis: PTAL from UMA/MediaInternals' perspective msramek: PTAL from privacy's perspective
dalecurtis@chromium.org changed required reviewers: + msramek@chromium.org
media/ lgtm assuming privacy stamp. https://codereview.chromium.org/2814843005/diff/1/content/browser/media/media... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/2814843005/diff/1/content/browser/media/media... content/browser/media/media_internals.cc:642: if (player_info.has_cdm && player_info.has_reached_have_enough) Needs && player_info.has_ever_played.
Privacy LGTM, thanks for checking! Incognito-specific metrics are allowed as per the new policy, and I find nothing alarming about this particular one.
LGTM % comments and Dale's comments. https://codereview.chromium.org/2814843005/diff/1/content/browser/media/media... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/2814843005/diff/1/content/browser/media/media... content/browser/media/media_internals.cc:95: bool IsIncognito(int render_process_id) { The function name is different from the method it calls. Should we be consistent? Does that affect the UMA name? Is Chrome OS Guest mode included? https://codereview.chromium.org/2814843005/diff/1/content/browser/media/media... content/browser/media/media_internals.cc:596: void MediaInternals::MediaInternalsUMAHandler::ReportUMAForPipelineStatus( This function name does not make it clear that it should only be called when the player/pipeline is destroyed. Instead, it sounds like it should be called when the status changes. We should probably rename it, though not necessarily here. https://codereview.chromium.org/2814843005/diff/1/content/browser/media/media... content/browser/media/media_internals.cc:640: // Report whether an encrypted playback is in incognito window, excluding Once we have data, we should compare the `false` numbers to our other metrics.
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
xhwang@chromium.org changed reviewers: + isherman@chromium.org
isherman: Please UMA OWNERS review! https://codereview.chromium.org/2814843005/diff/1/content/browser/media/media... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/2814843005/diff/1/content/browser/media/media... content/browser/media/media_internals.cc:596: void MediaInternals::MediaInternalsUMAHandler::ReportUMAForPipelineStatus( On 2017/04/13 16:52:59, ddorwin wrote: > This function name does not make it clear that it should only be called when the > player/pipeline is destroyed. Instead, it sounds like it should be called when > the status changes. We should probably rename it, though not necessarily here. Added a TODO. https://codereview.chromium.org/2814843005/diff/1/content/browser/media/media... content/browser/media/media_internals.cc:640: // Report whether an encrypted playback is in incognito window, excluding On 2017/04/13 16:52:59, ddorwin wrote: > Once we have data, we should compare the `false` numbers to our other metrics. Acknowledged. https://codereview.chromium.org/2814843005/diff/1/content/browser/media/media... content/browser/media/media_internals.cc:642: if (player_info.has_cdm && player_info.has_reached_have_enough) On 2017/04/12 21:38:19, DaleCurtis wrote: > Needs && player_info.has_ever_played. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2814843005/diff/20001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/2814843005/diff/20001/content/browser/media/m... content/browser/media/media_internals.cc:99: DCHECK(render_process_host); I think these might fail actually; the message comes in on IO and is then redirected to UI. https://codereview.chromium.org/2814843005/diff/20001/content/browser/media/m... content/browser/media/media_internals.cc:430: std::tie(it, success) = player_info_map.emplace( Neat hadn't seen this before, but how can we ever get a false for success here?
rebase only
The CQ bit was checked by xhwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Metrics LGTM % a nit: https://codereview.chromium.org/2814843005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814843005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26933: +<histogram name="Media.EME.IsIncognito" enum="Boolean"> nit: (Just in this file), please use a custom enum with labels like "Incognito" and "Non-Incognito".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2814843005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814843005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26933: +<histogram name="Media.EME.IsIncognito" enum="Boolean"> On 2017/04/14 01:10:17, Ilya Sherman wrote: > nit: (Just in this file), please use a custom enum with labels like "Incognito" > and "Non-Incognito". Done.
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, dalecurtis@chromium.org, ddorwin@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2814843005/#ps80001 (title: "enum")
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": 80001, "attempt_start_ts": 1492146070950900,
"parent_rev": "5b6458c1403e5cb2d24c8c29b8443ffd3a806e22", "commit_rev":
"29c5ad207da18352dbc6e2232aacabe74e9a7482"}
The CQ bit was unchecked by commit-bot@chromium.org
Prior attempt to commit was detected, but we were not able to check whether the issue was successfully committed. Please check Git history manually and re-check CQ or close this issue as needed.
On 2017/04/14 07:07:55, commit-bot: I haz the power wrote: > Prior attempt to commit was detected, but we were not able to check whether the > issue was successfully committed. Please check Git history manually and re-check > CQ or close this issue as needed. The CL is landed at https://chromium.googlesource.com/chromium/src.git/+/29c5ad207da18352dbc6e223...
Description was changed from ========== media: Add incognito EME playback UMA BUG=707866 TEST=Manually tested with EME playback in normal and incognito window. ========== to ========== media: Add incognito EME playback UMA BUG=707866 TEST=Manually tested with EME playback in normal and incognito window. ==========
Message was sent while issue was closed.
wez@chromium.org changed reviewers: + wez@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2814843005/diff/80001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/2814843005/diff/80001/content/browser/media/m... content/browser/media/media_internals.cc:102: // This could happen in tests. This also happens if you start a Hangout, for example, and then close the tab - if you have logging enabled then you'll see a slew of these messages. Does that indicate that we're receiving an event when in fact we shouldn't have, after the RenderProcessHost is already gone? Or is that case valid, in which case we should cope with it and avoid even calling this if the RenderProcessHost is already gone? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
