Non passive touch end or touch cancel listeners should not block scroll.

Non passive touch end or touch cancel listeners should not block scroll.

Ensure that touch end or touch cancel listeners don't block scroll by
removing these listeners from the touch hit test regions.

When no touch listener is encountered for TouchStart examine the touch
end/cancel listeners and determine if the touch start needs to be
dispatched non-blocking.

BUG=601990
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[dtapuska, 2016-04-13 15:40:45]

Description was changed from

==========
Non passive touch end or touch cancel listeners should not block scroll.

Ensure that touch end or touch cancel listeners don't block scroll by
removing these listeners from the touch hit test regions.

When no touch listener is encountered for TouchStart examine the touch
end/cancel listeners and determine if the touch start needs to be
dispatched non-blocking.

BUG=601990
==========

to

==========
Non passive touch end or touch cancel listeners should not block scroll.

Ensure that touch end or touch cancel listeners don't block scroll by
removing these listeners from the touch hit test regions.

When no touch listener is encountered for TouchStart examine the touch
end/cancel listeners and determine if the touch start needs to be
dispatched non-blocking.

BUG=601990
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[dtapuska, 2016-04-13 15:41:30]

Description was changed from

==========
Non passive touch end or touch cancel listeners should not block scroll.

Ensure that touch end or touch cancel listeners don't block scroll by
removing these listeners from the touch hit test regions.

When no touch listener is encountered for TouchStart examine the touch
end/cancel listeners and determine if the touch start needs to be
dispatched non-blocking.

BUG=601990
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Non passive touch end or touch cancel listeners should not block scroll.

Ensure that touch end or touch cancel listeners don't block scroll by
removing these listeners from the touch hit test regions.

When no touch listener is encountered for TouchStart examine the touch
end/cancel listeners and determine if the touch start needs to be
dispatched non-blocking.

BUG=601990
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[dtapuska, 2016-04-13 15:41:31]

dtapuska@chromium.org changed reviewers:
+ aelias@chromium.org, tdresser@chromium.org

[tdresser, 2016-04-13 16:52:36]

Test failures look real.

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.js
(right):

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.js:141:
// are not added to the compositor hit test rects.
Nit: we're adding listeners, not events.

I'm a bit confused by this comment. Why does adding these listeners effect
whether or not we add to the compositor hit test rects?

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:236: void
EventHandlerRegistry::updateHaveTouchEventListeners()
updateHasTouchEventListeners - I think we generally use third person singular.

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/EmptyClients.h (right):

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/EmptyClients.h:158: void
setHaveTouchEventListeners(bool) override {}
setHas

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/ChromeClient.h:199: virtual void
setHaveTouchEventListeners(bool) = 0;
setHas

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ChromeClientImpl.h (right):

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ChromeClientImpl.h:115: void
setHaveTouchEventListeners(bool) override;
setHas

https://codereview.chromium.org/1884863003/diff/1/ui/events/blink/input_handl...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/1884863003/diff/1/ui/events/blink/input_handl...
ui/events/blink/input_handler_proxy.cc:846: // If |result| is still DROP_EVENT
look at the touch end as
look at the touch end handler

https://codereview.chromium.org/1884863003/diff/1/ui/events/blink/input_handl...
File ui/events/blink/input_handler_proxy_unittest.cc (right):

https://codereview.chromium.org/1884863003/diff/1/ui/events/blink/input_handl...
ui/events/blink/input_handler_proxy_unittest.cc:2074: // The touch start is not
in a region but there is a touch end handler
not in a touch-region

https://codereview.chromium.org/1884863003/diff/1/ui/events/blink/input_handl...
ui/events/blink/input_handler_proxy_unittest.cc:2075: // so to maintain
targetting we need to dispatch the touch start as
targeting

[dtapuska, 2016-04-13 18:00:16]

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.js
(right):

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.js:141:
// are not added to the compositor hit test rects.
On 2016/04/13 16:52:35, tdresser wrote:
> Nit: we're adding listeners, not events.
> 
> I'm a bit confused by this comment. Why does adding these listeners effect
> whether or not we add to the compositor hit test rects?

touchend and touchcancel use to be added to the hit test rects to block scroll.
So this is a negative test; in that I add the listeners and ensure the test
doesn't fail in adding the entire document to the hit test rects.

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:236: void
EventHandlerRegistry::updateHaveTouchEventListeners()
On 2016/04/13 16:52:35, tdresser wrote:
> updateHasTouchEventListeners - I think we generally use third person singular.

Will rename. I was trying to be consistent with ChromeClient which uses "have".

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/EmptyClients.h (right):

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/EmptyClients.h:158: void
setHaveTouchEventListeners(bool) override {}
On 2016/04/13 16:52:35, tdresser wrote:
> setHas

Done.

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/ChromeClient.h:199: virtual void
setHaveTouchEventListeners(bool) = 0;
On 2016/04/13 16:52:35, tdresser wrote:
> setHas

Done.

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ChromeClientImpl.h (right):

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ChromeClientImpl.h:115: void
setHaveTouchEventListeners(bool) override;
On 2016/04/13 16:52:35, tdresser wrote:
> setHas

Done.

https://codereview.chromium.org/1884863003/diff/1/ui/events/blink/input_handl...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/1884863003/diff/1/ui/events/blink/input_handl...
ui/events/blink/input_handler_proxy.cc:846: // If |result| is still DROP_EVENT
look at the touch end as
On 2016/04/13 16:52:35, tdresser wrote:
> look at the touch end handler

Done.

https://codereview.chromium.org/1884863003/diff/1/ui/events/blink/input_handl...
File ui/events/blink/input_handler_proxy_unittest.cc (right):

https://codereview.chromium.org/1884863003/diff/1/ui/events/blink/input_handl...
ui/events/blink/input_handler_proxy_unittest.cc:2074: // The touch start is not
in a region but there is a touch end handler
On 2016/04/13 16:52:35, tdresser wrote:
> not in a touch-region

Done.

https://codereview.chromium.org/1884863003/diff/1/ui/events/blink/input_handl...
ui/events/blink/input_handler_proxy_unittest.cc:2075: // so to maintain
targetting we need to dispatch the touch start as
On 2016/04/13 16:52:35, tdresser wrote:
> targeting

Done.

[dtapuska, 2016-04-13 18:00:36]

PTAL

[tdresser, 2016-04-13 18:44:40]

LGTM, % the nittiest nit and tests passing.

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.js
(right):

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.js:141:
// are not added to the compositor hit test rects.
On 2016/04/13 18:00:15, dtapuska wrote:
> On 2016/04/13 16:52:35, tdresser wrote:
> > Nit: we're adding listeners, not events.
> > 
> > I'm a bit confused by this comment. Why does adding these listeners effect
> > whether or not we add to the compositor hit test rects?
> 
> touchend and touchcancel use to be added to the hit test rects to block
scroll.
> So this is a negative test; in that I add the listeners and ensure the test
> doesn't fail in adding the entire document to the hit test rects.

Ah, makes sense.

Maybe reword "to ensure" to "and ensure".

https://codereview.chromium.org/1884863003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.js
(right):

https://codereview.chromium.org/1884863003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.js:141:
// listeners do not effect compositor hit test rects.
Nittiest nit: effect -> affect

[dtapuska, 2016-04-13 18:49:08]

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.js
(right):

https://codereview.chromium.org/1884863003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.js:141:
// are not added to the compositor hit test rects.
On 2016/04/13 18:44:39, tdresser wrote:
> On 2016/04/13 18:00:15, dtapuska wrote:
> > On 2016/04/13 16:52:35, tdresser wrote:
> > > Nit: we're adding listeners, not events.
> > > 
> > > I'm a bit confused by this comment. Why does adding these listeners effect
> > > whether or not we add to the compositor hit test rects?
> > 
> > touchend and touchcancel use to be added to the hit test rects to block
> scroll.
> > So this is a negative test; in that I add the listeners and ensure the test
> > doesn't fail in adding the entire document to the hit test rects.
> 
> Ah, makes sense.
> 
> Maybe reword "to ensure" to "and ensure".

Done.

https://codereview.chromium.org/1884863003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.js
(right):

https://codereview.chromium.org/1884863003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/touch/resources/compositor-touch-hit-rects.js:141:
// listeners do not effect compositor hit test rects.
On 2016/04/13 18:44:40, tdresser wrote:
> Nittiest nit: effect -> affect

Done.

[aelias_OOO_until_Jul13, 2016-04-13 21:56:19]

lgtm

[dtapuska, 2016-04-13 22:07:07]

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

[dtapuska, 2016-04-13 22:25:57]

On 2016/04/13 21:56:19, aelias wrote:
> lgtm

Rbyers@ can you have a look at the blink changes ?

[Rick Byers, 2016-04-14 01:41:41]

https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:236: void
EventHandlerRegistry::updateHasTouchEventListeners()
Why put this logic here instead of ChromeClientImpl (where it was previously)? 
Doesn't this make for more of an opportunity for things to be inconsistent? 
I.e. each caller to setEventListenerProperties for touch must remember to also
call setHasTouchEventListeners and vice versa.

https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.h (right):

https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.h:35:
TouchEventPassive,
Please rename these to TouchStartOrMove* for symmetry and to avoid confusion.

https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.idl:147: unsigned long
touchEventHandlerCount(Document document);
nit: please rename this to make it clear it's only touchstart/move

Optionally while you're at it you could make both explicitly about "blocking"
(or change the impl to return a sum of passive+blocking).

[dtapuska, 2016-04-14 01:49:10]

https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:236: void
EventHandlerRegistry::updateHasTouchEventListeners()
On 2016/04/14 01:41:40, Rick Byers wrote:
> Why put this logic here instead of ChromeClientImpl (where it was previously)?

> Doesn't this make for more of an opportunity for things to be inconsistent? 
> I.e. each caller to setEventListenerProperties for touch must remember to also
> call setHasTouchEventListeners and vice versa.

There are only two callers to the setEventListenerProperties
1) This class
2) The FrameLoader which tries to reset the values to default values.

Ideally the FrameLoader should just call a reset method on this class.

https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.h (right):

https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.h:35:
TouchEventPassive,
On 2016/04/14 01:41:41, Rick Byers wrote:
> Please rename these to TouchStartOrMove* for symmetry and to avoid confusion.

Do you want the WebEventListenerClass enum renamed too?

[Rick Byers, 2016-04-14 02:07:15]

On 2016/04/14 01:49:10, dtapuska wrote:
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):
> 
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:236: void
> EventHandlerRegistry::updateHasTouchEventListeners()
> On 2016/04/14 01:41:40, Rick Byers wrote:
> > Why put this logic here instead of ChromeClientImpl (where it was
previously)?
> 
> > Doesn't this make for more of an opportunity for things to be inconsistent? 
> > I.e. each caller to setEventListenerProperties for touch must remember to
also
> > call setHasTouchEventListeners and vice versa.
> 
> There are only two callers to the setEventListenerProperties
> 1) This class
> 2) The FrameLoader which tries to reset the values to default values.
> 
> Ideally the FrameLoader should just call a reset method on this class.

But what's the value in adding an extra API to the ChromeClient interface at
all, isn't it strictly redundant with the other APIs?  I'm probably missing
something here since it was clearly an intentional change on your part.  I agree
what you've got isn't terrible so if there's a reason why it's better than the
old scheme (where the logic was in ChromeClientImpl) then that's OK with me. 
Just seems strictly more complex to me...

> 
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/EventHandlerRegistry.h (right):
> 
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/EventHandlerRegistry.h:35:
> TouchEventPassive,
> On 2016/04/14 01:41:41, Rick Byers wrote:
> > Please rename these to TouchStartOrMove* for symmetry and to avoid
confusion.
> 
> Do you want the WebEventListenerClass enum renamed too?

I would - it's misleading to call something just "TouchEvent" when really it
only applies to half of the types of touch events.  Hopefully it's just a
trivial rename though (eclipse / sed mechanically?) so won't take much effort.

[dtapuska, 2016-04-14 02:29:45]

On 2016/04/14 02:07:15, Rick Byers wrote:
> On 2016/04/14 01:49:10, dtapuska wrote:
> >
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:236: void
> > EventHandlerRegistry::updateHasTouchEventListeners()
> > On 2016/04/14 01:41:40, Rick Byers wrote:
> > > Why put this logic here instead of ChromeClientImpl (where it was
> previously)?
> > 
> > > Doesn't this make for more of an opportunity for things to be
inconsistent? 
> > > I.e. each caller to setEventListenerProperties for touch must remember to
> also
> > > call setHasTouchEventListeners and vice versa.
> > 
> > There are only two callers to the setEventListenerProperties
> > 1) This class
> > 2) The FrameLoader which tries to reset the values to default values.
> > 
> > Ideally the FrameLoader should just call a reset method on this class.
> 
> But what's the value in adding an extra API to the ChromeClient interface at
> all, isn't it strictly redundant with the other APIs?  I'm probably missing
> something here since it was clearly an intentional change on your part.  I
agree
> what you've got isn't terrible so if there's a reason why it's better than the
> old scheme (where the logic was in ChromeClientImpl) then that's OK with me. 
> Just seems strictly more complex to me...
> 
> > 
> >
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/frame/EventHandlerRegistry.h (right):
> > 
> >
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/frame/EventHandlerRegistry.h:35:
> > TouchEventPassive,
> > On 2016/04/14 01:41:41, Rick Byers wrote:
> > > Please rename these to TouchStartOrMove* for symmetry and to avoid
> confusion.
> > 
> > Do you want the WebEventListenerClass enum renamed too?
> 
> I would - it's misleading to call something just "TouchEvent" when really it
> only applies to half of the types of touch events.  Hopefully it's just a
> trivial rename though (eclipse / sed mechanically?) so won't take much effort.

For the ChromeClient interface; there will be two calls for touch event
listeners.
TouchStartOrMove, and TouchEndOrCancel...

It needs to be a union of these two. Previously it had all the information in
the call context. So the possible changes are to 
store state in the ChromeClient (don't care to do this). Change the API in
setting all event listener properties so it is only
one call (more complicated change as we could pass an array of event listener
states instead of individual calls) or add an API
to set the property for having touch listeners separately and query the value
inside the EventHandlerRegistry.

I chose the later.

[Rick Byers, 2016-04-14 03:21:06]

On 2016/04/14 02:29:45, dtapuska wrote:
> On 2016/04/14 02:07:15, Rick Byers wrote:
> > On 2016/04/14 01:49:10, dtapuska wrote:
> > >
> >
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:236: void
> > > EventHandlerRegistry::updateHasTouchEventListeners()
> > > On 2016/04/14 01:41:40, Rick Byers wrote:
> > > > Why put this logic here instead of ChromeClientImpl (where it was
> > previously)?
> > > 
> > > > Doesn't this make for more of an opportunity for things to be
> inconsistent? 
> > > > I.e. each caller to setEventListenerProperties for touch must remember
to
> > also
> > > > call setHasTouchEventListeners and vice versa.
> > > 
> > > There are only two callers to the setEventListenerProperties
> > > 1) This class
> > > 2) The FrameLoader which tries to reset the values to default values.
> > > 
> > > Ideally the FrameLoader should just call a reset method on this class.
> > 
> > But what's the value in adding an extra API to the ChromeClient interface at
> > all, isn't it strictly redundant with the other APIs?  I'm probably missing
> > something here since it was clearly an intentional change on your part.  I
> agree
> > what you've got isn't terrible so if there's a reason why it's better than
the
> > old scheme (where the logic was in ChromeClientImpl) then that's OK with me.

> > Just seems strictly more complex to me...
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/frame/EventHandlerRegistry.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/frame/EventHandlerRegistry.h:35:
> > > TouchEventPassive,
> > > On 2016/04/14 01:41:41, Rick Byers wrote:
> > > > Please rename these to TouchStartOrMove* for symmetry and to avoid
> > confusion.
> > > 
> > > Do you want the WebEventListenerClass enum renamed too?
> > 
> > I would - it's misleading to call something just "TouchEvent" when really it
> > only applies to half of the types of touch events.  Hopefully it's just a
> > trivial rename though (eclipse / sed mechanically?) so won't take much
effort.
> 
> For the ChromeClient interface; there will be two calls for touch event
> listeners.
> TouchStartOrMove, and TouchEndOrCancel...
> 
> It needs to be a union of these two. Previously it had all the information in
> the call context. So the possible changes are to 
> store state in the ChromeClient (don't care to do this). Change the API in
> setting all event listener properties so it is only
> one call (more complicated change as we could pass an array of event listener
> states instead of individual calls) or add an API
> to set the property for having touch listeners separately and query the value
> inside the EventHandlerRegistry.
> 
> I chose the later.

Ah, I see - thanks.  There's a 4th option: read the state from
LayerTreeHost::event_listener_properties() whenever one of the touch states is
changed.  This is cheap (it's just sitting there in a field).  You'd end up
calling into WebView::SetHasTouchHandlers twice but that skips sending an IPC if
there's no change so I assume it's also trivially cheap.  Unless maybe we need
to worry about cases where there is no LayerTreeHost yet?

Anyway I don't think this really matters, I'm fine with it as-is if you prefer
that option.

[dtapuska, 2016-04-14 14:39:41]

On 2016/04/14 03:21:06, Rick Byers wrote:
> On 2016/04/14 02:29:45, dtapuska wrote:
> > On 2016/04/14 02:07:15, Rick Byers wrote:
> > > On 2016/04/14 01:49:10, dtapuska wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:236: void
> > > > EventHandlerRegistry::updateHasTouchEventListeners()
> > > > On 2016/04/14 01:41:40, Rick Byers wrote:
> > > > > Why put this logic here instead of ChromeClientImpl (where it was
> > > previously)?
> > > > 
> > > > > Doesn't this make for more of an opportunity for things to be
> > inconsistent? 
> > > > > I.e. each caller to setEventListenerProperties for touch must remember
> to
> > > also
> > > > > call setHasTouchEventListeners and vice versa.
> > > > 
> > > > There are only two callers to the setEventListenerProperties
> > > > 1) This class
> > > > 2) The FrameLoader which tries to reset the values to default values.
> > > > 
> > > > Ideally the FrameLoader should just call a reset method on this class.
> > > 
> > > But what's the value in adding an extra API to the ChromeClient interface
at
> > > all, isn't it strictly redundant with the other APIs?  I'm probably
missing
> > > something here since it was clearly an intentional change on your part.  I
> > agree
> > > what you've got isn't terrible so if there's a reason why it's better than
> the
> > > old scheme (where the logic was in ChromeClientImpl) then that's OK with
me.
> 
> > > Just seems strictly more complex to me...
> > > 
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/core/frame/EventHandlerRegistry.h
(right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/core/frame/EventHandlerRegistry.h:35:
> > > > TouchEventPassive,
> > > > On 2016/04/14 01:41:41, Rick Byers wrote:
> > > > > Please rename these to TouchStartOrMove* for symmetry and to avoid
> > > confusion.
> > > > 
> > > > Do you want the WebEventListenerClass enum renamed too?
> > > 
> > > I would - it's misleading to call something just "TouchEvent" when really
it
> > > only applies to half of the types of touch events.  Hopefully it's just a
> > > trivial rename though (eclipse / sed mechanically?) so won't take much
> effort.
> > 
> > For the ChromeClient interface; there will be two calls for touch event
> > listeners.
> > TouchStartOrMove, and TouchEndOrCancel...
> > 
> > It needs to be a union of these two. Previously it had all the information
in
> > the call context. So the possible changes are to 
> > store state in the ChromeClient (don't care to do this). Change the API in
> > setting all event listener properties so it is only
> > one call (more complicated change as we could pass an array of event
listener
> > states instead of individual calls) or add an API
> > to set the property for having touch listeners separately and query the
value
> > inside the EventHandlerRegistry.
> > 
> > I chose the later.
> 
> Ah, I see - thanks.  There's a 4th option: read the state from
> LayerTreeHost::event_listener_properties() whenever one of the touch states is
> changed.  This is cheap (it's just sitting there in a field).  You'd end up
> calling into WebView::SetHasTouchHandlers twice but that skips sending an IPC
if
> there's no change so I assume it's also trivially cheap.  Unless maybe we need
> to worry about cases where there is no LayerTreeHost yet?
> 
> Anyway I don't think this really matters, I'm fine with it as-is if you prefer
> that option.

The WebLayerTreeHost is optionally set; so that may not always work. I know it
would
work in release although I'm not sure in all of the unit tests.

[dtapuska, 2016-04-14 14:41:11]

https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.h (right):

https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.h:35:
TouchEventPassive,
On 2016/04/14 01:49:10, dtapuska wrote:
> On 2016/04/14 01:41:41, Rick Byers wrote:
> > Please rename these to TouchStartOrMove* for symmetry and to avoid
confusion.
> 
> Do you want the WebEventListenerClass enum renamed too?

Done.

https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/1884863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.idl:147: unsigned long
touchEventHandlerCount(Document document);
On 2016/04/14 01:41:41, Rick Byers wrote:
> nit: please rename this to make it clear it's only touchstart/move
> 
> Optionally while you're at it you could make both explicitly about "blocking"
> (or change the impl to return a sum of passive+blocking).

Done.

[Rick Byers, 2016-04-14 14:41:35]

If there's no layertreehost then a constant (true?) for has touch listeners
is probably fine (no composited scrolling possible, right?)

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Rick Byers, 2016-04-14 14:41:37]

If there's no layertreehost then a constant (true?) for has touch listeners
is probably fine (no composited scrolling possible, right?)

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dtapuska, 2016-04-14 16:50:44]

On 2016/04/14 14:41:37, Rick Byers wrote:
> If there's no layertreehost then a constant (true?) for has touch listeners
> is probably fine (no composited scrolling possible, right?)
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

done

[Rick Byers, 2016-04-14 17:15:18]

Thanks! LGTM

[dtapuska, 2016-04-14 18:59:05]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-04-14 18:59:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/1884863003/#ps80001
(title: "Remove API from ChromeClient")

[commit-bot: I haz the power, 2016-04-14 18:59:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884863003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884863003/80001

[dtapuska, 2016-04-14 19:00:32]

dtapuska@chromium.org changed reviewers:
+ creis@chromium.org

[dtapuska, 2016-04-14 19:00:57]

On 2016/04/14 18:59:20, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1884863003/80001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1884863003/80001

+creis for content/*

[commit-bot: I haz the power, 2016-04-14 19:10:52]

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

[commit-bot: I haz the power, 2016-04-14 19:10:54]

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

[Charlie Reis, 2016-04-14 19:48:55]

content/ LGTM.

[dtapuska, 2016-04-14 19:49:32]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2016-04-14 19:49:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884863003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884863003/80001

[commit-bot: I haz the power, 2016-04-14 19:56:33]

Description was changed from

==========
Non passive touch end or touch cancel listeners should not block scroll.

Ensure that touch end or touch cancel listeners don't block scroll by
removing these listeners from the touch hit test regions.

When no touch listener is encountered for TouchStart examine the touch
end/cancel listeners and determine if the touch start needs to be
dispatched non-blocking.

BUG=601990
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Non passive touch end or touch cancel listeners should not block scroll.

Ensure that touch end or touch cancel listeners don't block scroll by
removing these listeners from the touch hit test regions.

When no touch listener is encountered for TouchStart examine the touch
end/cancel listeners and determine if the touch start needs to be
dispatched non-blocking.

BUG=601990
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-04-14 19:56:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-14 19:58:13]

Description was changed from

==========
Non passive touch end or touch cancel listeners should not block scroll.

Ensure that touch end or touch cancel listeners don't block scroll by
removing these listeners from the touch hit test regions.

When no touch listener is encountered for TouchStart examine the touch
end/cancel listeners and determine if the touch start needs to be
dispatched non-blocking.

BUG=601990
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Non passive touch end or touch cancel listeners should not block scroll.

Ensure that touch end or touch cancel listeners don't block scroll by
removing these listeners from the touch hit test regions.

When no touch listener is encountered for TouchStart examine the touch
end/cancel listeners and determine if the touch start needs to be
dispatched non-blocking.

BUG=601990
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2016-04-14 19:58:15]

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