Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
452643002:
Add UMA reporting to CdmPromise. (Closed)
|
Created:
6 years, 4 months ago by sandersd (OOO until July 31) Modified:
6 years, 4 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA reporting to CdmPromise.
This allows all CdmPromise objects to report their result (success or
exception) if given the name of a histogram at construction.
As an example, a UMA for the unprefixed NewSession method is included.
BUG=351501
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289453
Patch Set 1 #
Total comments: 24
Patch Set 2 : Fixes. #Patch Set 3 : Actually add the test. #
Total comments: 8
Patch Set 4 : Use COMPILE_ASSERT. #
Total comments: 2
Patch Set 5 : Fix nit. #
Total comments: 20
Patch Set 6 : Address comments. #
Total comments: 2
Patch Set 7 : Don't use UMA_HISTOGRAM_ENUMERATION. #
Total comments: 2
Patch Set 8 : Add braces. #
Messages
Total messages: 19 (0 generated)
https://codereview.chromium.org/452643002/diff/1/content/renderer/media/cdm_s... File content/renderer/media/cdm_session_adapter.cc (right): https://codereview.chromium.org/452643002/diff/1/content/renderer/media/cdm_s... content/renderer/media/cdm_session_adapter.cc:20: // TODO(sandersd): De-dup with WMPI. It's not the end of the world if they are duplicate since that code will be removed eventually https://codereview.chromium.org/452643002/diff/1/content/renderer/media/cdm_s... File content/renderer/media/cdm_session_adapter.h (right): https://codereview.chromium.org/452643002/diff/1/content/renderer/media/cdm_s... content/renderer/media/cdm_session_adapter.h:92: const std::string& GetKeySystemUMAPrefix(); ... const; https://codereview.chromium.org/452643002/diff/1/content/renderer/media/webco... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/452643002/diff/1/content/renderer/media/webco... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. Questions for discussion: Prefixed EME splits rejections (and even some resolves) up between Blink and Chromium. Should we record all of them? Are we not recording UMAs for total calls? Would total - (success + failure) be meaningful? https://codereview.chromium.org/452643002/diff/1/content/renderer/media/webco... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:18: const char kNewSessionUMASuffix[] = ".NewSession"; s/Suffix/Name/ - you can't have a prefix and suffix without a body. :) Also, consider putting the '.' someplace common. Maybe even have a BuildUMAName() function. https://codereview.chromium.org/452643002/diff/1/content/renderer/media/webco... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:18: const char kNewSessionUMASuffix[] = ".NewSession"; The actual method is createSession. It was just odd naming since we create the WCDMS before. Update the description too. https://codereview.chromium.org/452643002/diff/1/content/renderer/media/webco... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:86: void WebContentDecryptionModuleSessionImpl::initializeNewSession( I think this should be deleted now - it looks like an older version of the API at l139. https://codereview.chromium.org/452643002/diff/1/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/452643002/diff/1/media/base/cdm_promise.cc#ne... media/base/cdm_promise.cc:30: static CdmPromise::ResultCodeForUMA ConvertException( ..ToUMAResult https://codereview.chromium.org/452643002/diff/1/media/base/cdm_promise.cc#ne... media/base/cdm_promise.cc:49: return CdmPromise::UNKNOWN_ERROR; Maybe we should have a UMA value (1) for unexpected. (UNKNOWN is an actual DOM exception.) https://codereview.chromium.org/452643002/diff/1/media/base/cdm_promise.h File media/base/cdm_promise.h (right): https://codereview.chromium.org/452643002/diff/1/media/base/cdm_promise.h#new... media/base/cdm_promise.h:32: UNKNOWN_ERROR, These 3 may be removed someday. Oh well. (It will just leave a gap.) https://codereview.chromium.org/452643002/diff/1/media/base/cdm_promise.h#new... media/base/cdm_promise.h:56: CdmPromise(PromiseRejectedCB reject_cb, const std::string& uma_name); We should probably document that a UMA will be reported when the promise is resolved/rejected when this constructor is used. https://codereview.chromium.org/452643002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/452643002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10369: +<histogram name="Media.EME.ClearKey.NewSession" enum="CdmPromiseResult"> Update the name and comments. https://codereview.chromium.org/452643002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:36732: + <int value="1" label="Not Supported Error"/> These are really exception names, so we could remove the spaces.
https://codereview.chromium.org/452643002/diff/1/content/renderer/media/cdm_s... File content/renderer/media/cdm_session_adapter.cc (right): https://codereview.chromium.org/452643002/diff/1/content/renderer/media/cdm_s... content/renderer/media/cdm_session_adapter.cc:20: // TODO(sandersd): De-dup with WMPI. On 2014/08/08 03:35:40, ddorwin wrote: > It's not the end of the world if they are duplicate since that code will be > removed eventually Done. https://codereview.chromium.org/452643002/diff/1/content/renderer/media/cdm_s... File content/renderer/media/cdm_session_adapter.h (right): https://codereview.chromium.org/452643002/diff/1/content/renderer/media/cdm_s... content/renderer/media/cdm_session_adapter.h:92: const std::string& GetKeySystemUMAPrefix(); On 2014/08/08 03:35:40, ddorwin wrote: > ... const; Done. https://codereview.chromium.org/452643002/diff/1/content/renderer/media/webco... File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right): https://codereview.chromium.org/452643002/diff/1/content/renderer/media/webco... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/08/08 03:35:40, ddorwin wrote: > Questions for discussion: > Prefixed EME splits rejections (and even some resolves) up between Blink and > Chromium. Should we record all of them? > Are we not recording UMAs for total calls? Would total - (success + failure) be > meaningful? That was the original plan, but Xiaohan convinced me that UMAs are not really the place for this kind of debugging. Hopefully we don't need it either, since ContentDecryptorDelegate and ProxyMediaKeys both reject all pending promises when destructed, and AesDecryptor can't fail. https://codereview.chromium.org/452643002/diff/1/content/renderer/media/webco... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:18: const char kNewSessionUMASuffix[] = ".NewSession"; On 2014/08/08 03:35:40, ddorwin wrote: > s/Suffix/Name/ - you can't have a prefix and suffix without a body. :) > Also, consider putting the '.' someplace common. Maybe even have a > BuildUMAName() function. Done. https://codereview.chromium.org/452643002/diff/1/content/renderer/media/webco... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:18: const char kNewSessionUMASuffix[] = ".NewSession"; On 2014/08/08 03:35:40, ddorwin wrote: > The actual method is createSession. It was just odd naming since we create the > WCDMS before. > Update the description too. Done. https://codereview.chromium.org/452643002/diff/1/content/renderer/media/webco... content/renderer/media/webcontentdecryptionmodulesession_impl.cc:86: void WebContentDecryptionModuleSessionImpl::initializeNewSession( On 2014/08/08 03:35:40, ddorwin wrote: > I think this should be deleted now - it looks like an older version of the API > at l139. That's true, but it's still plumbed through to Blink, so I'd rather create separate CLs. https://codereview.chromium.org/452643002/diff/1/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/452643002/diff/1/media/base/cdm_promise.cc#ne... media/base/cdm_promise.cc:30: static CdmPromise::ResultCodeForUMA ConvertException( On 2014/08/08 03:35:40, ddorwin wrote: > ..ToUMAResult Done. https://codereview.chromium.org/452643002/diff/1/media/base/cdm_promise.cc#ne... media/base/cdm_promise.cc:49: return CdmPromise::UNKNOWN_ERROR; On 2014/08/08 03:35:40, ddorwin wrote: > Maybe we should have a UMA value (1) for unexpected. (UNKNOWN is an actual DOM > exception.) I'm a little reluctant to add a debugging feature here, perhaps it would be better to add a unittest that checks for a difference? https://codereview.chromium.org/452643002/diff/1/media/base/cdm_promise.h File media/base/cdm_promise.h (right): https://codereview.chromium.org/452643002/diff/1/media/base/cdm_promise.h#new... media/base/cdm_promise.h:32: UNKNOWN_ERROR, On 2014/08/08 03:35:40, ddorwin wrote: > These 3 may be removed someday. Oh well. (It will just leave a gap.) Acknowledged. https://codereview.chromium.org/452643002/diff/1/media/base/cdm_promise.h#new... media/base/cdm_promise.h:56: CdmPromise(PromiseRejectedCB reject_cb, const std::string& uma_name); On 2014/08/08 03:35:40, ddorwin wrote: > We should probably document that a UMA will be reported when the promise is > resolved/rejected when this constructor is used. Done. https://codereview.chromium.org/452643002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/452643002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:10369: +<histogram name="Media.EME.ClearKey.NewSession" enum="CdmPromiseResult"> On 2014/08/08 03:35:40, ddorwin wrote: > Update the name and comments. Done. https://codereview.chromium.org/452643002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:36732: + <int value="1" label="Not Supported Error"/> On 2014/08/08 03:35:41, ddorwin wrote: > These are really exception names, so we could remove the spaces. Done.
https://codereview.chromium.org/452643002/diff/40001/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/452643002/diff/40001/media/base/cdm_promise.c... media/base/cdm_promise.cc:49: return CdmPromise::NOT_SUPPORTED_ERROR; UNKNOWN was probably better (even though this should never happen). https://codereview.chromium.org/452643002/diff/40001/media/base/cdm_promise.c... media/base/cdm_promise.cc:106: DCHECK(!resolve_cb_.is_null()); DCHECK(!uma_name.empty()); https://codereview.chromium.org/452643002/diff/40001/media/base/cdm_promise_u... File media/base/cdm_promise_unittest.cc (right): https://codereview.chromium.org/452643002/diff/40001/media/base/cdm_promise_u... media/base/cdm_promise_unittest.cc:12: EXPECT_EQ(MediaKeys::NUM_EXCEPTIONS, CdmPromise::NUM_RESULT_CODES - 1); This can be a COMPILE_ASSERT at the conversion site. https://codereview.chromium.org/452643002/diff/40001/media/base/media_keys.h File media/base/media_keys.h (right): https://codereview.chromium.org/452643002/diff/40001/media/base/media_keys.h#... media/base/media_keys.h:58: NUM_EXCEPTIONS Should we add? //Insert new exceptions before
https://codereview.chromium.org/452643002/diff/40001/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/452643002/diff/40001/media/base/cdm_promise.c... media/base/cdm_promise.cc:49: return CdmPromise::NOT_SUPPORTED_ERROR; On 2014/08/08 18:29:04, ddorwin wrote: > UNKNOWN was probably better (even though this should never happen). Done. https://codereview.chromium.org/452643002/diff/40001/media/base/cdm_promise.c... media/base/cdm_promise.cc:106: DCHECK(!resolve_cb_.is_null()); On 2014/08/08 18:29:04, ddorwin wrote: > DCHECK(!uma_name.empty()); Done. https://codereview.chromium.org/452643002/diff/40001/media/base/cdm_promise_u... File media/base/cdm_promise_unittest.cc (right): https://codereview.chromium.org/452643002/diff/40001/media/base/cdm_promise_u... media/base/cdm_promise_unittest.cc:12: EXPECT_EQ(MediaKeys::NUM_EXCEPTIONS, CdmPromise::NUM_RESULT_CODES - 1); On 2014/08/08 18:29:04, ddorwin wrote: > This can be a COMPILE_ASSERT at the conversion site. Done. https://codereview.chromium.org/452643002/diff/40001/media/base/media_keys.h File media/base/media_keys.h (right): https://codereview.chromium.org/452643002/diff/40001/media/base/media_keys.h#... media/base/media_keys.h:58: NUM_EXCEPTIONS On 2014/08/08 18:29:04, ddorwin wrote: > Should we add? > //Insert new exceptions before Done.
lgtm % nit https://codereview.chromium.org/452643002/diff/60001/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/452643002/diff/60001/media/base/cdm_promise.c... media/base/cdm_promise.cc:13: COMPILE_ASSERT(MediaKeys::NUM_EXCEPTIONS == CdmPromise::NUM_RESULT_CODES - 1, tiny nit: This probably reads easier as CODES = EXCEPTIONS + 1.
https://codereview.chromium.org/452643002/diff/60001/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/452643002/diff/60001/media/base/cdm_promise.c... media/base/cdm_promise.cc:13: COMPILE_ASSERT(MediaKeys::NUM_EXCEPTIONS == CdmPromise::NUM_RESULT_CODES - 1, On 2014/08/08 20:07:10, ddorwin wrote: > tiny nit: This probably reads easier as CODES = EXCEPTIONS + 1. Done.
isherman@: Please review changes in histograms.xml.
https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.c... media/base/cdm_promise.cc:50: default: Please omit the default case, so that the compiler can detect a missing case. I bet this would also allow you to drop the COMPILE_ASSERT above, if you wanted to. https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.c... media/base/cdm_promise.cc:52: return CdmPromise::UNKNOWN_ERROR; Note that you'll still need this default return stmt outside of the switch stmt for those compilers (notable, the Windows compiler) that aren't smart enough to realize that all cases are covered. https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.c... media/base/cdm_promise.cc:63: UMA_HISTOGRAM_ENUMERATION(uma_name_, result_code, NUM_RESULT_CODES); The name passed to any of the UMA_HISTOGRAM_ macros needs to be a runtime constant. It looks like that isn't guaranteed here. https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.c... media/base/cdm_promise.cc:95: UMA_HISTOGRAM_ENUMERATION(uma_name_, SUCCESS, NUM_RESULT_CODES); Ditto. https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.c... media/base/cdm_promise.cc:124: UMA_HISTOGRAM_ENUMERATION(uma_name_, SUCCESS, NUM_RESULT_CODES); Ditto. https://codereview.chromium.org/452643002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/452643002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10346: +<histogram name="Media.EME.ClearKey.createSession" enum="CdmPromiseResult"> nit: The 'c' should probably be uppercase, to match the other histogram names. Ditto below. https://codereview.chromium.org/452643002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10348: + <summary>createSession result using the Clear Key key system.</summary> This description is really unclear to me as someone who's not familiar with the details of your codebase. Is there any way that you could make it clearer, without having to make the summary excessively verbose? https://codereview.chromium.org/452643002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10399: + <summary>createSession result using an unknown key system.</summary> Ditto. https://codereview.chromium.org/452643002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:36738: + <int value="7" label="OutputError"/> nit: If you prefer this formatting, that's fine; but it's also fine to include spaces in labels -- these are intended to be human readable.
isherman@: Requesting clarification on "runtime constant" UMA names. https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.c... media/base/cdm_promise.cc:50: default: On 2014/08/11 21:53:48, Ilya Sherman wrote: > Please omit the default case, so that the compiler can detect a missing case. I > bet this would also allow you to drop the COMPILE_ASSERT above, if you wanted > to. Done. https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.c... media/base/cdm_promise.cc:52: return CdmPromise::UNKNOWN_ERROR; On 2014/08/11 21:53:48, Ilya Sherman wrote: > Note that you'll still need this default return stmt outside of the switch stmt > for those compilers (notable, the Windows compiler) that aren't smart enough to > realize that all cases are covered. Done. https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.c... media/base/cdm_promise.cc:63: UMA_HISTOGRAM_ENUMERATION(uma_name_, result_code, NUM_RESULT_CODES); On 2014/08/11 21:53:48, Ilya Sherman wrote: > The name passed to any of the UMA_HISTOGRAM_ macros needs to be a runtime > constant. It looks like that isn't guaranteed here. What do you mean by this? https://codereview.chromium.org/452643002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/452643002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10346: +<histogram name="Media.EME.ClearKey.createSession" enum="CdmPromiseResult"> On 2014/08/11 21:53:48, Ilya Sherman wrote: > nit: The 'c' should probably be uppercase, to match the other histogram names. > Ditto below. Done. https://codereview.chromium.org/452643002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10348: + <summary>createSession result using the Clear Key key system.</summary> On 2014/08/11 21:53:48, Ilya Sherman wrote: > This description is really unclear to me as someone who's not familiar with the > details of your codebase. Is there any way that you could make it clearer, > without having to make the summary excessively verbose? Done. https://codereview.chromium.org/452643002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:10399: + <summary>createSession result using an unknown key system.</summary> On 2014/08/11 21:53:48, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/452643002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:36738: + <int value="7" label="OutputError"/> On 2014/08/11 21:53:48, Ilya Sherman wrote: > nit: If you prefer this formatting, that's fine; but it's also fine to include > spaces in labels -- these are intended to be human readable. Acknowledged.
https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.c... media/base/cdm_promise.cc:63: UMA_HISTOGRAM_ENUMERATION(uma_name_, result_code, NUM_RESULT_CODES); On 2014/08/12 16:05:51, sandersd wrote: > On 2014/08/11 21:53:48, Ilya Sherman wrote: > > The name passed to any of the UMA_HISTOGRAM_ macros needs to be a runtime > > constant. It looks like that isn't guaranteed here. > > What do you mean by this? The UMA_HISTOGRAM_ENUMERATION macro expands out to something like static base::HistogramBase* histogram = GetHistogram(name, ...); The important part is the 'static' keyword: It means that the histogram is only looked up by name the first time the code path is reached during a run of the program. Hence, if you pass "Media.EME.ClearKey.CreateSession" as the histogram name one time, and "Media.EME.Unknown.CreateSession" as the name another time, the data will actually be recorded to "Media.EME.ClearKey.CreateSession" both times. In short, the name passed to an of the UMA_HISTOGRAM_ macros should be a constant. Strictly speaking, it only needs to be constant during the run of the program. However, it's easiest if it's actually a compile-time constant. Since you're logging to three different histograms, this means that you should have three separate UMA_HISTOGRAM_ENUMERATION macro invocations. Alternately, if you're pretty sure you know what you'e doing, you could forgo the macro, and call the histograms code directly. https://codereview.chromium.org/452643002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/452643002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10402: + Result of createSession promisies for unknown key systems promises that were nit: "promisies" -> "promises"
https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.c... media/base/cdm_promise.cc:63: UMA_HISTOGRAM_ENUMERATION(uma_name_, result_code, NUM_RESULT_CODES); On 2014/08/12 18:00:33, Ilya Sherman wrote: > On 2014/08/12 16:05:51, sandersd wrote: > > On 2014/08/11 21:53:48, Ilya Sherman wrote: > > > The name passed to any of the UMA_HISTOGRAM_ macros needs to be a runtime > > > constant. It looks like that isn't guaranteed here. > > > > What do you mean by this? > > The UMA_HISTOGRAM_ENUMERATION macro expands out to something like > > static base::HistogramBase* histogram = GetHistogram(name, ...); > > The important part is the 'static' keyword: It means that the histogram is only > looked up by name the first time the code path is reached during a run of the > program. Hence, if you pass "Media.EME.ClearKey.CreateSession" as the histogram > name one time, and "Media.EME.Unknown.CreateSession" as the name another time, > the data will actually be recorded to "Media.EME.ClearKey.CreateSession" both > times. > > In short, the name passed to an of the UMA_HISTOGRAM_ macros should be a > constant. Strictly speaking, it only needs to be constant during the run of the > program. However, it's easiest if it's actually a compile-time constant. Since > you're logging to three different histograms, this means that you should have > three separate UMA_HISTOGRAM_ENUMERATION macro invocations. Alternately, if > you're pretty sure you know what you'e doing, you could forgo the macro, and > call the histograms code directly. Since this is not currently performance sensitive, I've removed the macros. https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.c... media/base/cdm_promise.cc:95: UMA_HISTOGRAM_ENUMERATION(uma_name_, SUCCESS, NUM_RESULT_CODES); On 2014/08/11 21:53:48, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/452643002/diff/80001/media/base/cdm_promise.c... media/base/cdm_promise.cc:124: UMA_HISTOGRAM_ENUMERATION(uma_name_, SUCCESS, NUM_RESULT_CODES); On 2014/08/11 21:53:48, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/452643002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/452643002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10402: + Result of createSession promisies for unknown key systems promises that were On 2014/08/12 18:00:34, Ilya Sherman wrote: > nit: "promisies" -> "promises" Done.
LGTM % nit. Thanks. https://codereview.chromium.org/452643002/diff/120001/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/452643002/diff/120001/media/base/cdm_promise.... media/base/cdm_promise.cc:92: if (!uma_name_.empty()) nit: Please add curly braces, here and below.
https://codereview.chromium.org/452643002/diff/120001/media/base/cdm_promise.cc File media/base/cdm_promise.cc (right): https://codereview.chromium.org/452643002/diff/120001/media/base/cdm_promise.... media/base/cdm_promise.cc:92: if (!uma_name_.empty()) On 2014/08/13 20:06:50, Ilya Sherman wrote: > nit: Please add curly braces, here and below. Done.
The CQ bit was checked by sandersd@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sandersd@chromium.org/452643002/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Message was sent while issue was closed.
Committed patchset #8 (140001) as 289453 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
