[webkit-reviews] review denied: [Bug 66681] Need API for getting surrounding text from webkit in chromium : [Attachment 105686] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Peng Huang
<penghuang at chromium.org>'s request for review:
Bug 66681: Need API for getting surrounding text from webkit in chromium
https://bugs.webkit.org/show_bug.cgi?id=66681

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=105686&action=review


> Source/WebCore/editing/FrameSelection.cpp:1887
> +bool FrameSelection::getSurroundingText(String& text, size_t& focus, size_t&
anchor) const

It seems like the function name should reflect the fact it only obtains text in
the content editable area.  Also, I'd expect the return value of a function
named get...Text to be a string.

> Source/WebCore/editing/FrameSelection.cpp:1893
> +    if (!isContentEditable())
> +	   return false;
> +
> +    Element* element = rootEditableElement();
> +    if (!element)

This check is redundant. If there's no root editable element, then it's surely
not editable.

> Source/WebCore/editing/FrameSelection.cpp:1900
> +    RefPtr<Range> testRange = Range::create(
> +	   element->document(), element, 0,
> +	   base().containerNode(), base().offsetInContainerNode());

You should call computeOffsetInContainerNode instead.  Otherwise this will hit
an assertion.  r- because of this.

> Source/WebCore/editing/FrameSelection.cpp:1905
> +    testRange->setEnd(extent().containerNode(),
> +	   extent().offsetInContainerNode(), ec);

Why don't you just do setEnd(extent(), ec) ?

> Source/WebCore/editing/FrameSelection.cpp:1908
> +    // FIXME truncate the text into reansonable size

Nit: need a colon after FIXME.

> Source/WebCore/editing/FrameSelection.h:246
> +    bool getSurroundingText(String&, size_t&, size_t&) const;

You probably need to say what these size_t's are.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1513
> +    String tmp;

Let's not use such an ambiguous shorthanded name.  We can rename text to
webText or passedText.	Alternatively we can rename tmp to coreText.