Bug 90219 - [Chromium] Teach WebView how to detect content at a given position
Summary: [Chromium] Teach WebView how to detect content at a given position
Status: RESOLVED DUPLICATE of bug 92346
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-06-28 16:06 PDT by Adam Barth
Modified: 2012-07-26 01:15 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.29 KB, patch)
2012-06-28 16:13 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (3.93 KB, patch)
2012-07-02 10:05 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (531.56 KB, application/zip)
2012-07-02 10:42 PDT, WebKit Review Bot
no flags Details
Patch (4.30 KB, patch)
2012-07-02 22:56 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-06-28 16:06:00 PDT
[Chromium] Teach WebView how to detect content at a given position
Comment 1 Adam Barth 2012-06-28 16:13:43 PDT
Created attachment 150039 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-28 16:15:53 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 James Robinson 2012-06-28 16:29:46 PDT
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?
Comment 4 Adam Barth 2012-06-28 16:41:54 PDT
(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.
Comment 5 Adam Barth 2012-06-28 16:42:43 PDT
Note: We should also be careful to distinguish between android.content.intent and WebIntents.
Comment 6 Adam Barth 2012-07-02 10:05:04 PDT
Created attachment 150440 [details]
Patch
Comment 7 Eric Seidel (no email) 2012-07-02 10:10:36 PDT
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?
Comment 8 Eric Seidel (no email) 2012-07-02 10:14:26 PDT
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.
Comment 9 Eric Seidel (no email) 2012-07-02 10:17:16 PDT
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.)
Comment 10 WebKit Review Bot 2012-07-02 10:42:27 PDT
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
Comment 11 WebKit Review Bot 2012-07-02 10:42:31 PDT
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
Comment 12 Adam Barth 2012-07-02 17:49:28 PDT
(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.
Comment 13 Adam Barth 2012-07-02 22:56:42 PDT
Created attachment 150539 [details]
Patch
Comment 14 Adam Barth 2012-07-26 01:14:58 PDT

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