Bug 74249 – Push more member functions from EditCommand to CompositeEditCommand

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Right now EditCommand has many member functions that are overridden by CompositeEditCommand. However, some of them only make sense on CompositeEditCommand. We should move them to CompositeEditCommand since CompositeEditCommand now exists only at the time of initial application of the command.

Also, it doesn't make much sense for EditCommandComposition to share the same super class (EditCommand) with SimpleEditCommand and CompositeEditCommand.

[Ryosuke Niwa]

Created attachment 118700 [details]
work in progress

[Ryosuke Niwa]

Created attachment 118747 [details]
Cleanup + regression fix

[Enrica Casucci]

Comment on attachment 118747 [details]
Cleanup + regression fix

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

> Source/WebCore/ChangeLog:15
> +        respondToUnappliedEditing exits early even when the unapplied command was a CreteLinkCommand.

Typo CreateLinkCommand.

> Source/WebCore/editing/CompositeEditCommand.cpp:74
> +    const VisibleSelection& startingSelection, const VisibleSelection endingSelection, bool wasCreateLink)

I suggest using was CreateLinkCommand as parameter name.

> Source/WebCore/editing/CompositeEditCommand.h:42
> +    static PassRefPtr<EditCommandComposition> create(Document*, const VisibleSelection&, const VisibleSelection, bool wasCreateLink);

Ditto.

> Source/WebCore/editing/CompositeEditCommand.h:48
> +    bool wasCreateLinkCommand() { return m_wasCreateLink; }

Again I would prefer m_wasCreateLinkCommand. It just feels more understandable.

> Source/WebCore/editing/CompositeEditCommand.h:55
> +    EditCommandComposition(Document* document, const VisibleSelection& startingSelection, const VisibleSelection& endingSelection, bool wasCreateLink)

Ditto.

> Source/WebCore/editing/CompositeEditCommand.h:62
> +    bool m_wasCreateLink;

Ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2246
> +    frame->editor()->replaceSelectionWithText(text, selectReplacement, smartReplace);

I'm not sure this code is equivalent. The PreventNesting option seems to be lost.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1129
> +    frame()->editor()->replaceSelectionWithText(text, selectReplace, smartReplace);

Ditto.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:1001
> +    coreFrame->editor()->replaceSelectionWithText(text, selectReplacement, smartReplace);

Ditto.

> Source/WebKit/mac/WebView/WebFrame.mm:1049
> +    _private->coreFrame->editor()->replaceSelectionWithFragment(core(fragment), selectReplacement, smartReplace, matchStyle);

Ditto.

[Enrica Casucci]

The patch shows build errors on mac.
It would be better to add layout tests for the fix relative to the spellchecker regression. If you want to write the test I could run it and validate it for you, if you want.

[Ryosuke Niwa]

(In reply to comment #3)
> (From update of attachment 118747 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118747&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        respondToUnappliedEditing exits early even when the unapplied command was a CreteLinkCommand.
> 
> Typo CreateLinkCommand.

Will fix.

> > Source/WebCore/editing/CompositeEditCommand.cpp:74
> > +    const VisibleSelection& startingSelection, const VisibleSelection endingSelection, bool wasCreateLink)
> 
> I suggest using was CreateLinkCommand as parameter name.

Sure, will change.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2246
> > +    frame->editor()->replaceSelectionWithText(text, selectReplacement, smartReplace);
> 
> I'm not sure this code is equivalent. The PreventNesting option seems to be lost.

PreventNesting is always set in Editor::replaceSelectionWithFragment.

[Ryosuke Niwa]

Created attachment 118817 [details]
Fixed per Enrica's comments

[Ryosuke Niwa]

Comment on attachment 118817 [details]
Fixed per Enrica's comments

Let me split this patch into 3 pieces:
1a - Remove dependency on ApplyCommand in WebKit
1b - Fix a regression in SpellingCorrectionController
2 - Push member functions from EditCommand to CompositeEditCommand.

[Enrica Casucci]

(In reply to comment #7)
> (From update of attachment 118817 [details])
> Let me split this patch into 3 pieces:
> 1a - Remove dependency on ApplyCommand in WebKit
> 1b - Fix a regression in SpellingCorrectionController
> 2 - Push member functions from EditCommand to CompositeEditCommand.

Sounds like a good plan to me.

[Ryosuke Niwa]

Created attachment 119173 [details]
cleanup

[Enrica Casucci]

Comment on attachment 119173 [details]
cleanup

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

The changes look good, but there is a build failure. Please fix that.

> Source/WebCore/editing/Editor.cpp:871
> +    EditCommandComposition* composition = cmd->composition();

Nit. I would ASSERT composition directly.

[Ryosuke Niwa]

Created attachment 119282 [details]
Fixed per Enrica's comment

[Enrica Casucci]

Comment on attachment 119282 [details]
Fixed per Enrica's comment

Thanks!

[Ryosuke Niwa]

Great. Thanks for the review.

[Ryosuke Niwa]

Committed r102833: <http://trac.webkit.org/changeset/102833>