[webkit-reviews] review denied: [Bug 82460] [Chromium] Selectively retrieve text around a touched point : [Attachment 135597] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Leandro Graciá
Gil <leandrogracia at chromium.org>'s request for review:
Bug 82460: [Chromium] Selectively retrieve text around a touched point
https://bugs.webkit.org/show_bug.cgi?id=82460

Attachment 135597: Patch
https://bugs.webkit.org/attachment.cgi?id=135597&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.