[webkit-reviews] review denied: [Bug 66681] Need API for getting surrounding text from webkit in chromium : [Attachment 106046] 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 106046: Patch
https://bugs.webkit.org/attachment.cgi?id=106046&action=review

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


r- due to various nits.

> Source/WebKit/chromium/ChangeLog:7
> +

Again, please add description here.

> Source/WebKit/chromium/public/WebWidget.h:157
> +    virtual bool surroundingTextWithSelection(
> +	   WebString& text,
> +	   size_t& focus,
> +	   size_t& anchor) const { return false; }

I'd put everything in one line.  Also, "text" doesn't add any value here so you
can omit.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1522
> +    const FrameSelection* selection = frame->selection();
> +    if (!selection->isContentEditable())
> +	   return false;
> +
> +    Element* element = selection->rootEditableElement();

I would have done:
Element* element = selection->rootEditableElement();
if (!element)
    return false;
instead of checking isContentEditable then implicitly assuming that element is
not null.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1532
> +    if (!range.get())
> +	   return false;
> +
> +    if (!TextIterator::locationAndLengthFromRange(range.get(), location,
length))
> +	   return false;

You can combine these two statements:
if (!range || TextIterator::locationAndLengthFromRange(range.get(), location,
length))
(notice there's no need to call get() on the nullity check).

> Source/WebKit/chromium/src/WebViewImpl.h:123
> +    virtual bool surroundingTextWithSelection(
> +	   WebString& text,
> +	   size_t& focus,
> +	   size_t& anchor) const;

Ditto about putting it in one line and omitting text.