Adding tests for diagonal scrolling in telemetry.

Adding tests for diagonal scrolling in telemetry.

Adding tests for diagonal scrolling to telemetry.

BUG=466867
Committed: https://crrev.com/68c68147e5e374b24e5855dc9b24b2159dd3e8f7
Cr-Commit-Position: refs/heads/master@{#324346}

[cblume, 2015-03-12 07:04:17]

cblume@chromium.org changed reviewers:
+ nednguyen@google.com

[cblume, 2015-03-12 07:04:17]

This is a unit test for a change made in
https://codereview.chromium.org/945393002/

The test is separate so that we can create a binary that supports the new
feature. Once that binary lands it will have a release number, which the test
can check. It needs to check this to make sure it only tests the feature if it
is indeed supported. Otherwise, it returns early without asserting.

This CL will be updated with the correct release number once it is available.

[nednguyen, 2015-03-13 01:34:38]

nednguyen@google.com changed reviewers:
+ aiolos@chromium.org, eakuefner@chromium.org

[cblume, 2015-03-14 01:02:39]

Ned suggested I check the version of Chromium inside ScrollAction. However, I
couldn't find a great way to do that.

I am checking for support via browser_info.

[nednguyen, 2015-03-16 16:52:40]

I think we can land this patch without the new benchmark & the "CanRunForPage
usage".

https://codereview.chromium.org/999243003/diff/100001/tools/perf/page_sets/to...
File tools/perf/page_sets/tough_scrolling_while_zoomed_in_cases.py (right):

https://codereview.chromium.org/999243003/diff/100001/tools/perf/page_sets/to...
tools/perf/page_sets/tough_scrolling_while_zoomed_in_cases.py:23: return True
I think we will not support CanRunOnBrowser as the page level. Can you hold on
adding the new smoothness benchmark for a while?

https://codereview.chromium.org/999243003/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/actions/scroll.py (right):

https://codereview.chromium.org/999243003/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/actions/scroll.py:19: 'upleft', 'upright']:
Can you add a check of chrome branch number here and throw ValueException if the
diagonal actions are not supported?
You also want to add a TODO to remove the check once the chrome stable passes
that branch.

[aiolos (Not reviewing), 2015-03-16 21:30:03]

https://codereview.chromium.org/999243003/diff/100001/tools/perf/page_sets/to...
File tools/perf/page_sets/tough_scrolling_while_zoomed_in_cases.py (right):

https://codereview.chromium.org/999243003/diff/100001/tools/perf/page_sets/to...
tools/perf/page_sets/tough_scrolling_while_zoomed_in_cases.py:23: return True
On 2015/03/16 16:52:40, nednguyen wrote:
> I think we will not support CanRunOnBrowser as the page level. Can you hold on
> adding the new smoothness benchmark for a while? 

+1

[cblume, 2015-03-17 21:54:55]

cblume@chromium.org changed reviewers:
+ aelias@chromium.org, jdduke@chromium.org

[cblume, 2015-03-17 21:54:56]

>
https://codereview.chromium.org/999243003/diff/100001/tools/perf/page_sets/to...
> tools/perf/page_sets/tough_scrolling_while_zoomed_in_cases.py:23: return True
> I think we will not support CanRunOnBrowser as the page level. Can you hold on
> adding the new smoothness benchmark for a while?

I checked and it soudns like this can wait, yes. Is it waiting for a better
alternative to CanRunOnBrowser? What should I be listening for?


>
https://codereview.chromium.org/999243003/diff/100001/tools/telemetry/telemet...
> tools/telemetry/telemetry/page/actions/scroll.py:19: 'upleft', 'upright']:
> Can you add a check of chrome branch number here and throw ValueException if
the
> diagonal actions are not supported?
> You also want to add a TODO to remove the check once the chrome stable passes
> that branch.

Will do.

[nednguyen, 2015-03-17 22:05:12]

On 2015/03/17 21:54:56, cblume wrote:
> >
>
https://codereview.chromium.org/999243003/diff/100001/tools/perf/page_sets/to...
> > tools/perf/page_sets/tough_scrolling_while_zoomed_in_cases.py:23: return
True
> > I think we will not support CanRunOnBrowser as the page level. Can you hold
on
> > adding the new smoothness benchmark for a while?
> 
> I checked and it soudns like this can wait, yes. Is it waiting for a better
> alternative to CanRunOnBrowser? What should I be listening for?
> 
Yes. I filed a bug here:
https://code.google.com/p/chromium/issues/detail?id=468085#makechanges. When I
convert existing usage of CanRunOnBrowser to the new way, I will cc you. :-)
> 
> >
>
https://codereview.chromium.org/999243003/diff/100001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/page/actions/scroll.py:19: 'upleft', 'upright']:
> > Can you add a check of chrome branch number here and throw ValueException if
> the
> > diagonal actions are not supported?
> > You also want to add a TODO to remove the check once the chrome stable
passes
> > that branch.
> 
> Will do.

[cblume, 2015-03-26 00:15:06]

https://codereview.chromium.org/999243003/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/actions/scroll.py (right):

https://codereview.chromium.org/999243003/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/actions/scroll.py:19: 'upleft', 'upright']:
On 2015/03/16 16:52:40, nednguyen wrote:
> Can you add a check of chrome branch number here and throw ValueException if
the
> diagonal actions are not supported?
> You also want to add a TODO to remove the check once the chrome stable passes
> that branch.

Ideally, right here in the constructor is where the ValueError would be thrown.
However, from the constructor I don't seem to have a good way to check the
Chrome branch number.

I can check from within WillRunAction or RunAction, since those are provided a
tab object. What do you think?

[nednguyen, 2015-03-26 00:51:54]

On 2015/03/26 00:15:06, cblume wrote:
>
https://codereview.chromium.org/999243003/diff/100001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/page/actions/scroll.py (right):
> 
>
https://codereview.chromium.org/999243003/diff/100001/tools/telemetry/telemet...
> tools/telemetry/telemetry/page/actions/scroll.py:19: 'upleft', 'upright']:
> On 2015/03/16 16:52:40, nednguyen wrote:
> > Can you add a check of chrome branch number here and throw ValueException if
> the
> > diagonal actions are not supported?
> > You also want to add a TODO to remove the check once the chrome stable
passes
> > that branch.
> 
> Ideally, right here in the constructor is where the ValueError would be
thrown.
> However, from the constructor I don't seem to have a good way to check the
> Chrome branch number.
> 
> I can check from within WillRunAction or RunAction, since those are provided a
> tab object. What do you think?

Ah there, WillRunAction is great. I forgot that the constructor doesn't really
have anything in it, sorry.

[cblume, 2015-04-06 19:22:47]

I just rebased since some files have moved.

Are we okay to land this? Are there more changes we want to make?

I have walked away from this for a little while and lost context.

It looks like we still need to make some changes to
https://codereview.chromium.org/1033733010/ but that is separate so this CL may
be ready to go.

[nednguyen, 2015-04-06 21:31:24]

nednguyen@google.com changed reviewers:
+ cylee@google.com

[nednguyen, 2015-04-06 21:31:25]

lgtm

Cheng can help you with running the new benchmarks on cros bots. Please make
sure that this doesn't cause break on cros machines before you land.

https://codereview.chromium.org/999243003/diff/160001/tools/perf/benchmarks/s...
File tools/perf/benchmarks/smoothness.py (right):

https://codereview.chromium.org/999243003/diff/160001/tools/perf/benchmarks/s...
tools/perf/benchmarks/smoothness.py:219: @benchmark.Enabled('android',
'chromeos')
Maybe just enable this on chromeos in this patch. Then after making sure things
work fine on chromeos, you can make another patch to also enable it on android.

https://codereview.chromium.org/999243003/diff/160001/tools/perf/page_sets/to...
File tools/perf/page_sets/tough_scrolling_while_zoomed_in_cases.py (right):

https://codereview.chromium.org/999243003/diff/160001/tools/perf/page_sets/to...
tools/perf/page_sets/tough_scrolling_while_zoomed_in_cases.py:27: # 20.0 was
chosen because at the time it was close to the maximum.
Nits: can you indent these lines to the left?

https://codereview.chromium.org/999243003/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/actions/scroll.js (right):

https://codereview.chromium.org/999243003/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/actions/scroll.js:51:
ScrollAction.prototype.getScrollDistanceDown_ = function() {
+1 for this refactoring.

https://codereview.chromium.org/999243003/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/actions/scroll.py (right):

https://codereview.chromium.org/999243003/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/actions/scroll.py:40: if self._direction in
['downleft', 'downright', 'upleft', 'upright']:
Nits: make this a tuple instead of list

[cblume, 2015-04-06 21:58:44]

You make great nits. Please feel free to nit away. :)

https://codereview.chromium.org/999243003/diff/180001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/actions/scroll.py (right):

https://codereview.chromium.org/999243003/diff/180001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/actions/scroll.py:17: if direction not in
('down', 'up', 'left', 'right',
I also made this location a tuple.

[cblume, 2015-04-06 22:27:57]

In this CL our tests only run against ChromeOS.
We will later add Android, which can be seen in this CL:
https://codereview.chromium.org/1061133002

[cylee1, 2015-04-07 19:00:08]

On 2015/04/06 22:27:57, cblume wrote:
> In this CL our tests only run against ChromeOS.
> We will later add Android, which can be seen in this CL:
> https://codereview.chromium.org/1061133002

There're still some issues on running remote CrOS benchmark test with a patch.
I'm trying to work it out.
In the mean time, I've manually patched this CL and it seems to work on CrOS
fine.
cblume,  Did you every try it on CrOS?

[cblume, 2015-04-07 19:06:55]

On 2015/04/07 19:00:08, cylee1 wrote:
> On 2015/04/06 22:27:57, cblume wrote:
> > In this CL our tests only run against ChromeOS.
> > We will later add Android, which can be seen in this CL:
> > https://codereview.chromium.org/1061133002
> 
> There're still some issues on running remote CrOS benchmark test with a patch.
> I'm trying to work it out.
> In the mean time, I've manually patched this CL and it seems to work on CrOS
> fine.
> cblume,  Did you every try it on CrOS?

No, I did not. I have been testing on Android. I am new and do not yet know how
to test on CrOS.

[chromium-reviews, 2015-04-07 19:12:10]

Actually I'm also quite new to telemetry on CrOS :)
If you need to run it on CrOS,
https://www.chromium.org/developers/telemetry/run_locally is a good
document.
If you just need to verify the patch works on CrOS, I think it passed.


On Wed, Apr 8, 2015 at 3:06 AM, <cblume@chromium.org> wrote:

> On 2015/04/07 19:00:08, cylee1 wrote:
>
>> On 2015/04/06 22:27:57, cblume wrote:
>> > In this CL our tests only run against ChromeOS.
>> > We will later add Android, which can be seen in this CL:
>> > https://codereview.chromium.org/1061133002
>>
>
>  There're still some issues on running remote CrOS benchmark test with a
>> patch.
>> I'm trying to work it out.
>> In the mean time, I've manually patched this CL and it seems to work on
>> CrOS
>> fine.
>> cblume,  Did you every try it on CrOS?
>>
>
> No, I did not. I have been testing on Android. I am new and do not yet
> know how
> to test on CrOS.
>
> https://codereview.chromium.org/999243003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[cblume, 2015-04-08 23:39:33]

The CQ bit was checked by cblume@chromium.org

[cblume, 2015-04-08 23:39:33]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/999243003/#ps180001
(title: "Changing python lists to tuple, changing comment indentation, and
changing a test to only run on ch…")

[commit-bot: I haz the power, 2015-04-08 23:40:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/999243003/180001

[commit-bot: I haz the power, 2015-04-09 02:51:04]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-04-09 02:51:51]

Patchset 10 (id:??) landed as
https://crrev.com/68c68147e5e374b24e5855dc9b24b2159dd3e8f7
Cr-Commit-Position: refs/heads/master@{#324346}