Bug 89098

Created attachment 147580 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Hmm, it looks like the new html files in tests/data haven't been picked up in the patch.

Created attachment 147588 [details] Patch

Patch 2 has the new test html files.

@rniwa: Would you be willing to review this patch?

Comment on attachment 147588 [details] Patch 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.

Thanks for the review. I'll rework this to take into account all your comments.

Created attachment 147835 [details] Patch

@rniwa: Are you happy with this patch now?

Comment on attachment 147835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147835&action=review r=me provided the following nits are addressed. > Source/WebCore/editing/Editor.cpp:2352 > + Element* rootEditable = m_frame->selection()->rootEditableElement(); Nit: root editable what? "editable" is an adjective, not a noun. I'd prefer spelling out rootEditableElement. Element is fine as well. > Source/WebCore/editing/Editor.cpp:2360 > + return m_frame->selection()->setSelectedRange(range.get(), WebCore::VP_DEFAULT_AFFINITY, false); Nit: No need for WebCore::

Created attachment 148878 [details] Patch for landing

Comment on attachment 148878 [details] Patch for landing Clearing flags on attachment: 148878 Committed r120985: <http://trac.webkit.org/changeset/120985>

All reviewed patches have been landed. Closing bug.