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

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


>> Source/WebKit/chromium/public/WebWidget.h:154
>> +	virtual bool surrounding(WebString& text, size_t& cursor, size_t&
anchor) { return false; }
> 
> You can return WebSize. See (Source/WebKit/public/WebSize.h)

The standard term used is focus and anchor.  "cursor" usually refers to mouse
cursor, and it's confusing.  Alos, Why isn't this function const?  Also the
last time I checked, Chromium's coding style guide forbid mutable reference and
instead forces pass by pointer.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1530
> +    Element* scope = selectionRoot ? selectionRoot :
frame->document()->documentElement();

Wait a minute, if rootEditableElement can be null, then element->innerText()
would have blown up by null-pointer access, no?  r- because of this.

Also, this line appears to be much longer than 80 characters, yet chromium
coding guideline mandates the column length to be at most 80 characters.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1539
> +    anchor = TextIterator::rangeLength(testRange.get());
> +
> +    ExceptionCode ec;
> +    testRange->setEnd(selection->extent().containerNode(),
selection->extent().offsetInContainerNode(), ec);
> +    cursor = TextIterator::rangeLength(testRange.get());

You should call TextIterator::locationAndLengthFromRange instead.