Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
253313004:
CdmAdapter: Add UMA for output protection queries and results. (Closed)
|
Created:
6 years, 7 months ago by xhwang Modified:
6 years, 7 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, tzik, binji+watch_chromium.org, raymes+watch_chromium.org, asvitkine+watch_chromium.org, feature-media-reviews_chromium.org, teravest+watch_chromium.org, yzshen+watch_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, jar (doing other things), ihf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCdmAdapter: Add UMA for output protection queries and results.
BUG=368743
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268025
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : comments addressed #Patch Set 5 : comments addressed #
Total comments: 12
Patch Set 6 : comments addressed #
Total comments: 2
Patch Set 7 : comments addressed #
Messages
Total messages: 26 (0 generated)
PTAL
https://codereview.chromium.org/253313004/diff/30001/media/cdm/ppapi/cdm_adap... File media/cdm/ppapi/cdm_adapter.h (right): https://codereview.chromium.org/253313004/diff/30001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.h:109: // These are reported to UMA server. Do not change the existing values! Should you hard-code the values to make it harder to make a mistake? https://codereview.chromium.org/253313004/diff/30001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.h:114: OUTPUT_PROTECTION_EXTERNAL_LINK_PROTECTED, ALL_...LINKS_...? https://codereview.chromium.org/253313004/diff/30001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.h:115: OUTPUT_PROTECTION_EXTERNAL_LINK_NOT_PROTECTED, Will be calculated rather than reported?
comments addressed
PTAL again. https://codereview.chromium.org/253313004/diff/30001/media/cdm/ppapi/cdm_adap... File media/cdm/ppapi/cdm_adapter.h (right): https://codereview.chromium.org/253313004/diff/30001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.h:109: // These are reported to UMA server. Do not change the existing values! On 2014/04/30 19:49:03, ddorwin wrote: > Should you hard-code the values to make it harder to make a mistake? Done. https://codereview.chromium.org/253313004/diff/30001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.h:114: OUTPUT_PROTECTION_EXTERNAL_LINK_PROTECTED, On 2014/04/30 19:49:03, ddorwin wrote: > ALL_...LINKS_...? Done. https://codereview.chromium.org/253313004/diff/30001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.h:115: OUTPUT_PROTECTION_EXTERNAL_LINK_NOT_PROTECTED, On 2014/04/30 19:49:03, ddorwin wrote: > Will be calculated rather than reported? Done.
comments addressed
https://codereview.chromium.org/253313004/diff/70001/media/cdm/ppapi/cdm_adap... File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/253313004/diff/70001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.cc:847: if (!uma_for_output_protection_query_reported_) { Why not handle this logic inside the Report function. This isolates the details from the rest of the code and is consistent with the other Report function. Oh, because we reuse that function. hmm. https://codereview.chromium.org/253313004/diff/70001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.cc:904: uma_for_output_protection_positive_result_reported_ = true; A bit weird, but okay. Hmm, maybe we can handle this in ReportOutputProtectionStatus too based on the status. https://codereview.chromium.org/253313004/diff/70001/media/cdm/ppapi/cdm_adap... File media/cdm/ppapi/cdm_adapter.h (right): https://codereview.chromium.org/253313004/diff/70001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.h:111: OUTPUT_PROTECTION_QUERIED = 0, It seems odd to have the "total" value in the same UMA. Shouldn't it be a separate count? https://codereview.chromium.org/253313004/diff/70001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.h:112: OUTPUT_PROTECTION_QUERY_ERROR = 1, This is not "positive". Should we just consider this negative and ignore it? https://codereview.chromium.org/253313004/diff/70001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.h:202: // - Whether a positive result (no unprotected external link) is returned. What about QUERY_ERROR? That's not positive. "Whether" seems wrong. We report queries and positive results (+ query errors) https://codereview.chromium.org/253313004/diff/70001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/253313004/diff/70001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:8851: + <summary>Output protection query status and result.</summary> Should we note that the count is the number of CDM instances that did this?
PTAL again. https://codereview.chromium.org/253313004/diff/70001/media/cdm/ppapi/cdm_adap... File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/253313004/diff/70001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.cc:847: if (!uma_for_output_protection_query_reported_) { On 2014/04/30 23:13:35, ddorwin wrote: > Why not handle this logic inside the Report function. This isolates the details > from the rest of the code and is consistent with the other Report function. > > Oh, because we reuse that function. hmm. Added a helper function to do this. https://codereview.chromium.org/253313004/diff/70001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.cc:904: uma_for_output_protection_positive_result_reported_ = true; On 2014/04/30 23:13:35, ddorwin wrote: > A bit weird, but okay. Hmm, maybe we can handle this in > ReportOutputProtectionStatus too based on the status. Done. https://codereview.chromium.org/253313004/diff/70001/media/cdm/ppapi/cdm_adap... File media/cdm/ppapi/cdm_adapter.h (right): https://codereview.chromium.org/253313004/diff/70001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.h:111: OUTPUT_PROTECTION_QUERIED = 0, On 2014/04/30 23:13:35, ddorwin wrote: > It seems odd to have the "total" value in the same UMA. Shouldn't it be a > separate count? Agreed. But that makes it harder to dig out data. Also, UMA_Private doesn't provide a way to send counts yet :( https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/private/... https://codereview.chromium.org/253313004/diff/70001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.h:112: OUTPUT_PROTECTION_QUERY_ERROR = 1, On 2014/04/30 23:13:35, ddorwin wrote: > This is not "positive". Should we just consider this negative and ignore it? Done. https://codereview.chromium.org/253313004/diff/70001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.h:202: // - Whether a positive result (no unprotected external link) is returned. On 2014/04/30 23:13:35, ddorwin wrote: > What about QUERY_ERROR? That's not positive. > > "Whether" seems wrong. We report queries and positive results (+ query errors) Done. https://codereview.chromium.org/253313004/diff/70001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/253313004/diff/70001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:8851: + <summary>Output protection query status and result.</summary> On 2014/04/30 23:13:35, ddorwin wrote: > Should we note that the count is the number of CDM instances that did this? Done.
comments addressed
lgtm https://codereview.chromium.org/253313004/diff/90001/media/cdm/ppapi/cdm_adap... File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/253313004/diff/90001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.cc:925: // Instead, we will calculate number of negatives using the totally number of nit: total
comments addressed
https://codereview.chromium.org/253313004/diff/90001/media/cdm/ppapi/cdm_adap... File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/253313004/diff/90001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.cc:925: // Instead, we will calculate number of negatives using the totally number of On 2014/05/01 04:41:20, ddorwin wrote: > nit: total Done.
jar@: Please OWNERS review histograms.xml. Thanks!
On 2014/05/01 16:17:13, xhwang wrote: > jar@: Please OWNERS review histograms.xml. Thanks! jar@: Ping!
+asvitkine: Please help OWNERS review histograms.xml. Thanks!
lgtm
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/253313004/110001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/253313004/110001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/253313004/110001
Message was sent while issue was closed.
Change committed as 268025 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
