[webkit-reviews] review granted: [Bug 58049] [Mac] Editor::setComposition() should not trigger correction panel timer. : [Attachment 88656] Patch (v1)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jia Pu <jpu at apple.com>'s request for
review:
Bug 58049: [Mac] Editor::setComposition() should not trigger correction panel
timer.
https://bugs.webkit.org/show_bug.cgi?id=58049

Attachment 88656: Patch (v1)
https://bugs.webkit.org/attachment.cgi?id=88656&action=review

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

This looks good. Is there any way to add some kind of test coverage?

> Source/WebCore/editing/TypingCommand.cpp:139
>      RefPtr<TypingCommand> typingCommand = TypingCommand::create(document,
ForwardDeleteKey, "", options, granularity);
> -    typingCommand->setSmartDelete(smartDelete);
>      typingCommand->apply();

I don’t think you need a local variable here any more. You can just create and
apply the command all on one line.

> Source/WebCore/editing/TypingCommand.h:58
>      enum TypingCommandOption {
>	   SelectInsertedText = 1 << 0,
>	   KillRing = 1 << 1,
> -	   RetainAutocorrectionIndicator = 1 << 2
> +	   RetainAutocorrectionIndicator = 1 << 2,
> +	   PreventSpellChecking = 1 << 3,
> +	   SmartDelete = 1 << 4
>      };
>      typedef unsigned TypingCommandOptions;

It’s annoying that these have such long names. They should be named just Option
and Options since they are class members already.

> Source/WebCore/editing/TypingCommand.h:62
> -    static void deleteSelection(Document*, bool smartDelete = false);
> -    static void deleteKeyPressed(Document*, bool smartDelete = false,
TextGranularity = CharacterGranularity, bool killRing = false);
> -    static void forwardDeleteKeyPressed(Document*, bool smartDelete = false,
TextGranularity = CharacterGranularity, bool killRing = false);
> +    static void deleteSelection(Document*, TypingCommandOptions);
> +    static void deleteKeyPressed(Document*, TypingCommandOptions,
TextGranularity = CharacterGranularity);
> +    static void forwardDeleteKeyPressed(Document*, TypingCommandOptions,
TextGranularity = CharacterGranularity);

Since the options had default values before, I think TypingCommandOptions
should still have a default of 0 after.