Adding support for diagonal scrolling to telemetry.

Adding support for diagonal scrolling to telemetry.

BUG=466867
Committed: https://crrev.com/8eb56468216ac1f10ebc599f89621dbaaf605313
Cr-Commit-Position: refs/heads/master@{#320419}

[cblume, 2015-02-25 23:28:01]

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

[cblume, 2015-02-26 00:06:16]

The goal of this CL is to add a new test to telemetry. This new test is meant to
allow better measurement of checkerboarding. (Is it getting better or worse over
time?) This is accomplished by pinchzooming in and then scrolling diagonally,
which stresses the rasterization.

Some changes had to be made to the existing scroll test structure in order to
add support for diagonal scrolling.

[jdduke (slow), 2015-02-26 01:05:35]

Are there unit tests we can add for any of these additions? I know there are
some telemetry unit tests, but to be honest I haven't done enough work in this
part of the code to know for sure.

https://codereview.chromium.org/945393002/diff/20001/content/renderer/gpu/gpu...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/945393002/diff/20001/content/renderer/gpu/gpu...
content/renderer/gpu/gpu_benchmarking_extension.cc:373: else if (direction ==
"upright") {
Nit: else should start on the same line as the preceding brace ("git cl format"
should take care of that).

I suppose you could also do something like:

if (direction.find("up") != string::npos)
  direction.set_y(-distance_length);
else if (direction.find("down") != string::npos)
  direction.set_y(distance_length);

and similarly for right/left, then return false if distance is null. But maybe
it's better to be explicit here.

https://codereview.chromium.org/945393002/diff/20001/tools/perf/page_sets/tou...
File tools/perf/page_sets/tough_pinch_zoom_diagonal_scrolling_cases.py (right):

https://codereview.chromium.org/945393002/diff/20001/tools/perf/page_sets/tou...
tools/perf/page_sets/tough_pinch_zoom_diagonal_scrolling_cases.py:49:
'file://tough_scrolling_cases/background_fixed.html',
I wonder if we want all of these? Not sure, would have to look at the content.

[cblume, 2015-02-26 20:21:02]

On 2015/02/26 01:05:35, jdduke wrote:
> Are there unit tests we can add for any of these additions? I know there are
> some telemetry unit tests, but to be honest I haven't done enough work in this
> part of the code to know for sure.
> 
>
https://codereview.chromium.org/945393002/diff/20001/content/renderer/gpu/gpu...
> File content/renderer/gpu/gpu_benchmarking_extension.cc (right):
> 
>
https://codereview.chromium.org/945393002/diff/20001/content/renderer/gpu/gpu...
> content/renderer/gpu/gpu_benchmarking_extension.cc:373: else if (direction ==
> "upright") {
> Nit: else should start on the same line as the preceding brace ("git cl
format"
> should take care of that).
> 
> I suppose you could also do something like:
> 
> if (direction.find("up") != string::npos)
>   direction.set_y(-distance_length);
> else if (direction.find("down") != string::npos)
>   direction.set_y(distance_length);
> 
> and similarly for right/left, then return false if distance is null. But maybe
> it's better to be explicit here.
> 
>
https://codereview.chromium.org/945393002/diff/20001/tools/perf/page_sets/tou...
> File tools/perf/page_sets/tough_pinch_zoom_diagonal_scrolling_cases.py
(right):
> 
>
https://codereview.chromium.org/945393002/diff/20001/tools/perf/page_sets/tou...
> tools/perf/page_sets/tough_pinch_zoom_diagonal_scrolling_cases.py:49:
> 'file://tough_scrolling_cases/background_fixed.html',
> I wonder if we want all of these? Not sure, would have to look at the content.

There are some unit tests inside the telemetry area.

The file I added (the new telemetry test) sits alongside other telemetry tests
with no unit tests. We can change these to have unit tests if we like.

Of the files I modified:
scroll.py / .js has two tests. One of them is disabled due to flakiness. The
other tests viewport size which is important for scrolling, but not scrolling
itself. The coverage looks pretty low. There is room for improvement here.
smoothness.py and the other files in /benchmarks have no unit tests (unless the
tests live elsewhere?).
gpy_benchmark_extension.cc and the other files in /gpu seem to have no unit
tests (unless the tests live elsewere?).

I would be happy to add tests to all these.

[cblume, 2015-02-26 20:22:00]

https://codereview.chromium.org/945393002/diff/20001/content/renderer/gpu/gpu...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/945393002/diff/20001/content/renderer/gpu/gpu...
content/renderer/gpu/gpu_benchmarking_extension.cc:373: else if (direction ==
"upright") {
On 2015/02/26 01:05:35, jdduke wrote:
> Nit: else should start on the same line as the preceding brace ("git cl
format"
> should take care of that).
> 
> I suppose you could also do something like:
> 
> if (direction.find("up") != string::npos)
>   direction.set_y(-distance_length);
> else if (direction.find("down") != string::npos)
>   direction.set_y(distance_length);
> 
> and similarly for right/left, then return false if distance is null. But maybe
> it's better to be explicit here. 

Oh, thank you. I've fixed this locally and will upload it along with the change
to the tests once we decide how best to handle them.

I went with the non-.find() option but am open to changing that. My thought was
if someone put "up down" it would mimic "down". But since "up down" doesn't make
sense anyway, maybe I shouldn't care about these odd cases.

https://codereview.chromium.org/945393002/diff/20001/tools/perf/page_sets/tou...
File tools/perf/page_sets/tough_pinch_zoom_diagonal_scrolling_cases.py (right):

https://codereview.chromium.org/945393002/diff/20001/tools/perf/page_sets/tou...
tools/perf/page_sets/tough_pinch_zoom_diagonal_scrolling_cases.py:49:
'file://tough_scrolling_cases/background_fixed.html',
On 2015/02/26 01:05:35, jdduke wrote:
> I wonder if we want all of these? Not sure, would have to look at the content.

We might not want all of these. I copied the list from the
tough_scrolling_cases, figuring both were stressing the scrolling. It seemed
like a fair enough place to start and perhaps some tests which looked
unimportant were actually beneficial in a way I didn't realize.

I've run the test several times and the general ordering of
mean_pixels_approximated for the page sets seem consistent. What I mean by that
is background_fixed.html seems to consistently be the worst offender.
cust_scrollbar.html and fixed_stacking.html both seem to perform well.

We can set the scrolling speeds just right to stress rasterization. If something
has 100% mean_pixels_approximated then we won't know if things have gotten
worse. Likewise, if it is 0% we won't know if things have gotten better. So I
was aiming to get the tests ~50%.

I was testing on a Nexus 5. background_fixed.html (the worst offender) is the
only one that is near 50% at this scroll speed. The next most visible is
iframe_scrolls.html at ~25%, then fixed_nonstacking.html at ~18%. As you can
see, it diverts away from 50% pretty quickly.

Perhaps I should alter the scroll speed to maximize the quantity near 50% and
then remove the other tests which aren't close?

[jdduke (slow), 2015-02-26 20:34:12]

If we don't already have unit tests for comparable interactions, I don't think
you should be burdened with adding them in this change.

https://codereview.chromium.org/945393002/diff/20001/tools/perf/page_sets/tou...
File tools/perf/page_sets/tough_pinch_zoom_diagonal_scrolling_cases.py (right):

https://codereview.chromium.org/945393002/diff/20001/tools/perf/page_sets/tou...
tools/perf/page_sets/tough_pinch_zoom_diagonal_scrolling_cases.py:8: class
ToughPinchZoomDiagonalScrollingCasesPage(page_module.Page):
I wonder if we want to tweak the name here. It almost sounds like we're testing
pinch zoom while diagonal scrolling, or both pinch zoom then diagonal scrolling,
but it's really just testing diagonal scrolling after we're already zoomed in.

Maybe ToughZoomedInDiagonalScrollingCases or
TouchPinchZoomedDiagonalScrollingCases?

https://codereview.chromium.org/945393002/diff/20001/tools/perf/page_sets/tou...
tools/perf/page_sets/tough_pinch_zoom_diagonal_scrolling_cases.py:49:
'file://tough_scrolling_cases/background_fixed.html',
On 2015/02/26 20:22:00, cblume1 wrote:
> On 2015/02/26 01:05:35, jdduke wrote:
> > I wonder if we want all of these? Not sure, would have to look at the
content.
> 
> We might not want all of these. I copied the list from the
> tough_scrolling_cases, figuring both were stressing the scrolling. It seemed
> like a fair enough place to start and perhaps some tests which looked
> unimportant were actually beneficial in a way I didn't realize.
> 
> I've run the test several times and the general ordering of
> mean_pixels_approximated for the page sets seem consistent. What I mean by
that
> is background_fixed.html seems to consistently be the worst offender.
> cust_scrollbar.html and fixed_stacking.html both seem to perform well.
> 
> We can set the scrolling speeds just right to stress rasterization. If
something
> has 100% mean_pixels_approximated then we won't know if things have gotten
> worse. Likewise, if it is 0% we won't know if things have gotten better. So I
> was aiming to get the tests ~50%.
> 
> I was testing on a Nexus 5. background_fixed.html (the worst offender) is the
> only one that is near 50% at this scroll speed. The next most visible is
> iframe_scrolls.html at ~25%, then fixed_nonstacking.html at ~18%. As you can
> see, it diverts away from 50% pretty quickly.
> 
> Perhaps I should alter the scroll speed to maximize the quantity near 50% and
> then remove the other tests which aren't close?

I think the speed you have is fine, let's pick the worst 4 or so and add a
comment before the urls_list saying why, e.g.,

# Why: Exhibit >20% checkerboarding at high velocities

or something like that.

[cblume, 2015-02-26 21:33:08]

https://codereview.chromium.org/945393002/diff/20001/tools/perf/page_sets/tou...
File tools/perf/page_sets/tough_pinch_zoom_diagonal_scrolling_cases.py (right):

https://codereview.chromium.org/945393002/diff/20001/tools/perf/page_sets/tou...
tools/perf/page_sets/tough_pinch_zoom_diagonal_scrolling_cases.py:8: class
ToughPinchZoomDiagonalScrollingCasesPage(page_module.Page):
On 2015/02/26 20:34:12, jdduke wrote:
> I wonder if we want to tweak the name here. It almost sounds like we're
testing
> pinch zoom while diagonal scrolling, or both pinch zoom then diagonal
scrolling,
> but it's really just testing diagonal scrolling after we're already zoomed in.
> 
> Maybe ToughZoomedInDiagonalScrollingCases or
> TouchPinchZoomedDiagonalScrollingCases?

I agree with you. I'm not a fan of this name and struggled to find something
that clearly conveyed intention. Maybe ToughScrollingCasesWhileZoomedIn? But
many of these end in "Cases" so that would break the mold. However, I don't
think that mold holds much weight.

https://codereview.chromium.org/945393002/diff/20001/tools/perf/page_sets/tou...
tools/perf/page_sets/tough_pinch_zoom_diagonal_scrolling_cases.py:49:
'file://tough_scrolling_cases/background_fixed.html',
On 2015/02/26 20:34:12, jdduke wrote:
> On 2015/02/26 20:22:00, cblume1 wrote:
> > On 2015/02/26 01:05:35, jdduke wrote:
> > > I wonder if we want all of these? Not sure, would have to look at the
> content.
> > 
> > We might not want all of these. I copied the list from the
> > tough_scrolling_cases, figuring both were stressing the scrolling. It seemed
> > like a fair enough place to start and perhaps some tests which looked
> > unimportant were actually beneficial in a way I didn't realize.
> > 
> > I've run the test several times and the general ordering of
> > mean_pixels_approximated for the page sets seem consistent. What I mean by
> that
> > is background_fixed.html seems to consistently be the worst offender.
> > cust_scrollbar.html and fixed_stacking.html both seem to perform well.
> > 
> > We can set the scrolling speeds just right to stress rasterization. If
> something
> > has 100% mean_pixels_approximated then we won't know if things have gotten
> > worse. Likewise, if it is 0% we won't know if things have gotten better. So
I
> > was aiming to get the tests ~50%.
> > 
> > I was testing on a Nexus 5. background_fixed.html (the worst offender) is
the
> > only one that is near 50% at this scroll speed. The next most visible is
> > iframe_scrolls.html at ~25%, then fixed_nonstacking.html at ~18%. As you can
> > see, it diverts away from 50% pretty quickly.
> > 
> > Perhaps I should alter the scroll speed to maximize the quantity near 50%
and
> > then remove the other tests which aren't close?
> 
> I think the speed you have is fine, let's pick the worst 4 or so and add a
> comment before the urls_list saying why, e.g.,
> 
> # Why: Exhibit >20% checkerboarding at high velocities
> 
> or something like that.

Good idea. I'll do that.

[jdduke (slow), 2015-02-26 21:42:52]

https://codereview.chromium.org/945393002/diff/20001/tools/perf/page_sets/tou...
File tools/perf/page_sets/tough_pinch_zoom_diagonal_scrolling_cases.py (right):

https://codereview.chromium.org/945393002/diff/20001/tools/perf/page_sets/tou...
tools/perf/page_sets/tough_pinch_zoom_diagonal_scrolling_cases.py:8: class
ToughPinchZoomDiagonalScrollingCasesPage(page_module.Page):
On 2015/02/26 21:33:07, cblume1 wrote:
> On 2015/02/26 20:34:12, jdduke wrote:
> > I wonder if we want to tweak the name here. It almost sounds like we're
> testing
> > pinch zoom while diagonal scrolling, or both pinch zoom then diagonal
> scrolling,
> > but it's really just testing diagonal scrolling after we're already zoomed
in.
> > 
> > Maybe ToughZoomedInDiagonalScrollingCases or
> > TouchPinchZoomedDiagonalScrollingCases?
> 
> I agree with you. I'm not a fan of this name and struggled to find something
> that clearly conveyed intention. Maybe ToughScrollingCasesWhileZoomedIn? But
> many of these end in "Cases" so that would break the mold. However, I don't
> think that mold holds much weight.

ToughScrollingWhileZoomedInCases or TouchZoomedInScrollingCases both seem fine.

[cblume, 2015-02-27 18:49:06]

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

[cblume, 2015-02-27 18:52:42]

On 2015/02/27 00:04:43, cblume wrote:

sievers@, I am adding you as a reviewer to approve my changes in
content/renderer/gpu/gpu_benchmarking_extension.cc

nednguyen@google.com, I am adding you as a reviewer because you made many
changes in tools/perf/benchmarks/smoothness.py and are also an owner of
tools/telemetry.

Thanks!

[nednguyen, 2015-02-27 18:57:27]

On 2015/02/27 18:52:52, cblume wrote:

Can you add simple functional test for that new scroll?

[no sievers, 2015-02-27 19:36:18]

On 2015/02/27 18:52:42, cblume wrote:
> On 2015/02/27 00:04:43, cblume wrote:
> 
> sievers@, I am adding you as a reviewer to approve my changes in
> content/renderer/gpu/gpu_benchmarking_extension.cc
> 

lgtm

[cblume, 2015-03-04 05:36:20]

I have reenabled the existing scroll test which was once flaky but no longer
seems to be.

In addition, I have added a test for diagonal scrolling. There was already a
test for pinch zooming.

[nednguyen, 2015-03-04 05:40:49]

https://codereview.chromium.org/945393002/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/actions/scroll_unittest.py (left):

https://codereview.chromium.org/945393002/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/actions/scroll_unittest.py:13:
@decorators.Disabled  # Disabled due to flakiness: crbug.com/330544
Can you make a separate patch for enabling this? The less moving pieces in a
patch, the easier it's to keep track & revert if things go wrong.

[cblume, 2015-03-06 18:12:59]

As suggested, I moved reenabling the old test to a separate patch:
https://codereview.chromium.org/976063003/

I then noticed something off about that old test and updated it. I reflected
that update here.

The test was checking if it had reached the bottom of the page within 1px. But
the test does not specify the scroll speed (uses default) or the test/scroll
duration (again, uses default). So there is no expectation to reach the bottom
of the page. On Android in landscape the tests would pass but in portrait they
would fail because they were 2px from the bottom. This is strange and possibly
worth investigating.

The tests had already asserted that the viewport starts at the top of the page
before scrolling. I changed the test to now assert it is no longer at the top of
the page after scrolling. So it no longer assumes how far it can scroll in a
given time.

[nednguyen, 2015-03-06 19:07:51]

Thanks for the fixing the old test cblume!

https://codereview.chromium.org/945393002/diff/260001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/actions/scroll_unittest.py (right):

https://codereview.chromium.org/945393002/diff/260001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/actions/scroll_unittest.py:73:
self.assertTrue(self._tab.EvaluateJavaScript('window.__didEndMeasuring'))
Why do we need this?

[cblume, 2015-03-09 22:47:53]

I've removed the asserts for test began and test ended. If we scrolled at all,
the test has completed as far as we are concerned.

[nednguyen, 2015-03-09 22:55:07]

lgtm

[nednguyen, 2015-03-09 22:59:59]

sorry, not lgtm yet.

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

https://codereview.chromium.org/945393002/diff/280001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/actions/scroll.py:19: 'upleft', 'upright']:
Actually, since this goes with gpu_benchmarking_extension.cc change, you may
want to have a way to check whether these 4 new modes is supported of chrome.

If that's not easy, I am fine with adding comment saying that these new 4 modes
only work on Chrome after revision xyz.

You also want to update the documentation of Scroll.. methods in
action_runner.py

[cblume, 2015-03-09 23:00:01]

The CQ bit was checked by cblume@chromium.org

[cblume, 2015-03-09 23:00:02]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/945393002/#ps280001
(title: "More changes from code review.")

[cblume, 2015-03-09 23:00:10]

The CQ bit was unchecked by cblume@chromium.org

[cblume, 2015-03-12 07:02:13]

I have moved the unit test to https://codereview.chromium.org/999243003
This allows us to update the binary to add support for diagonal scrolling as a
first step. Then, once there is a build number we can check, the test will look
for that build number to make sure it is testing something which is supported.

[nednguyen, 2015-03-12 15:52:39]

On 2015/03/12 07:02:13, cblume wrote:
> I have moved the unit test to https://codereview.chromium.org/999243003
> This allows us to update the binary to add support for diagonal scrolling as a
> first step. Then, once there is a build number we can check, the test will
look
> for that build number to make sure it is testing something which is supported.

Not only the test but also telemetry related code need to be modified so that it
throws s.t like ValueError('Diagonal screen is not supported for chrome branch <
...') when that happen, right?

[nednguyen, 2015-03-12 20:57:01]

lgtm (to remove my red comment) -- defer reviewing that file to others

[jdduke (slow), 2015-03-12 21:20:55]

On 2015/03/12 20:57:01, nednguyen wrote:
> lgtm (to remove my red comment) -- defer reviewing that file to others

Do we have a bug for this? If not, you get to file your first one :)
https://code.google.com/p/chromium/issues/entry

[cblume, 2015-03-12 21:32:18]

The CQ bit was checked by cblume@chromium.org

[cblume, 2015-03-12 21:32:18]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/945393002/#ps320001
(title: "Moving more of the non-Chromium-binary into the other CL.")

[commit-bot: I haz the power, 2015-03-12 21:32:54]

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

[jdduke (slow), 2015-03-12 21:58:04]

On 2015/03/12 21:32:54, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/945393002/320001

Hmm, it's nice to associate the bug with the patch before it lands, but maybe
next time.

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

The CQ bit was unchecked by nednguyen@google.com

[cblume, 2015-03-13 01:11:48]

The CQ bit was checked by cblume@chromium.org

[commit-bot: I haz the power, 2015-03-13 01:12:22]

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

[commit-bot: I haz the power, 2015-03-13 01:14:22]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-03-13 01:15:22]

Patchset 17 (id:??) landed as
https://crrev.com/8eb56468216ac1f10ebc599f89621dbaaf605313
Cr-Commit-Position: refs/heads/master@{#320419}