Bug 92346 – [Chromium-Android] We should hueristically detect content intents on touch

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

[Chromium-Android] We should hueristically detect content intents on touch

[Adam Barth]

Created attachment 154569 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Created attachment 154572 [details]
Patch

[Adam Barth]

This patch is written assuming https://gerrit-int.chromium.org/#change,22392 is going to land in something like it's current shape.

[Adam Barth]

I'm not sure whether the handle*Event functions in WebViewClient make sense.  This patch is mostly a starting point for discussion.

[Adam Barth]

*** Bug 90219 has been marked as a duplicate of this bug. ***

[Darin Fisher (:fishd, Google)]

Comment on attachment 154572 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154572&action=review

> Source/WebKit/chromium/public/WebInputEvent.h:156
> +        LinkPreviewTap   = 1 << 20,

It seems unfortunate that we would need to have this flag on WebInputEvent.
I would think that the link preview popup should be its own instance of
WebWidget if we need to handle input events on it separately.

> Source/WebKit/chromium/src/WebViewImpl.cpp:231
> +static WebColor tapHighlightColorForNode(const Node* node)

nit: recommend only using WebColor at the interface with the embedder.
return Color instead.

Shouldn't this be wrapped in a #if ENABLE(TOUCH_EVENTS) guard since
RenderStyle::initialTapHighlightColor is so guarded?

> Source/WebKit/chromium/src/WebViewImpl.cpp:723
> +        m_client->handleTapDownEvent(event.x, event.y);

it seems pretty unfortunate to squirrel through WebKit like this and then
duck out immediately.  it seems like it would be helpful to understand what
handle{Tap,TapDown,LongPress}Event do.

> Source/WebKit/chromium/src/WebViewImpl.cpp:728
> +            m_client->handleTapEvent(event.x, event.y, event.modifiers & WebInputEvent::LinkPreviewTap);

is this the only case where LinkPreviewTap is OR'd into the event modifiers?  maybe it
could just be a separate parameter to handleTapEvent or handleTapEvent could always
know that a link preview window was hit?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3859
> +    for (; node && !node->hasTagName(HTMLNames::bodyTag); node = node->parentNode()) {

it feels like much of this is being done instead of leveraging EventHandler
for gesture event dispatch.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3871
> +    WebContentDetectionResult content = m_client->detectContentAround(touchHit);

it seems odd to delegate to the embedder to perform this function.
it seems like we could have the detection algorithm live in webkit.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3875
> +    if (touchType == WebInputEvent::GestureLongPress) {

shouldn't this be a default action for the DOM element that was long-pressed?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3886
> +    m_client->scheduleContentIntent(content.intent());

how is this intent related to WebIntent?  i'm guessing this is an 
android intent, which suggests that we are going to have possible
confusion between android intents and web intents, especially if we
refer to each as intents.

[Adam Barth]

(In reply to comment #7)
> (From update of attachment 154572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154572&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:728
> > +            m_client->handleTapEvent(event.x, event.y, event.modifiers & WebInputEvent::LinkPreviewTap);
> 
> is this the only case where LinkPreviewTap is OR'd into the event modifiers?  maybe it
> could just be a separate parameter to handleTapEvent or handleTapEvent could always
> know that a link preview window was hit?

This is the only use of LinkPreviewTap in WebKit.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:3859
> > +    for (; node && !node->hasTagName(HTMLNames::bodyTag); node = node->parentNode()) {
> 
> it feels like much of this is being done instead of leveraging EventHandler
> for gesture event dispatch.

Yes.  What's happening here is that we're hacking into the beginning of event dispatching rather than catching the event at the end in a default event handler.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:3871
> > +    WebContentDetectionResult content = m_client->detectContentAround(touchHit);
> 
> it seems odd to delegate to the embedder to perform this function.
> it seems like we could have the detection algorithm live in webkit.

My understanding is that detecting content involves interacting with the rest of the system because installed apps can register new intents on Android.  That means it needs to happen outside the sandbox.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:3875
> > +    if (touchType == WebInputEvent::GestureLongPress) {
> 
> shouldn't this be a default action for the DOM element that was long-pressed?

Yeah, another approach to implementing this feature is via default event handlers.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:3886
> > +    m_client->scheduleContentIntent(content.intent());
> 
> how is this intent related to WebIntent? i'm guessing this is an 
> android intent, which suggests that we are going to have possible
> confusion between android intents and web intents, especially if we
> refer to each as intents.

Yes, this is an Android intent.  We can use the term AndroidIntent rather than ContentIntent if that's clearer.  The official name of intents on Android is android.content.intent.

[Alexandre Elias]

(In reply to comment #7)
> (From update of attachment 154572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154572&action=review
> 
> > Source/WebKit/chromium/public/WebInputEvent.h:156
> > +        LinkPreviewTap   = 1 << 20,
> 
> It seems unfortunate that we would need to have this flag on WebInputEvent.
> I would think that the link preview popup should be its own instance of
> WebWidget if we need to handle input events on it separately.

The flag approach also strikes me as questionable, but I don't think we want to introduce a new WebWidget either.  The tap event on the link disambiguation popup is almost entirely treated as a normal tap.  The only reason for the flag is that we need to avoid trying to generate another link disambiguation popup in the case where the tap appears ambiguous again.  Another way to tackle this would be to have the renderer remember that it requested a link disambiguation popup.

> it seems odd to delegate to the embedder to perform this function.
> it seems like we could have the detection algorithm live in webkit.

I earlier also asked Leandro to move this to WebKit, but he listed some hairy requirements preventing it.  I'll let Leandro explain them himself.

[Leandro Graciá Gil]

Comment on attachment 154572 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154572&action=review

>>> Source/WebKit/chromium/src/WebViewImpl.cpp:3871
>>> +    WebContentDetectionResult content = m_client->detectContentAround(touchHit);
>> 
>> it seems odd to delegate to the embedder to perform this function.
>> it seems like we could have the detection algorithm live in webkit.
> 
> My understanding is that detecting content involves interacting with the rest of the system because installed apps can register new intents on Android.  That means it needs to happen outside the sandbox.

There are a few reasons why we do need to delegate to the client.

First, as Adam said we need to get out of the sandbox and reach the browser and the Java side in order to be able to launch new Android intents in the system.
Besides that we have reasons concerning the detectors themselves. For example, in the case of address detection the parser is not trivial at all and we need it also to implement this API: http://developer.android.com/reference/android/webkit/WebView.html#findAddress(java.lang.String) . Unfortunately for various reasons the implementation cannot use directly any renderer code, not to mention WebKit, which forces us to have the parser in Chromium's content common code.
Phone number detection is also in a more or less similar situation. It uses the third party library libphonenumber in order to properly detect international and per-country phone number formats, which enforces again having detection outside WebKit.

As you see there are good reasons to delegate to the embedder. Please feel free to ask me any further questions you might have.

[Adam Barth]

Created attachment 155338 [details]
Patch

[Adam Barth]

This patch strips out the controversial parts of the patch.  Hopefully we can make progress incrementally by landing this patch and tackling the event integration issues in the next patch.

[Nate Chapin]

Comment on attachment 155338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155338&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:153
> +#include "platform/WebFloatQuad.h"

Nit: this is already included in the .h

[Adam Barth]

Created attachment 155603 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 155603 [details]
Patch for landing

Rejecting attachment 155603 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
n "['Tools/Scripts/build-webkit', '--debug', '--chromium', '--update-chromium']" exit_code: 2
rce/WebCore/WebCore.gyp/libwebcore_test_support.a
Source/WebKit/chromium/src/WebViewImpl.cpp: In member function 'bool WebKit::WebViewImpl::detectContentIntentOnTouch(const WebKit::WebPoint&, WebKit::WebInputEvent::Type)':
Source/WebKit/chromium/src/WebViewImpl.cpp:3826: error: 'type' was not declared in this scope
make: *** [out/Debug/obj.target/webkit/Source/WebKit/chromium/src/WebViewImpl.o] Error 1

Full output: http://queues.webkit.org/results/13386952

[Adam Barth]

Created attachment 155613 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 155613 [details]
Patch for landing

Clearing flags on attachment: 155613

Committed r124252: <http://trac.webkit.org/changeset/124252>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Leandro Graciá Gil]

Comment on attachment 155613 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=155613&action=review

> Source/WebKit/chromium/public/WebViewClient.h:374
>      // content detectors (e.g., from the Android intent system) to analyze the

There's no such a thing as "content detectors from the Android intent system". The content detectors live in Chromium, and their only relation with Android is that they fire Android intents if appropriate.

> Source/WebKit/chromium/public/WebViewClient.h:376
> +    virtual WebContentDetectionResult detectContentIntentAround(const WebHitTestResult&) { return WebContentDetectionResult(); }

Why was this method renamed to detectContentIntentAround? I think the name is plainly wrong: we don't detect content intents, but generate Android intents based on the content we detect around where the user taps. I'll be renaming this if you don't mind.

[Adam Barth]

(In reply to comment #19)
> (From update of attachment 155613 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155613&action=review
> 
> > Source/WebKit/chromium/public/WebViewClient.h:374
> >      // content detectors (e.g., from the Android intent system) to analyze the
> 
> There's no such a thing as "content detectors from the Android intent system". The content detectors live in Chromium, and their only relation with Android is that they fire Android intents if appropriate.

Ah, that was my misunderstanding.  I had assumed that Android provided library functions for detecting addresses and the like so that it would be consistent across the operating system.

> > Source/WebKit/chromium/public/WebViewClient.h:376
> > +    virtual WebContentDetectionResult detectContentIntentAround(const WebHitTestResult&) { return WebContentDetectionResult(); }
> 
> Why was this method renamed to detectContentIntentAround? I think the name is plainly wrong: we don't detect content intents, but generate Android intents based on the content we detect around where the user taps. I'll be renaming this if you don't mind.

Please do.  The issue here is that we're trying to draw a distinction between the intents from the Android system and Web Intents.  I looked at the documentation for the Intents in Android and learned that the full name was "android.content.intent" hence the name "content intent" as opposed to "web intent".  If that's a misnomer, we should fix it.