Adds UMA histograms for desktop tab and audio capture through

Adds UMA histograms for desktop tab and audio capture through
JavaScript chrome.chooseDesktopMedia.

BUG=557222
Committed: https://crrev.com/bd22413e5369434195c200a1d21e7d5e444b45e0
Cr-Commit-Position: refs/heads/master@{#383772}

[GeorgeZ, 2016-03-22 20:53:12]

gyzhou@chromium.org changed reviewers:
+ asvitkine@chromium.org, dalecurtis@chromium.org, miu@chromium.org

[GeorgeZ, 2016-03-22 20:53:12]

Miu, dalecurtis and asvitkine,

Could you take a look of this CL?

Thanks,

[GeorgeZ, 2016-03-23 15:59:55]

Patchset #2 (id:20001) has been deleted

[GeorgeZ, 2016-03-23 17:03:19]

Patchset #2 (id:40001) has been deleted

[GeorgeZ, 2016-03-23 17:03:30]

Patchset #1 (id:1) has been deleted

[GeorgeZ, 2016-03-23 18:41:50]

Patchset #2 (id:80001) has been deleted

[GeorgeZ, 2016-03-23 20:04:33]

Patchset #1 (id:60001) has been deleted

[GeorgeZ, 2016-03-23 20:04:41]

Patchset #1 (id:100001) has been deleted

[Alexei Svitkine (slow), 2016-03-24 15:18:07]

https://codereview.chromium.org/1825633002/diff/120001/content/browser/media/...
File content/browser/media/capture/desktop_capture_device_uma_types.h (right):

https://codereview.chromium.org/1825633002/diff/120001/content/browser/media/...
content/browser/media/capture/desktop_capture_device_uma_types.h:16:
TAB_VIDEO_CAPTURER_CREATED,
Entries used in histograms shouldn't be renumbered - as this causes problems
when analyzing aggregated data between different versions. Please add all new
entries at the end.

[miu, 2016-03-24 20:51:03]

not lgtm (the part about UMA-counting the first frame capture)

https://codereview.chromium.org/1825633002/diff/120001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1825633002/diff/120001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.cc:883: if
(response == READBACK_SUCCESS)
It's very possible for the first few frames to fail, and this would be normal
behavior.  This is one reason why it doesn't seem useful to UMA-count the first
frame capture (see comment below).

https://codereview.chromium.org/1825633002/diff/120001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device.h (right):

https://codereview.chromium.org/1825633002/diff/120001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.h:31: enum
JavaScriptType {
This is an abstraction violation.  There's no reason the WCVCD should care about
the JavaScript API that was being used when creating it.

Instead, please do these UMAs in the render process, preferably in the code
modules specific to each API.  Also, it's worth considering whether you really
need to do this at all.  Meaning, do you really need a UMA for the first frame? 
Why isn't it enough to just have a UMA when the capture device is created (in
VideoCaptureManager)?  What will this information be used for?

[GeorgeZ, 2016-03-25 15:28:49]

Patchset #2 (id:140001) has been deleted

[GeorgeZ, 2016-03-25 16:35:04]

miu and Alexei,

A new patch has uploaded based on your review comments.

Thanks

https://codereview.chromium.org/1825633002/diff/120001/content/browser/media/...
File content/browser/media/capture/desktop_capture_device_uma_types.h (right):

https://codereview.chromium.org/1825633002/diff/120001/content/browser/media/...
content/browser/media/capture/desktop_capture_device_uma_types.h:16:
TAB_VIDEO_CAPTURER_CREATED,
On 2016/03/24 15:18:07, Alexei Svitkine (OOO 25-27th) wrote:
> Entries used in histograms shouldn't be renumbered - as this causes problems
> when analyzing aggregated data between different versions. Please add all new
> entries at the end.

Done.

https://codereview.chromium.org/1825633002/diff/120001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1825633002/diff/120001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.cc:883: if
(response == READBACK_SUCCESS)
On 2016/03/24 20:51:02, miu wrote:
> It's very possible for the first few frames to fail, and this would be normal
> behavior.  This is one reason why it doesn't seem useful to UMA-count the
first
> frame capture (see comment below).

Very valuable comments. I guess this is why a owner's review is necessary.

I will revert all the changes in web_contents_video_capture_device*.*

https://codereview.chromium.org/1825633002/diff/120001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device.h (right):

https://codereview.chromium.org/1825633002/diff/120001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device.h:31: enum
JavaScriptType {
On 2016/03/24 20:51:03, miu wrote:
> This is an abstraction violation.  There's no reason the WCVCD should care
about
> the JavaScript API that was being used when creating it.
> 
> Instead, please do these UMAs in the render process, preferably in the code
> modules specific to each API.  Also, it's worth considering whether you really
> need to do this at all.  Meaning, do you really need a UMA for the first
frame? 
> Why isn't it enough to just have a UMA when the capture device is created (in
> VideoCaptureManager)?  What will this information be used for?

Thanks for pointing this out.

I was asked to mimic UMAs for desktop screen and window captures where they put
UMAs for first frame.

I will revert all the changes in the this class and add a UMA in
VideoCaptureManager.

[miu, 2016-03-25 21:41:17]

PS2 lgtm.

[GeorgeZ, 2016-03-28 21:06:45]

Ping Alexei and DaleCurtis.

[DaleCurtis, 2016-03-28 22:07:41]

lgtm

[Alexei Svitkine (slow), 2016-03-29 14:52:28]

lgtm

https://codereview.chromium.org/1825633002/diff/160001/content/browser/render...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/1825633002/diff/160001/content/browser/render...
content/browser/renderer_host/media/audio_input_renderer_host.cc:471: device_id
== media::AudioManagerBase::kLoopbackInputDeviceId)
Nit: {}'s

[GeorgeZ, 2016-03-29 15:05:22]

The CQ bit was checked by gyzhou@chromium.org

[GeorgeZ, 2016-03-29 15:05:37]

The CQ bit was unchecked by gyzhou@chromium.org

[commit-bot: I haz the power, 2016-03-29 15:05:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1825633002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1825633002/160001

[GeorgeZ, 2016-03-29 16:27:10]

The CQ bit was checked by gyzhou@chromium.org

[GeorgeZ, 2016-03-29 16:27:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, dalecurtis@chromium.org, miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1825633002/#ps180001
(title: " ")

[commit-bot: I haz the power, 2016-03-29 16:27:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1825633002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1825633002/180001

[commit-bot: I haz the power, 2016-03-29 16:36:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-29 16:36:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[GeorgeZ, 2016-03-29 16:44:43]

gyzhou@chromium.org changed reviewers:
+ avi@chromium.org

[GeorgeZ, 2016-03-29 16:44:43]

avi,

could you have a review of content/browser/BUILD.gn.

I missed an owner of Build.gn.

Thanks,

[Avi (use Gerrit), 2016-03-29 18:05:53]

lgtm

[GeorgeZ, 2016-03-29 18:10:22]

The CQ bit was checked by gyzhou@chromium.org

[commit-bot: I haz the power, 2016-03-29 18:10:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1825633002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1825633002/180001

[commit-bot: I haz the power, 2016-03-29 18:16:34]

Committed patchset #3 (id:180001)

[commit-bot: I haz the power, 2016-03-29 18:18:11]

Description was changed from

==========
Adds UMA histograms for desktop tab and audio capture through
JavaScript chrome.chooseDesktopMedia.

BUG=557222
==========

to

==========
Adds UMA histograms for desktop tab and audio capture through
JavaScript chrome.chooseDesktopMedia.

BUG=557222

Committed: https://crrev.com/bd22413e5369434195c200a1d21e7d5e444b45e0
Cr-Commit-Position: refs/heads/master@{#383772}
==========

[commit-bot: I haz the power, 2016-03-29 18:18:12]

Patchset 3 (id:??) landed as
https://crrev.com/bd22413e5369434195c200a1d21e7d5e444b45e0
Cr-Commit-Position: refs/heads/master@{#383772}