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

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


> Source/WebKit/chromium/ChangeLog:4
> +	   Add API for getting surrounding text and selection range
> +	   from webkit in chromium.

Why is this line awkwardly wrapped in the middle of a sentence?

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

You need to describe what kind of changes you're making.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1533
> +    if (selection->base().containerNode() ==
selection->extent().containerNode()) {
> +	   // In most case focus and anchor are in the same element. We get the

> +	   // text from this element.
> +	   Node* node = selection->base().containerNode();
> +	   if (!node || !node->isElementNode())
> +	       return false;
> +	   Element* element =
static_cast<Element*>(selection->base().containerNode());
> +	   text = element->innerText();
> +	   focus = selection->base().computeOffsetInContainerNode();
> +	   anchor = selection->extent().computeOffsetInContainerNode();
> +	   return true;
> +    }

Let's not do this.  It's anti-pattern, and IME probably needs more context than
what it fits in one element.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1538
> +    // If focus and anchor are in different nodes, we will get the text from

> +    // the root editable element.
> +    // FIXME: Find the smaller scope which can conver the focus and anchor
> +    // instead of the root editable element.

This is a completely tangential issue that's present throughout WebKit's code
that came out of the discussion ap & I had.  I don't think you should include
FIXME like this here.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1556
> +    Element* element = selection->rootEditableElement();

You should do this where you check whether selection is in a contenteditable
region or not.