[webkit-reviews] review granted: [Bug 57649] Make WebKit2 text input handling more like WebKit1 : [Attachment 87897] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 57649: Make WebKit2 text input handling more like WebKit1
https://bugs.webkit.org/show_bug.cgi?id=57649

Attachment 87897: proposed patch
https://bugs.webkit.org/attachment.cgi?id=87897&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87897&action=review

Patch doesn’t apply so you are not getting the benefit of EWS.

> Source/WebCore/dom/KeyboardEvent.h:42
> -	   String commandName;
> +	   String commandName; // Actually, a selector name - it may have a
trailing colon, and a name that can be different from an editor command name.

Can we change the data member name to selector or selectorString? Is this used
only with selectors, or is it sometimes used for command names?

> Source/WebKit2/Shared/mac/TextInputState.h:42
> +};

This semicolon is incorrect.

> Source/WebKit2/Shared/mac/TextInputState.h:45
> +
> +
> +#endif // TextInputState_h

Extra blank line.

> Source/WebKit2/UIProcess/WebPageProxy.h:256
> +    void setComposition(const String& text,
Vector<WebCore::CompositionUnderline> underlines, uint64_t selectionStart,
uint64_t selectionEnd, uint64_t replacementRangeStart, uint64_t
replacementRangeEnd, TextInputState& newState);

const Vector& instead of Vector?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:109
> +};
> +
> +

Extra blank line here.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1063
> +/*
> +	   // Make sure that only direct calls to doCommandBySelector: see the
parameters by setting to 0.
> +	   _data->interpretKeyEventsParameters = 0;
> +
> +	   bool eventWasHandled;
> +
> +	   Editor::Command command = [self coreCommandBySelector:selector];
> +	   if (command.isSupported())
> +	       eventWasHandled = command.execute(event);
> +	   else {
> +	       // If WebKit does not support this command, we need to pass the
selector to super.
> +	       _private->selectorForDoCommandBySelector = selector;
> +
> +	       // The sink does two things: 1) Tells us if the responder went
unhandled, and
> +	       // 2) prevents any NSBeep; we don't ever want to beep here.
> +	       WebResponderChainSink *sink = [[WebResponderChainSink alloc]
initWithResponderChain:self];
> +	       [super doCommandBySelector:selector];
> +	       eventWasHandled = ![sink receivedUnhandledCommand];
> +	       [sink detach];
> +	       [sink release];
> +
> +	       _private->selectorForDoCommandBySelector = 0;
> +	   }
> +
> +	   if (parameters)
> +	       parameters->eventInterpretationHadSideEffects |=
eventWasHandled;
> +
> +	   _private->interpretKeyEventsParameters = parameters;
> +*/

Are you really planning on checking in all this code commented out? Is there
some better placeholder?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1084
> +	   // FIXME: We ignore most attributes from the string, so for example
inserting from Character Palette loses font and glyph variation data.
> +	   // It does not look like any input methods ever use insertText: with
attributes other than NSTextInputReplacementRangeAttributeName.

Those two comments seem almost contradictory. Maybe the point is that Character
Palette is not an input method.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1102
> +	   parameters->commands->append(KeypressCommand("insertText:", text));

Can we use a string constant for insertText:?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1230
> +    if (result.location == NSNotFound)
> +	   LOG(TextInput, "selectedRange -> (NSNotFound, %u)", result.length);
> +    else
> +	   LOG(TextInput, "selectedRange -> (%u, %u)", result.location,
result.length);

I believe that %u is incorrect for 64-bit for NSUInteger; surprised it works. I
see code elsewhere making the same mistake.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:168
> -#if !PLATFORM(MAC)
> +#if PLATFORM(MAC)
> +    bool handleEditingKeyboardEvent(WebCore::KeyboardEvent*, bool
saveCommands);
> +#else
>      bool handleEditingKeyboardEvent(WebCore::KeyboardEvent*);
>  #endif

I wish there was some way we could unify these.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:97
> +    map->add("insertNewlineIgnoringFieldEditor:", "InsertNewline");
> +    map->add("insertParagraphSeparator:", "InsertNewline");
> +    map->add("insertTabIgnoringFieldEditor:", "InsertTab");
> +    map->add("pageDown:", "MovePageDown");
> +    map->add("pageDownAndModifySelection:",
"MovePageDownAndModifySelection");
> +    map->add("pageUp:", "MovePageUp");
> +    map->add("pageUpAndModifySelection:", "MovePageUpAndModifySelection");

It’s a bit annoying to have the mapping from selectors to command names here
rather than in the class where the selectors are supported, WKView. I see that
you want to avoid converting selectors to editor command names because they may
round trip back to the UI process, but still it’s not great to have the rules
about selectors distributed between classes.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:155
> +	       // FIXME: WebHTMLView sends the event up the responder chain
with WebResponderChainSink if it's not supported by the editor. Should we do
the same?

Strange that this comment is here, but the commented-out code is over in
WKView.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:523
> +    // FIXME: Commoon behaviors like scrolling down on Space should probably
be implemented in WebCore.

Typo: "Commoon".