Bug 92346

Created attachment 154569 [details] Patch

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.

Created attachment 154572 [details] Patch

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

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

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

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.

(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.

(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.

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.

Created attachment 155338 [details] Patch

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.

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

Created attachment 155603 [details] Patch for landing

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

Created attachment 155613 [details] Patch for landing

Comment on attachment 155613 [details] Patch for landing Clearing flags on attachment: 155613 Committed r124252: <http://trac.webkit.org/changeset/124252>

All reviewed patches have been landed. Closing bug.

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.

(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.