[webkit-reviews] review granted: [Bug 52221] Autocorrection should respect undo. : [Attachment 82079] Proposed patch (v8)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted jpu at apple.com's request for review:
Bug 52221: Autocorrection should respect undo.
https://bugs.webkit.org/show_bug.cgi?id=52221

Attachment 82079: Proposed patch (v8)
https://bugs.webkit.org/attachment.cgi?id=82079&action=review

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

> Source/WebCore/editing/Editor.cpp:2415
> +#endif
> +	       if (selectionToReplace != m_frame->selection()->selection()) {

There should be a blank line here.

> Source/WebCore/editing/Editor.cpp:3524
> +	   bool doSpellGrammarChecking = true;
> +#if SUPPORT_AUTOCORRECTION_PANEL
> +	   doSpellGrammarChecking = !(options &
SelectionController::SpellCorrectionTriggered);
> +#endif // SUPPORT_AUTOCORRECTION_PANEL

The comment on the endif here is not helpful. I think there are less confusing
ways to write this. I would name the boolean something more like
shouldDoSpellingAndGrammerChecking. Naming a boolean with a verb phrase is not
great.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:352
> +ReplaceSelectionCommand::ReplaceSelectionCommand(Document* document,
PassRefPtr<DocumentFragment> fragment, CommandOptions options, EditAction
editAction)
>      : CompositeEditCommand(document),
> -	 m_selectReplacement(selectReplacement), 
> -	 m_smartReplace(smartReplace),
> -	 m_matchStyle(matchStyle),
> -	 m_documentFragment(fragment),
> -	 m_preventNesting(preventNesting),
> -	 m_movingParagraph(movingParagraph),
> -	 m_editAction(editAction),
> -	 m_shouldMergeEnd(false)
> +    m_selectReplacement(options & SelectReplacement),
> +    m_smartReplace(options & SmartReplace),
> +    m_matchStyle(options & MatchStyle),
> +    m_documentFragment(fragment),
> +    m_preventNesting(options & PreventNesting),
> +    m_movingParagraph(options & MovingParagraph),
> +    m_editAction(editAction),
> +    m_shouldMergeEnd(false)

Neither the old nor the new code matches the normal WebKit style, which is:

    : CompositeEditCommand(document)
    , m_selectReplacement(options & SelectReplacement)

And so on.

> Source/WebCore/editing/ReplaceSelectionCommand.h:47
> +    enum CommandFlag {
> +	   SelectReplacement = 1 << 0,
> +	   SmartReplace = 1 << 1,
> +	   MatchStyle = 1 << 2,
> +	   PreventNesting = 1 << 3,
> +	   MovingParagraph = 1 << 4
> +    };
> +
> +    typedef unsigned CommandOptions;

It would be better to name this enum and typedef so it was clearer they match.
With one named flag and the other options, it’s a bit unclear.

> Source/WebCore/editing/SelectionController.h:63
> +    enum SetSelectionFlags {
> +	   CloseTyping = 1 << 0,
> +	   ClearTypingStyle = 1 << 1,
> +	   UserTriggered = 1 << 2,
> +	   SpellCorrectionTriggered = 1 << 3,
> +    };
> +    typedef unsigned SetSelectionOptions;

Same flags vs. options issue here.

> Source/WebCore/editing/SetSelectionCommand.cpp:58
> +void SetSelectionCommand::doApply()
> +{
> +    SelectionController* selectionController =
document()->frame()->selection();
> +    ASSERT(selectionController);
> +
> +    if (selectionController->shouldChangeSelection(m_selectionToSet)) {
> +	   selectionController->setSelection(m_selectionToSet, m_options);
> +	   setEndingSelection(m_selectionToSet);
> +    }
> +}
> +
> +void SetSelectionCommand::doUnapply()
> +{
> +    SelectionController* selectionController =
document()->frame()->selection();
> +    ASSERT(selectionController);
> +
> +    if (selectionController->shouldChangeSelection(startingSelection()))
> +	   selectionController->setSelection(startingSelection(), m_options);
> +}

I think this command needs more checks in its undo and redo code paths.
Commands like this need to work even if all the nodes involved in the original
selection are no longer in the document or if they have been moved to unusual
places. Most simple commands have checks for this type of thing, such as the
checks you’ll see in commands like RemoveNodeCommand.

I believe that both starting selection and m_selectionToSet will still point to
nodes even if they are no longer appropriate selection endpoints so this won’t
be sufficiently bulletproof.

> Source/WebCore/editing/SetSelectionCommand.h:42
> +    SetSelectionCommand(const VisibleSelection&, 
SelectionController::SetSelectionOptions);

Extra space after comma here.

> Source/WebCore/editing/SpellingCorrectionCommand.cpp:49
> +    SpellingCorrectionRecordUndoCommand(Document* document, const String&
corrected, const String& correction)
> +    : SimpleEditCommand(document)
> +    , m_corrected(corrected)
> +    , m_correction(correction)
> +    {
> +    }

Member initialization is supposed to be indented in WebKit style.

> Source/WebCore/editing/SpellingCorrectionCommand.cpp:58
> +    virtual void doApply()
> +    {
> +    }
> +
> +    virtual void doUnapply()
> +    {
> +	  
document()->frame()->editor()->unappliedSpellCorrection(startingSelection(),
m_corrected, m_correction);
> +    }

Needs a comment for why doApply isn’t needed. Does this work OK with redo?

> Source/WebCore/editing/SpellingCorrectionCommand.cpp:89
> +}
> +} // namespace WebCore

Need a blank line here.

> Source/WebCore/editing/SpellingCorrectionCommand.h:49
> +};
> +} // namespace WebCore

Need a blank line here.