[webkit-reviews] review denied: [Bug 107737] Editor::m_compositionNode not updated on HTMLInputElement::setValue() : [Attachment 185795] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Aurimas Liutikas
<aurimas at chromium.org>'s request for review:
Bug 107737: Editor::m_compositionNode not updated on
HTMLInputElement::setValue()
https://bugs.webkit.org/show_bug.cgi?id=107737

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

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


> Source/WebCore/ChangeLog:7
> +

Please describe the nature of your change.

> Source/WebCore/ChangeLog:8
> +	   No new tests (OOPS!).

Please explain that an existing test,
editing/input/setting-input-value-cancel-ime-composition.html, covers this
change.

> Source/WebCore/ChangeLog:22
> +
> +	   * WebCore.exp.in:
> +	   * editing/Editor.cpp:
> +	   (WebCore::Editor::cancelCompositionIfSelectionIsInvalid):
> +	   (WebCore):
> +	   * editing/Editor.h:
> +	   (Editor):

Why is this repeated here?

> Source/WebKit/chromium/ChangeLog:12
> +	   Adding a check whether composition is valid after the selection
change.
> +	   If there is no valid composition, then the composition should get
cancelled
> +	   and the WebViewClient should get notified about this cancellation.
> +
> +	   This bug already had a test that had expectation set to fail, but
now it will be passing.

This explanation should probably be done in Source/WebCore/ChangeLog instead.

> Source/WebKit/mac/WebView/WebHTMLView.mm:6048
>      if (coreFrame->editor()->getCompositionSelection(start, end))
>	   [[NSInputManager currentInputManager]
markedTextSelectionChanged:NSMakeRange(start, end - start) client:self];

Now this code is executed even if hasComposition() and
coreFrame->editor()->ignoreCompositionSelectionChange() are both false.
r- because of this.

> LayoutTests/ChangeLog:10
> +	   Adding a check whether composition is valid after the selection
change.
> +	   If there is no valid composition, then the composition should get
cancelled
> +	   and the WebViewClient should get notified about this cancellation.

This comment applies to Source/WebCore or Source/WebKit, not LayoutTests.
Please remove.