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>
Created attachment 88656 [details] Patch (v1)
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.
(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 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?
(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.
(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.
Created attachment 88688 [details] Patch (v2)
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.
Created attachment 88692 [details] Path (v3)
Comment on attachment 88692 [details] Path (v3) Clearing flags on attachment: 88692 Committed r83245: <http://trac.webkit.org/changeset/83245>
All reviewed patches have been landed. Closing bug.