Extract actions from WebUI JS

Extract actions from WebUI JS

Similar to how we extract actions from C++ calls to
UserMetricsAction, extract actions from JS calls to
coreOptionsUserMetricsAction via chrome.send.

Committed: https://crrev.com/1b6c3e7b9c10309d3a9503ebd4f33ec5e0197b4e
Cr-Commit-Position: refs/heads/master@{#358888}

[michaelpg, 2015-03-20 21:29:52]

Patchset #1 (id:1) has been deleted

[michaelpg, 2015-03-20 21:30:02]

Patchset #1 (id:20001) has been deleted

[michaelpg, 2015-03-20 21:31:24]

michaelpg@chromium.org changed reviewers:
+ isherman@chromium.org

[michaelpg, 2015-03-20 21:31:57]

PTAL. I don't write much Python, but it worked!

[Ilya Sherman, 2015-03-20 21:57:39]

Thanks!  LGTM % one comment:

https://codereview.chromium.org/1025673004/diff/40001/tools/metrics/actions/e...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/1025673004/diff/40001/tools/metrics/actions/e...
tools/metrics/actions/extract_actions.py:66: ([^']+?)                    # A
sequence of characters for the param.
"+?" is already non-greedy, so you can replace "[^']" with "."

But, if you don't take this suggestion, I have a question: Should this line
include a double quote as well as a single quote in the list of non-matched
characters?

[michaelpg, 2015-03-20 22:14:29]

On 2015/03/20 21:57:39, Ilya Sherman wrote:
> Thanks!  LGTM % one comment:
> 
>
https://codereview.chromium.org/1025673004/diff/40001/tools/metrics/actions/e...
> File tools/metrics/actions/extract_actions.py (right):
> 
>
https://codereview.chromium.org/1025673004/diff/40001/tools/metrics/actions/e...
> tools/metrics/actions/extract_actions.py:66: ([^']+?)                    # A
> sequence of characters for the param.
> "+?" is already non-greedy, so you can replace "[^']" with "."
> 
> But, if you don't take this suggestion, I have a question: Should this line
> include a double quote as well as a single quote in the list of non-matched
> characters?

Good points. I changed it to use QUOTED_STRING_RE since, why not? And added
tests.

[Dan Beam, 2015-03-20 22:43:56]

excuse the drive-by

https://codereview.chromium.org/1025673004/diff/60001/tools/metrics/actions/e...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/1025673004/diff/60001/tools/metrics/actions/e...
tools/metrics/actions/extract_actions.py:517: return
nit: shouldn't this be 2\s instead of 4\s?  how does this work?  i thought
python would throw in this case?

[Ilya Sherman, 2015-03-20 22:52:08]

(Still LGTM)

https://codereview.chromium.org/1025673004/diff/60001/tools/metrics/actions/e...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/1025673004/diff/60001/tools/metrics/actions/e...
tools/metrics/actions/extract_actions.py:74: QUOTED_STRING_RE =
re.compile(r'[\'\"](.+?)[\'\"]')
Optional nit: I think you can write the regex with less escaping as

r"""['"](.+?)['"]"""

Now that I've written it, though, I'm not actually so sure that it's more
readable this way... -- maybe a little bit? :P

[michaelpg, 2015-03-20 23:27:28]

In Patch 2 I used the QUOTED_STRING_RE for checking JS actions, making the
USER_METRICS_ACTION_RE_JS a bit more liberal. But this doesn't quite work,
because for this code:

      chrome.send('coreOptionsUserMetricsAction',
                  ['Options_Languages_InputMethodCheckbox' +
                   (checkbox.checked ? '_Enable' : '_Disable')]);

it would extract 'Options_Languages_InputMethodCheckbox' as an action, which is
wrong.

Should I:

1. Go back to Patch 1, where this is ignored, or
2. Use Patch 2 but throw an exception here, to force JS that can be parsed with
this script

I'm leaning toward 1, but the example above is the only instance where code
would have to change for 2.

https://codereview.chromium.org/1025673004/diff/60001/tools/metrics/actions/e...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/1025673004/diff/60001/tools/metrics/actions/e...
tools/metrics/actions/extract_actions.py:517: return
On 2015/03/20 22:43:55, Dan Beam wrote:
> nit: shouldn't this be 2\s instead of 4\s?  how does this work?  i thought
> python would throw in this case?

Yes, it should be 2. But I don't think Python cares whether the indentation is
"consistent" with past indentation once you've started a new "block" (and this
doesn't throw).

if 1:
    ok
if 2:
  still_ok
  if 3:
      also_ok
    not_ok

[Ilya Sherman, 2015-03-20 23:53:23]

On 2015/03/20 23:27:28, michaelpg wrote:
> In Patch 2 I used the QUOTED_STRING_RE for checking JS actions, making the
> USER_METRICS_ACTION_RE_JS a bit more liberal. But this doesn't quite work,
> because for this code:
> 
>       chrome.send('coreOptionsUserMetricsAction',
>                   ['Options_Languages_InputMethodCheckbox' +
>                    (checkbox.checked ? '_Enable' : '_Disable')]);
> 
> it would extract 'Options_Languages_InputMethodCheckbox' as an action, which
is
> wrong.

Maybe I'm misreading the regex, but it looks like it shouldn't match
'Options_Languages_InputMethodCheckbox', since there's no closing ']' for the
regex to match unless it gobbles up more than just that prefix string.  Could
you explain why that matches the regex?

But, in any case, your point is valid -- if this were written on a single line,
then something like """'Options_Foo' + (bar ? '_Enable' : '_Disable')""" could
be matched by the regex.  In C++, we do force the parseable formatting, so that
the Python script can catch all user actions.  (At least, we try to... looking
at the code, I'm wondering whether this got broken in a refactoring.)  In JS, we
could either provide a best-effort parsing (patch set 1) or also force a
parseable formatting.  I'm fine with either option, as I assume that there are
fewer actions recorded from JS than from C++.

> Should I:
> 
> 1. Go back to Patch 1, where this is ignored, or
> 2. Use Patch 2 but throw an exception here, to force JS that can be parsed
with
> this script
> 
> I'm leaning toward 1, but the example above is the only instance where code
> would have to change for 2.

[Ilya Sherman, 2015-03-20 23:58:24]

On 2015/03/20 23:53:23, Ilya Sherman wrote:
> On 2015/03/20 23:27:28, michaelpg wrote:
> > In Patch 2 I used the QUOTED_STRING_RE for checking JS actions, making the
> > USER_METRICS_ACTION_RE_JS a bit more liberal. But this doesn't quite work,
> > because for this code:
> > 
> >       chrome.send('coreOptionsUserMetricsAction',
> >                   ['Options_Languages_InputMethodCheckbox' +
> >                    (checkbox.checked ? '_Enable' : '_Disable')]);
> > 
> > it would extract 'Options_Languages_InputMethodCheckbox' as an action, which
> is
> > wrong.
> 
> Maybe I'm misreading the regex, but it looks like it shouldn't match
> 'Options_Languages_InputMethodCheckbox', since there's no closing ']' for the
> regex to match unless it gobbles up more than just that prefix string.  Could
> you explain why that matches the regex?

Oh, okay, I see that the regexes are defined with DOTALL, so that .+? can match
newlines.  This addresses my concern about the C++ parsing code not throwing
exceptions as often as it should.  I'm still not sure why your Patch Set 2 would
extract the wrong action -- 'Options_Languages_InputMethodCheckbox' -- rather
than throwing an exception.

FWIW, checkboxes are usually annotated in HTML, rather than implementing custom
user action code in JS.  So, we could probably fix the one problematic case in
the existing JS codebase by annotating the corresponding HTML instead, and then
throw exceptions for any future code that violates the simple formatting that we
know how to parse.  That might be slightly annoying for developers, but on the
plus side would make it harder to forget to update the set of actions.  I'm fine
either way, but I'm leaning toward matching what C++ does if there's currently
only one violation across the entire JS codebase.

[michaelpg, 2015-03-21 00:44:06]

On 2015/03/20 23:58:24, Ilya Sherman wrote:
> On 2015/03/20 23:53:23, Ilya Sherman wrote:
> > On 2015/03/20 23:27:28, michaelpg wrote:
> > > In Patch 2 I used the QUOTED_STRING_RE for checking JS actions, making the
> > > USER_METRICS_ACTION_RE_JS a bit more liberal. But this doesn't quite work,
> > > because for this code:
> > > 
> > >       chrome.send('coreOptionsUserMetricsAction',
> > >                   ['Options_Languages_InputMethodCheckbox' +
> > >                    (checkbox.checked ? '_Enable' : '_Disable')]);
> > > 
> > > it would extract 'Options_Languages_InputMethodCheckbox' as an action,
which
> > is
> > > wrong.
> > 
> > Maybe I'm misreading the regex, but it looks like it shouldn't match
> > 'Options_Languages_InputMethodCheckbox', since there's no closing ']' for
the
> > regex to match unless it gobbles up more than just that prefix string. 
Could
> > you explain why that matches the regex?
> 
> Oh, okay, I see that the regexes are defined with DOTALL, so that .+? can
match
> newlines.  This addresses my concern about the C++ parsing code not throwing
> exceptions as often as it should.  I'm still not sure why your Patch Set 2
would
> extract the wrong action -- 'Options_Languages_InputMethodCheckbox' -- rather
> than throwing an exception.

Adding a $ to the end of QUOTED_STRING_RE makes my example throw an exception
and
doesn't seem to have other effects.

> 
> FWIW, checkboxes are usually annotated in HTML, rather than implementing
custom
> user action code in JS.  So, we could probably fix the one problematic case in
> the existing JS codebase by annotating the corresponding HTML instead, and
then
> throw exceptions for any future code that violates the simple formatting that
we
> know how to parse.

We generate these <input>s in JS so fixing this would mean splitting out the
ternary condition into an if/else with two fully separate chrome.send calls.

>  That might be slightly annoying for developers, but on the
> plus side would make it harder to forget to update the set of actions.  I'm
fine
> either way, but I'm leaning toward matching what C++ does if there's currently
> only one violation across the entire JS codebase.

I did find more violations where we pass around function parameters to use as
the
action name, in autofill_options.js for example. So I think we should take the
route of not warning/throwing for JS.

[Ilya Sherman, 2015-03-21 00:55:56]

On 2015/03/21 00:44:06, michaelpg wrote:
> On 2015/03/20 23:58:24, Ilya Sherman wrote:
> > On 2015/03/20 23:53:23, Ilya Sherman wrote:
> > > On 2015/03/20 23:27:28, michaelpg wrote:
> > > > In Patch 2 I used the QUOTED_STRING_RE for checking JS actions, making
the
> > > > USER_METRICS_ACTION_RE_JS a bit more liberal. But this doesn't quite
work,
> > > > because for this code:
> > > > 
> > > >       chrome.send('coreOptionsUserMetricsAction',
> > > >                   ['Options_Languages_InputMethodCheckbox' +
> > > >                    (checkbox.checked ? '_Enable' : '_Disable')]);
> > > > 
> > > > it would extract 'Options_Languages_InputMethodCheckbox' as an action,
> which
> > > is
> > > > wrong.
> > > 
> > > Maybe I'm misreading the regex, but it looks like it shouldn't match
> > > 'Options_Languages_InputMethodCheckbox', since there's no closing ']' for
> the
> > > regex to match unless it gobbles up more than just that prefix string. 
> Could
> > > you explain why that matches the regex?
> > 
> > Oh, okay, I see that the regexes are defined with DOTALL, so that .+? can
> match
> > newlines.  This addresses my concern about the C++ parsing code not throwing
> > exceptions as often as it should.  I'm still not sure why your Patch Set 2
> would
> > extract the wrong action -- 'Options_Languages_InputMethodCheckbox' --
rather
> > than throwing an exception.
> 
> Adding a $ to the end of QUOTED_STRING_RE makes my example throw an exception
> and
> doesn't seem to have other effects.

Ah, good call.  Could you please make this change at least for the C++ parsing
code, and add a test there?

> > FWIW, checkboxes are usually annotated in HTML, rather than implementing
> custom
> > user action code in JS.  So, we could probably fix the one problematic case
in
> > the existing JS codebase by annotating the corresponding HTML instead, and
> then
> > throw exceptions for any future code that violates the simple formatting
that
> we
> > know how to parse.
> 
> We generate these <input>s in JS so fixing this would mean splitting out the
> ternary condition into an if/else with two fully separate chrome.send calls.
> 
> >  That might be slightly annoying for developers, but on the
> > plus side would make it harder to forget to update the set of actions.  I'm
> fine
> > either way, but I'm leaning toward matching what C++ does if there's
currently
> > only one violation across the entire JS codebase.
> 
> I did find more violations where we pass around function parameters to use as
> the
> action name, in autofill_options.js for example. So I think we should take the
> route of not warning/throwing for JS.

Okay, sounds good.

[Ilya Sherman, 2015-03-21 00:57:22]

Erm, hit reply before actually looking at the code.  Code LGTM, though please
add test coverage for the change to QUOTED_STRING_RE.  Thanks!

[michaelpg, 2015-11-02 21:54:59]

Patchset #4 (id:100001) has been deleted

[michaelpg, 2015-11-02 21:59:50]

Patchset #4 (id:120001) has been deleted

[michaelpg, 2015-11-02 22:01:42]

Look what I found gathering dust...

I added some more test coverage and adjusted QUOTED_RE so it doesn't accept
multiple quoted strings:

    code = """chrome.send('coreOptionsUserMetricsAction',
      ['Foo.' + foo_bar ? 'Bar' : 'Foo']);"""

[Ilya Sherman, 2015-11-02 23:31:02]

LGTM, thanks.

https://codereview.chromium.org/1025673004/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/1025673004/diff/140001/tools/metrics/actions/...
tools/metrics/actions/extract_actions.py:45: USER_METRICS_ACTION_RE_CPP =
re.compile(r"""
nit: This regex says CPP, but it includes Java code.  I don't have a suggestion
for a clearer name, though :/

[Dan Beam, 2015-11-02 23:44:24]

https://codereview.chromium.org/1025673004/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/1025673004/diff/140001/tools/metrics/actions/...
tools/metrics/actions/extract_actions.py:45: USER_METRICS_ACTION_RE_CPP =
re.compile(r"""
On 2015/11/02 23:31:02, Ilya Sherman wrote:
> nit: This regex says CPP, but it includes Java code.  I don't have a
suggestion
> for a clearer name, though :/

_NATIVE

[michaelpg, 2015-11-02 23:47:55]

On 2015/11/02 23:31:02, Ilya Sherman wrote:
> LGTM, thanks.
> 
>
https://codereview.chromium.org/1025673004/diff/140001/tools/metrics/actions/...
> File tools/metrics/actions/extract_actions.py (right):
> 
>
https://codereview.chromium.org/1025673004/diff/140001/tools/metrics/actions/...
> tools/metrics/actions/extract_actions.py:45: USER_METRICS_ACTION_RE_CPP =
> re.compile(r"""
> nit: This regex says CPP, but it includes Java code.  I don't have a
suggestion
> for a clearer name, though :/

How about just removing _CPP for that, and letting _JS be the special case?

[Ilya Sherman, 2015-11-02 23:54:57]

On 2015/11/02 23:47:55, michaelpg wrote:
> On 2015/11/02 23:31:02, Ilya Sherman wrote:
> > LGTM, thanks.
> > 
> >
>
https://codereview.chromium.org/1025673004/diff/140001/tools/metrics/actions/...
> > File tools/metrics/actions/extract_actions.py (right):
> > 
> >
>
https://codereview.chromium.org/1025673004/diff/140001/tools/metrics/actions/...
> > tools/metrics/actions/extract_actions.py:45: USER_METRICS_ACTION_RE_CPP =
> > re.compile(r"""
> > nit: This regex says CPP, but it includes Java code.  I don't have a
> suggestion
> > for a clearer name, though :/
> 
> How about just removing _CPP for that, and letting _JS be the special case?

Either this or Dan's suggestion is ok with me.

[Dan Beam, 2015-11-03 01:10:12]

On 2015/11/02 23:54:57, Ilya Sherman wrote:
> On 2015/11/02 23:47:55, michaelpg wrote:
> > On 2015/11/02 23:31:02, Ilya Sherman wrote:
> > > LGTM, thanks.
> > > 
> > >
> >
>
https://codereview.chromium.org/1025673004/diff/140001/tools/metrics/actions/...
> > > File tools/metrics/actions/extract_actions.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1025673004/diff/140001/tools/metrics/actions/...
> > > tools/metrics/actions/extract_actions.py:45: USER_METRICS_ACTION_RE_CPP =
> > > re.compile(r"""
> > > nit: This regex says CPP, but it includes Java code.  I don't have a
> > suggestion
> > > for a clearer name, though :/
> > 
> > How about just removing _CPP for that, and letting _JS be the special case?
> 
> Either this or Dan's suggestion is ok with me.

+1 cool with either

[Dan Beam, 2015-11-03 01:18:18]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2015-11-03 01:18:19]

lgtm with suggestions

https://codereview.chromium.org/1025673004/diff/160001/tools/metrics/actions/...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/1025673004/diff/160001/tools/metrics/actions/...
tools/metrics/actions/extract_actions.py:63: # Name of the C++ function.
not really a C++ function, but a handled webui message (</pedant>)

https://codereview.chromium.org/1025673004/diff/160001/tools/metrics/actions/...
tools/metrics/actions/extract_actions.py:64:
['"]coreOptionsUserMetricsAction['"]
btw, 'coreOptionsUserMetricsAction' should probably be sufficient (we discourage
" somewhere else in the presubmit, I think...)

https://codereview.chromium.org/1025673004/diff/160001/tools/metrics/actions/...
tools/metrics/actions/extract_actions.py:69: (.+?)                       # A
sequence of characters for the param.
can you add a test for

  chrome.send('coreOptionsUserMetricsAction', [objOrArray[prop]]);

https://codereview.chromium.org/1025673004/diff/160001/tools/metrics/actions/...
tools/metrics/actions/extract_actions.py:78: QUOTED_STRING_RE =
re.compile(r"""['"]([^'"]+)['"]$""")
nit: '[^']+'|"[^"]+" is mildly better, IMO, as it enforces 'thing' or "thing"
but doesn't allow 'bad" or "bad'

[michaelpg, 2015-11-10 18:09:39]

Patchset #6 (id:180001) has been deleted

[michaelpg, 2015-11-10 18:10:53]

Patchset #6 (id:200001) has been deleted

[michaelpg, 2015-11-10 18:16:28]

Patchset #7 (id:240001) has been deleted

[michaelpg, 2015-11-10 18:17:07]

dbeam, ptal, thanks!

https://codereview.chromium.org/1025673004/diff/160001/tools/metrics/actions/...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/1025673004/diff/160001/tools/metrics/actions/...
tools/metrics/actions/extract_actions.py:63: # Name of the C++ function.
On 2015/11/03 01:18:19, Dan Beam wrote:
> not really a C++ function, but a handled webui message (</pedant>)

Done.

https://codereview.chromium.org/1025673004/diff/160001/tools/metrics/actions/...
tools/metrics/actions/extract_actions.py:64:
['"]coreOptionsUserMetricsAction['"]
On 2015/11/03 01:18:19, Dan Beam wrote:
> btw, 'coreOptionsUserMetricsAction' should probably be sufficient (we
discourage
> " somewhere else in the presubmit, I think...)

Done.

https://codereview.chromium.org/1025673004/diff/160001/tools/metrics/actions/...
tools/metrics/actions/extract_actions.py:69: (.+?)                       # A
sequence of characters for the param.
On 2015/11/03 01:18:19, Dan Beam wrote:
> can you add a test for
> 
>   chrome.send('coreOptionsUserMetricsAction', [objOrArray[prop]]);

done for: property lookups, functions, mismatched quotation marks

https://codereview.chromium.org/1025673004/diff/160001/tools/metrics/actions/...
tools/metrics/actions/extract_actions.py:78: QUOTED_STRING_RE =
re.compile(r"""['"]([^'"]+)['"]$""")
On 2015/11/03 01:18:19, Dan Beam wrote:
> nit: '[^']+'|"[^"]+" is mildly better, IMO, as it enforces 'thing' or "thing"
> but doesn't allow 'bad" or "bad'

done (had to change the grouping, though)

[Dan Beam, 2015-11-10 18:26:15]

bitchin', lgtm

[michaelpg, 2015-11-10 19:46:12]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2015-11-10 19:46:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1025673004/#ps220001
(title: "dbeam + rebase")

[commit-bot: I haz the power, 2015-11-10 19:47:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1025673004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1025673004/220001

[commit-bot: I haz the power, 2015-11-10 20:05:52]

Committed patchset #6 (id:220001)

[commit-bot: I haz the power, 2015-11-10 20:06:50]

Patchset 6 (id:??) landed as
https://crrev.com/1b6c3e7b9c10309d3a9503ebd4f33ec5e0197b4e
Cr-Commit-Position: refs/heads/master@{#358888}