Add an option to not automatically record interaction for gesture actions.

Add an option to not automatically record interaction for gesture actions.

BUG=361809
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269581

[chrishenry, 2014-05-08 16:41:16]

Splitting of the other patch for expediency (this is blocking some of our
project's pageset creation effort).

[nednguyen, 2014-05-08 16:51:41]

lgtm

[nduca, 2014-05-08 16:55:19]

https://codereview.chromium.org/278593004/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/actions/gesture_action.py (right):

https://codereview.chromium.org/278593004/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/actions/gesture_action.py:14:
self.automatically_record_interaction = True
can this also read an attribute as well if hasattr() for completeness?

GestureAction({automatically_record_interaction=False})

lgtm

[chrishenry, 2014-05-08 17:05:06]

Ok, once I got an lgtm, what do I do next? I think Nat would have to commit this
patch? Do I need to do anything else?

https://codereview.chromium.org/278593004/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/actions/gesture_action.py (right):

https://codereview.chromium.org/278593004/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/actions/gesture_action.py:14:
self.automatically_record_interaction = True
On 2014/05/08 16:55:19, nduca wrote:
> can this also read an attribute as well if hasattr() for completeness?
> 
> GestureAction({automatically_record_interaction=False})
> 
> lgtm

I'm still a bit fuzzy as to why we use this attributes style thing (is this a
common Python idioms?). Anyway, I think this is what you mean, but lmk if it
isn't.

[nednguyen, 2014-05-08 17:12:19]

On 2014/05/08 17:05:06, chrishenry wrote:
> Ok, once I got an lgtm, what do I do next? I think Nat would have to commit
this
> patch? Do I need to do anything else?
> 
>
https://codereview.chromium.org/278593004/diff/1/tools/telemetry/telemetry/pa...
> File tools/telemetry/telemetry/page/actions/gesture_action.py (right):
> 
>
https://codereview.chromium.org/278593004/diff/1/tools/telemetry/telemetry/pa...
> tools/telemetry/telemetry/page/actions/gesture_action.py:14:
> self.automatically_record_interaction = True
> On 2014/05/08 16:55:19, nduca wrote:
> > can this also read an attribute as well if hasattr() for completeness?
> > 
> > GestureAction({automatically_record_interaction=False})
> > 
> > lgtm
> 
> I'm still a bit fuzzy as to why we use this attributes style thing (is this a
> common Python idioms?). Anyway, I think this is what you mean, but lmk if it
> isn't.

It's a legacy style caused by json page set. We will remove all of these dict
based set methods after all page sets use action_runner API.
Nat, I guess that you just want the style to be consistent for now?

[chrishenry, 2014-05-09 00:31:41]

Thanks for the context Ned. We've already moved all json pageset away to Python
right? So eventually we can move away from these attributes style parameter
passing and just use optional parameters instead? This attr stuff can be buggy,
since a lot of checks involve hasattr + another check on the actual value and
can easily be gotten wrong. See this for e.g.:

https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

The call above should also check for self.scroll_require_touch but doesn't
currently.

[nednguyen, 2014-05-09 03:01:58]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2014-05-09 03:05:49]

On 2014/05/09 00:31:41, chrishenry wrote:
> Thanks for the context Ned. We've already moved all json pageset away to
Python
> right? So eventually we can move away from these attributes style parameter
> passing and just use optional parameters instead? This attr stuff can be
buggy,
> since a lot of checks involve hasattr + another check on the actual value and
> can easily be gotten wrong. 

Yes, we definitely should remove these attribute style parameter. They are buggy
and make the code hard to understand, hard to add documentation.

> See this for e.g.:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> 
> The call above should also check for self.scroll_require_touch but doesn't
> currently.

Nice catch!

[commit-bot: I haz the power, 2014-05-09 03:07:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishenry@google.com/278593004/20001

[commit-bot: I haz the power, 2014-05-09 04:33:21]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-09 04:55:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-09 04:55:05]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[nednguyen, 2014-05-09 13:10:17]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-05-09 13:13:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishenry@google.com/278593004/20001

[commit-bot: I haz the power, 2014-05-09 13:58:56]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-09 14:26:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-09 14:26:19]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[nednguyen, 2014-05-09 16:32:27]

On 2014/05/09 14:26:19, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
>   mac_chromium_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

Can you take a look at buildbot failures? They look legit to me.

[chrishenry, 2014-05-09 17:29:43]

On 2014/05/09 16:32:27, nednguyen wrote:
> On 2014/05/09 14:26:19, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_rel on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
> >   mac_chromium_rel on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
> 
> Can you take a look at buildbot failures? They look legit to me.

Fixed on Patch 3. Re-run tools/telemetry/run_tests locally, they passed. Can you
retry this again? Thanks!

[chrishenry, 2014-05-09 20:10:24]

The CQ bit was checked by chrishenry@google.com

[commit-bot: I haz the power, 2014-05-09 20:13:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishenry@google.com/278593004/40001

[commit-bot: I haz the power, 2014-05-10 18:03:06]

Change committed as 269581