Bug 58049 – [Mac] Editor::setComposition() should not trigger correction panel timer.

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Jia Pu]

When using Japanese or Chinese input method on Mac OS X that supports autocorrection panel,  Editor::setComposition() shouldn't not trigger the correction panel timer. 
<rdar://problem/9226305>

[Jia Pu]

Created attachment 88656 [details]
Patch (v1)

[Darin Adler]

Comment on attachment 88656 [details]
Patch (v1)

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.

[Jia Pu]

(In reply to comment #2)
> (From update of attachment 88656 [details])
> 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?
> 
Automated test is difficult since this involves the timer. We have yet to identify a particular input sequence that triggers the recursive call. The patch is based on analyzing one available backtrace. I will try a little bit to see if I can find a deterministically reproducible case.

[Hajime Morrita]

Comment on attachment 88656 [details]
Patch (v1)

View in context: https://bugs.webkit.org/attachment.cgi?id=88656&action=review

> Source/WebCore/editing/Editor.cpp:1706
> +        TypingCommand::deleteSelection(m_frame->document(), TypingCommand::PreventSpellChecking);

Do we really need these #if s?
I think we don't need spellchecking during the spell-checking anyway
and I want to have as small number of #if as possible.
In addition, this looks to change the behavior even if the panel is disabled at the runtime.
What do you think?

[Jia Pu]

(In reply to comment #4)
> (From update of attachment 88656 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88656&action=review
> 
> > Source/WebCore/editing/Editor.cpp:1706
> > +        TypingCommand::deleteSelection(m_frame->document(), TypingCommand::PreventSpellChecking);
> 
> Do we really need these #if s?

I agree that we probably should prevent spell checking here across the board. I was playing safe. 

> I think we don't need spellchecking during the spell-checking anyway
> and I want to have as small number of #if as possible.
> In addition, this looks to change the behavior even if the panel is disabled at the runtime.
> What do you think?

If we decide to always use PreventSpellChecking here, then I suppose the last pony is irrelevant.

[Hajime Morrita]

(In reply to comment #5)
> 
> If we decide to always use PreventSpellChecking here, then I suppose the last pony is irrelevant.
Yes, always using PreventSpellChecking sounds good to me.

[Jia Pu]

Created attachment 88688 [details]
Patch (v2)

[Darin Adler]

Comment on attachment 88688 [details]
Patch (v2)

View in context: https://bugs.webkit.org/attachment.cgi?id=88688&action=review

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

Can’t land a patch with this line in it. Please replace with an explanation of why there are no new tests.

[Jia Pu]

Created attachment 88692 [details]
Path (v3)

[WebKit Commit Bot]

Comment on attachment 88692 [details]
Path (v3)

Clearing flags on attachment: 88692

Committed r83245: <http://trac.webkit.org/changeset/83245>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.