[webkit-reviews] review denied: [Bug 82460] [Chromium] Selectively retrieve text around a touched point : [Attachment 134829] 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 134829: Patch
https://bugs.webkit.org/attachment.cgi?id=134829&action=review

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