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

webkit/glue/webmediaplayer_impl.cc

 // Copyright (c) 2011 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 "webkit/glue/webmediaplayer_impl.h"
 
 #include <limits>
 #include <string>
 
 #include "base/bind.h"
@@ skipping 102 matching lines @@
       message_loop_factory_(message_loop_factory),
       paused_(true),
       seeking_(false),
       playback_rate_(0.0f),
       pending_seek_(false),
       client_(client),
       proxy_(NULL),
       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_);
   main_loop_ = MessageLoop::current();
   media_log_->AddEvent(
       media_log_->CreateEvent(media::MediaLogEvent::WEBMEDIAPLAYER_CREATED));
 }
 
 bool WebMediaPlayerImpl::Initialize(
     WebKit::WebFrame* frame,
@@ skipping 12 matching lines @@
   // Also, delaying GC until after player starts gets rid of starting lag --
   // collection happens in parallel with playing.
   // TODO(enal): remove when we get rid of per-audio-stream thread.
   if (!incremented_externally_allocated_memory_) {
     MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(&WebMediaPlayerImpl::IncrementExternallyAllocatedMemory,
                    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_);
 
   // Also we want to be notified of |main_loop_| destruction.
   main_loop_->AddDestructionObserver(this);
 
   // Creates the proxy.
   proxy_ = new WebMediaPlayerProxy(main_loop_, this);
   web_video_renderer->SetWebMediaPlayerProxy(proxy_);
   proxy_->SetVideoRenderer(web_video_renderer);
@@ skipping 63 matching lines @@
   if (delegate_)
     delegate_->PlayerGone(this);
 
   // Finally tell the |main_loop_| we don't want to be notified of destruction
   // event.
   if (main_loop_) {
     main_loop_->RemoveDestructionObserver(this);
   }
 }
 
+// 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;
     scoped_refptr<media::VideoDecoder> new_decoder =
         media_stream_client_->GetVideoDecoder(url, message_loop_factory_.get());
     if (new_decoder.get()) {
       // Remove the default decoder.
       scoped_refptr<media::VideoDecoder> old_videodecoder;
       filter_collection_->SelectVideoDecoder(&old_videodecoder);
       filter_collection_->AddVideoDecoder(new_decoder.get());
@@ skipping 447 matching lines @@
 void WebMediaPlayerImpl::OnPipelineInitialize(PipelineStatus status) {
   DCHECK_EQ(main_loop_, MessageLoop::current());
   if (status == media::PIPELINE_OK) {
     // Only keep one time range starting from 0.
     WebKit::WebTimeRanges new_buffered(static_cast<size_t>(1));
     new_buffered[0].start = 0.0f;
     new_buffered[0].end =
         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);
     }
 
     // Since we have initialized the pipeline, say we have everything otherwise
     // we'll remain either loading/idle.
     // TODO(hclam): change this to report the correct status.
     SetReadyState(WebKit::WebMediaPlayer::HaveMetadata);
     SetReadyState(WebKit::WebMediaPlayer::HaveEnoughData);
   } else {
@@ skipping 147 matching lines @@
   return client_;
 }
 
 void WebMediaPlayerImpl::IncrementExternallyAllocatedMemory() {
   DCHECK_EQ(main_loop_, MessageLoop::current());
   incremented_externally_allocated_memory_ = true;
   v8::V8::AdjustAmountOfExternalAllocatedMemory(kPlayerExtraMemory);
 }
 
 }  // namespace webkit_glue