[webkit-reviews] review granted: [Bug 42781] WebKit2 needs undo/redo support : [Attachment 66670] Patch

[bugzilla-daemon at webkit.org]

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