Only report Media.AcceleratedCompositingActive for video-having playbacks and add Media.URLScheme.

webkit/glue/webmediaplayer_impl.cc

Index: webkit/glue/webmediaplayer_impl.cc
diff --git a/webkit/glue/webmediaplayer_impl.cc b/webkit/glue/webmediaplayer_impl.cc
index 01ddcc06c9191fef3966592df87892058792b880..c3e5d0e543f302648a65afe9352a577e2a9db106 100644
--- a/webkit/glue/webmediaplayer_impl.cc
+++ b/webkit/glue/webmediaplayer_impl.cc
@@ -120,6 +120,7 @@ WebMediaPlayerImpl::WebMediaPlayerImpl(
       delegate_(delegate),
       media_stream_client_(media_stream_client),
       media_log_(media_log),
+      is_accelerated_compositing_active_(false),
       incremented_externally_allocated_memory_(false) {
   // Saves the current message loop.
   DCHECK(!main_loop_);
@@ -152,8 +153,8 @@ bool WebMediaPlayerImpl::Initialize(
                    AsWeakPtr()));
   }
 
-  UMA_HISTOGRAM_BOOLEAN("Media.AcceleratedCompositingActive",
-                        frame->view()->isAcceleratedCompositingActive());
+  is_accelerated_compositing_active_ =
+      frame->view()->isAcceleratedCompositingActive();
 
   pipeline_ = new media::PipelineImpl(pipeline_message_loop, media_log_);
 
@@ -237,10 +238,40 @@ WebMediaPlayerImpl::~WebMediaPlayerImpl() {
   }
 }
 
+// Helper enum for reporting scheme histograms.
+enum URLSchemeForHistogram {

[tony, 2011/11/15 20:50:54]

Nit: Can we put this in the anonymous namespace?

[Ami GONE FROM CHROMIUM, 2011/11/15 21:03:53]

I prefer to keep this near the (only) site of use, since it's not meant for
anything else to use (name hints at this, comment says this, but things at the
top of files tend to be "general-purpose").

[tony, 2011/11/15 21:33:21]

Style guide says to put it in an anonymous namespace.  You can keep the code
here, just wrap it in namespace {}.
http://www.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces

[Ami GONE FROM CHROMIUM, 2011/11/15 21:47:35]

Ok, but under protest.  
The justification in the style guide is bogus since the shared build now uses
hidden visibility by default (hence the FOO_EXPORT macros everywhere).

+  kUnknownURLScheme,
+  kMissingURLScheme,
+  kHttpURLScheme,
+  kHttpsURLScheme,
+  kFtpURLScheme,
+  kChromeExtensionURLScheme,
+  kJavascriptURLScheme,
+  kFileURLScheme,
+  kBlobURLScheme,
+  kDataURLScheme,
+  kMaxURLScheme = kDataURLScheme  // Must be equal to highest enum value.
+};
+
+static URLSchemeForHistogram URLScheme(const GURL& url) {

[tony, 2011/11/15 20:50:54]

Nit: This could also go in the anonymous namespace.

[Ami GONE FROM CHROMIUM, 2011/11/15 21:03:53]

Ditto.
(plus anonymous namespaces are generally less fun to deal with in debuggers than
file-static functions, which is why I prefer file-statics)

+  if (!url.has_scheme()) return kMissingURLScheme;
+  if (url.SchemeIs("http")) return kHttpURLScheme;
+  if (url.SchemeIs("https")) return kHttpsURLScheme;
+  if (url.SchemeIs("ftp")) return kFtpURLScheme;
+  if (url.SchemeIs("chrome-extension")) return kChromeExtensionURLScheme;
+  if (url.SchemeIs("javascript")) return kJavascriptURLScheme;
+  if (url.SchemeIs("file")) return kFileURLScheme;
+  if (url.SchemeIs("blob")) return kBlobURLScheme;
+  if (url.SchemeIs("data")) return kDataURLScheme;

[tony, 2011/11/15 20:50:54]

Nit: You could just make an array of char* pointers and return the index in the
array.

[Ami GONE FROM CHROMIUM, 2011/11/15 21:03:53]

I started out really liking this suggestion, and coded it up (see below) but ran
into a couple of icky points:
1) To support future-extensibility I want the "unknown" value to be something
other than arraysize(kURLSchemes) (so that future schemes don't mix with today's
"unknown" bucket).  This requires the hack of using a never-matching (all caps
means no canonicalized scheme match) magic string, which then the function needs
to know about ("0" below).
2) Since the point is to end up passing this value through
UMA_HISTOGRAM_ENUMERATION, it feels icky to give it an index into a magical
array.  The lying would then continue into tools/histograms/histograms.xml where
I'd have to claim this was an enum and give its "values".

Somehow the accumulation makes me prefer the existing code.  WDYT?


This is the code I came up with; let me know if you meant something entirely
different:

static const char* kURLSchemes[] = {
  "UNKNOWN", "", "http", "https", "ftp", "chrome-extension", "javascript",
  "file", "blob", "data",
};

static URLSchemeForHistogram URLScheme(const GURL& url) {
  const std::string scheme = url.scheme();
  // Could also start the loop at 1 since UNKNOWN will never match.
  for (size_t i = 0; i < arraysize(kURLSchemes); ++i) {
    if (scheme == kURLSchemes[i])
      return i;
  }
  return 0;  // Magical key of "UNKNOWN".
}

[tony, 2011/11/15 21:33:21]

You could avoid some of the ickiness by doing:
static const char* kURLSchemes[] = {
  "",  // missing
  "http", "https", "ftp", "chrome-extension", "javascript",
  "file", "blob", "data",
};

And adding 1 to the return value, but I agree that it makes the xml file kind of
magical.  I don't feel that strongly one way or another (hence, it was a nit).

+  return kUnknownURLScheme;
+}
+
 void WebMediaPlayerImpl::load(const WebKit::WebURL& url) {
   DCHECK_EQ(main_loop_, MessageLoop::current());
   DCHECK(proxy_);
 
+  UMA_HISTOGRAM_ENUMERATION("Media.URLScheme", URLScheme(url), kMaxURLScheme);
+
   if (media_stream_client_) {
     bool has_video = false;
     bool has_audio = false;
@@ -708,6 +739,11 @@ void WebMediaPlayerImpl::OnPipelineInitialize(PipelineStatus status) {
         static_cast<float>(pipeline_->GetMediaDuration().InSecondsF());
     buffered_.swap(new_buffered);
 
+    if (hasVideo()) {
+      UMA_HISTOGRAM_BOOLEAN("Media.AcceleratedCompositingActive",
+                            is_accelerated_compositing_active_);
+    }
+
     if (pipeline_->IsLoaded()) {
       SetNetworkState(WebKit::WebMediaPlayer::Loaded);
     }