Add WebKit API for doing a hit-test that mimics GestureTap

Add WebKit API for doing a hit-test that mimics GestureTap

Added blink::WebView::hitTestResultForTap to do so.
blink::WebView::hitTestResultAt will be removed in a follow up patch.

See also https://codereview.chromium.org/475633002/ on how chromium uses it.

BUG=403865
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185063

[hush (inactive), 2014-08-14 17:56:49]

PTAL

[abarth-chromium, 2014-08-14 20:51:03]

Why is the android_webview doing it's own hit testing?  Is there a design doc
that explains the context for this API?

[boliu, 2014-08-14 21:10:27]

On 2014/08/14 20:51:03, abarth wrote:
> Why is the android_webview doing it's own hit testing?  Is there a design doc
> that explains the context for this API?

One of the really really bad public APIs android webview inherited from the
pre-chromium version:
http://developer.android.com/reference/android/webkit/WebView.html#getHitTest...

The api assumed the webkit main thread was the UI thread (true at the time it
was written), so it's synchronous. We implemented it by doing hit tests on touch
down, and then send the result back to UI, just in case it is needed. All that
is done in android webview only code in chromium, but we still need this entry
point into blink.

[abarth-chromium, 2014-08-14 22:20:25]

On 2014/08/14 at 21:10:27, boliu wrote:
> On 2014/08/14 20:51:03, abarth wrote:
> > Why is the android_webview doing it's own hit testing?  Is there a design
doc
> > that explains the context for this API?
> 
> One of the really really bad public APIs android webview inherited from the
pre-chromium version:
>
http://developer.android.com/reference/android/webkit/WebView.html#getHitTest...
> 
> The api assumed the webkit main thread was the UI thread (true at the time it
was written), so it's synchronous. We implemented it by doing hit tests on touch
down, and then send the result back to UI, just in case it is needed. All that
is done in android webview only code in chromium, but we still need this entry
point into blink.

I wonder if there's a better approach.  For example, we already do hit tests on
touchstart.  We should be able to re-use the result instead of performing
another hit test.

[boliu, 2014-08-14 23:12:59]

On 2014/08/14 22:20:25, abarth wrote:
> On 2014/08/14 at 21:10:27, boliu wrote:
> > On 2014/08/14 20:51:03, abarth wrote:
> > > Why is the android_webview doing it's own hit testing?  Is there a design
> doc
> > > that explains the context for this API?
> > 
> > One of the really really bad public APIs android webview inherited from the
> pre-chromium version:
> >
>
http://developer.android.com/reference/android/webkit/WebView.html#getHitTest...
> > 
> > The api assumed the webkit main thread was the UI thread (true at the time
it
> was written), so it's synchronous. We implemented it by doing hit tests on
touch
> down, and then send the result back to UI, just in case it is needed. All that
> is done in android webview only code in chromium, but we still need this entry
> point into blink.
> 
> I wonder if there's a better approach.  For example, we already do hit tests
on
> touchstart.  We should be able to re-use the result instead of performing
> another hit test.

Yeah would be nice. Something similar to  WebViewClient::focusedNodeChanged
today.

I don't know the input system well enough to know if that always works though.
What if there are no touch handlers and the input stream is handled entirely on
the compositor thread? +jdduke might know?

[hush (inactive), 2014-08-14 23:20:08]

Hi Adam,
Any pointers (in the code) that Blink is already doing a hit test on TouchStart?
I will then store the hit test result (if it is not already stored), and read it
when
webview needs it.


On 2014/08/14 23:12:59, boliu wrote:
> On 2014/08/14 22:20:25, abarth wrote:
> > On 2014/08/14 at 21:10:27, boliu wrote:
> > > On 2014/08/14 20:51:03, abarth wrote:
> > > > Why is the android_webview doing it's own hit testing?  Is there a
design
> > doc
> > > > that explains the context for this API?
> > > 
> > > One of the really really bad public APIs android webview inherited from
the
> > pre-chromium version:
> > >
> >
>
http://developer.android.com/reference/android/webkit/WebView.html#getHitTest...
> > > 
> > > The api assumed the webkit main thread was the UI thread (true at the time
> it
> > was written), so it's synchronous. We implemented it by doing hit tests on
> touch
> > down, and then send the result back to UI, just in case it is needed. All
that
> > is done in android webview only code in chromium, but we still need this
entry
> > point into blink.
> > 
> > I wonder if there's a better approach.  For example, we already do hit tests
> on
> > touchstart.  We should be able to re-use the result instead of performing
> > another hit test.
> 
> Yeah would be nice. Something similar to  WebViewClient::focusedNodeChanged
> today.
> 
> I don't know the input system well enough to know if that always works though.
> What if there are no touch handlers and the input stream is handled entirely
on
> the compositor thread? +jdduke might know?

[abarth-chromium, 2014-08-14 23:49:11]

I'm sorry that I don't have bandwidth to help you with this CL.

[jdduke (slow), 2014-08-14 23:54:19]

On 2014/08/14 23:20:08, hush wrote:
> Any pointers (in the code) that Blink is already doing a hit test on
TouchStart?
> I will then store the hit test result (if it is not already stored), and read
it
> when
> webview needs it.

+Rick who has been working on some optimizations in this area and might be of
help.  Rick, it looks like WebView makes this query on every ACTION_DOWN event
(as an IPC trip outside of the normal touch pipeline), regardless of how the
event is dispatched to or consumed by the web content.

[Rick Byers, 2014-08-15 16:29:48]

On 2014/08/14 23:54:19, jdduke wrote:
> On 2014/08/14 23:20:08, hush wrote:
> > Any pointers (in the code) that Blink is already doing a hit test on
> TouchStart?
> > I will then store the hit test result (if it is not already stored), and
read
> it
> > when
> > webview needs it.
> 
> +Rick who has been working on some optimizations in this area and might be of
> help.  Rick, it looks like WebView makes this query on every ACTION_DOWN event
> (as an IPC trip outside of the normal touch pipeline), regardless of how the
> event is dispatched to or consumed by the web content.

Yeah we work hard to minimize hit tests, and in particular to cause only a
single hit test on touchstart.  The hit node for each touch is stored in
EventHandler::m_targetForTouchID (updated by EventHandler::handleTouchEvent). 
But you're right that we won't necessary reach this code path if there's no
touch handlers on the page (or CC decides there's definitely no handlers at this
point).

We also do a hit test at GestureTapDown time, but we'll never get this event if
the page consumed a touchstart.

The right behavior really depends on the semantics you want.  In particular, do
you want a "touch adjusted" hit test (which is what uses the TouchMajor to find
the best candidate target under the finger - use for the GestureTap* events and
ultimately 'click'), or a raw un-adjusted hit test (which is what touchstart
does)?

[boliu, 2014-08-15 16:57:40]

On 2014/08/15 16:29:48, Rick Byers wrote:
> On 2014/08/14 23:54:19, jdduke wrote:
> > On 2014/08/14 23:20:08, hush wrote:
> > > Any pointers (in the code) that Blink is already doing a hit test on
> > TouchStart?
> > > I will then store the hit test result (if it is not already stored), and
> read
> > it
> > > when
> > > webview needs it.
> > 
> > +Rick who has been working on some optimizations in this area and might be
of
> > help.  Rick, it looks like WebView makes this query on every ACTION_DOWN
event
> > (as an IPC trip outside of the normal touch pipeline), regardless of how the
> > event is dispatched to or consumed by the web content.
> 
> Yeah we work hard to minimize hit tests, and in particular to cause only a
> single hit test on touchstart.  The hit node for each touch is stored in
> EventHandler::m_targetForTouchID (updated by EventHandler::handleTouchEvent). 
> But you're right that we won't necessary reach this code path if there's no
> touch handlers on the page (or CC decides there's definitely no handlers at
this
> point).
> 
> We also do a hit test at GestureTapDown time, but we'll never get this event
if
> the page consumed a touchstart.

Sounds like neither of those will work in general. Maybe we just have to accept
this is one android webview api that comes with perf implications. It's not the
first :(

> 
> The right behavior really depends on the semantics you want.  In particular,
do
> you want a "touch adjusted" hit test (which is what uses the TouchMajor to
find
> the best candidate target under the finger - use for the GestureTap* events
and
> ultimately 'click'), or a raw un-adjusted hit test (which is what touchstart
> does)?

If I'm understanding correctly, android webview is doing "raw un-adjusted" hit
tests right now, and we want to change it to do "touch adjusted" hit tests.

The api is mostly used by apps for generating context menus after a long press,
so touch adjusted hit tests make more sense.

[hush (inactive), 2014-08-15 18:17:03]

To sum up a little:
1. EventHandler::handleTouchEvent is not always reached because it could be
consumed by chromium and not passed to blink. (relevant code is here I suppose:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...)
Also GestureTapDown handler may not always be reached either.

2. WebView wants to do "touch adjusted" hit tests with TouchMajor (that is the
bug we're trying to fix here.)
Because of #1, android webview cannot reliably use the touch result stored in
blink. So webview has to request a separate hitTestResult each time on
touchDown. 

3. Looking at the EventHandler::targetGestureEvent here
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
blink firstly request "hitTestResultAtPoint" with a touchRadius (WebView is
doing this now with this patch, seeWebViewImpl::hitTestResultForWindowPos). Then
the hitTestResult is adjusted by calling "applyTouchAdjustment". This step is
missing. 


On 2014/08/15 16:57:40, boliu wrote:
> On 2014/08/15 16:29:48, Rick Byers wrote:
> > On 2014/08/14 23:54:19, jdduke wrote:
> > > On 2014/08/14 23:20:08, hush wrote:
> > > > Any pointers (in the code) that Blink is already doing a hit test on
> > > TouchStart?
> > > > I will then store the hit test result (if it is not already stored), and
> > read
> > > it
> > > > when
> > > > webview needs it.
> > > 
> > > +Rick who has been working on some optimizations in this area and might be
> of
> > > help.  Rick, it looks like WebView makes this query on every ACTION_DOWN
> event
> > > (as an IPC trip outside of the normal touch pipeline), regardless of how
the
> > > event is dispatched to or consumed by the web content.
> > 
> > Yeah we work hard to minimize hit tests, and in particular to cause only a
> > single hit test on touchstart.  The hit node for each touch is stored in
> > EventHandler::m_targetForTouchID (updated by
EventHandler::handleTouchEvent). 
> > But you're right that we won't necessary reach this code path if there's no
> > touch handlers on the page (or CC decides there's definitely no handlers at
> this
> > point).
> > 
> > We also do a hit test at GestureTapDown time, but we'll never get this event
> if
> > the page consumed a touchstart.
> 
> Sounds like neither of those will work in general. Maybe we just have to
accept
> this is one android webview api that comes with perf implications. It's not
the
> first :(
> 
> > 
> > The right behavior really depends on the semantics you want.  In particular,
> do
> > you want a "touch adjusted" hit test (which is what uses the TouchMajor to
> find
> > the best candidate target under the finger - use for the GestureTap* events
> and
> > ultimately 'click'), or a raw un-adjusted hit test (which is what touchstart
> > does)?
> 
> If I'm understanding correctly, android webview is doing "raw un-adjusted" hit
> tests right now, and we want to change it to do "touch adjusted" hit tests.
> 
> The api is mostly used by apps for generating context menus after a long
press,
> so touch adjusted hit tests make more sense.

[abarth-chromium, 2014-08-15 18:24:06]

It's not clear to me whether we should land this hacky approach or whether we
should wait for the proper design.  Can you give me some sense of the priority
for resolving this issue?  Clearly we've been shipping android_webview for a
while and if this change is just to service a legacy andrdoid_webview API, I'd
be inclined to wait until we can implement this properly.

[blink-reviews, 2014-08-15 18:36:25]

This bug is not a regression and it was reported in January, so it is in
KitKat already. And the approach has been hacky since we released the API.
I am trying to get it to m37 because an internal team is having trouble
with it. But the priority is not super high, so if I can't get it done
before m37, I can wait for m38.


On Fri, Aug 15, 2014 at 11:24 AM, <abarth@chromium.org> wrote:

> It's not clear to me whether we should land this hacky approach or whether
> we
> should wait for the proper design.  Can you give me some sense of the
> priority
> for resolving this issue?  Clearly we've been shipping android_webview for
> a
> while and if this change is just to service a legacy andrdoid_webview API,
> I'd
> be inclined to wait until we can implement this properly.
>
> https://codereview.chromium.org/470833002/
>

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

[abarth-chromium, 2014-08-15 18:39:20]

Let's wait then.

[Rick Byers, 2014-08-15 20:13:30]

On 2014/08/15 18:17:03, hush wrote:
> To sum up a little:
> 1. EventHandler::handleTouchEvent is not always reached because it could be
> consumed by chromium and not passed to blink. (relevant code is here I
suppose:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...)
> Also GestureTapDown handler may not always be reached either.
> 
> 2. WebView wants to do "touch adjusted" hit tests with TouchMajor (that is the
> bug we're trying to fix here.)
> Because of #1, android webview cannot reliably use the touch result stored in
> blink. So webview has to request a separate hitTestResult each time on
> touchDown. 

Having a WebView API to request a touch-adjusted hit-test seems completely
reasonable to me.  In fact I think this process should ultimately be web-exposed
(though perhaps through some lower-level primitives that allow more control).

Note though that there are still subtle ways that your hit test may differ from
what the user activates with a tap / longpress.  Eg. if the DOM changes between
the time you to your hit test, and the ones we ultimately rely on for the click
(eg. significant dom changes in mousemove handlers are unfortunately common -
see http://crbug.com/396652).  Worse there are issues with the nodes returned
from touch adjustment (which we inherited from WebKit) which were masked by our
redundant hit tests and so for now you might actually need to hit-test a second
time at the adjusted position to get the right node (see
http://crbug.com/398914).

I'm working to fix all this, but it's turning into a big job with lots of dark
corners and surprises.  Sadly touch adjustment is still somewhat of a messy set
of heuristics.  I think it needs a major overhaul at some point (which probably
requires some rigorous system for validating the quality of it's results on real
content).

> 3. Looking at the EventHandler::targetGestureEvent here
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> blink firstly request "hitTestResultAtPoint" with a touchRadius (WebView is
> doing this now with this patch, seeWebViewImpl::hitTestResultForWindowPos).
Then
> the hitTestResult is adjusted by calling "applyTouchAdjustment". This step is
> missing. 

Exactly.  Splitting targetGestureEvent up so that you can reuse the 'hit test +
adjust' portion in your new WebView method makes sense to me.

Note that applyTouchAdjustment runs in one of two modes depending on the event
in question, as it has to decide which nodes are likely targets of the events. 
Either those activated by tapping, or those activated by long pressing.  Your
new method should perhaps let the caller decide which mode it wants, while also
making it clear that it's a special sort of hit test. 

> 
> 
> On 2014/08/15 16:57:40, boliu wrote:
> > On 2014/08/15 16:29:48, Rick Byers wrote:
> > > On 2014/08/14 23:54:19, jdduke wrote:
> > > > On 2014/08/14 23:20:08, hush wrote:
> > > > > Any pointers (in the code) that Blink is already doing a hit test on
> > > > TouchStart?
> > > > > I will then store the hit test result (if it is not already stored),
and
> > > read
> > > > it
> > > > > when
> > > > > webview needs it.
> > > > 
> > > > +Rick who has been working on some optimizations in this area and might
be
> > of
> > > > help.  Rick, it looks like WebView makes this query on every ACTION_DOWN
> > event
> > > > (as an IPC trip outside of the normal touch pipeline), regardless of how
> the
> > > > event is dispatched to or consumed by the web content.
> > > 
> > > Yeah we work hard to minimize hit tests, and in particular to cause only a
> > > single hit test on touchstart.  The hit node for each touch is stored in
> > > EventHandler::m_targetForTouchID (updated by
> EventHandler::handleTouchEvent). 
> > > But you're right that we won't necessary reach this code path if there's
no
> > > touch handlers on the page (or CC decides there's definitely no handlers
at
> > this
> > > point).
> > > 
> > > We also do a hit test at GestureTapDown time, but we'll never get this
event
> > if
> > > the page consumed a touchstart.
> > 
> > Sounds like neither of those will work in general. Maybe we just have to
> accept
> > this is one android webview api that comes with perf implications. It's not
> the
> > first :(
> > 
> > > 
> > > The right behavior really depends on the semantics you want.  In
particular,
> > do
> > > you want a "touch adjusted" hit test (which is what uses the TouchMajor to
> > find
> > > the best candidate target under the finger - use for the GestureTap*
events
> > and
> > > ultimately 'click'), or a raw un-adjusted hit test (which is what
touchstart
> > > does)?
> > 
> > If I'm understanding correctly, android webview is doing "raw un-adjusted"
hit
> > tests right now, and we want to change it to do "touch adjusted" hit tests.
> > 
> > The api is mostly used by apps for generating context menus after a long
> press,
> > so touch adjusted hit tests make more sense.

[Rick Byers, 2014-08-15 20:19:10]

By the way, if you're interested in more details/demos of how touch adjustment
behaves - see https://plus.sandbox.google.com/+RickByers/posts/TzmJkM3HdCt and
the links from there.

[hush (inactive), 2014-08-15 21:35:36]

Thanks for Rick for the comments!
From android webview's perspective, it just needs to call the right blink
functions (HitTestResultAtPoint + applyTouchAdjustment) and is on par with what
blink does, including all the subtleties and dark corners. (And get your work on
improving touch adjustments for free)

I think we're passed the deadline to add a new parameter in the android webview
getHitTestResult API to decide what kind of mode the user wants for the
hitTestResult (longpress vs tap). But I suppose webview can guess what mode it
is based on the most recent touch event, whether it is a longpress or tap.

The work to do:
1. Splitting targetGestureEvent up so that webview can reuse the 'hit test +
adjust' portion
2. webview needs to figure out what mode of HitTestResult the user wants.

[Rick Byers, 2014-08-18 16:39:33]

On 2014/08/15 21:35:36, hush wrote:
> Thanks for Rick for the comments!
> From android webview's perspective, it just needs to call the right blink
> functions (HitTestResultAtPoint + applyTouchAdjustment) and is on par with
what
> blink does, including all the subtleties and dark corners. (And get your work
on
> improving touch adjustments for free)
>
> I think we're passed the deadline to add a new parameter in the android
webview
> getHitTestResult API to decide what kind of mode the user wants for the
> hitTestResult (longpress vs tap).

Right, I'm not saying this needs to be exposed to the android WebView API, but
on the blink::WebView API you add (too many "WebView"s here <grin>).  Blink
shouldn't be having to guess this sort of thing.
  
> But I suppose webview can guess what mode it
> is based on the most recent touch event, whether it is a longpress or tap.

You said you expect to do the hit test on touch start, right?  You can't tell at
that point whether it's going to be a tap or a long press.  Perhaps you should
just always use tap mode (if that's the common use case).

> 
> The work to do:
> 1. Splitting targetGestureEvent up so that webview can reuse the 'hit test +
> adjust' portion
> 2. webview needs to figure out what mode of HitTestResult the user wants.

[Rick Byers, 2014-10-27 21:59:36]

If we're more principled with the approach, I think we can proceed down this
path (see discussion in the bug).  Also please add some basic tests that
validate the difference in behavior.

https://codereview.chromium.org/470833002/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/470833002/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3250: {
This needs to share code with EventHandler::targetGestureEvent - in particular
between line 2586 ("perform the rect-based hit test") and line 2616 (just above
"Now apply hover/active").  Please factor that code out to it's own function -
eg. maybe 
HitTestResult EventHandler::hitTestForGestureEvent(const PlatformEvent::Type
type, const IntPoint& windowPosition, const IntSize& area)

Or maybe it's better to take just a PlatformGestureEvent (keeping functions like
EventHandler::applyTouchAdjustment unmodified) and require WebViewImpl to create
a PlatformGestureEvent instance.

https://codereview.chromium.org/470833002/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3254: HitTestResult
WebViewImpl::coreHitTestResultAt(const WebPoint& point, const WebSize& padding)
I think we should avoid changing this function - the use cases are too
different.

https://codereview.chromium.org/470833002/diff/20001/public/web/WebView.h
File public/web/WebView.h (right):

https://codereview.chromium.org/470833002/diff/20001/public/web/WebView.h#new...
public/web/WebView.h:326: // Do a hit test at given point with padding around
the point
Let's raise the level of abstraction here so that blink implementation details
(like when touch adjustment is enabled or not) remains encapsulated.  How about
something like:

// Do a hit test equivalent to what would be done for a GestureTap event that
has width/height corresponding to the supplied tapArea.
virtual WebHitTestResult hitTestResultForTap(const WebPoint&, const WebSize&
tapArea) = 0;

[Rick Byers, 2014-10-27 23:39:33]

On 2014/10/27 21:59:36, Rick Byers (Slow until 10-30) wrote:
> If we're more principled with the approach, I think we can proceed down this
> path (see discussion in the bug).  Also please add some basic tests that
> validate the difference in behavior.
> 
>
https://codereview.chromium.org/470833002/diff/20001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/470833002/diff/20001/Source/web/WebViewImpl.c...
> Source/web/WebViewImpl.cpp:3250: {
> This needs to share code with EventHandler::targetGestureEvent - in particular
> between line 2586 ("perform the rect-based hit test") and line 2616 (just
above
> "Now apply hover/active").  Please factor that code out to it's own function -
> eg. maybe 
> HitTestResult EventHandler::hitTestForGestureEvent(const PlatformEvent::Type
> type, const IntPoint& windowPosition, const IntSize& area)
> 
> Or maybe it's better to take just a PlatformGestureEvent (keeping functions
like
> EventHandler::applyTouchAdjustment unmodified) and require WebViewImpl to
create
> a PlatformGestureEvent instance.
> 
>
https://codereview.chromium.org/470833002/diff/20001/Source/web/WebViewImpl.c...
> Source/web/WebViewImpl.cpp:3254: HitTestResult
> WebViewImpl::coreHitTestResultAt(const WebPoint& point, const WebSize&
padding)
> I think we should avoid changing this function - the use cases are too
> different.
> 
> https://codereview.chromium.org/470833002/diff/20001/public/web/WebView.h
> File public/web/WebView.h (right):
> 
>
https://codereview.chromium.org/470833002/diff/20001/public/web/WebView.h#new...
> public/web/WebView.h:326: // Do a hit test at given point with padding around
> the point
> Let's raise the level of abstraction here so that blink implementation details
> (like when touch adjustment is enabled or not) remains encapsulated.  How
about
> something like:
> 
> // Do a hit test equivalent to what would be done for a GestureTap event that
> has width/height corresponding to the supplied tapArea.
> virtual WebHitTestResult hitTestResultForTap(const WebPoint&, const WebSize&
> tapArea) = 0;

Also please update the CL description, to reflect this goal - eg: "Add WebKit
API for doing a hit-test that mimics GestureTap behavior"

[hush (inactive), 2014-10-28 22:48:16]

https://codereview.chromium.org/470833002/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/470833002/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3250: {
On 2014/10/27 21:59:35, Rick Byers (Slow until 10-30) wrote:
> This needs to share code with EventHandler::targetGestureEvent - in particular
> between line 2586 ("perform the rect-based hit test") and line 2616 (just
above
> "Now apply hover/active").  Please factor that code out to it's own function -
> eg. maybe 
> HitTestResult EventHandler::hitTestForGestureEvent(const PlatformEvent::Type
> type, const IntPoint& windowPosition, const IntSize& area)
> 
> Or maybe it's better to take just a PlatformGestureEvent (keeping functions
like
> EventHandler::applyTouchAdjustment unmodified) and require WebViewImpl to
create
> a PlatformGestureEvent instance.

I created a function in EventHandler that takes a PlatformGestureEvent and a
hitType.
WebViewImpl is responsible for creating the PlatformGestureEvent object.

https://codereview.chromium.org/470833002/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:3254: HitTestResult
WebViewImpl::coreHitTestResultAt(const WebPoint& point, const WebSize& padding)
On 2014/10/27 21:59:35, Rick Byers (Slow until 10-30) wrote:
> I think we should avoid changing this function - the use cases are too
> different.

Yes. It looks like both coreHitTestResultAt and  hitTestResultForWindowPos are
used in other places in the code, in different use cases. So I leave them
untouched.

I just added hitTestResultForTap.

https://codereview.chromium.org/470833002/diff/20001/public/web/WebView.h
File public/web/WebView.h (right):

https://codereview.chromium.org/470833002/diff/20001/public/web/WebView.h#new...
public/web/WebView.h:326: // Do a hit test at given point with padding around
the point
On 2014/10/27 21:59:35, Rick Byers (Slow until 10-30) wrote:
> Let's raise the level of abstraction here so that blink implementation details
> (like when touch adjustment is enabled or not) remains encapsulated.  How
about
> something like:
> 
> // Do a hit test equivalent to what would be done for a GestureTap event that
> has width/height corresponding to the supplied tapArea.
> virtual WebHitTestResult hitTestResultForTap(const WebPoint&, const WebSize&
> tapArea) = 0;

Done.

[hush (inactive), 2014-10-28 22:52:25]

PTAL!

https://codereview.chromium.org/470833002/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/470833002/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4071: // EventHandler does not care about the global
position.
What should be the correct global position here for gestureEvent?

https://codereview.chromium.org/470833002/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4077:
result.setToShadowHostIfInUserAgentShadowRoot();
Not sure about what this line does, but hitTestResultForWindowPos does this, so
I copied it here.

[hush (inactive), 2014-11-04 03:06:29]

PS4:
Sorry I missed the scaling logic in PS3.
EventHandler::hitTestResultForGestureEvent eventually calls
EventHandler::hitTestResultAtPoint. The latter assumes the tap point and tap
padding are already scaled by 1/pageScaleFactor(). 
WebViewImpl::coreHitTestResultAt does the scaling before it calls
EventHandler::hitTestResultAtPoint, which was the implementation before this
patch.

[hush (inactive), 2014-11-04 03:13:25]

Patchset #4 (id:60001) has been deleted

[Rick Byers, 2014-11-04 06:09:19]

Sorry for the delay - I've been (and still am) travelling.

This looks good - nice and simple.  Please add a test (probably a WebView unit
test - see for example WebViewTest::HitTestResultAtWithPageScale for a similar
related test.  You'll want a test case where touch adjustment is actually having
an inpact (eg. tap just outside a link and verify the that the new hit test
function returns the link, but the old one does not).

https://codereview.chromium.org/470833002/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/470833002/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4071: // EventHandler does not care about the global
position.
On 2014/10/28 22:52:25, hush wrote:
> What should be the correct global position here for gestureEvent?

In the general tap case, the screen position can be passed through to JavaScript
(eg. as the MouseEvent.screen* properties).  In this case since you're doing
only a hit test, it should be completely unused.  I'd set it to 0,0 instead of
something that looks real.

Also strictly EventHandler does care (in the sense that it'll pass them
through), but the hit-testing phase doesn't.  So replace "EventHandler" with
"hit testing".

https://codereview.chromium.org/470833002/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4077:
result.setToShadowHostIfInUserAgentShadowRoot();
On 2014/10/28 22:52:25, hush wrote:
> Not sure about what this line does, but hitTestResultForWindowPos does this,
so
> I copied it here.

You do want this.  It means, for example, that if a user taps on a form element,
you'll get the <input> element returned, not one of the hidden elements inside
of it used to implement <input> under the covers.

[hush (inactive), 2014-11-04 22:42:39]

https://codereview.chromium.org/470833002/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/470833002/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4071: // EventHandler does not care about the global
position.
On 2014/11/04 06:09:19, Rick Byers (Slow until 11-6) wrote:
> On 2014/10/28 22:52:25, hush wrote:
> > What should be the correct global position here for gestureEvent?
> 
> In the general tap case, the screen position can be passed through to
JavaScript
> (eg. as the MouseEvent.screen* properties).  In this case since you're doing
> only a hit test, it should be completely unused.  I'd set it to 0,0 instead of
> something that looks real.
> 
> Also strictly EventHandler does care (in the sense that it'll pass them
> through), but the hit-testing phase doesn't.  So replace "EventHandler" with
> "hit testing".

Done

[hush (inactive), 2014-11-04 22:52:09]

Added a new hit_test.html file for the unit test.
Please note that I have to put the img inside an <a> tag, because the touch
adjustment method for GestureTap is "findBestClickableCandidate". And if the
test html page only has an image without link, it is considered not clickable
and touch adjustment result will not have the img tag.

[Rick Byers, 2014-11-07 03:33:47]

On 2014/11/04 22:52:09, hush wrote:
> Added a new hit_test.html file for the unit test.
> Please note that I have to put the img inside an <a> tag, because the touch
> adjustment method for GestureTap is "findBestClickableCandidate". And if the
> test html page only has an image without link, it is considered not clickable
> and touch adjustment result will not have the img tag.

Makes sense.  Note you can accomplish the same thing by adding an onclick
handler to the img, but this is fine too.  Thanks for the good test!

This LGTM with one nit.

[Rick Byers, 2014-11-07 03:34:08]

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

[Rick Byers, 2014-11-07 03:34:26]

+aelias for Source/web OWNERS

https://codereview.chromium.org/470833002/diff/100001/Source/web/tests/WebVie...
File Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/470833002/diff/100001/Source/web/tests/WebVie...
Source/web/tests/WebViewTest.cpp:417: WebHitTestResult negativeResult =
toWebViewImpl(webView)->coreHitTestResultAt(hitPoint);
nit: why change these?  This is supposed to be a unit test of the WebView API -
no reason to dig into the WebViewImpl implementation details I think?  Or are
you still thinking that we might want to remove the hitTestResultAt API?

https://codereview.chromium.org/470833002/diff/100001/Source/web/tests/WebVie...
Source/web/tests/WebViewTest.cpp:439: WebHitTestResult negativeResult =
toWebViewImpl(webView)->coreHitTestResultAt(hitPoint);
again, assuming we don't want to remove the point-based API, we should test
against WebView instead of WebViewImpl here.

[aelias_OOO_until_Jul13, 2014-11-07 03:41:37]

https://codereview.chromium.org/470833002/diff/100001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/470833002/diff/100001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:4072: scaledTapPoint.scale(1 / pageScaleFactor(), 1 /
pageScaleFactor());
Could you use PlatformGestureEventBuilder instead of rolling your own scaling? 
You would just need to generate a synthetic WebGestureEvent instead of a
synthetic PlatformGestureEvent.

[hush (inactive), 2014-11-07 19:13:37]

https://codereview.chromium.org/470833002/diff/100001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/470833002/diff/100001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:4072: scaledTapPoint.scale(1 / pageScaleFactor(), 1 /
pageScaleFactor());
On 2014/11/07 03:41:37, aelias wrote:
> Could you use PlatformGestureEventBuilder instead of rolling your own scaling?

> You would just need to generate a synthetic WebGestureEvent instead of a
> synthetic PlatformGestureEvent.

Hi Alex, it looks like I don't get scaling done "for free" when I use
PlatformGestureEventBuilder. I still need to roll my own scaling. There is an
example of doing this in WebViewImpl::handleGestureEvent.
What do you think?

https://codereview.chromium.org/470833002/diff/100001/Source/web/tests/WebVie...
File Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/470833002/diff/100001/Source/web/tests/WebVie...
Source/web/tests/WebViewTest.cpp:417: WebHitTestResult negativeResult =
toWebViewImpl(webView)->coreHitTestResultAt(hitPoint);
On 2014/11/07 03:34:25, Rick Byers (OOO 11-7) wrote:
> nit: why change these?  This is supposed to be a unit test of the WebView API
-
> no reason to dig into the WebViewImpl implementation details I think?  Or are
> you still thinking that we might want to remove the hitTestResultAt API?

Yes. I'm thinking we just remove the hitTestResultAt API. The new API covers the
single point hit tests by passing a 0x0 tapArea. Actually I am removing all the
usages of the old API in chromium code here:
https://codereview.chromium.org/475633002/

What do you think? Is there any reason we should keep the old API?

https://codereview.chromium.org/470833002/diff/100001/Source/web/tests/WebVie...
Source/web/tests/WebViewTest.cpp:439: WebHitTestResult negativeResult =
toWebViewImpl(webView)->coreHitTestResultAt(hitPoint);
On 2014/11/07 03:34:25, Rick Byers (OOO 11-7) wrote:
> again, assuming we don't want to remove the point-based API, we should test
> against WebView instead of WebViewImpl here.

Please see the other comment..

[aelias_OOO_until_Jul13, 2014-11-07 22:38:13]

https://codereview.chromium.org/470833002/diff/100001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/470833002/diff/100001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:4072: scaledTapPoint.scale(1 / pageScaleFactor(), 1 /
pageScaleFactor());
On 2014/11/07 19:13:37, hush wrote:
> On 2014/11/07 03:41:37, aelias wrote:
> > Could you use PlatformGestureEventBuilder instead of rolling your own
scaling?
> 
> > You would just need to generate a synthetic WebGestureEvent instead of a
> > synthetic PlatformGestureEvent.
> 
> Hi Alex, it looks like I don't get scaling done "for free" when I use
> PlatformGestureEventBuilder. I still need to roll my own scaling. There is an
> example of doing this in WebViewImpl::handleGestureEvent.
> What do you think?

You should get it for free, that's what the scaleSizeToWindow and
convertHitPointToWindow calls do.  WebViewImpl::handleGestureEvent rolls its own
scaling only for some side use cases.

[hush (inactive), 2014-11-07 23:38:42]

https://codereview.chromium.org/470833002/diff/100001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/470833002/diff/100001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:4072: scaledTapPoint.scale(1 / pageScaleFactor(), 1 /
pageScaleFactor());
On 2014/11/07 22:38:13, aelias wrote:
> On 2014/11/07 19:13:37, hush wrote:
> > On 2014/11/07 03:41:37, aelias wrote:
> > > Could you use PlatformGestureEventBuilder instead of rolling your own
> scaling?
> > 
> > > You would just need to generate a synthetic WebGestureEvent instead of a
> > > synthetic PlatformGestureEvent.
> > 
> > Hi Alex, it looks like I don't get scaling done "for free" when I use
> > PlatformGestureEventBuilder. I still need to roll my own scaling. There is
an
> > example of doing this in WebViewImpl::handleGestureEvent.
> > What do you think?
> 
> You should get it for free, that's what the scaleSizeToWindow and
> convertHitPointToWindow calls do.  WebViewImpl::handleGestureEvent rolls its
own
> scaling only for some side use cases.

Yeah. Thanks for the explanation! I uploaded a patch with
"PlatformGestureEventBuilder". And I verified that the touch positions are
scaled by the builder.
Also I added a test case in WebViewTest to make sure hitTestResultForTap does
not ignore the page scale.

[aelias_OOO_until_Jul13, 2014-11-08 00:07:06]

Source/web lgtm

[Rick Byers, 2014-11-10 18:43:56]

> On 2014/11/07 03:34:25, Rick Byers (OOO 11-7) wrote:
> > nit: why change these?  This is supposed to be a unit test of the WebView
API
> -
> > no reason to dig into the WebViewImpl implementation details I think?  Or
are
> > you still thinking that we might want to remove the hitTestResultAt API?
> 
> Yes. I'm thinking we just remove the hitTestResultAt API. The new API covers
the
> single point hit tests by passing a 0x0 tapArea. Actually I am removing all
the
> usages of the old API in chromium code here:
> https://codereview.chromium.org/475633002/
> 
> What do you think? Is there any reason we should keep the old API?

I think we should probably leave the existing API in place.  The two APIs have a
different contract (even though the implementation means passing an area of 0,0
to one is the same as calling the other).  One API does a simple point-based
hit-test, and one API does a hit-test mimicking a tap at a particular
location+area.  How their implementations differ is an implementation detail of
blink.

[hush (inactive), 2014-11-10 19:10:48]

On 2014/11/10 18:43:56, Rick Byers wrote:
> > On 2014/11/07 03:34:25, Rick Byers (OOO 11-7) wrote:
> > > nit: why change these?  This is supposed to be a unit test of the WebView
> API
> > -
> > > no reason to dig into the WebViewImpl implementation details I think?  Or
> are
> > > you still thinking that we might want to remove the hitTestResultAt API?
> > 
> > Yes. I'm thinking we just remove the hitTestResultAt API. The new API covers
> the
> > single point hit tests by passing a 0x0 tapArea. Actually I am removing all
> the
> > usages of the old API in chromium code here:
> > https://codereview.chromium.org/475633002/
> > 
> > What do you think? Is there any reason we should keep the old API?
> 
> I think we should probably leave the existing API in place.  The two APIs have
a
> different contract (even though the implementation means passing an area of
0,0
> to one is the same as calling the other).  One API does a simple point-based
> hit-test, and one API does a hit-test mimicking a tap at a particular
> location+area.  How their implementations differ is an implementation detail
of
> blink.

Okay. I will just leave the old API alone. And also use it in the unit tests.
See PS7.

[Rick Byers, 2014-11-10 20:09:59]

lgtm with nit

https://codereview.chromium.org/470833002/diff/140001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/470833002/diff/140001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:4078: tapEvent.data.tap.width = tapArea.width;
nit: set tapEvent.data.tap.tapCount = 1 for consistency with real scenarios.

[hush (inactive), 2014-11-10 20:21:52]

On 2014/11/10 20:09:59, Rick Byers wrote:
> lgtm with nit
> 
>
https://codereview.chromium.org/470833002/diff/140001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/470833002/diff/140001/Source/web/WebViewImpl....
> Source/web/WebViewImpl.cpp:4078: tapEvent.data.tap.width = tapArea.width;
> nit: set tapEvent.data.tap.tapCount = 1 for consistency with real scenarios.

Done.

[hush (inactive), 2014-11-10 20:22:12]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2014-11-10 20:23:24]

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

[commit-bot: I haz the power, 2014-11-10 21:30:08]

Committed patchset #8 (id:160001) as 185063