Bug 58049 - [Mac] Editor::setComposition() should not trigger correction panel timer.
Summary: [Mac] Editor::setComposition() should not trigger correction panel timer.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Jia Pu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-04-07 09:22 PDT by Jia Pu
Modified: 2011-04-07 20:59 PDT (History)
4 users (show)

See Also:


Attachments
Patch (v1) (15.26 KB, patch)
2011-04-07 10:12 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v2) (19.57 KB, patch)
2011-04-07 13:51 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Path (v3) (19.76 KB, patch)
2011-04-07 14:02 PDT, Jia Pu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jia Pu 2011-04-07 09:22:45 PDT
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>
Comment 1 Jia Pu 2011-04-07 10:12:23 PDT
Created attachment 88656 [details]
Patch (v1)
Comment 2 Darin Adler 2011-04-07 11:11:22 PDT
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.
Comment 3 Jia Pu 2011-04-07 11:17:20 PDT
(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.
Comment 4 Hajime Morrita 2011-04-07 11:37:10 PDT
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?
Comment 5 Jia Pu 2011-04-07 11:49:50 PDT
(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.
Comment 6 Hajime Morrita 2011-04-07 12:58:44 PDT
(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.
Comment 7 Jia Pu 2011-04-07 13:51:57 PDT
Created attachment 88688 [details]
Patch (v2)
Comment 8 Darin Adler 2011-04-07 13:53:38 PDT
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.
Comment 9 Jia Pu 2011-04-07 14:02:48 PDT
Created attachment 88692 [details]
Path (v3)
Comment 10 WebKit Commit Bot 2011-04-07 20:59:52 PDT
Comment on attachment 88692 [details]
Path (v3)

Clearing flags on attachment: 88692

Committed r83245: <http://trac.webkit.org/changeset/83245>
Comment 11 WebKit Commit Bot 2011-04-07 20:59:58 PDT
All reviewed patches have been landed.  Closing bug.