Update UMA user actions parsing, primarily to include WebUI metrics.

Update UMA user actions parsing, primarily to include WebUI metrics.

* Update extract_actions.py to scan for metrics specified in the WebUI code.  A common pattern is <input pref="blah" metric="Options_Blah" type="checkbox">.

* Clean up almost all of the existing warnings thrown by extract_actions.py.  There is one remaining use of RecordComputedAction that is a bit too subtle for me to lump into this patch.

* Add a new warning to extract_actions.py that scans for uses of UserMetricsAction with a non-literal string.  Any such uses are going to be missed by the script, and hence should use RecordComputedAction instead.

BUG=none
TEST=none

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

[Ilya Sherman, 2011-07-07 08:07:28]

James, please take a look at the WebUI changes.

Evan, please take a look at everything else.

[James Hawkins, 2011-07-07 16:30:50]

http://codereview.chromium.org/7314020/diff/5001/chrome/browser/pdf_unsupport...
File chrome/browser/pdf_unsupported_feature.cc (left):

http://codereview.chromium.org/7314020/diff/5001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:352:
UserMetrics::RecordAction(UserMetricsAction(reader_installed_ ?
What's wrong with this version?

http://codereview.chromium.org/7314020/diff/5001/chrome/browser/resources/opt...
File chrome/browser/resources/options/autofill_options.html (right):

http://codereview.chromium.org/7314020/diff/5001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options.html:7:
metric="Options_AutofillAuxiliaryProfiles" />
s/ \///

In HTML, input elements do not have an ending slash.

[Evan Martin, 2011-07-07 16:41:28]

Can you make the change description describe what you're trying to do?  It's
hard to review when the motivation for the change is (paraphrasing) "update
code".

[Ilya Sherman, 2011-07-07 18:56:52]

On 2011/07/07 16:41:28, Evan Martin wrote:
> Can you make the change description describe what you're trying to do?  It's
> hard to review when the motivation for the change is (paraphrasing) "update
> code".

Done.

[Ilya Sherman, 2011-07-07 18:59:43]

http://codereview.chromium.org/7314020/diff/5001/chrome/browser/pdf_unsupport...
File chrome/browser/pdf_unsupported_feature.cc (left):

http://codereview.chromium.org/7314020/diff/5001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:352:
UserMetrics::RecordAction(UserMetricsAction(reader_installed_ ?
On 2011/07/07 16:30:50, James Hawkins wrote:
> What's wrong with this version?

This script scans for the literal string """ UserMetricsAction(" """ to detect
metrics used with UserMetrics::RecordAction.  I could rewrite this as

UserMetrics::RecordAction(reader_installed_ ?
    UserMetricsAction("PDF_UseReaderInfoBarCancel") :
    UserMetricsAction("PDF_InstallReaderInfoBarCancel"));

if that style is preferable.

[James Hawkins, 2011-07-07 20:44:19]

http://codereview.chromium.org/7314020/diff/5001/chrome/browser/pdf_unsupport...
File chrome/browser/pdf_unsupported_feature.cc (left):

http://codereview.chromium.org/7314020/diff/5001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:352:
UserMetrics::RecordAction(UserMetricsAction(reader_installed_ ?
On 2011/07/07 18:59:44, Ilya Sherman wrote:
> On 2011/07/07 16:30:50, James Hawkins wrote:
> > What's wrong with this version?
> 
> This script scans for the literal string """ UserMetricsAction(" """ to detect
> metrics used with UserMetrics::RecordAction.  I could rewrite this as
> 
> UserMetrics::RecordAction(reader_installed_ ?
>     UserMetricsAction("PDF_UseReaderInfoBarCancel") :
>     UserMetricsAction("PDF_InstallReaderInfoBarCancel"));
> 
> if that style is preferable.

Yea, I think I prefer that. It's at least in the spirit of the old code.

[Ilya Sherman, 2011-07-07 21:01:01]

http://codereview.chromium.org/7314020/diff/5001/chrome/browser/pdf_unsupport...
File chrome/browser/pdf_unsupported_feature.cc (left):

http://codereview.chromium.org/7314020/diff/5001/chrome/browser/pdf_unsupport...
chrome/browser/pdf_unsupported_feature.cc:352:
UserMetrics::RecordAction(UserMetricsAction(reader_installed_ ?
On 2011/07/07 20:44:19, James Hawkins wrote:
> On 2011/07/07 18:59:44, Ilya Sherman wrote:
> > On 2011/07/07 16:30:50, James Hawkins wrote:
> > > What's wrong with this version?
> > 
> > This script scans for the literal string """ UserMetricsAction(" """ to
detect
> > metrics used with UserMetrics::RecordAction.  I could rewrite this as
> > 
> > UserMetrics::RecordAction(reader_installed_ ?
> >     UserMetricsAction("PDF_UseReaderInfoBarCancel") :
> >     UserMetricsAction("PDF_InstallReaderInfoBarCancel"));
> > 
> > if that style is preferable.
> 
> Yea, I think I prefer that. It's at least in the spirit of the old code.

Ok, switched to that style.

http://codereview.chromium.org/7314020/diff/5001/chrome/browser/resources/opt...
File chrome/browser/resources/options/autofill_options.html (right):

http://codereview.chromium.org/7314020/diff/5001/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options.html:7:
metric="Options_AutofillAuxiliaryProfiles" />
On 2011/07/07 16:30:50, James Hawkins wrote:
> s/ \///
> 
> In HTML, input elements do not have an ending slash.

Done.

http://codereview.chromium.org/7314020/diff/5001/chrome/tools/extract_actions.py
File chrome/tools/extract_actions.py (right):

http://codereview.chromium.org/7314020/diff/5001/chrome/tools/extract_actions...
chrome/tools/extract_actions.py:279: is_boolean = True
Actually, James, could you look at this bit of logic too?  I tried to distill
this from pref_ui.js, but I'm not sure I got it 100% correct.

[James Hawkins, 2011-07-07 21:10:08]

http://codereview.chromium.org/7314020/diff/5001/chrome/tools/extract_actions.py
File chrome/tools/extract_actions.py (right):

http://codereview.chromium.org/7314020/diff/5001/chrome/tools/extract_actions...
chrome/tools/extract_actions.py:279: is_boolean = True
On 2011/07/07 21:01:01, Ilya Sherman wrote:
> Actually, James, could you look at this bit of logic too?  I tried to distill
> this from pref_ui.js, but I'm not sure I got it 100% correct.

This part looks OK to me.

[Ilya Sherman, 2011-07-11 22:18:18]

Evan, I've updated the CL's description.  Please take a look at the changes.

[Evan Martin, 2011-07-11 22:21:26]

Jim is the metrics expert, sorry for not putting him on here earlier.  I just
did a drive-by comment before.

[Ilya Sherman, 2011-07-11 22:29:02]

On 2011/07/11 22:21:26, Evan Martin wrote:
> Jim is the metrics expert, sorry for not putting him on here earlier.  I just
> did a drive-by comment before.

Hmm, ok.  I had tagged you for review since you are listed as the author for the
extract_actions.py script.

[Ilya Sherman, 2011-07-20 03:43:11]

Jim, please take a look at this change when you have a chance.  Thanks :)

[Ilya Sherman, 2011-07-29 04:37:19]

Jim: ping

[jar (doing other things), 2011-08-01 17:26:44]

LGTM... please see comment below.

I've added Ian Fette to the CC list, since he's generally involved in this
metric infrastructure.

http://codereview.chromium.org/7314020/diff/7002/chrome/tools/extract_actions.py
File chrome/tools/extract_actions.py (right):

http://codereview.chromium.org/7314020/diff/7002/chrome/tools/extract_actions...
chrome/tools/extract_actions.py:229: malformed_action_re =
re.compile(r'[^a-zA-Z]UserMetricsAction\([^"]')
I like your definition better than the one in 228.  Why not just switch to it? 
It is not perfect.... but it is better (precluding a prefix that would imply the
"name" is not really the one we were looking for).

[James Hawkins, 2011-08-01 19:00:25]

WebUI LGTM

[Ilya Sherman, 2011-08-06 03:23:51]

http://codereview.chromium.org/7314020/diff/7002/chrome/tools/extract_actions.py
File chrome/tools/extract_actions.py (right):

http://codereview.chromium.org/7314020/diff/7002/chrome/tools/extract_actions...
chrome/tools/extract_actions.py:229: malformed_action_re =
re.compile(r'[^a-zA-Z]UserMetricsAction\([^"]')
On 2011/08/01 17:26:45, jar wrote:
> I like your definition better than the one in 228.  Why not just switch to it?

> It is not perfect.... but it is better (precluding a prefix that would imply
the
> "name" is not really the one we were looking for).

There are two differences between |action_re| and |malformed_action_re|:
  (1) The latter includes [^a-zA-Z] at the beginning, i.e. precludes irrrelevant
prefixes.
  (2) The prior expects for a quotation mark (") immediately after the
parenthesis, whereas the latter checks for any other character.

I think that your point is that (1) should be included in |action_re| as well. 
I've updated the code in this manner.

[Ilya Sherman, 2011-08-08 20:03:17]

Scott, could you take a glance at the content/browser/ change?

[sky, 2011-08-08 21:26:29]

LGTM