[webkit-reviews] review denied: [Bug 42958] Pasting should fire textInput event : [Attachment 63440] Patch

[bugzilla-daemon at webkit.org]

Tony Chang (Google) <tony at chromium.org> has denied MORITA Hajime
<morrita at google.com>'s request for review:
Bug 42958: Pasting should fire textInput event
https://bugs.webkit.org/show_bug.cgi?id=42958

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

------- Additional Comments from Tony Chang (Google) <tony at chromium.org>
> diff --git a/LayoutTests/editing/pasteboard/script-tests/paste-text-events.js
b/LayoutTests/editing/pasteboard/script-tests/paste-text-events.js
> +function pastingTextInputHandler(evt)
> +{
> +    shouldBe("event.data", toStringLiteral(expectedTextEventData));
> +    textInputCount++;
> +    if (willCancelTextInput)
> +	evt.preventDefault();

Nit: Looks like a tab character got in here.

> +willCancelTextInput = false;
> +for (var i = 0; i < proceedingTestCases.length; ++i)
> +    runSingleTest(proceedingTestCases[i]);
> +shouldBe("textInputCount", "proceedingTestCases.length");
> +
> +textInputCount = 0;
> +willCancelTextInput = true;
> +for (var i = 0; i < cancelingTestCases.length; ++i)
> +    runSingleTest(cancelingTestCases[i]);
> +shouldBe("textInputCount", "cancelingTestCases.length");

These tests look great!  Does this event fire for ontextinput declared in the
HTML?  If so, it'd be nice to add some tests for that too.

> diff --git a/WebCore/editing/Editor.cpp b/WebCore/editing/Editor.cpp
> +void Editor::pasteAsPlainText(const String& pastingText, bool smartReplace)
> +void Editor::pasteAsFragment(PassRefPtr<DocumentFragment> pastingFragment,
bool smartReplace, bool matchStyle)

These methods seem to have only one caller.  Since they're pretty short, maybe
we should just inline them?  I think it's confusing to have pasteAsPlainText
and pasteAsPlainTextWithPasteboard.


> +class FragmentPasteTextEvent : public TextEvent {
> +public:
> +    static PassRefPtr<FragmentPasteTextEvent>
create(PassRefPtr<AbstractView> view, PassRefPtr<DocumentFragment> pastingData,
bool smartReplace, bool matchStyle);

Subclassing seems like overkill just to keep track of the type of event.  Can
we just add an argument to the TextEvent constructor that keeps track of the
event type?  The spec mentions an inputMode parameter to initTextEvent, but
that should probably happen in a different patch.