Summary: | Push more member functions from EditCommand to CompositeEditCommand | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||
Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cshu, darin, enrica, morrita, ojan, tkent, tonikitoo, tony | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 64414, 74337 | ||||||||||||||
Bug Blocks: | 74490 | ||||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2011-12-10 17:10:10 PST
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> |