Adds UMA histograms for desktop capture device.

Adds UMA histograms for desktop capture device.

BUG=366288
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266583

[jiayl, 2014-04-24 00:25:45]

PTAL

[Ilya Sherman, 2014-04-24 01:57:43]

Histograms LGTM with comment addressed.

https://codereview.chromium.org/252393002/diff/1/content/browser/media/captur...
File content/browser/media/capture/desktop_capture_device.cc (right):

https://codereview.chromium.org/252393002/diff/1/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device.cc:38:
DESKTOP_CAPTURE_COUNTER_BOUNDARY
nit: Please document that since this enum is used to back an UMA histogram, it
should be treated as append-only.

https://codereview.chromium.org/252393002/diff/1/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device.cc:231:
HISTOGRAM_TIMES("WebRTC.WindowCaptureTime", capture_time);
These should both be UMA_HISTOGRAM_TIMES rather than HISTOGRAM_TIMES.

https://codereview.chromium.org/252393002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/252393002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:30161: +<histogram
name="WebRTC.ScreenCaptureTime">
nit: Please add units.

https://codereview.chromium.org/252393002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:30218: +<histogram
name="WebRTC.WindowCaptureTime">
nit: Please add units.

[Sergey Ulanov, 2014-04-24 02:21:19]

you need similar change for DesktopCaptureDeviceAura - otherwise we won't get
anything for ChromeOS

https://codereview.chromium.org/252393002/diff/1/content/browser/media/captur...
File content/browser/media/capture/desktop_capture_device.cc (right):

https://codereview.chromium.org/252393002/diff/1/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device.cc:202: if (capturer_type_
== DesktopMediaID::TYPE_SCREEN)
maybe use :? to make this code shorter:

if (capturer_type_ == DesktopMediaID::TYPE_SCREEN) {
  IncrementCounter(frame ? FIRST_SCREEN_CAPTURE_SUCCEEDED :
                           FIRST_SCREEN_CAPTURE_FAILED);
} else {
  ...
}

https://codereview.chromium.org/252393002/diff/1/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device.cc:203:
IncrementCounter(FIRST_SCREEN_CAPTURE_SUCCEEDED);
add {} please

https://codereview.chromium.org/252393002/diff/1/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device.cc:229:
HISTOGRAM_TIMES("WebRTC.ScreenCaptureTime", capture_time);
add {} please

[jiayl, 2014-04-24 16:44:01]

All comments addressed. PTAL

[Sergey Ulanov, 2014-04-24 19:33:23]

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
File content/browser/media/capture/desktop_capture_device.cc (right):

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device.cc:216:
UMA_HISTOGRAM_TIMES(kUMAScreenCaptureTime, capture_time);
add {}

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device_aura.cc:320:
FIRST_SCREEN_CAPTURE_FAILED);
nit: indentation. Also : can be wrapped. clang-format formats this code as
follows:

  IncrementCounter(succeeded ? FIRST_SCREEN_CAPTURE_SUCCEEDED
                             : FIRST_SCREEN_CAPTURE_FAILED);

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device_aura.cc:328: bool
DesktopVideoCaptureMachine::DidCopyOutputHelper(
Call this something like ProcessCopyOutputResponse()

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device_aura.cc:392:
base::TimeDelta capture_time = base::TimeTicks::Now() - start_time;
Move this to DidCopyOutput() so that all UMA logic is in one place?

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device_aura.cc:394:
UMA_HISTOGRAM_TIMES(kUMAScreenCaptureTime, capture_time);
{}

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
File content/browser/media/capture/desktop_capture_device_uma_types.h (right):

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device_uma_types.h:23: const char
kUMADesopCaptureCounters[] = "WebRTC.DesktopCaptureCounters";
1. s/UMA/Uma/
2. typo: Desop
3. for string consts you need to declare them here and define in a .cc file
(e.g. as in base/base_switches.h)

[jiayl, 2014-04-24 20:39:02]

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
File content/browser/media/capture/desktop_capture_device.cc (right):

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device.cc:216:
UMA_HISTOGRAM_TIMES(kUMAScreenCaptureTime, capture_time);
On 2014/04/24 19:33:24, Sergey Ulanov wrote:
> add {}

That's not required for single line conditionals:
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[jiayl, 2014-04-24 20:58:54]

PTAL

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device_aura.cc:320:
FIRST_SCREEN_CAPTURE_FAILED);
On 2014/04/24 19:33:24, Sergey Ulanov wrote:
> nit: indentation. Also : can be wrapped. clang-format formats this code as
> follows:
> 
>   IncrementCounter(succeeded ? FIRST_SCREEN_CAPTURE_SUCCEEDED
>                              : FIRST_SCREEN_CAPTURE_FAILED);

Done.

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device_aura.cc:328: bool
DesktopVideoCaptureMachine::DidCopyOutputHelper(
On 2014/04/24 19:33:24, Sergey Ulanov wrote:
> Call this something like ProcessCopyOutputResponse()

Done.

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device_aura.cc:392:
base::TimeDelta capture_time = base::TimeTicks::Now() - start_time;
On 2014/04/24 19:33:24, Sergey Ulanov wrote:
> Move this to DidCopyOutput() so that all UMA logic is in one place?

Done.

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device_aura.cc:394:
UMA_HISTOGRAM_TIMES(kUMAScreenCaptureTime, capture_time);
On 2014/04/24 19:33:24, Sergey Ulanov wrote:
> {}

It's not required for single line conditionals.

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
File content/browser/media/capture/desktop_capture_device_uma_types.h (right):

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device_uma_types.h:23: const char
kUMADesopCaptureCounters[] = "WebRTC.DesktopCaptureCounters";
On 2014/04/24 19:33:24, Sergey Ulanov wrote:
> 1. s/UMA/Uma/
> 2. typo: Desop
> 3. for string consts you need to declare them here and define in a .cc file
> (e.g. as in base/base_switches.h)

Done.

[Sergey Ulanov, 2014-04-24 21:01:51]

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
File content/browser/media/capture/desktop_capture_device.cc (right):

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device.cc:216:
UMA_HISTOGRAM_TIMES(kUMAScreenCaptureTime, capture_time);
On 2014/04/24 20:39:03, jiayl wrote:
> On 2014/04/24 19:33:24, Sergey Ulanov wrote:
> > add {}
> 
> That's not required for single line conditionals:
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
> 

They are not required, but they improve readability when there is else,
especially when you have a macro in the body.

BTW you can use "?:" here and then we wouldn't have this argument :)

[jiayl, 2014-04-24 21:27:35]

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
File content/browser/media/capture/desktop_capture_device.cc (right):

https://codereview.chromium.org/252393002/diff/2/content/browser/media/captur...
content/browser/media/capture/desktop_capture_device.cc:216:
UMA_HISTOGRAM_TIMES(kUMAScreenCaptureTime, capture_time);
On 2014/04/24 21:01:51, Sergey Ulanov wrote:
> On 2014/04/24 20:39:03, jiayl wrote:
> > On 2014/04/24 19:33:24, Sergey Ulanov wrote:
> > > add {}
> > 
> > That's not required for single line conditionals:
> >
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
> > 
> 
> They are not required, but they improve readability when there is else,
> especially when you have a macro in the body.
> 
> BTW you can use "?:" here and then we wouldn't have this argument :)

OK, "?:" wins.

[Sergey Ulanov, 2014-04-24 21:46:17]

lgtm

[jiayl, 2014-04-24 21:54:25]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-04-24 22:01:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiayl@chromium.org/252393002/50001

[commit-bot: I haz the power, 2014-04-24 23:55:18]

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

[commit-bot: I haz the power, 2014-04-24 23:55:19]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[jiayl, 2014-04-25 00:00:23]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-04-25 00:01:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiayl@chromium.org/252393002/70001

[commit-bot: I haz the power, 2014-04-25 00:04:10]

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

[commit-bot: I haz the power, 2014-04-25 00:04:11]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[jiayl, 2014-04-25 00:08:01]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-04-25 00:09:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiayl@chromium.org/252393002/70001

[commit-bot: I haz the power, 2014-04-25 01:39:44]

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

[commit-bot: I haz the power, 2014-04-25 01:39:45]

Failed to apply patch for
content/browser/media/capture/desktop_capture_device_uma_types.cc:
  patching file
content/browser/media/capture/desktop_capture_device_uma_types.cc
  A         content/browser/media/capture/desktop_capture_device_uma_types.cc
Transient error: [Errno 3] No such process
Patch:   N   content/browser/media/capture/desktop_capture_device_uma_types.cc
Index: content/browser/media/capture/desktop_capture_device_uma_types.cc
diff --git a/content/browser/media/capture/desktop_capture_device_uma_types.cc
b/content/browser/media/capture/desktop_capture_device_uma_types.cc
new file mode 100644
index
0000000000000000000000000000000000000000..c91641c5336f6c50bb1825d9f2dec365c11c2623
--- /dev/null
+++ b/content/browser/media/capture/desktop_capture_device_uma_types.cc
@@ -0,0 +1,20 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/media/capture/desktop_capture_device_uma_types.h"
+
+#include "base/metrics/histogram.h"
+
+namespace content {
+
+const char kUmaScreenCaptureTime[] = "WebRTC.ScreenCaptureTime";
+const char kUmaWindowCaptureTime[] = "WebRTC.WindowCaptureTime";
+
+void IncrementDesktopCaptureCounter(DesktopCaptureCounters counter) {
+  UMA_HISTOGRAM_ENUMERATION("WebRTC.DesktopCaptureCounters",
+                            counter,
+                            DESKTOP_CAPTURE_COUNTER_BOUNDARY);
+}
+
+}  // namespace content

[jiayl, 2014-04-28 16:09:42]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-04-28 16:09:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiayl@chromium.org/252393002/70001

[commit-bot: I haz the power, 2014-04-28 16:14:49]

Change committed as 266583