[Webkit-unassigned] [Bug 42781] WebKit2 needs undo/redo support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=42781


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66670|review?                     |review+
               Flag|                            |




--- Comment #5 from Darin Adler <darin at apple.com>  2010-09-07 09:24:00 PST ---
(From update of attachment 66670)
> +    ~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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.