Ignore the rest of the scroll sequence if GSB is ignored.

Ignore the rest of the scroll sequence if GSB is ignored.

This cl also modifies ContentViewCoreScrollingTests to initialize the
compositor's outer viewport before starting the test. This change is
required, as without properly initializing the compositor's outer
viewport, GSB events and the rest of the scroll sequences will get
ignored.

BUG=730197
TEST=AnimateInput/InputHandlerProxyTest.GestureScrollIgnored/*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2924953002
Cr-Commit-Position: refs/heads/master@{#479871}
Committed: https://chromium.googlesource.com/chromium/src/+/dad0c1edb6da7ce815e5019c87cd1f474b3d055c

[sahel, 2017-06-06 21:46:37]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 21:47:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sahel, 2017-06-06 22:00:35]

Description was changed from

==========
Ignore the rest of the scroll sequence if GSB is ignored.

BUG=730197
==========

to

==========
Ignore the rest of the scroll sequence if GSB is ignored.

BUG=730197
TEST=AnimateInput/InputHandlerProxyTest.GestureScrollIgnored/*
==========

[sahel, 2017-06-06 22:02:34]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 22:03:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 23:04:12]

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

[commit-bot: I haz the power, 2017-06-06 23:04:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sahel, 2017-06-13 18:32:27]

Description was changed from

==========
Ignore the rest of the scroll sequence if GSB is ignored.

BUG=730197
TEST=AnimateInput/InputHandlerProxyTest.GestureScrollIgnored/*
==========

to

==========
Ignore the rest of the scroll sequence if GSB is ignored.

BUG=730197
TEST=AnimateInput/InputHandlerProxyTest.GestureScrollIgnored/*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sahel, 2017-06-13 18:34:05]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 18:34:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sahel, 2017-06-13 18:40:03]

Description was changed from

==========
Ignore the rest of the scroll sequence if GSB is ignored.

BUG=730197
TEST=AnimateInput/InputHandlerProxyTest.GestureScrollIgnored/*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Ignore the rest of the scroll sequence if GSB is ignored.

This changed caused some of the ContentViewCoreScrollingTests to fail.
This is because scrolling in these tests used to happen before
initialization of the outer viewport on the compositor side. This cl also
includes the fix for failing tests.

BUG=730197
TEST=AnimateInput/InputHandlerProxyTest.GestureScrollIgnored/*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sahel, 2017-06-13 19:54:31]

sahel@chromium.org changed reviewers:
+ tdresser@chromium.org

[commit-bot: I haz the power, 2017-06-13 20:00:06]

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

[commit-bot: I haz the power, 2017-06-13 20:00:07]

Dry run: This issue passed the CQ dry run.

[tdresser, 2017-06-14 17:59:24]

In an ideal world, we'd land the viewport fix before landing this, but they're
pretty minor changes, so I think this is okay.

I think we can be a bit clearer in the CL description about what's happening
though. 
"This changed caused some of the ContentViewCoreScrollingTests to fail" could
imply that this change had been landed previously, and was reverted. Maybe just
state something more like: "Also modifies ContentViewCoreScrollingTests to
initialize the compositor's outer viewport before starting the test. This is
required by this change, as without properly initializing the compositor's out
viewport ..."

LGTM % nits



This changed caused some of the ContentViewCoreScrollingTests to fail.
This is because scrolling in these tests used to happen before
initialization of the outer viewport on the compositor side. This cl also
includes the fix for failing tests.

https://codereview.chromium.org/2924953002/diff/40001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java
(right):

https://codereview.chromium.org/2924953002/diff/40001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java:109:
private void assertWaitForViewportInitialization() {
Why does this include "assert" in the name? Is it an assertion?

[sahel, 2017-06-14 18:31:05]

Description was changed from

==========
Ignore the rest of the scroll sequence if GSB is ignored.

This changed caused some of the ContentViewCoreScrollingTests to fail.
This is because scrolling in these tests used to happen before
initialization of the outer viewport on the compositor side. This cl also
includes the fix for failing tests.

BUG=730197
TEST=AnimateInput/InputHandlerProxyTest.GestureScrollIgnored/*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Ignore the rest of the scroll sequence if GSB is ignored.

This cl also modifies ContentViewCoreScrollingTests to initialize the
compositor's outer viewport before starting the test. This change is
required, as without properly initializing the compositor's outer
viewport, GSB events and the rest of the scroll sequences will get
ignored.

BUG=730197
TEST=AnimateInput/InputHandlerProxyTest.GestureScrollIgnored/*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sahel, 2017-06-14 18:31:38]

https://codereview.chromium.org/2924953002/diff/40001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java
(right):

https://codereview.chromium.org/2924953002/diff/40001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java:90:
private void assertWaitForScroll(final boolean hugLeft, final boolean hugTop) {
The function name has assert without actual assertion in the body of the
function.

https://codereview.chromium.org/2924953002/diff/40001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java:109:
private void assertWaitForViewportInitialization() {
On 2017/06/14 17:59:24, tdresser wrote:
> Why does this include "assert" in the name? Is it an assertion?

I just followed naming pattern in this file. assertWaitForScroll doesn't have
any assertions in its body, either.

Do you think I should still change the name?

[tdresser, 2017-06-14 18:44:03]

https://codereview.chromium.org/2924953002/diff/40001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java
(right):

https://codereview.chromium.org/2924953002/diff/40001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java:109:
private void assertWaitForViewportInitialization() {
On 2017/06/14 18:31:37, sahel wrote:
> On 2017/06/14 17:59:24, tdresser wrote:
> > Why does this include "assert" in the name? Is it an assertion?
> 
> I just followed naming pattern in this file. assertWaitForScroll doesn't have
> any assertions in its body, either.
> 
> Do you think I should still change the name?

I looked up when assertWaitForScroll was first added
(https://chromiumcodereview.appspot.com/12621006/diff/20002/content/public/and...),
and it used to actually assert.

Now that they don't assert, can you rename both of them?

[sahel, 2017-06-14 19:25:06]

Patchset #4 (id:60001) has been deleted

[sahel, 2017-06-14 19:35:48]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 19:36:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sahel, 2017-06-14 20:17:56]

sahel@chromium.org changed reviewers:
+ ericrk@chromium.org, nick@chromium.org

[sahel, 2017-06-14 20:17:57]

nick@chromium.org: Please review changes in 
cc/trees/layer_tree_host_impl.cc

ericrk@chromium.org: Please review changes in 
content/*

https://codereview.chromium.org/2924953002/diff/40001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java
(right):

https://codereview.chromium.org/2924953002/diff/40001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java:109:
private void assertWaitForViewportInitialization() {
On 2017/06/14 18:44:03, tdresser wrote:
> On 2017/06/14 18:31:37, sahel wrote:
> > On 2017/06/14 17:59:24, tdresser wrote:
> > > Why does this include "assert" in the name? Is it an assertion?
> > 
> > I just followed naming pattern in this file. assertWaitForScroll doesn't
have
> > any assertions in its body, either.
> > 
> > Do you think I should still change the name?
> 
> I looked up when assertWaitForScroll was first added
>
(https://chromiumcodereview.appspot.com/12621006/diff/20002/content/public/and...),
> and it used to actually assert.
> 
> Now that they don't assert, can you rename both of them?

Done.

[commit-bot: I haz the power, 2017-06-14 20:45:54]

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

[commit-bot: I haz the power, 2017-06-14 20:45:54]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2017-06-14 20:55:01]

content/ lgtm

(I'm not a cc/ reviewer, you might have made a mistake in your first email
requesting a review)

[sahel, 2017-06-14 20:58:06]

On 2017/06/14 20:55:01, ncarter (slow) wrote:
> content/ lgtm
> 
> (I'm not a cc/ reviewer, you might have made a mistake in your first email
> requesting a review)

Yes, I meant the other way around: content/ for you and cc/ for ericrk@.
Thanks for the call!

[ericrk, 2017-06-15 21:09:10]

cc/ LGTM

[sahel, 2017-06-15 21:09:28]

The CQ bit was checked by sahel@chromium.org

[sahel, 2017-06-15 21:09:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2924953002/#ps80001
(title: "merged with master, redundant 'assert' deleted from function names.")

[commit-bot: I haz the power, 2017-06-15 21:10:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 22:32:17]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497560968470780,
"parent_rev": "1a4c6704de5d1ddcacf79149c7320501bccba41f", "commit_rev":
"dad0c1edb6da7ce815e5019c87cd1f474b3d055c"}

[commit-bot: I haz the power, 2017-06-15 22:32:28]

Description was changed from

==========
Ignore the rest of the scroll sequence if GSB is ignored.

This cl also modifies ContentViewCoreScrollingTests to initialize the
compositor's outer viewport before starting the test. This change is
required, as without properly initializing the compositor's outer
viewport, GSB events and the rest of the scroll sequences will get
ignored.

BUG=730197
TEST=AnimateInput/InputHandlerProxyTest.GestureScrollIgnored/*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Ignore the rest of the scroll sequence if GSB is ignored.

This cl also modifies ContentViewCoreScrollingTests to initialize the
compositor's outer viewport before starting the test. This change is
required, as without properly initializing the compositor's outer
viewport, GSB events and the rest of the scroll sequences will get
ignored.

BUG=730197
TEST=AnimateInput/InputHandlerProxyTest.GestureScrollIgnored/*
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2924953002
Cr-Commit-Position: refs/heads/master@{#479871}
Committed:
https://chromium.googlesource.com/chromium/src/+/dad0c1edb6da7ce815e5019c87cd...
==========

[commit-bot: I haz the power, 2017-06-15 22:32:30]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/dad0c1edb6da7ce815e5019c87cd...