Bug 82460

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

Comment on attachment 134291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134291&action=review > Source/WebKit/chromium/public/WebHitTestInfo.h:46 > + WebHitTestInfo(); > + WebHitTestInfo(const WebHitTestInfo&); Do we need to mark these functions as WEBKIT_EXPORT? (I'm not sure.) > Source/WebKit/chromium/public/WebView.h:437 > +#if defined(ANDROID) Why is this specific to Android? Presumably the API makes sense on other platforms as well.

Comment on attachment 134291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134291&action=review This generally looks good. I have a couple minor questions below. > Source/WebKit/chromium/src/WebSurroundingText.cpp:57 > + if (!node->inDocument() && element && element->inDocument()) How can a node's parent be inDocument() without the node itself being inDocument() ? > Source/WebKit/chromium/src/WebSurroundingText.cpp:59 > + m_private.reset(new SurroundingText(node->renderer()->positionForPoint(hitTestInfo.point()), maxLength)); Won't this crash if the node doesn't have a renderer? Maybe that's impossible if the node is in a HitTestInfo (but then when do you need to check whether the node is inDocument)? I wonder if you mean to be null-checking renderer() rather than checking inDocument(). > Source/WebKit/chromium/src/WebViewImpl.cpp:3470 > +#if OS(ANDROID) Again this shouldn't be Android-specific.

(Also, I'm going to ask fishd to double-check the review of the WebKit API aspects of this change. I'm still learning how to review those changes properly.)

Created attachment 134369 [details] Patch

Comment on attachment 134291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134291&action=review >> Source/WebKit/chromium/public/WebHitTestInfo.h:46 >> + WebHitTestInfo(const WebHitTestInfo&); > > Do we need to mark these functions as WEBKIT_EXPORT? (I'm not sure.) I've never seen the constructors of WebKit embedders using WEBKIT_EXPORT, but I'm not 100% sure either. >> Source/WebKit/chromium/public/WebView.h:437 >> +#if defined(ANDROID) > > Why is this specific to Android? Presumably the API makes sense on other platforms as well. Fixed. >> Source/WebKit/chromium/src/WebSurroundingText.cpp:57 >> + if (!node->inDocument() && element && element->inDocument()) > > How can a node's parent be inDocument() without the node itself being inDocument() ? This actually came from a code snippet in WebCore that said it was the correct way to do this. However, it seems it wasn't so correct since I can't find it anymore. >> Source/WebKit/chromium/src/WebSurroundingText.cpp:59 >> + m_private.reset(new SurroundingText(node->renderer()->positionForPoint(hitTestInfo.point()), maxLength)); > > Won't this crash if the node doesn't have a renderer? Maybe that's impossible if the node is in a HitTestInfo (but then when do you need to check whether the node is inDocument)? I wonder if you mean to be null-checking renderer() rather than checking inDocument(). I'm following your suggestion. It makes more sense than the code snippet this was based on. >> Source/WebKit/chromium/src/WebViewImpl.cpp:3470 >> +#if OS(ANDROID) > > Again this shouldn't be Android-specific. Fixed.

Comment on attachment 134369 [details] Patch Attachment 134369 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12159271

Created attachment 134400 [details] Patch

(In reply to comment #9) > Created an attachment (id=134400) [details] > Patch Fixing the build issues in the bot.

(In reply to comment #10) > (In reply to comment #9) > > Created an attachment (id=134400) [details] [details] > > Patch > > Fixing the build issues in the bot. Any reviews on this when possible? Thanks.

Created attachment 134829 [details] Patch

(In reply to comment #12) > Created an attachment (id=134829) [details] > Patch Updated the exported names of some methods in WebSurroundingText to match the names in WebCore's SurroundingText after the review changes (hitOffsetInContent -> positionOffsetInContent and minor variable name changes).

Comment on attachment 134829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134829&action=review > Source/WebKit/chromium/public/WebFrame.h:439 > + virtual void selectRange(const WebRange&) = 0; should we deprecate the former selectRange method in terms of this one? > Source/WebKit/chromium/public/WebHitTestInfo.h:45 > + WebHitTestInfo(); these will break the shared library build of chrome if not tagged with WEBKIT_EXPORT, but we generally do not export constructors and destructors. you should make these in terms of assign and reset methods. see WebNode.h > Source/WebKit/chromium/public/WebHitTestInfo.h:56 > + WEBKIT_EXPORT WebNode node() const; what if more than one node got hit? > Source/WebKit/chromium/public/WebSurroundingText.h:43 > + WebSurroundingText(); ditto > Source/WebKit/chromium/public/WebWidget.h:173 > + virtual WebHitTestInfo hitTestInfoForWindowPos(const WebPoint&) { return WebHitTestInfo(); } since hit testing returns dom nodes, i think this API belongs on WebView. do you need it to live on WebWidget? > Source/WebKit/chromium/src/WebViewImpl.cpp:3470 > +WebVector<WebFloatQuad> WebViewImpl::getTouchHighlightQuads(const WebRange& webRange, WebColor& outTapHighlightColor) why is the parameter prefixed with "out"? is this to convey the fact that it is an output parameter? normally, we don't use "out" and "in" prefixes like this. > Source/WebKit/chromium/src/WebViewImpl.cpp:3478 > + if (node && node->renderer()) it seems like getting the tapHighlightColor is really a function of the firstNode represented by the given range. it really doesn't seem to have anything to do with highlight quads (i.e., the second half of this function). does it really make sense for this function to aggregate both of these things? > Source/WebKit/chromium/src/WebViewImpl.cpp:3484 > + Frame* frame = page()->focusController()->focusedOrMainFrame(); how do you know that the given range corresponds to this frame? shouldn't you verify that? should you perhaps get the frame from the range? Range has a document(), which has a frame().

Created attachment 135343 [details] Patch

Comment on attachment 134829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134829&action=review >> Source/WebKit/chromium/public/WebFrame.h:439 >> + virtual void selectRange(const WebRange&) = 0; > > should we deprecate the former selectRange method in terms of this one? I don't think so. I see other code making use of the former not related to the one that will use this API. For example, RenderViewImpl::OnSelectRange. >> Source/WebKit/chromium/public/WebHitTestInfo.h:45 >> + WebHitTestInfo(); > > these will break the shared library build of chrome if not tagged with WEBKIT_EXPORT, > but we generally do not export constructors and destructors. you should make these > in terms of assign and reset methods. see WebNode.h Making these in terms of init, assign and reset methods following the example of other embedders using WebPrivateOwnPtr. >> Source/WebKit/chromium/public/WebHitTestInfo.h:56 >> + WEBKIT_EXPORT WebNode node() const; > > what if more than one node got hit? In this case we're interested to expose only the point-based hit result, not the rect-based one. Therefore the result is the inner node of the HitTestResult instead of a set of nodes. I'm renaming the property to innerNode() make this clearer. >> Source/WebKit/chromium/public/WebSurroundingText.h:43 >> + WebSurroundingText(); > > ditto Done. >> Source/WebKit/chromium/public/WebWidget.h:173 >> + virtual WebHitTestInfo hitTestInfoForWindowPos(const WebPoint&) { return WebHitTestInfo(); } > > since hit testing returns dom nodes, i think this API belongs on WebView. > do you need it to live on WebWidget? Thanks for spotting this. This actually belongs to WebView and it seems I was missing the changes to WebViewImpl.h/cpp in this patch. Fixed. >> Source/WebKit/chromium/src/WebViewImpl.cpp:3470 >> +WebVector<WebFloatQuad> WebViewImpl::getTouchHighlightQuads(const WebRange& webRange, WebColor& outTapHighlightColor) > > why is the parameter prefixed with "out"? is this to convey the fact that it is > an output parameter? normally, we don't use "out" and "in" prefixes like this. Fixed. >> Source/WebKit/chromium/src/WebViewImpl.cpp:3478 >> + if (node && node->renderer()) > > it seems like getting the tapHighlightColor is really a function of the firstNode > represented by the given range. it really doesn't seem to have anything to do with > highlight quads (i.e., the second half of this function). does it really make sense > for this function to aggregate both of these things? I'm breaking down this method in two: one for getting the tap highlight color for a given node, and another to get the list of quads covering a text range. >> Source/WebKit/chromium/src/WebViewImpl.cpp:3484 >> + Frame* frame = page()->focusController()->focusedOrMainFrame(); > > how do you know that the given range corresponds to this frame? shouldn't > you verify that? should you perhaps get the frame from the range? > > Range has a document(), which has a frame(). Fixed.

Drive-by comment: are you sure it's necessary to have WebSurroundingText and WebHitTestInfo classes at all? How about just creating a WebViewImpl function that takes a point to hit test and returns surrounding text as a WebString? Then you could examine this string on the Chrome side with all your detectors, without needing to manipulate WebKit objects there.

(In reply to comment #17) > Drive-by comment: are you sure it's necessary to have WebSurroundingText and WebHitTestInfo classes at all? How about just creating a WebViewImpl function that takes a point to hit test and returns surrounding text as a WebString? Then you could examine this string on the Chrome side with all your detectors, without needing to manipulate WebKit objects there. I don't think that would be enough. The reason is that we don't need only the string containing the surrounding text, but also to be able to find where in the string we tapped and to get a proper range based on a substring of it. These last features (especially the last one) are the reason of why we need WebSurroundingText. WebHitTestInfo on the other hand is required in order to provide other kind of checks based on, for example, the node being hit. We might want to perform some operations only if we don't have click listeners among the tapped node's ancestors.

I see. Makes sense, thanks for the explanation.

Created attachment 135597 [details] Patch

(In reply to comment #20) > Created an attachment (id=135597) [details] > Patch Protected WebSurroundingText against a null visible position during construction. Also renamed WebHitTestInfo::point to localPoint in order to match WebCore naming.

Comment on attachment 135597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135597&action=review > Source/WebKit/chromium/public/WebFrame.h:440 > nit: please preserve two blank lines before section comment > Source/WebKit/chromium/public/WebHitTestInfo.h:29 > +#include "WebNode.h" nit: you can just forward declare WebNode, WebPoint and WebURL. no need to #include here. > Source/WebKit/chromium/public/WebHitTestInfo.h:43 > +class WebHitTestInfo { is WebHitTestInfo the best name for this? or perhaps it should be named WebHitTestResult? this might be a better name given that it is the return value from a function named hitTest*, right? > Source/WebKit/chromium/public/WebHitTestInfo.h:45 > + WebHitTestInfo() { init(); } why do you need to initialize in the constructor? reset() makes the object null (maybe you should have an isNull function?), so it seems like the default constructor could also let the object start out being null. See WebHistoryItem for example. > Source/WebKit/chromium/public/WebHitTestInfo.h:59 > + WEBKIT_EXPORT WebNode innerNode() const; what does inner node mean? is there an outer node? given only this interface, innerNode seems like a peculiar name. the comment doesn't help at all. > Source/WebKit/chromium/public/WebHitTestInfo.h:61 > + // Point coordinates of the hit. what coordinate system does this use? is it document coordinates or window (viewport) coordinates? > Source/WebKit/chromium/public/WebHitTestInfo.h:64 > + // True iff the hit was on an editable field or node. nit: we generally avoid "iff" what if the hit test was for an INPUT element? would this be false? is that why you are calling it "isContentEditable" instead of "isEditable"? > Source/WebKit/chromium/public/WebHitTestInfo.h:74 > + WEBKIT_EXPORT void init(); nit: "init" -> "initialize" > Source/WebKit/chromium/public/WebSurroundingText.h:43 > + WebSurroundingText() { init(); } why do you need to make the object non-null in the default constructor? provide an isNull method since reset() can make the object null? > Source/WebKit/chromium/public/WebSurroundingText.h:50 > + WEBKIT_EXPORT WebSurroundingText(const WebHitTestInfo&, size_t maxLength); nit: normally constructors are not exported from the webkit API: http://trac.webkit.org/wiki/ChromiumWebKitAPI#Methods there should instead be methods to initialize or assign values to a WebSurroundingText object. > Source/WebKit/chromium/public/WebSurroundingText.h:52 > + // Creates a new text content walker centered in the selected offset of the given text node. why does the comment say "text content walker" instead of "surrounding text"? it seems like this function is the constructor for WebSurroundingText instead of WebTextContentWalker. was that perhaps the old name of this class in an earlier revision of this patch? > Source/WebKit/chromium/public/WebSurroundingText.h:56 > + // Text content retrieved by the walker. nit: "walker"? > Source/WebKit/chromium/public/WebSurroundingText.h:57 > + WEBKIT_EXPORT WebString content() const; nit: perhaps this method should be named textContent. > Source/WebKit/chromium/public/WebSurroundingText.h:59 > + // Position of the initial text node offset in the content string. these two functions are a bit awkwardly named and confusing to understand. > Source/WebKit/chromium/public/WebSurroundingText.h:63 > + WEBKIT_EXPORT WebRange rangeFromContentOffsets(size_t startOffsetInContent, size_t endOffsetInContent); how is this different from WebRange::fromDocumentRange? > Source/WebKit/chromium/public/WebSurroundingText.h:66 > + WEBKIT_EXPORT void init(); nit: "init" -> "initialize" > Source/WebKit/chromium/public/WebView.h:443 > + virtual WebHitTestInfo hitTestInfoForWindowPos(const WebPoint&) = 0; It seems like this function could just be named hitTest. It is normal for coordinates passed to a WebView or WebWidget to be in viewport space, which is what "ForWindowPos" refers to. You shouldn't need to repeat that here. Plus, there is no companion function to hit test in document space. > Source/WebKit/chromium/public/WebView.h:449 > + virtual WebVector<WebFloatQuad> getTextQuadsForRange(const WebRange&) = 0; Why are you using WebFloatQuad here? What are these coordinates? Why not use a WebRect or a WebFloatRect? It is very unclear what the result is that this function computes. Does it describe a bounding region for the DOM range? > Source/WebKit/chromium/src/WebSurroundingText.cpp:47 > + VisiblePosition visiblePosition(node->renderer()->positionForPoint(hitTestInfo.localPoint())); it seems like the only information that you really need from the hit test data structure is a node and a point. > Source/WebKit/chromium/src/WebViewImpl.cpp:3514 > + WebColor tapHighlightColor = RenderStyle::initialTapHighlightColor().rgb(); this function does not seem to be dependent on the WebView at all. it seems like it could either be a WebNode function or it should live on some other interface. probably some other interface since WebNode is just supposed to be a reflection of a DOM node. > Source/WebKit/chromium/src/WebViewImpl.cpp:3516 > + return tapHighlightColor; nit: indentation > Source/WebKit/chromium/src/WebViewImpl.cpp:3536 > + range->textQuads(quads); hmm... so WebCore makes this a function of Range. we could similarly make it a function of WebRange. this function doesn't really depend on |this| at all! you infer the Frame and the FrameView from the given Range!! one thing that is perhaps awkward is that WebRange is meant to be a reflection of a DOM range. same issue that comes up with getNodeTapHighlightColor.

Comment on attachment 135597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135597&action=review >> Source/WebKit/chromium/public/WebFrame.h:440 >> > > nit: please preserve two blank lines before section comment Fixed. >> Source/WebKit/chromium/public/WebHitTestInfo.h:29 >> +#include "WebNode.h" > > nit: you can just forward declare WebNode, WebPoint and WebURL. no need to #include here. Done. >> Source/WebKit/chromium/public/WebHitTestInfo.h:43 >> +class WebHitTestInfo { > > is WebHitTestInfo the best name for this? or perhaps it should be named > WebHitTestResult? this might be a better name given that it is the return > value from a function named hitTest*, right? Agreed. Changing it. >> Source/WebKit/chromium/public/WebHitTestInfo.h:45 >> + WebHitTestInfo() { init(); } > > why do you need to initialize in the constructor? reset() makes > the object null (maybe you should have an isNull function?), so > it seems like the default constructor could also let the object > start out being null. See WebHistoryItem for example. Fixed as suggested. >> Source/WebKit/chromium/public/WebHitTestInfo.h:59 >> + WEBKIT_EXPORT WebNode innerNode() const; > > what does inner node mean? is there an outer node? given only this > interface, innerNode seems like a peculiar name. the comment doesn't > help at all. Renaming to node and clarifying a bit the comment. >> Source/WebKit/chromium/public/WebHitTestInfo.h:61 >> + // Point coordinates of the hit. > > what coordinate system does this use? is it document coordinates or > window (viewport) coordinates? These are coordinates relative to the node. >> Source/WebKit/chromium/public/WebHitTestInfo.h:64 >> + // True iff the hit was on an editable field or node. > > nit: we generally avoid "iff" > > what if the hit test was for an INPUT element? would this be false? > is that why you are calling it "isContentEditable" instead of "isEditable"? After double-checking, it looks like we can get rid of this and a few other entries in this file. >> Source/WebKit/chromium/public/WebHitTestInfo.h:74 >> + WEBKIT_EXPORT void init(); > > nit: "init" -> "initialize" Done. >> Source/WebKit/chromium/public/WebSurroundingText.h:43 >> + WebSurroundingText() { init(); } > > why do you need to make the object non-null in the default constructor? > provide an isNull method since reset() can make the object null? Fixed as suggested. >> Source/WebKit/chromium/public/WebSurroundingText.h:50 >> + WEBKIT_EXPORT WebSurroundingText(const WebHitTestInfo&, size_t maxLength); > > nit: normally constructors are not exported from the webkit API: > http://trac.webkit.org/wiki/ChromiumWebKitAPI#Methods > > there should instead be methods to initialize or assign values to > a WebSurroundingText object. Fixed. >> Source/WebKit/chromium/public/WebSurroundingText.h:52 >> + // Creates a new text content walker centered in the selected offset of the given text node. > > why does the comment say "text content walker" instead of "surrounding text"? it seems like > this function is the constructor for WebSurroundingText instead of WebTextContentWalker. > was that perhaps the old name of this class in an earlier revision of this patch? Sorry, this "text content walker" was the old name of the SurroundingText class in WebCore before it got reviewed and landed. Updating the comments. >> Source/WebKit/chromium/public/WebSurroundingText.h:56 >> + // Text content retrieved by the walker. > > nit: "walker"? Fixed. >> Source/WebKit/chromium/public/WebSurroundingText.h:57 >> + WEBKIT_EXPORT WebString content() const; > > nit: perhaps this method should be named textContent. Fixed. >> Source/WebKit/chromium/public/WebSurroundingText.h:59 >> + // Position of the initial text node offset in the content string. > > these two functions are a bit awkwardly named and confusing to understand. Fixed. >> Source/WebKit/chromium/public/WebSurroundingText.h:63 >> + WEBKIT_EXPORT WebRange rangeFromContentOffsets(size_t startOffsetInContent, size_t endOffsetInContent); > > how is this different from WebRange::fromDocumentRange? This is not related to the current selection, and the range it returns is purely based on a substring of the returned text content. >> Source/WebKit/chromium/public/WebSurroundingText.h:66 >> + WEBKIT_EXPORT void init(); > > nit: "init" -> "initialize" Fixed. >> Source/WebKit/chromium/public/WebView.h:443 >> + virtual WebHitTestInfo hitTestInfoForWindowPos(const WebPoint&) = 0; > > It seems like this function could just be named hitTest. It is normal for > coordinates passed to a WebView or WebWidget to be in viewport space, which > is what "ForWindowPos" refers to. You shouldn't need to repeat that here. > Plus, there is no companion function to hit test in document space. This method is being removed as further code (in a later patch) is moving to WebViewImpl. >> Source/WebKit/chromium/public/WebView.h:449 >> + virtual WebVector<WebFloatQuad> getTextQuadsForRange(const WebRange&) = 0; > > Why are you using WebFloatQuad here? What are these coordinates? Why not > use a WebRect or a WebFloatRect? > > It is very unclear what the result is that this function computes. Does it > describe a bounding region for the DOM range? WebFloatQuad it's a wrapper around the FloatQuad type that the DOM range. Unlike rects, FloatQuads provide a transform-friendly result. Check Range::textQuads for details. >> Source/WebKit/chromium/src/WebSurroundingText.cpp:47 >> + VisiblePosition visiblePosition(node->renderer()->positionForPoint(hitTestInfo.localPoint())); > > it seems like the only information that you really need from the hit test data structure > is a node and a point. Yes, I'm cleaning the structure up. >> Source/WebKit/chromium/src/WebViewImpl.cpp:3514 >> + WebColor tapHighlightColor = RenderStyle::initialTapHighlightColor().rgb(); > > this function does not seem to be dependent on the WebView at all. > it seems like it could either be a WebNode function or it should > live on some other interface. probably some other interface since > WebNode is just supposed to be a reflection of a DOM node. I'm temporarily removing this method. It will be added later in a separate patch introducing the rest of the content detection functionality. Following your advice it won't be a member of WebViewImpl, but moving it to WebNode doesn't sound good either. Perhaps a static method in WebViewImpl.cpp? >> Source/WebKit/chromium/src/WebViewImpl.cpp:3516 >> + return tapHighlightColor; > > nit: indentation Fixed. >> Source/WebKit/chromium/src/WebViewImpl.cpp:3536 >> + range->textQuads(quads); > > hmm... so WebCore makes this a function of Range. we could similarly make it > a function of WebRange. this function doesn't really depend on |this| at all! > you infer the Frame and the FrameView from the given Range!! > > one thing that is perhaps awkward is that WebRange is meant to be a reflection > of a DOM range. same issue that comes up with getNodeTapHighlightColor. Agreed, it makes more sense to move this to WebRange.

Created attachment 137574 [details] Patch

Comment on attachment 137574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137574&action=review > Source/WebKit/chromium/src/WebRange.cpp:124 > + nit: delete extra new line > Source/WebKit/chromium/src/WebSurroundingText.cpp:43 > +void WebSurroundingText::initialize(const WebHitTestResult& hitTestInfo, size_t maxLength) it could also make sense for these to be static create functions.

Comment on attachment 137574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137574&action=review >> Source/WebKit/chromium/src/WebRange.cpp:124 >> + > > nit: delete extra new line Fixing directly in the commit. >> Source/WebKit/chromium/src/WebSurroundingText.cpp:43 >> +void WebSurroundingText::initialize(const WebHitTestResult& hitTestInfo, size_t maxLength) > > it could also make sense for these to be static create functions. After checking other similar files in chromium/src it looks like it's more usual and appropriate to use initialize for cases like this. Static create methods are normally used when there is a constructor to build the WebKit embedder object from a reference-counted WebCore object, so that we could create embedder objects as we please for a WebCore object. In this case the embedder creates and owns the WebCore object, making not much sense to have such constructor. So, I'll leave it as initialize and proceed with landing. If you have any objections please let me know.

Committed r115463: <http://trac.webkit.org/changeset/115463>

Reverted r115463 for reason: Broke Mac build. Committed r115467: <http://trac.webkit.org/changeset/115467>

Committed r115472: <http://trac.webkit.org/changeset/115472>

Comment on attachment 137574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137574&action=review > Source/WebKit/chromium/public/WebHitTestResult.h:38 > +class WebPoint; I think this should be 'struct WebPoint'

That's exactly what was fixed when landing this for the second time. The mac bot (apparently clang) detected that problem and a similar one. Both are fixed in the landed code. (In reply to comment #30) > (From update of attachment 137574 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137574&action=review > > > Source/WebKit/chromium/public/WebHitTestResult.h:38 > > +class WebPoint; > > I think this should be 'struct WebPoint'