[webkit-reviews] review denied: [Bug 62637] [Qt] [WK2] Qt WebKit2 needs undo/redo support : [Attachment 97338] fix patch 3

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Chang Shu <cshu at webkit.org>'s
request for review:
Bug 62637: [Qt] [WK2] Qt WebKit2 needs undo/redo support
https://bugs.webkit.org/show_bug.cgi?id=62637

Attachment 97338: fix patch 3
https://bugs.webkit.org/attachment.cgi?id=97338&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=97338&action=review

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:215
> +void QWKPagePrivate::registerEditCommand(PassRefPtr<WebEditCommandProxy>
commandPassRef, WebPageProxy::UndoOrRedo undoOrRedo)

commandPassRef is not a good variable name.

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:219
> +	   const WebUndoCommandQt* cmd = static_cast<const
WebUndoCommandQt*>(undoStack->command(undoStack->index()));

cmd is not a good variable name.

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:223
> +	   undoStack->push(new WebUndoCommandQt(command));

This is overly complicated.
You should make the WebUndoCommandQt ctor take a
PassRefPtr<WebEditCommandProxy> instead, and just pass on the PassRefPtr that
this function received.

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:240
> +    return (undoOrRedo == WebPageProxy::Undo) ? undoStack->canUndo() :
undoStack->canRedo();

This hurts my eyes, please use boring old if/else instead.

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:246
> +    (undoOrRedo == WebPageProxy::Undo) ? undoStack->undo() :
undoStack->redo();

Ditto (even moreso.)

> Source/WebKit2/UIProcess/qt/WebUndoCommandQt.cpp:56
> +    if (m_first) {
> +	   m_first = false;
> +	   m_inUndoRedo = false;
> +	   return;
> +    }

What is the purpose of this?
If this is needed, we definitely need a comment explaining why the first call
to redo() should be ignored.

> Source/WebKit2/UIProcess/qt/WebUndoCommandQt.h:34
> +    public:

Wrong indentation starting here.

> Source/WebKit2/UIProcess/qt/WebUndoCommandQt.h:47
> +	   WTF::RefPtr<WebKit::WebEditCommandProxy> m_cmd;

m_cmd -> m_command, we don't abbreviate in WebKit.