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

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Sam Weinig <sam at webkit.org>  2010-09-07 11:57:19 PST ---
(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.

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