Bug 90219 – [Chromium] Teach WebView how to detect content at a given position

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

[Adam Barth]

[Chromium] Teach WebView how to detect content at a given position

[Adam Barth]

Created attachment 150039 [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.

[James Robinson]

Comment on attachment 150039 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.h:617
> +    bool detectContentAtPosition(const WebPoint&, bool scheduleIntent);

I'm not very familiar with web intents - what does the term "content" mean in this context?  Is this a term of art in intents?

[Adam Barth]

(In reply to comment #3)
> (From update of attachment 150039 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150039&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.h:617
> > +    bool detectContentAtPosition(const WebPoint&, bool scheduleIntent);
> 
> I'm not very familiar with web intents - what does the term "content" mean in this context?  Is this a term of art in intents?

It appears so:

http://developer.android.com/reference/android/content/Intent.html

My understanding is that an intent is something latent in a piece of content.  For example, a physical address is a piece of content that might contain the intent to view the location on a map, or an email address is a piece of content that might contain the intent to email that address.

Perhaps we should call this function detectContentIntentAtPosition or detectIntentAtPosition?  Someone more familiar with Android might be able to help us select the best name.

[Adam Barth]

Note: We should also be careful to distinguish between android.content.intent and WebIntents.

[Adam Barth]

Created attachment 150440 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 150440 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:3718
> +bool WebViewImpl::detectIntentAtPosition(const WebPoint& position, bool scheduleIntent)
> +{
> +    HitTestResult hitTestResult = hitTestResultForWindowPos(position);

Does WebPoint include all the touch information?  Does this need fancier hittesting for touch devices?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3721
> +    if (hitTestResult.isContentEditable())
> +        return false;

I thought some links worked in contentEditable?  I guess detected content doesn't though?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3725
> +    Node* node = hitTestResult.innerNode();
> +    if (!node || !node->isTextNode())
> +        return false;

Only text nodes can have content?  This may be worth a comment.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3731
> +    // FIXME: Ignore when tapping on links or nodes listening to click events.
> +
> +    WebContentDetectionResult detectedIntent = m_client->detectContentAround(hitTestResult);
> +    if (!detectedIntent.isValid())
> +        return false;

Do we have a way to represent a hittest back out to the embedder? I wonder if they shouldn't be driving this process?  This feels slightly inverted.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3739
> +    m_client->scheduleContentIntent(detectedIntent.intent());
> +    return true;

Yeah, this is more of the inversion feeling for me.  ideally schedulIntent wouuld be an enum in this case, but maybe Chromium's WebKit layer doesn't use enums for bools?

[Eric Seidel (no email)]

Comment on attachment 150440 [details]
Patch

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3739
>> +    return true;
> 
> Yeah, this is more of the inversion feeling for me.  ideally schedulIntent wouuld be an enum in this case, but maybe Chromium's WebKit layer doesn't use enums for bools?

OK.  After discussion in person, I'm no longer feeling inverted here.  This is called as part of hittesting in WebKit.  m_client is the embedder, but I agree that this is appropriate to do at the WebKit layer, and not in the embedder or WebCore.

[Eric Seidel (no email)]

Comment on attachment 150440 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:3733
> +    if (!scheduleIntent) {

I think making this an enum (With two values of SelectText vs. ScheduleIntentForText, or similar.)

[WebKit Review Bot]

Comment on attachment 150440 [details]
Patch

Attachment 150440 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13132167

New failing tests:
plugins/hidden-iframe-with-swf-plugin.html

[WebKit Review Bot]

Created attachment 150446 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Adam Barth]

(In reply to comment #7)
> (From update of attachment 150440 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150440&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:3721
> > +    if (hitTestResult.isContentEditable())
> > +        return false;
> 
> I thought some links worked in contentEditable?  I guess detected content doesn't though?

In content editable, you probably want to use tab and long tap for editing rather than for linkifying.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:3725
> > +    Node* node = hitTestResult.innerNode();
> > +    if (!node || !node->isTextNode())
> > +        return false;
> 
> Only text nodes can have content?  This may be worth a comment.

Will do.

[Adam Barth]

Created attachment 150539 [details]
Patch

[Adam Barth]

*** This bug has been marked as a duplicate of bug 92346 ***