[webkit-reviews] review granted: [Bug 42781] WebKit2 needs undo/redo support : [Attachment 66670] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Sep 7 09:24:00 PDT 2010
Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 42781: WebKit2 needs undo/redo support
https://bugs.webkit.org/show_bug.cgi?id=42781
Attachment 66670: Patch
https://bugs.webkit.org/attachment.cgi?id=66670&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
> + ~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::RegisterEditComman
dForUndo, 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
More information about the webkit-reviews
mailing list