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.
Created attachment 118700 [details] work in progress
Created attachment 118747 [details] Cleanup + regression fix
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.
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.
(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.
Created attachment 118817 [details] Fixed per Enrica's comments
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.
(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.
Created attachment 119173 [details] cleanup
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.
Created attachment 119282 [details] Fixed per Enrica's comment
Comment on attachment 119282 [details] Fixed per Enrica's comment Thanks!
Great. Thanks for the review.
Committed r102833: <http://trac.webkit.org/changeset/102833>