[webkit-reviews] review granted: [Bug 38564] Caret keeps blinking during forward-delete : [Attachment 105823] Proposed fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Van Lam <vanlam at google.com>'s request
for review:
Bug 38564: Caret keeps blinking during forward-delete
https://bugs.webkit.org/show_bug.cgi?id=38564

Attachment 105823: Proposed fix
https://bugs.webkit.org/attachment.cgi?id=105823&action=review

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


r=me, but I think this could be slightly improved coding-style-wise

I also would like to see us come up with a way to test.

> Source/WebCore/editing/FrameSelection.cpp:1604
> +    bool shouldStopBlinkingDueToTypingCommand = (m_frame && (lastEditCommand
= m_frame->editor()->lastEditCommand())) ?
> +	   lastEditCommand->shouldStopCaretBlinking() : false;

The last part of this should also be written using && rather than ? :

Also, this is hard to read with the assignment in the middle of it. I think it
would be clearer like this:

    EditCommand* lastEditCommand = m_frame ?
m_frame->editor()->lastEditCommand() : 0;
    bool shouldStopBlinkingDueToTypingCommand = lastEditCommand &&
lastEditCommand->shouldStopCaretBlinking();

> Source/WebCore/editing/TypingCommand.h:73
> +    bool shouldStopCaretBlinking() const { return true; }

This override could, and should, be private rather than public. It would be a
programming mistake to call this on a TypingCommand*.