WebKit2 needs to undo/redo support
<rdar://problem/7660519>
*** Bug 45276 has been marked as a duplicate of this bug. ***
Created attachment 66670 [details] Patch
Attachment 66670 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/WebProcess/WebPage/WebEditCommand.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit2/UIProcess/WebEditCommandProxy.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 66670 [details] Patch > + ~WebEditCommandProxy(); Darin Fisher has suggested we start making RefCounted objects' destructors protected or private and making the RefCounted template class a friend. I think that is probably a good idiom. > +private: > + explicit WebEditCommandProxy(uint64_t commandID, WebCore::EditAction, WebPageProxy*); There is never a need for "explicit" on a constructor unless it has exactly one non-default argument. So it’s not helpful here. > + Vector<WebEditCommandProxy*> editCommandVector; > + copyToVector(m_editCommandSet, editCommandVector); > + for (size_t i = 0, size = editCommandVector.size(); i < size; ++i) > + editCommandVector[i]->invalidate(); > + m_editCommandSet.clear(); I don’t understand why we’re doing the clear after the invalidate calls. Why not clear right away after copying to the vector? > + case WebPageProxyMessage::ClearAllEditCommands: { > + clearAllEditCommands(); > + break; > + } No need for the braces here. > +void WebPageProxy::registerEditCommandForUndo(uint64_t commandID, EditAction editAction) > +{ > + RefPtr<WebEditCommandProxy> commandProxy = WebEditCommandProxy::create(commandID, editAction, this); > + registerEditCommandForUndo(commandProxy.release()); > +} This would read better without the local variable. > + void registerEditCommandForUndo(uint64_t commandID, WebCore::EditAction editAction); No need for the editAction argument name here. > +// -- BEGIN HELPER CLASSES Unusual comment. Is this really helpful? > +- (void)undoEditing:(id)arg; > +- (void)redoEditing:(id)arg; This argument is normally called "sender" rather than "arg". And I prefer not to abbreviate words like "arg". > + // Add assertion that commanding being reapplied is the same command that is > + // being passed to us. "commanding"? Is this a FIXME comment? > + WebProcess::shared().connection()->send(WebPageProxyMessage::RegisterEditCommandForUndo, m_page->pageID(), > + CoreIPC::In(webCommand->commandID(), static_cast<uint32_t>(webCommand->command()->editingAction()))); We normally don't like things up like this in WebKit code, since the alignment gets screwed up if we rename something. Is there any clean way to avoid the static_cast? Maybe a local variable instead? > void WebEditorClient::registerCommandForRedo(PassRefPtr<EditCommand>) > { > + // INTENTIONALLY LEFT NOT IMPLEMENTED > } WHAT DOES THIS COMMENT MEAN AND WHY IS IT SHOUTING? > +static uint64_t generateCommandID() > +{ > + static uint64_t uniqueCommandID = 1; > + return uniqueCommandID++; > +} I would name the global lastCommandIDUsed or something like that. And use preincrement instead of postincrement. Also, I like making the create function non-inline and the constructor inline. Same number of function calls, but less code at each call site. r=me
(In reply to comment #5) > (From update of attachment 66670 [details]) > > + ~WebEditCommandProxy(); > > Darin Fisher has suggested we start making RefCounted objects' destructors protected or private and making the RefCounted template class a friend. I think that is probably a good idiom. Ok. > > > +private: > > + explicit WebEditCommandProxy(uint64_t commandID, WebCore::EditAction, WebPageProxy*); > > There is never a need for "explicit" on a constructor unless it has exactly one non-default argument. So it’s not helpful here. Ok. > > > + Vector<WebEditCommandProxy*> editCommandVector; > > + copyToVector(m_editCommandSet, editCommandVector); > > + for (size_t i = 0, size = editCommandVector.size(); i < size; ++i) > > + editCommandVector[i]->invalidate(); > > + m_editCommandSet.clear(); > > I don’t understand why we’re doing the clear after the invalidate calls. Why not clear right away after copying to the vector? No reason. Fixed. > > > + case WebPageProxyMessage::ClearAllEditCommands: { > > + clearAllEditCommands(); > > + break; > > + } > > No need for the braces here. Ok. > > > +void WebPageProxy::registerEditCommandForUndo(uint64_t commandID, EditAction editAction) > > +{ > > + RefPtr<WebEditCommandProxy> commandProxy = WebEditCommandProxy::create(commandID, editAction, this); > > + registerEditCommandForUndo(commandProxy.release()); > > +} > > This would read better without the local variable. Done. > > > + void registerEditCommandForUndo(uint64_t commandID, WebCore::EditAction editAction); > > No need for the editAction argument name here. Done. > > > +// -- BEGIN HELPER CLASSES > > Unusual comment. Is this really helpful? No. Removed. > > > +- (void)undoEditing:(id)arg; > > +- (void)redoEditing:(id)arg; > > This argument is normally called "sender" rather than "arg". And I prefer not to abbreviate words like "arg". Changed (though not charged) to sender. > > > + // Add assertion that commanding being reapplied is the same command that is > > + // being passed to us. > > "commanding"? Is this a FIXME comment? Fixed typo and made a FIXME. > > > + WebProcess::shared().connection()->send(WebPageProxyMessage::RegisterEditCommandForUndo, m_page->pageID(), > > + CoreIPC::In(webCommand->commandID(), static_cast<uint32_t>(webCommand->command()->editingAction()))); > > We normally don't like things up like this in WebKit code, since the alignment gets screwed up if we rename something. Is there any clean way to avoid the static_cast? Maybe a local variable instead? We have been doing this a bit in WebKit2. We should definitely look at all the places we do it and fix them. > > > void WebEditorClient::registerCommandForRedo(PassRefPtr<EditCommand>) > > { > > + // INTENTIONALLY LEFT NOT IMPLEMENTED > > } > > WHAT DOES THIS COMMENT MEAN AND WHY IS IT SHOUTING? > Removed. > > +static uint64_t generateCommandID() > > +{ > > + static uint64_t uniqueCommandID = 1; > > + return uniqueCommandID++; > > +} > > I would name the global lastCommandIDUsed or something like that. And use preincrement instead of postincrement. This pattern is idiomatic in WebKit2. I would rather change them all at once then be inconsistent here. > Also, I like making the create function non-inline and the constructor inline. Same number of function calls, but less code at each call site. Done. > r=me Thank.
> > > > > + WebProcess::shared().connection()->send(WebPageProxyMessage::RegisterEditCommandForUndo, m_page->pageID(), > > > + CoreIPC::In(webCommand->commandID(), static_cast<uint32_t>(webCommand->command()->editingAction()))); > > > > We normally don't like things up like this in WebKit code, since the alignment gets screwed up if we rename something. Is there any clean way to avoid the static_cast? Maybe a local variable instead? > > We have been doing this a bit in WebKit2. We should definitely look at all the places we do it and fix them. Never mind. I misunderstood this. Moved into local.
Landed in r66897.