Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
446553002:
Add histogram WebRTC.UserMediaRequest.NoResultState to track user media requests with no result. (Closed)
|
Created:
6 years, 4 months ago by andresp-chromium 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, tommi (sloooow) - chröme Base URL:
https://chromium.googlesource.com/chromium/src.git@b399835 Project:
chromium Visibility:
Public. |
DescriptionAdd histogram WebRTC.UserMediaRequest.NoResultState to track user media requests with no result.
BUG=399835
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287862
Patch Set 1 : Diffbase https://codereview.chromium.org/427713004/ #
Total comments: 24
Patch Set 2 : Comments addressed (and diffbase is now submitted, so diffing to master now) #
Total comments: 12
Patch Set 3 : Addressed tommi comments. #
Total comments: 6
Patch Set 4 : Address nits #Patch Set 5 : Actually fix histogram.xml nit. #
Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/w... File content/renderer/media/webrtc_uma_histograms.cc (right): https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/w... content/renderer/media/webrtc_uma_histograms.cc:14: enum { Please explicitly state that elements in this enum should not be deleted or rearranged; the only permitted operation is to add new elements to the end. https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/w... content/renderer/media/webrtc_uma_histograms.cc:15: USER_MEDIA_REQUEST_CANCELLED, Please explicitly use = 0, = 1, etc. https://codereview.chromium.org/446553002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/446553002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:35614: + Counters for state of UserMediaRequests when they get destroyed before Consider: Counters for state of UserMediaRequests when they get ... -> The state of a UserMediaRequest when it gets ...
https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... File content/renderer/media/media_stream_impl.cc (right): https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:202: LogUserMediaRequestWithNoResult(true, In general, boolean parameters are not good practice. This function call is pretty hard to understand unless you go to the definition of the call -- e.g. what does "true" mean? See comment in LogUserMediaRequestWithNoResult -- you should modify the logic so that you can simply say here: LogUserMediaRequestWithNoResult(USER_MEDIA_REQUEST_CANCELLED) https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:280: return OnStreamGeneratedForCancelledRequest(audio_array, video_array); Why did you make this into its own function? Also shouldn't return the result of a void function - I'm assuming that was a mistake? https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:341: nit: extra line https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:725: LogUserMediaRequestWithNoResult(false, As stated elsewhere, this call should instead look like if (!(*request_it)->generated) { DCHECK(!(*request_it)->HasPendingSources()); LogUserMediaRequestWithNoResult(USER_MEDIA_REQUEST_NOT_GENERATED) } else { DCHECK((*request_it)->HasPendingSources()); LogUserMediaRequestWithNoResult(USER_MEDIA_REQUEST_PENDING_MEDIA_TRACKS) } (You could also do a DCHECK_EQ((*request_it)->HasPendingSources(), (*request_it)->generated) before the if/else) https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:779: DeleteAllUserMediaRequests(); Why did you move this into its own function? https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... File content/renderer/media/media_stream_impl.h (right): https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... content/renderer/media/media_stream_impl.h:149: bool AreAllSourcesRemoved() const { return sources_.empty(); } I think both AreAllSourcesRemoved and HasPendingSources should be un-inlined (http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...) https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... content/renderer/media/media_stream_impl.h:150: bool HasPendingSources() { return !sources_waiting_for_callback_.empty(); } const? https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/w... File content/renderer/media/webrtc_uma_histograms.cc (right): https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/w... content/renderer/media/webrtc_uma_histograms.cc:14: enum { Move this enum to content/public/common/media_stream_request.h and change the signature of this method to LogUserMediaRequestWithNoResult(enum) and shift the logic accordingly. A logging method in the WebRTC UMA class should not need to know about things like "if stream_generated is true, then pending_sources is true" -- that logic should be kept localized to the stream generation code in MediaStreamImpl. https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/w... content/renderer/media/webrtc_uma_histograms.cc:24: DCHECK(stream_generated ? pending_sources : !pending_sources); FYI, could have done DCHECK_EQ(stream_generated, pending_sources)
All comments addressed. Note for reviewers for the media stream part: Please verify that all code paths that remove a user media request log exactly once with one of LogUserMediaRequestWithResult/WithNoResult If you do think I should have a comment somewhere on media_stream_impl with the above notice for next time someone changes the possible code paths, please have a suggestion on where to. https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... File content/renderer/media/media_stream_impl.cc (right): https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:202: LogUserMediaRequestWithNoResult(true, On 2014/08/05 19:05:40, Victoria Kirst wrote: > In general, boolean parameters are not good practice. This function call is > pretty hard to understand unless you go to the definition of the call -- e.g. > what does "true" mean? See comment in LogUserMediaRequestWithNoResult -- you > should modify the logic so that you can simply say here: > LogUserMediaRequestWithNoResult(USER_MEDIA_REQUEST_CANCELLED) Done. https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:280: return OnStreamGeneratedForCancelledRequest(audio_array, video_array); On 2014/08/05 19:05:40, Victoria Kirst wrote: > Why did you make this into its own function? Also shouldn't return the result of > a void function - I'm assuming that was a mistake? This was re factoring when reading the code. Originally i was going to add an histogram count for this scenario but then found out that those are counted as Cancelled or NotGenerated already. Anyhow it might be interesting to add a counter here and on OnStreamGenerationFailedForCancelledRequest, if we want to investigate those buckets further. Regarding return value: I think that contrary to Java, return void is valid in C++. I think I just acquired this style once I spent too much time cleaning WebRTC with unused return values. Since this seems to cause confusion I am switching to the most common use case. https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:341: On 2014/08/05 19:05:40, Victoria Kirst wrote: > nit: extra line Done. https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:725: LogUserMediaRequestWithNoResult(false, On 2014/08/05 19:05:40, Victoria Kirst wrote: > As stated elsewhere, this call should instead look like > if (!(*request_it)->generated) { > DCHECK(!(*request_it)->HasPendingSources()); > LogUserMediaRequestWithNoResult(USER_MEDIA_REQUEST_NOT_GENERATED) > } else { > DCHECK((*request_it)->HasPendingSources()); > LogUserMediaRequestWithNoResult(USER_MEDIA_REQUEST_PENDING_MEDIA_TRACKS) > } > > (You could also do a DCHECK_EQ((*request_it)->HasPendingSources(), > (*request_it)->generated) before the if/else) Done. I liked this suggestion. https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:779: DeleteAllUserMediaRequests(); On 2014/08/05 19:05:40, Victoria Kirst wrote: > Why did you move this into its own function? Re-factoring when reading the code. I wanted to isolate all the places where user_media_requests_ vector gets touched into simple methods. Thus allowing to more easily verify that all code paths were logging MediaRequest with no result. https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... File content/renderer/media/media_stream_impl.h (right): https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... content/renderer/media/media_stream_impl.h:149: bool AreAllSourcesRemoved() const { return sources_.empty(); } On 2014/08/05 19:05:40, Victoria Kirst wrote: > I think both AreAllSourcesRemoved and HasPendingSources should be un-inlined > (http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...) Done. https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/m... content/renderer/media/media_stream_impl.h:150: bool HasPendingSources() { return !sources_waiting_for_callback_.empty(); } On 2014/08/05 19:05:41, Victoria Kirst wrote: > const? Done. https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/w... File content/renderer/media/webrtc_uma_histograms.cc (right): https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/w... content/renderer/media/webrtc_uma_histograms.cc:14: enum { On 2014/08/05 18:50:23, Mark P wrote: > Please explicitly state that elements in this enum should not be deleted or > rearranged; the only permitted operation is to add new elements to the end. Done. https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/w... content/renderer/media/webrtc_uma_histograms.cc:14: enum { On 2014/08/05 19:05:41, Victoria Kirst wrote: > Move this enum to content/public/common/media_stream_request.h and change the > signature of this method to LogUserMediaRequestWithNoResult(enum) and shift the > logic accordingly. A logging method in the WebRTC UMA class should not need to > know about things like "if stream_generated is true, then pending_sources is > true" -- that logic should be kept localized to the stream generation code in > MediaStreamImpl. Done. But moved the enum into webrtc_uma_histogram, i think it is a better place and it can be moved to content/public/common/media_stream_request whenever the need to use it somewhere else arises. https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/w... content/renderer/media/webrtc_uma_histograms.cc:15: USER_MEDIA_REQUEST_CANCELLED, On 2014/08/05 18:50:23, Mark P wrote: > Please explicitly use = 0, = 1, etc. Done. https://codereview.chromium.org/446553002/diff/60001/content/renderer/media/w... content/renderer/media/webrtc_uma_histograms.cc:24: DCHECK(stream_generated ? pending_sources : !pending_sources); On 2014/08/05 19:05:41, Victoria Kirst wrote: > FYI, could have done DCHECK_EQ(stream_generated, pending_sources) Acknowledged. https://codereview.chromium.org/446553002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/446553002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:35614: + Counters for state of UserMediaRequests when they get destroyed before On 2014/08/05 18:50:23, Mark P wrote: > Consider: > Counters for state of UserMediaRequests when they get > ... > -> > The state of a UserMediaRequest when it gets > ... Done.
https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/m... File content/renderer/media/media_stream_impl.cc (right): https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:324: // Only stop the device if the device is not used in another MediaStream. nit: documentation for methods tends to stay in the header https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:723: if ((*request_it)->generated) { should this be an else? https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:728: DCHECK(!(*request_it)->HasPendingSources()); ...and this looks like it should be in the scope where we call CancelGenerateStream() (unless I'm missing something) https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:931: bool MediaStreamImpl::UserMediaRequestInfo::AreAllSourcesRemoved() const { ugh, grammar :-/ not sure we should fix that now though https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/w... File content/renderer/media/webrtc_uma_histograms.h (right): https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/w... content/renderer/media/webrtc_uma_histograms.h:20: // operation is to add new elements to the end. to the end or before NUM_MEDIA_STREAM_REQUEST_WITH_NO_RESULT? https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/w... content/renderer/media/webrtc_uma_histograms.h:22: MEDIA_STREAM_REQUEST_EXPLICITLY_CANCELLED = 0, too much indent
https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/m... File content/renderer/media/media_stream_impl.cc (right): https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:324: // Only stop the device if the device is not used in another MediaStream. On 2014/08/06 11:42:21, tommi wrote: > nit: documentation for methods tends to stay in the header Split documentation to the relevant places. https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:723: if ((*request_it)->generated) { On 2014/08/06 11:42:21, tommi wrote: > should this be an else? Done. https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:728: DCHECK(!(*request_it)->HasPendingSources()); On 2014/08/06 11:42:21, tommi wrote: > ...and this looks like it should be in the scope where we call > CancelGenerateStream() (unless I'm missing something) Done. Yeah you are right! Since I was looking as two diffs (one refactor + one adding logging) I never noticed I had the same condition above. Collapsing them together now. https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_impl.cc:931: bool MediaStreamImpl::UserMediaRequestInfo::AreAllSourcesRemoved() const { On 2014/08/06 11:42:21, tommi wrote: > ugh, grammar :-/ not sure we should fix that now though This method is never called. Removing. https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/w... File content/renderer/media/webrtc_uma_histograms.h (right): https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/w... content/renderer/media/webrtc_uma_histograms.h:20: // operation is to add new elements to the end. On 2014/08/06 11:42:21, tommi wrote: > to the end or before NUM_MEDIA_STREAM_REQUEST_WITH_NO_RESULT? NUM_MEDIA_STREAM_REQUEST_WITH_NO_RESULT https://codereview.chromium.org/446553002/diff/80001/content/renderer/media/w... content/renderer/media/webrtc_uma_histograms.h:22: MEDIA_STREAM_REQUEST_EXPLICITLY_CANCELLED = 0, On 2014/08/06 11:42:21, tommi wrote: > too much indent Done.
lgtm
lgtm Looks great! Just a few tiny nits. https://codereview.chromium.org/446553002/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_impl.cc (right): https://codereview.chromium.org/446553002/diff/100001/content/renderer/media/... content/renderer/media/media_stream_impl.cc:931: } nit: extra blank line between } and } // namespace content https://codereview.chromium.org/446553002/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_impl.h (right): https://codereview.chromium.org/446553002/diff/100001/content/renderer/media/... content/renderer/media/media_stream_impl.h:194: void OnStreamGeneratedForCancelledRequest( nit: Can you comment on what these arguments are?
one minor comment; otherwise histograms.xml lgtm https://codereview.chromium.org/446553002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/446553002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:35777: + The state of UserMediaRequests when it gets destroyed before having a nit (singular plural mismatch; you didn't follow my previous suggestion fully :-P): UserMediaRequests -> a UserMediaRequest
https://codereview.chromium.org/446553002/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_impl.cc (right): https://codereview.chromium.org/446553002/diff/100001/content/renderer/media/... content/renderer/media/media_stream_impl.cc:931: } On 2014/08/06 17:28:36, Victoria Kirst wrote: > nit: extra blank line between } and } // namespace content Done. https://codereview.chromium.org/446553002/diff/100001/content/renderer/media/... File content/renderer/media/media_stream_impl.h (right): https://codereview.chromium.org/446553002/diff/100001/content/renderer/media/... content/renderer/media/media_stream_impl.h:194: void OnStreamGeneratedForCancelledRequest( On 2014/08/06 17:28:36, Victoria Kirst wrote: > nit: Can you comment on what these arguments are? Nope. They are also not documented for OnStreamGenerated and I am not the best to document that :| I also considered naming this method describing what it does, but couldn't find such a name and so named it describing when it gets called. https://codereview.chromium.org/446553002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/446553002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:35777: + The state of UserMediaRequests when it gets destroyed before having a On 2014/08/06 17:33:28, Mark P wrote: > nit (singular plural mismatch; you didn't follow my previous suggestion fully > :-P): > UserMediaRequests -> a UserMediaRequest Done.
The CQ bit was checked by andresp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresp@chromium.org/446553002/140001
Message was sent while issue was closed.
Change committed as 287862 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
