[Webkit-unassigned] [Bug 66681] Need API for getting surrounding text from webkit in chromium

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=66681


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #106046|review?                     |review-
               Flag|                            |




--- Comment #37 from Ryosuke Niwa <rniwa at webkit.org>  2011-09-01 15:27:25 PST ---
(From update of attachment 106046)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.