cc: Add a test for the elastic overscroll synchronization.

cc: Add a test for the elastic overscroll synchronization.

During each BeginMainFrame, we send the delta for the elastic overcroll
to the main thread. This value is never mutated on the main thread, and
is updated only from the impl thread. Add a test to verify this
expectation and also ensure that the callbacks to the InputHandlerClient
necessary for elastic overscroll synchronization are made.

BUG=639046
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/e978686794916d58369484725af4a0b937bf5338
Cr-Commit-Position: refs/heads/master@{#413667}

[Khushal, 2016-08-22 21:17:38]

Description was changed from

==========
cc: Add a test for the elastic overscroll synchronization.

During each BeginMainFrame, we send the delta for the elastic overcroll
to the main thread. This value is never mutated on the main thread, and
is updated only from the impl thread. Add a test to verify this
expectation and also ensure that the callbacks to the InputHandlerClient
necessary for elastic overscroll synchronization are made.

BUG=639046
==========

to

==========
cc: Add a test for the elastic overscroll synchronization.

During each BeginMainFrame, we send the delta for the elastic overcroll
to the main thread. This value is never mutated on the main thread, and
is updated only from the impl thread. Add a test to verify this
expectation and also ensure that the callbacks to the InputHandlerClient
necessary for elastic overscroll synchronization are made.

BUG=639046
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Khushal, 2016-08-22 21:27:09]

khushalsagar@chromium.org changed reviewers:
+ aelias@chromium.org

[Khushal, 2016-08-22 21:27:09]

Added a test to make sure we don't regress on this.

While I was doing this, I tried to understand how the elastic overscroll
synchronization is working right now and I wasn't sure if there is a reason we
use SyncedProperty for this here. The value is never mutated on the main thread
and is always reflected back. The pending tree uses this reflected value but
When we make a call to the InputHandlerClient after the commit completes, it
reads the StretchAmount from the active tree and updates that value, and finally
when we activate, this is the value that will be used. It seems like this could
be implemented by simply reporting the current elastic overscroll value from the
active tree to the main thread during BeginMainFrame, since it used by blink
only for hit testing, and then replacing the pending tree value with the active
tree value on activation. May be I'm understanding something incorrectly, it'd
be great if you could clarify. Thanks!

[aelias_OOO_until_Jul13, 2016-08-22 21:43:13]

lgtm, thanks!

>It seems like this could be implemented by simply reporting the current elastic
overscroll value from the active tree to the main thread during BeginMainFrame,
since it used by blink only for hit testing, and then replacing the pending tree
value with the active tree value on activation.

Well, to me that doesn't really sound simpler than synced_property in that
synced_property is an established primitive used for every scrolling-related
thing, whereas you're proposing a special-case behavior used nowhere else.  In
practice it would be an additional wrinkle to understand.

Also, in principle, the main thread gesture handling code ought to modify the
elastic_overscroll, it's just that it doesn't bother to today (one of the many
bugs with main-thread gesture handling).  So the additional capabilities of
synced_property may eventually be useful.

[Khushal, 2016-08-22 23:16:39]

On 2016/08/22 21:43:13, aelias wrote:
> lgtm, thanks!

Thanks!

> 
> >It seems like this could be implemented by simply reporting the current
elastic
> overscroll value from the active tree to the main thread during
BeginMainFrame,
> since it used by blink only for hit testing, and then replacing the pending
tree
> value with the active tree value on activation.
> 
> Well, to me that doesn't really sound simpler than synced_property in that
> synced_property is an established primitive used for every scrolling-related
> thing, whereas you're proposing a special-case behavior used nowhere else.  In
> practice it would be an additional wrinkle to understand.

Oh, I only mentioned this because SyncedProperty is meant to synchronize
mutations to shared state, which means there ends up being an implicit
assumption that the main thread can modify that value while it actually never
does, so I was thinking if its better to have this be explicit in the code, that
it can't. But I didn't realize that it actually should be doing it.

> 
> Also, in principle, the main thread gesture handling code ought to modify the
> elastic_overscroll, it's just that it doesn't bother to today (one of the many
> bugs with main-thread gesture handling).  So the additional capabilities of
> synced_property may eventually be useful.


Do you mean for the single threaded case? I stumbled across a doc from dtapuska@
who was enabling it for the UI so the single threaded mode. But he was
structuring it so that it uses the same InputHandler setup on the *impl* thread
and just passes the deltas back in SingleThreadProxy.

For the threaded case, we handle the any scroll changes which happened on the
main thread when we get back to the impl thread itself,
https://cs.chromium.org/chromium/src/ui/events/blink/input_scroll_elasticity_...

[Khushal, 2016-08-22 23:17:31]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2016-08-22 23:18:20]

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

[aelias_OOO_until_Jul13, 2016-08-22 23:50:02]

On 2016/08/22 at 23:16:39, khushalsagar wrote:
> Do you mean for the single threaded case? I stumbled across a doc from
dtapuska@ who was enabling it for the UI so the single threaded mode. But he was
structuring it so that it uses the same InputHandler setup on the *impl* thread
and just passes the deltas back in SingleThreadProxy.
> 
> For the threaded case, we handle the any scroll changes which happened on the
main thread when we get back to the impl thread itself,
https://cs.chromium.org/chromium/src/ui/events/blink/input_scroll_elasticity_...

I mean in the threaded case, but where the scroll gesture itself needs to be
handled on the main thread due to a corner case such as the combination of LCD
text and a fixed-pos element.  Then the main thread ought to generate new scroll
elasticity itself.  (Although it seems like that particular corner case might
not come up because we don't support LCD text on Mac?  If I'm reading
http://crbug.com/581166 correctly.  Not certain of the situation though.)

[Khushal, 2016-08-23 00:54:06]

On 2016/08/22 23:50:02, aelias wrote:
> On 2016/08/22 at 23:16:39, khushalsagar wrote:
> > Do you mean for the single threaded case? I stumbled across a doc from
> dtapuska@ who was enabling it for the UI so the single threaded mode. But he
was
> structuring it so that it uses the same InputHandler setup on the *impl*
thread
> and just passes the deltas back in SingleThreadProxy.
> > 
> > For the threaded case, we handle the any scroll changes which happened on
the
> main thread when we get back to the impl thread itself,
>
https://cs.chromium.org/chromium/src/ui/events/blink/input_scroll_elasticity_...
> 
> I mean in the threaded case, but where the scroll gesture itself needs to be
> handled on the main thread due to a corner case such as the combination of LCD
> text and a fixed-pos element.  Then the main thread ought to generate new
scroll
> elasticity itself.  (Although it seems like that particular corner case might
> not come up because we don't support LCD text on Mac?  If I'm reading
> http://crbug.com/581166 correctly.  Not certain of the situation though.)

From what I could grasp with elasticity overscroll handling logic, even for main
thread gestures we just post a message to the InputHandlerManager to handle the
overscroll on the impl thread,
https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?l=844.

[aelias_OOO_until_Jul13, 2016-08-23 01:07:35]

Yeah, I had forgotten about how that worked.

Anyway, while I don't like speculative code for possible future bugfixes when it
actually adds lines/complexity, I don't think there's a particular problem with
using a more powerful primitive when that doesn't add lines (I imagine it might
even have less than your proposal) and makes the code more consistent.

[Khushal, 2016-08-23 01:20:47]

On 2016/08/23 01:07:35, aelias wrote:
> Yeah, I had forgotten about how that worked.
> 
> Anyway, while I don't like speculative code for possible future bugfixes when
it
> actually adds lines/complexity, I don't think there's a particular problem
with
> using a more powerful primitive when that doesn't add lines (I imagine it
might
> even have less than your proposal) and makes the code more consistent.

No problem. The current structure is fine too, I just wanted to confirm that
this use of synced property was intended since the test was in particular
verifying that the value shouldn't changed on the main thread and I wanted to
make sure I was getting that assumption right. :)

[commit-bot: I haz the power, 2016-08-23 01:36:19]

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

[commit-bot: I haz the power, 2016-08-23 01:36:20]

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

[Khushal, 2016-08-23 02:26:48]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-08-23 02:26:48]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2261383002/#ps20001
(title: "overflow fix.")

[commit-bot: I haz the power, 2016-08-23 02:27:16]

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, 2016-08-23 03:52:19]

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

[commit-bot: I haz the power, 2016-08-23 03:52:20]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Khushal, 2016-08-23 04:12:23]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2016-08-23 04:12:42]

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, 2016-08-23 04:50:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-23 04:52:08]

Description was changed from

==========
cc: Add a test for the elastic overscroll synchronization.

During each BeginMainFrame, we send the delta for the elastic overcroll
to the main thread. This value is never mutated on the main thread, and
is updated only from the impl thread. Add a test to verify this
expectation and also ensure that the callbacks to the InputHandlerClient
necessary for elastic overscroll synchronization are made.

BUG=639046
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Add a test for the elastic overscroll synchronization.

During each BeginMainFrame, we send the delta for the elastic overcroll
to the main thread. This value is never mutated on the main thread, and
is updated only from the impl thread. Add a test to verify this
expectation and also ensure that the callbacks to the InputHandlerClient
necessary for elastic overscroll synchronization are made.

BUG=639046
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/e978686794916d58369484725af4a0b937bf5338
Cr-Commit-Position: refs/heads/master@{#413667}
==========

[commit-bot: I haz the power, 2016-08-23 04:52:09]

Patchset 2 (id:??) landed as
https://crrev.com/e978686794916d58369484725af4a0b937bf5338
Cr-Commit-Position: refs/heads/master@{#413667}