Bundle DidOverscrollParams with the InputEventAck

Bundle DidOverscrollParams with the input event ack

Currently, overscroll notifications are always sent as a separate IPC message,
regardless of the causal event.  Instead, bundle the overscroll metadata with
the event ack if the event caused the overscroll, saving an IPC message in the
most common overscroll case.

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

[jdduke (slow), 2014-05-14 17:18:03]

tdresser@, I explicitly didn't wire up the |DidOverscrollParams| to the TEQ or
OverscrollController, but this affords us that freedom in the future.

I'm still not sure what kinds of ack data would be useful for other event types.
 Should we find such examples in the future, we can generalize |InputEventAck|
to take event-specific metadata for a given event type (like the WebGestureEvent
union).  

Perhaps TouchStart evenst could bundle the touch-action response at some point?
Anything that saves an extra IPC is a win.

[tdresser, 2014-05-14 18:20:07]

LGTM, but make sure to get thorough review from some folks more familiar with
IPC stuff than I am.

https://codereview.chromium.org/281723010/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/281723010/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:501: if
(ack.overscroll)
Although its redundant, I think it would be clearer to test if the event is a
GestureScrollUpdate before checking if it overscrolled.

https://codereview.chromium.org/281723010/diff/40001/content/common/input_mes...
File content/common/input_messages.h (right):

https://codereview.chromium.org/281723010/diff/40001/content/common/input_mes...
content/common/input_messages.h:207: // restricions on the root scroll offset.
restricions -> restrictions

[jdduke (slow), 2014-05-19 15:39:43]

aelias@: PTAL, both at a high level and particularly at
content/browser/renderer_host/ and content/renderer.  I think saving an IPC is
worthwhile here (it's not a lot of extra plumbing), even if we have to support
the async case when a fling generates overscroll.

I've also got another WIP patch where I'm trying to hook up Aura's
OverscrollController to use the |DidOverscrollParams| notification, rather than
just looking at event acks (also moving it out of the InputRouter into
RenderWidgetHostViewAura).

[aelias_OOO_until_Jul13, 2014-05-19 18:34:46]

lgtm

[jdduke (slow), 2014-05-19 20:48:02]

PTAL, thanks.

tsepez@: Security review for content/common/{input,view}_messages.h and
ipc/ipc_message_utils.h.  Note that the added scoped_ptr serialization code is
the same from a previous patch (previously added to but subsequently removed
from content/common/input/input_param_traits.h in
https://chromiumcodereview.appspot.com/19624005). It's a shame we don't have an
optional<T> type for cases such as this.

piman@: Review for content/renderer/render_widget{_unittest}.cc.

https://codereview.chromium.org/281723010/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/281723010/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:501: if
(ack.overscroll)
On 2014/05/14 18:20:08, tdresser wrote:
> Although its redundant, I think it would be clearer to test if the event is a
> GestureScrollUpdate before checking if it overscrolled.

Done.

https://codereview.chromium.org/281723010/diff/40001/content/common/input_mes...
File content/common/input_messages.h (right):

https://codereview.chromium.org/281723010/diff/40001/content/common/input_mes...
content/common/input_messages.h:207: // restricions on the root scroll offset.
On 2014/05/14 18:20:08, tdresser wrote:
> restricions -> restrictions

Done.

[Tom Sepez, 2014-05-19 21:01:14]

lgtm

[piman, 2014-05-19 21:51:12]

https://codereview.chromium.org/281723010/diff/60001/content/common/input/inp...
File content/common/input/input_event_ack.h (right):

https://codereview.chromium.org/281723010/diff/60001/content/common/input/inp...
content/common/input/input_event_ack.h:17: struct CONTENT_EXPORT InputEventAck {
nit: do you need this as an explicit separate struct, or would defining it in
input_messages.h as an IPC_STRUCT_BEGIN/END (instead of
IPC_STRUCT_TRAITS_BEGIN/END) be enough? The former is a little better in that
you don't have to worry about keeping the _messages.h file in sync with this
one.

https://codereview.chromium.org/281723010/diff/60001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/281723010/diff/60001/ipc/ipc_message_utils.h#...
ipc/ipc_message_utils.h:715: WriteParam(m, valid);
What is the value of this over having 'valid' be explicit in the InputEventAck,
and have overscroll then be inline?

It would avoid extra dynamic allocations.

[jdduke (slow), 2014-05-19 22:00:20]

https://codereview.chromium.org/281723010/diff/60001/content/common/input/inp...
File content/common/input/input_event_ack.h (right):

https://codereview.chromium.org/281723010/diff/60001/content/common/input/inp...
content/common/input/input_event_ack.h:17: struct CONTENT_EXPORT InputEventAck {
On 2014/05/19 21:51:13, piman wrote:
> nit: do you need this as an explicit separate struct, or would defining it in
> input_messages.h as an IPC_STRUCT_BEGIN/END (instead of
> IPC_STRUCT_TRAITS_BEGIN/END) be enough? The former is a little better in that
> you don't have to worry about keeping the _messages.h file in sync with this
> one.

Not particularly, defining in input_messages.h works just as well.

https://codereview.chromium.org/281723010/diff/60001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/281723010/diff/60001/ipc/ipc_message_utils.h#...
ipc/ipc_message_utils.h:715: WriteParam(m, valid);
On 2014/05/19 21:51:13, piman wrote:
> What is the value of this over having 'valid' be explicit in the
InputEventAck,
> and have overscroll then be inline?
> 
> It would avoid extra dynamic allocations.

I considered that.  My only concern was the cost of serializing 6 additional
floats in every event ack (unless we special-cased the serialization of the
InputEventAck struct), particularly the cases where it's unused which is the 99%
case.  If you thinking avoiding the dynamic allocation is preferable I can
switch to that (sigh, optional<T> would be lovely =/).

[piman, 2014-05-19 22:08:22]

On Mon, May 19, 2014 at 3:00 PM, <jdduke@chromium.org> wrote:

>
> https://codereview.chromium.org/281723010/diff/60001/
> content/common/input/input_event_ack.h
> File content/common/input/input_event_ack.h (right):
>
> https://codereview.chromium.org/281723010/diff/60001/
> content/common/input/input_event_ack.h#newcode17
> content/common/input/input_event_ack.h:17: struct CONTENT_EXPORT
> InputEventAck {
> On 2014/05/19 21:51:13, piman wrote:
>
>> nit: do you need this as an explicit separate struct, or would
>>
> defining it in
>
>> input_messages.h as an IPC_STRUCT_BEGIN/END (instead of
>> IPC_STRUCT_TRAITS_BEGIN/END) be enough? The former is a little better
>>
> in that
>
>> you don't have to worry about keeping the _messages.h file in sync
>>
> with this
>
>> one.
>>
>
> Not particularly, defining in input_messages.h works just as well.
>
>
> https://codereview.chromium.org/281723010/diff/60001/ipc/
> ipc_message_utils.h
> File ipc/ipc_message_utils.h (right):
>
> https://codereview.chromium.org/281723010/diff/60001/ipc/
> ipc_message_utils.h#newcode715
> ipc/ipc_message_utils.h:715: WriteParam(m, valid);
> On 2014/05/19 21:51:13, piman wrote:
>
>> What is the value of this over having 'valid' be explicit in the
>>
> InputEventAck,
>
>> and have overscroll then be inline?
>>
>
>  It would avoid extra dynamic allocations.
>>
>
> I considered that.  My only concern was the cost of serializing 6
> additional floats in every event ack (unless we special-cased the
> serialization of the InputEventAck struct), particularly the cases where
> it's unused which is the 99% case.


If it's unused 99% of the case, that's probably a good reason.


>  If you thinking avoiding the dynamic
> allocation is preferable I can switch to that (sigh, optional<T> would
> be lovely =/).
>

optional<T> doesn't seem like a bad choice. Maybe it makes sense to
implement this in base and use it?


>
> https://codereview.chromium.org/281723010/
>

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

[jdduke (slow), 2014-05-20 00:10:16]

OK, I removed the InputEventAck standalone type, and added a TODO to the
|scoped_ptr<DidOverscrollParamss>| entry to adopt Optional<T>.  I figure it best
to do the latter in a separate patch.

[piman, 2014-05-20 00:24:21]

lgtm

[jdduke (slow), 2014-05-20 02:04:18]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-05-20 02:05:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/281723010/100001

[commit-bot: I haz the power, 2014-05-20 02:35:02]

Change committed as 271541