Split UserMetrics into API vs. implementation. Move API to content/public.

Split UserMetrics into API vs. implementation. Move API to content/public.

TBR=davemoore@chromium.org
BUG=98716
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114416

[jam, 2011-12-12 23:06:55]

http://codereview.chromium.org/8919017/diff/2001/content/browser/user_metrics...
File content/browser/user_metrics_impl.cc (right):

http://codereview.chromium.org/8919017/diff/2001/content/browser/user_metrics...
content/browser/user_metrics_impl.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
actually, please leave this as user_metrics.cc. I had spoken to folks working on
the WebKit API. they only add a _impl to the cc if there's a corresponding
_impl.h. If it's just a X.header in the public directory, and one c++ file, then
that file is just named X.cc

http://codereview.chromium.org/8919017/diff/2001/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/8919017/diff/2001/content/content_browser.gypi...
content/content_browser.gypi:619: 'browser/user_metrics_impl.h',
nit: no such file, you can just remove the header entry now while you're at it
(to avoid conflicts later :) )

http://codereview.chromium.org/8919017/diff/2001/content/public/browser/user_...
File content/public/browser/user_metrics.h (right):

http://codereview.chromium.org/8919017/diff/2001/content/public/browser/user_...
content/public/browser/user_metrics.h:30: class CONTENT_EXPORT UserMetrics {
it seems that we don't need the UserMetrics class for scoping now that we have a
content namespace. for other APIs, i've been removing such classes and putting
the static functions in the content namespace.

it's much easier to tell people that we have a simple rule (everything in the
content namespace, similar to WebKit namespace for the webkit API), rather than
to start making exceptions

[Jói, 2011-12-12 23:27:34]

Hi John,

Will remove the _impl from the filename and the _impl.h file from the
gypi (I had it temporarily).

The issue with changing the naming for UserMetrics::RecordAction (in
particular) is that there is a script that parses for an exact
single-line invocation of the function; from the comment:

 34   // WARNING: Call this function exactly like this:	
 35   //   UserMetrics::RecordAction(UserMetricsAction("foo bar"));

While I could most likely update the script to match (would have to
consult with jar@ first though - adding him to CC), if we use the
content namespace to contain, then in a header file (worst case
because of restrictions on using the 'using' keyword):

void SomeFunctionInHeaderFile() {
  content::RecordUserMetricsAction(content::UserMetricsAction("foo bar"));
}

which is a lot harder to keep on a single line than the original version, or

void SomeFunctionInHeaderFile() {
  using content::RecordUserMetricsAction;
  using content::UserMetricsAction;
  RecordUserMetricsAction(UserMetricsAction("foo bar"));
}

which is correct, and can be kept on a single line, but is pretty verbose.

Personally, I think it's a lot simpler to just skip the content
namespace here, not much harm will come of it, the compiler will very
quickly tell people if they're using it wrong.

As a fallback position, I would suggest enclosing the UserMetrics
class and UserMetricsAction struct in the content namespace, i.e. not
skipping the UserMetrics enclosure, that way we won't have to modify
the script that parses these things as we can just add the necessary
using clauses.

Cheers,
Jói




On Mon, Dec 12, 2011 at 11:06 PM,  <jam@chromium.org> wrote:
>
>
http://codereview.chromium.org/8919017/diff/2001/content/browser/user_metrics...
> File content/browser/user_metrics_impl.cc (right):
>
>
http://codereview.chromium.org/8919017/diff/2001/content/browser/user_metrics...
> content/browser/user_metrics_impl.cc:1: // Copyright (c) 2011 The
> Chromium Authors. All rights reserved.
> actually, please leave this as user_metrics.cc. I had spoken to folks
> working on the WebKit API. they only add a _impl to the cc if there's a
> corresponding _impl.h. If it's just a X.header in the public directory,
> and one c++ file, then that file is just named X.cc
>
> http://codereview.chromium.org/8919017/diff/2001/content/content_browser.gypi
> File content/content_browser.gypi (right):
>
>
http://codereview.chromium.org/8919017/diff/2001/content/content_browser.gypi...
> content/content_browser.gypi:619: 'browser/user_metrics_impl.h',
> nit: no such file, you can just remove the header entry now while you're
> at it (to avoid conflicts later :) )
>
>
http://codereview.chromium.org/8919017/diff/2001/content/public/browser/user_...
> File content/public/browser/user_metrics.h (right):
>
>
http://codereview.chromium.org/8919017/diff/2001/content/public/browser/user_...
> content/public/browser/user_metrics.h:30: class CONTENT_EXPORT
> UserMetrics {
> it seems that we don't need the UserMetrics class for scoping now that
> we have a content namespace. for other APIs, i've been removing such
> classes and putting the static functions in the content namespace.
>
> it's much easier to tell people that we have a simple rule (everything
> in the content namespace, similar to WebKit namespace for the webkit
> API), rather than to start making exceptions
>
> http://codereview.chromium.org/8919017/

[jar (doing other things), 2011-12-12 23:37:44]

+cc ian@

I've focused mostly on the histograms, which does not scan for patterns that
match.  

The events code does indeed scan source files and do pattern matching.  I think
Ian may be most involved with this... so I'll leave it to him to comment.

[Jói, 2011-12-13 09:26:41]

Ah, thanks Jim, I had a mapping [ user_metrics === Jim ] in my head :)

On Mon, Dec 12, 2011 at 11:37 PM,  <jar@chromium.org> wrote:
> +cc ian@
>
> I've focused mostly on the histograms, which does not scan for patterns that
> match.
>
> The events code does indeed scan source files and do pattern matching.  I
> think
> Ian may be most involved with this... so I'll leave it to him to comment.
>
> http://codereview.chromium.org/8919017/

[Jói, 2011-12-13 12:37:02]

John: PTAL.  The real changes are in content/*/user_metrics* and
chrome/tools/extract_actions.py, the rest is just fixing up usage.

Ian:  Please look at chrome/tools/extract_actions.py

I looked into the chrome/tools/extract_actions.py script and it turns
out the parsing is a bit more flexible than the comment in
user_metrics.h would have you believe.  I moved everything to the
content namespace, fixed usages, and updated the script.  While in the
script, I fixed it to work in a git checkout, and slightly improved
error reporting so that on an HTML parsing error you get the path
being parsed as well as the stack trace.

Apart from the normal trybot runs which are pending, I tested my
changes by running the extract_actions.py script on a client before
and after my changes.  With the --hash parameter, the
chromeactions.txt files generated before and after changes are
identical.  Without it, the output is identical except for the line
numbers on a couple of mesages that have changed because line numbers
in code changed.

Cheers,
Jói



2011/12/13 Jói Sigurðsson <joi@chromium.org>:
> Ah, thanks Jim, I had a mapping [ user_metrics === Jim ] in my head :)
>
> On Mon, Dec 12, 2011 at 11:37 PM,  <jar@chromium.org> wrote:
>> +cc ian@
>>
>> I've focused mostly on the histograms, which does not scan for patterns that
>> match.
>>
>> The events code does indeed scan source files and do pattern matching.  I
>> think
>> Ian may be most involved with this... so I'll leave it to him to comment.
>>
>> http://codereview.chromium.org/8919017/

[Jói, 2011-12-13 20:23:34]

Friendly ping.

[jam, 2011-12-14 01:18:38]

just looked at the user_metrics files, lgtm

http://codereview.chromium.org/8919017/diff/10002/content/browser/user_metric...
File content/browser/user_metrics.cc (right):

http://codereview.chromium.org/8919017/diff/10002/content/browser/user_metric...
content/browser/user_metrics.cc:11: #include
"content/public/browser/user_metrics.h"
nit: redundant

http://codereview.chromium.org/8919017/diff/10002/content/public/browser/user...
File content/public/browser/user_metrics.h (right):

http://codereview.chromium.org/8919017/diff/10002/content/public/browser/user...
content/public/browser/user_metrics.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: is this file moved or copied? the svn history isn't there.

[ian fette, 2011-12-14 01:24:40]

lgtm

apologies for the delay, in tokyo this week

[Jói, 2011-12-14 09:13:56]

Thanks for the review guys!

>
http://codereview.chromium.org/8919017/diff/10002/content/public/browser/user...
> content/public/browser/user_metrics.h:1: // Copyright (c) 2011 The
>
> Chromium Authors. All rights reserved.
> nit: is this file moved or copied? the svn history isn't there.

It's moved, but I use git and the file changed enough that it doesn't
detect it as a move.  Before I commit I'll see if I can tune up the
"maximum change" threshold of the move detection.

Cheers,
Jói


On Wed, Dec 14, 2011 at 1:24 AM,  <ian@chromium.org> wrote:
> lgtm
>
> apologies for the delay, in tokyo this week
>
> http://codereview.chromium.org/8919017/

[Jói, 2011-12-14 13:22:53]

TBR davemoore@ for OWNERS approval of trivial changes in
chrome/browser/profiles/profile_impl.cc
chrome/browser/profiles/profile_manager.cc
chrome/browser/chromeos/status/input_method_menu.cc

Cheers,
Jói