Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
878633004:
Add additional UMAs to keep track of unprefixed EME promise results (Closed)
|
Created:
5 years, 11 months ago by jrummell Modified:
5 years, 10 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd additional UMAs to keep track of unprefixed EME promise results
Also renames existing CreateSession to GenerateRequest to better match
the purpose of the promise.
BUG=351501
TEST=existing EME tests pass
Committed: https://crrev.com/1737fefb92ae7152ad6eeffb25919fcd6fd5e13a
Cr-Commit-Position: refs/heads/master@{#313609}
Patch Set 1 #
Total comments: 8
Patch Set 2 : suffixes #
Total comments: 2
Patch Set 3 : less text #Patch Set 4 : rebase #
Messages
Total messages: 17 (5 generated)
jrummell@chromium.org changed reviewers: + sandersd@chromium.org
PTAL.
lgtm
jrummell@chromium.org changed reviewers: + isherman@chromium.org
+isherman@ for OWNERS review of histograms.xml. Losing access to the old CreateSession UMA is not that important as the functionality was behind a flag (which is in the process of being removed).
https://codereview.chromium.org/878633004/diff/1/media/blink/webcontentdecryp... File media/blink/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/878633004/diff/1/media/blink/webcontentdecryp... media/blink/webcontentdecryptionmodulesession_impl.cc:31: const char kRemoveSessionUMAName[] = "RemoveSession"; nit: Mebbe alphabetize? https://codereview.chromium.org/878633004/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/878633004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12350: -<histogram name="Media.EME.ClearKey.CreateSession" enum="CdmPromiseResult"> Please mark the old histogram as <obsolete>. https://codereview.chromium.org/878633004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12419: -<histogram name="Media.EME.Unknown.CreateSession" enum="CdmPromiseResult"> Ditto. https://codereview.chromium.org/878633004/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/878633004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12574: +</histogram> It might be worth using a <histogram_suffixes> element to reduce on the amount of repetition in these histogram descriptions.
Updated to use <histogram_suffixes>. https://codereview.chromium.org/878633004/diff/1/media/blink/webcontentdecryp... File media/blink/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/878633004/diff/1/media/blink/webcontentdecryp... media/blink/webcontentdecryptionmodulesession_impl.cc:31: const char kRemoveSessionUMAName[] = "RemoveSession"; On 2015/01/27 00:28:31, Ilya Sherman wrote: > nit: Mebbe alphabetize? Done. https://codereview.chromium.org/878633004/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/878633004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12350: -<histogram name="Media.EME.ClearKey.CreateSession" enum="CdmPromiseResult"> On 2015/01/27 00:28:31, Ilya Sherman wrote: > Please mark the old histogram as <obsolete>. Done. https://codereview.chromium.org/878633004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12419: -<histogram name="Media.EME.Unknown.CreateSession" enum="CdmPromiseResult"> On 2015/01/27 00:28:31, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/878633004/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/878633004/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:12574: +</histogram> On 2015/01/27 00:28:31, Ilya Sherman wrote: > It might be worth using a <histogram_suffixes> element to reduce on the amount > of repetition in these histogram descriptions. Done.
LGTM % a phrasing suggestion: https://codereview.chromium.org/878633004/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/878633004/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:59532: + code."/> The resulting label for e.g. the Widevine histogram is currently "Result of promises for Widevine key systems that were handled by Chromium code. Result of GenerateRequest promises that were handled by Chromium code.". I'd suggest using something a little less redundant, e.g. "Result of promises for Widevine key systems that were handled by Chromium code, restricted to GenerateRequest promises."
Thanks for the reviews. https://codereview.chromium.org/878633004/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/878633004/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:59532: + code."/> On 2015/01/27 01:24:02, Ilya Sherman wrote: > The resulting label for e.g. the Widevine histogram is currently "Result of > promises for Widevine key systems that were handled by Chromium code. Result of > GenerateRequest promises that were handled by Chromium code.". I'd suggest > using something a little less redundant, e.g. "Result of promises for Widevine > key systems that were handled by Chromium code, restricted to GenerateRequest > promises." Done. It looks like this text is displayed on a separate line, so no comma.
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/878633004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...) win8_chromium_gn_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/878633004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1737fefb92ae7152ad6eeffb25919fcd6fd5e13a Cr-Commit-Position: refs/heads/master@{#313609} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
