Track whether a scroll sequence has been partially prevented

Track whether a scroll sequence has been partially prevented

Listeners and consumers of a gesture event stream may wish to know if
part of the gesture stream has been prevented, i.e., whether the page
has consumed any of the underlying touchmove events. Route this bit
for scroll update events from the TouchDispositionGestureFilter through
to the generated WebGestureEvent types.

This change depends on the Blink patch:
https://codereview.chromium.org/690173002/

BUG=428429
Committed: https://crrev.com/019a29ecaa7e23348b65e1c86807554ded255028
Cr-Commit-Position: refs/heads/master@{#304665}

[jdduke (slow), 2014-11-10 21:27:10]

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

[jdduke (slow), 2014-11-10 21:27:11]

tdresser@: PTAL, thanks.

[tdresser, 2014-11-10 21:51:01]

LGTM, but I'd like to hear your thoughts on whether that |mutable| is actually
the right long term decision.

https://codereview.chromium.org/712133003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/web_input_event_util.h (left):

https://codereview.chromium.org/712133003/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/web_input_event_util.h:26: // |event| by
|scale|.
Thanks.

https://codereview.chromium.org/712133003/diff/20001/ui/events/gesture_event_...
File ui/events/gesture_event_details.h (right):

https://codereview.chromium.org/712133003/diff/20001/ui/events/gesture_event_...
ui/events/gesture_event_details.h:160: mutable bool
previous_update_in_sequence_prevented;
I'm not sure this is superior to making all the event passing non-const.

Do you think this is better?

[Rick Byers, 2014-11-10 22:03:08]

rbyers@chromium.org changed reviewers:
+ rbyers@chromium.org

[Rick Byers, 2014-11-10 22:03:09]

https://codereview.chromium.org/712133003/diff/20001/ui/events/gesture_event_...
File ui/events/gesture_event_details.h (right):

https://codereview.chromium.org/712133003/diff/20001/ui/events/gesture_event_...
ui/events/gesture_event_details.h:160: mutable bool
previous_update_in_sequence_prevented;
On 2014/11/10 21:51:01, tdresser wrote:
> I'm not sure this is superior to making all the event passing non-const.
> 
> Do you think this is better?

We hit a similar issue with touch-action, and there we decided that we should
mutate a copy of the event (passing non-const where needed), since that's really
what's happening in the system.  Using mutable and calling the events 'const'
seems like an abuse of the notion of 'const' to me.  What's the downside to
changing to non-const where necessary?

[jdduke (slow), 2014-11-10 22:23:43]

https://codereview.chromium.org/712133003/diff/20001/ui/events/gesture_event_...
File ui/events/gesture_event_details.h (right):

https://codereview.chromium.org/712133003/diff/20001/ui/events/gesture_event_...
ui/events/gesture_event_details.h:160: mutable bool
previous_update_in_sequence_prevented;
On 2014/11/10 22:03:08, Rick Byers wrote:
> On 2014/11/10 21:51:01, tdresser wrote:
> > I'm not sure this is superior to making all the event passing non-const.
> > 
> > Do you think this is better?
> 
> We hit a similar issue with touch-action, and there we decided that we should
> mutate a copy of the event (passing non-const where needed), since that's
really
> what's happening in the system.  Using mutable and calling the events 'const'
> seems like an abuse of the notion of 'const' to me.  What's the downside to
> changing to non-const where necessary?

This is a very different case.

This extra bit I consider metadata. It gives additional context to the event,
but is not an integral part of the event's identity. For that reason, and that
the bit will (always?) be modified downstream from the event's creation, I feel
like mutable is a meaningful modifier.

Rick, your change fundamentally mutated the core of the event itself, so it
makes sense to mutate the event rather than make a few core members immutable.

I have similar plans with LatencyInfo: its modification should not require us to
copy an entire event, and I feel like making it mutable is a worthwhile
sacrifice to allow us keeping the associated event const.

[jdduke (slow), 2014-11-10 22:31:32]

https://codereview.chromium.org/712133003/diff/20001/ui/events/gesture_event_...
File ui/events/gesture_event_details.h (right):

https://codereview.chromium.org/712133003/diff/20001/ui/events/gesture_event_...
ui/events/gesture_event_details.h:160: mutable bool
previous_update_in_sequence_prevented;
Anyhow, if you guys prefer we not make this mutable, that's fine, I would just
copy the GestureEventData when necessary to modify the property...

[jdduke (slow), 2014-11-10 22:41:31]

OK, went ahead and removed mutable, it appears to be anathema to most developers
these days =/

[jdduke (slow), 2014-11-10 23:45:30]

jdduke@chromium.org changed reviewers:
+ sadrul@chromium.org

[jdduke (slow), 2014-11-10 23:45:31]

+sadrul@ for content/browser/renderer_host and ui/events/ owner review.

[sadrul, 2014-11-11 15:04:01]

The test failures look related. Mind taking a look at those?

[jdduke (slow), 2014-11-11 16:22:40]

On 2014/11/11 15:04:01, sadrul wrote:
> The test failures look related. Mind taking a look at those?

Ooph, yeah, looks like we've had a lurking bug that's only now bubbled to the
surface...

[Rick Byers, 2014-11-11 17:05:09]

On 2014/11/10 22:41:31, jdduke wrote:
> OK, went ahead and removed mutable, it appears to be anathema to most
developers
> these days =/

FWIW, Personally I think mutable is great for cases where some implementation
detail of a class needs to update, even though the conceptual state of the
object hasn't changed.  Eg. any lazily-created / updated members (caches,
indexes, etc.).  That's just pretty rare, compared to "the object state is now
changing, but I'd rather not update usage everywhere to show that" arguments ;-)

[Rick Byers, 2014-11-11 19:05:15]

On 2014/11/11 17:05:09, Rick Byers wrote:
> On 2014/11/10 22:41:31, jdduke wrote:
> > OK, went ahead and removed mutable, it appears to be anathema to most
> developers
> > these days =/
> 
> FWIW, Personally I think mutable is great for cases where some implementation
> detail of a class needs to update, even though the conceptual state of the
> object hasn't changed.  Eg. any lazily-created / updated members (caches,
> indexes, etc.).  That's just pretty rare, compared to "the object state is now
> changing, but I'd rather not update usage everywhere to show that" arguments
;-)

Just realized I missed your comments in #msg6.  So your argument is that there's
a distinction between primary state ("fundamental identity') and auxiliary
observable state.  I can see the argument there (but seems a little fuzzy).  One
way to model that in general is by having the aux state stored through an
indirection - your const object can still return a non-const pointer to the aux
state so that it can be mutated...

Anyway I didn't mean to sound religious about this - I'm ok using mutable here
if you feel strongly that it's better.

[jdduke (slow), 2014-11-11 19:34:23]

On 2014/11/11 19:05:15, Rick Byers wrote:
> Just realized I missed your comments in #msg6.  So your argument is that
there's
> a distinction between primary state ("fundamental identity') and auxiliary
> observable state.  I can see the argument there (but seems a little fuzzy). 
One
> way to model that in general is by having the aux state stored through an
> indirection - your const object can still return a non-const pointer to the
aux
> state so that it can be mutated...
> 
> Anyway I didn't mean to sound religious about this - I'm ok using mutable here
> if you feel strongly that it's better.

Nah, this is fine, in fact it uncovered some pre-existing bugs =/

[jdduke (slow), 2014-11-12 19:21:19]

OK, I think the tests are happy. sadrul@ would you mind taking another look?

[jdduke (slow), 2014-11-18 16:14:05]

On 2014/11/12 19:21:19, jdduke wrote:
> OK, I think the tests are happy. sadrul@ would you mind taking another look?

sadrul@: Anything else you'd like to see from ui/events? Thanks.

[sadrul, 2014-11-18 17:53:45]

A question, LGTM otherwise.

https://codereview.chromium.org/712133003/diff/180001/ui/events/gesture_event...
File ui/events/gesture_event_details.cc (right):

https://codereview.chromium.org/712133003/diff/180001/ui/events/gesture_event...
ui/events/gesture_event_details.cc:81: break;
NOTREACHED()? Or no because the caller is responsible to ensure this?

[jdduke (slow), 2014-11-18 19:50:46]

https://codereview.chromium.org/712133003/diff/180001/ui/events/gesture_event...
File ui/events/gesture_event_details.cc (right):

https://codereview.chromium.org/712133003/diff/180001/ui/events/gesture_event...
ui/events/gesture_event_details.cc:81: break;
On 2014/11/18 17:53:45, sadrul wrote:
> NOTREACHED()? Or no because the caller is responsible to ensure this?

For these cases, we don't really care about the source type as we're not using
the details union.

[jdduke (slow), 2014-11-18 19:50:59]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-11-18 19:52:09]

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

[commit-bot: I haz the power, 2014-11-18 20:44:43]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-11-18 20:46:25]

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