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

Only report Media.AcceleratedCompositingActive for video-having playbacks.
Added Media.URLScheme histogram.

BUG=none
TEST=manual observation of chrome://histograms/Media


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110201

[scherkus (not reviewing), 2011-11-15 18:55:46]

LGTM

[Ami GONE FROM CHROMIUM, 2011-11-15 19:18:11]

PTAL - added URLScheme.

[scherkus (not reviewing), 2011-11-15 20:01:07]

LGTM w/ suggestion

http://codereview.chromium.org/8468011/diff/1004/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8468011/diff/1004/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:259: if (url.SchemeIs("data")) return
kDataURLScheme;
chrome-extension is another good one

what about schemes that we know we don't support, such as:
javascript
ftp

[Ami GONE FROM CHROMIUM, 2011-11-15 20:33:28]

http://codereview.chromium.org/8468011/diff/1004/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8468011/diff/1004/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:259: if (url.SchemeIs("data")) return
kDataURLScheme;
On 2011/11/15 20:01:07, scherkus wrote:
> chrome-extension is another good one

Added.

> what about schemes that we know we don't support, such as:
> javascript
> ftp

Good ideas.  Added.

[commit-bot: I haz the power, 2011-11-15 20:33:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/8468011/5002

[commit-bot: I haz the power, 2011-11-15 20:33:47]

Presubmit check for 8468011-5002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/glue/webmediaplayer_impl.cc,webkit/glue/webmediaplayer_impl.h

[Ami GONE FROM CHROMIUM, 2011-11-15 20:40:38]

tony: i can haz OWNERS approval?

[tony, 2011-11-15 20:50:53]

LGTM, but a for loop might be a bit more concise.

http://codereview.chromium.org/8468011/diff/5002/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8468011/diff/5002/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:242: enum URLSchemeForHistogram {
Nit: Can we put this in the anonymous namespace?

http://codereview.chromium.org/8468011/diff/5002/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:256: static URLSchemeForHistogram
URLScheme(const GURL& url) {
Nit: This could also go in the anonymous namespace.

http://codereview.chromium.org/8468011/diff/5002/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:265: if (url.SchemeIs("data")) return
kDataURLScheme;
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]

tony: thanks for the quick review.  I rejected your suggestions so will wait for
another LGTM from you before assuming they were optional :)

http://codereview.chromium.org/8468011/diff/5002/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8468011/diff/5002/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:242: enum URLSchemeForHistogram {
On 2011/11/15 20:50:54, tony wrote:
> Nit: Can we put this in the anonymous namespace?

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").

http://codereview.chromium.org/8468011/diff/5002/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:256: static URLSchemeForHistogram
URLScheme(const GURL& url) {
On 2011/11/15 20:50:54, tony wrote:
> Nit: This could also go in the anonymous namespace.

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

http://codereview.chromium.org/8468011/diff/5002/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:265: if (url.SchemeIs("data")) return
kDataURLScheme;
On 2011/11/15 20:50:54, tony wrote:
> Nit: You could just make an array of char* pointers and return the index in
the
> array.

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:20]

http://codereview.chromium.org/8468011/diff/5002/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8468011/diff/5002/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:242: enum URLSchemeForHistogram {
On 2011/11/15 21:03:53, Ami Fischman wrote:
> On 2011/11/15 20:50:54, tony wrote:
> > Nit: Can we put this in the anonymous namespace?
> 
> 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").

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

http://codereview.chromium.org/8468011/diff/5002/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:265: if (url.SchemeIs("data")) return
kDataURLScheme;
On 2011/11/15 21:03:53, Ami Fischman wrote:
> On 2011/11/15 20:50:54, tony wrote:
> > Nit: You could just make an array of char* pointers and return the index in
> the
> > array.
> 
> I started out really liking this suggestion, and coded it up (see below) but
ran
> into a couple of icky points:

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).

[tony, 2011-11-15 21:33:38]

oh, so lgtm

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

http://codereview.chromium.org/8468011/diff/5002/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8468011/diff/5002/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:242: enum URLSchemeForHistogram {
On 2011/11/15 21:33:21, tony wrote:
> On 2011/11/15 21:03:53, Ami Fischman wrote:
> > On 2011/11/15 20:50:54, tony wrote:
> > > Nit: Can we put this in the anonymous namespace?
> > 
> > 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").
> 
> 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

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).

[commit-bot: I haz the power, 2011-11-15 21:47:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/8468011/1005

[commit-bot: I haz the power, 2011-11-15 23:01:00]

Change committed as 110201