Bug 42781 – WebKit2 needs undo/redo support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Sam Weinig]

WebKit2 needs to undo/redo support

[Sam Weinig]

<rdar://problem/7660519>

[Sam Weinig]

*** Bug 45276 has been marked as a duplicate of this bug. ***

[Sam Weinig]

Created attachment 66670 [details]
Patch

[WebKit Review Bot]

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.

[Darin Adler]

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

[Sam Weinig]

(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.

[Sam Weinig]

> > 
> > > +    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.

[Sam Weinig]

Landed in r66897.