[webkit-reviews] review denied: [Bug 89098] [chromium] Add a method to WebViewImpl to select between offsets in an editable field. : [Attachment 147588] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 14 10:38:04 PDT 2012


Ryosuke Niwa <rniwa at webkit.org> has denied Oli Lan <olilan at chromium.org>'s
request for review:
Bug 89098: [chromium] Add a method to WebViewImpl to select between offsets in
an editable field.
https://bugs.webkit.org/show_bug.cgi?id=89098

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

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


> Source/WebKit/chromium/ChangeLog:15
> +	   Reviewed by NOBODY (OOPS!).

This line should appear before the long description bug after the bug url.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2098
> +    Node* node = focusedWebCoreNode();
> +    if (node && node->isElementNode()) {
> +	   Element* elementNode = toElement(node);
> +	   if (elementNode->isTextFormControl()) {
> +	       HTMLTextFormControlElement* formElement =
toTextFormControl(elementNode);
> +	       formElement->setSelectionRange(start, end);
> +	       return;
> +	   }
> +    }

This code is redundant.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2102
> +    // For contenteditable nodes the focusedWebCoreNode may not be what we
want (the editable
> +    // node may not be focused even during editing). Also, we need to go to
the ancestor node
> +    // to apply the offsets.

This comment is redundant for anyone who understands editing code.
I'd suggest we move all of this code to Editor.cpp so that we don't have to add
this comment.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2106
> +    node = frame->selection()->start().containerNode();

Why do you want to use start's container node here? That sounds completely
arbitrary except when both start & end are in the same node.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2107
> +    if (node && node->shouldUseInputMethod()) {

If anything, what you want to do is to obtain the root editable element and use
TextIterator's static functions to obtain range given start & length offsets
from the beginning of the element.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2110
> +	   Position startPosition(node, start,
Position::PositionIsOffsetInAnchor);
> +	   Position endPosition(node, end, Position::PositionIsOffsetInAnchor);


What guarantees that start and end are in the same node? If this is specific to
composition, then the method name should reflect that.
Also, I'd prefer moving this code to Editor.h/cpp next to
getCompositionSelection.


More information about the webkit-reviews mailing list