Bug 62637 – [Qt] [WK2] Qt WebKit2 needs undo/redo support

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

[Chang Shu]

Many layout tests under editing/undo are failing.

[Chang Shu]

Created attachment 97121 [details]
fix patch

[Eric Seidel (no email)]

Comment on attachment 97121 [details]
fix patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97121&action=review

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:116
> +    delete undoStack;

Don't we have a smart pointer for these?

> Source/WebKit2/UIProcess/API/qt/qwkpage_p.h:142
> +    QUndoStack *undoStack;

I take it Qt style is to put the * close to the name instead of the type?  (That's the opposite of webkit style)

> Source/WebKit2/UIProcess/qt/WebUndoCommandQt.h:38
> +        WebUndoCommandQt(WTF::RefPtr<WebKit::WebEditCommandProxy> cmd, QUndoCommand *parent = 0);
> +#else
> +        WebUndoCommandQt(WTF::RefPtr<WebKit::WebEditCommandProxy> cmd);

In WebKit style we only name arguments when they provide clarity.  In this case, the cmd doesn't help understand the argument here.

[Yael]

Comment on attachment 97121 [details]
fix patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97121&action=review

I am not a reviewer, here are my comments.
Overall this patch is great!

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

prpCommand? Please choose meaningful names.

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

Extra space between WebUndoCommandQt and * .

>> Source/WebKit2/UIProcess/API/qt/qwkpage_p.h:142
>> +    QUndoStack *undoStack;
> 
> I take it Qt style is to put the * close to the name instead of the type?  (That's the opposite of webkit style)

We should be using webkit style, not Qt style here.

> Source/WebKit2/UIProcess/qt/WebUndoCommandQt.cpp:27
> +WebUndoCommandQt::WebUndoCommandQt(WTF::RefPtr<WebEditCommandProxy> cmd, QUndoCommand *parent)

ditto.

> Source/WebKit2/UIProcess/qt/WebUndoCommandQt.h:36
> +        WebUndoCommandQt(WTF::RefPtr<WebKit::WebEditCommandProxy> cmd, QUndoCommand *parent = 0);

ditto.

[Chang Shu]

Created attachment 97145 [details]
fix patch 2

[Kenneth Rohde Christiansen]

Comment on attachment 97145 [details]
fix patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=97145&action=review

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:116
> +    delete undoStack;

I agree that we should start using smart pointers. OwnPtr works quite well with Qt types, just like QScopedPointer. delete should be a thing of the past or at least you should explain why a smart pointer cannot be used

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

Who are freeing these?

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:234
> +    undoStack->clear();

Will this free everything in the stack? That is not normal behavior in webcore afaik so we might need a comment if that is Qt behavior

> Source/WebKit2/UIProcess/qt/WebUndoCommandQt.h:49
> +        bool m_inUndoRedo; // our undo stack works differently - don't re-enter!

Our*

Maybe a better variable name can be found.

[Chang Shu]

> > Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:116
> > +    delete undoStack;
> 
> I agree that we should start using smart pointers. OwnPtr works quite well with Qt types, just like QScopedPointer. delete should be a thing of the past or at least you should explain why a smart pointer cannot be used

Oops, forgot to address this.
> 
> > Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:226
> > +        undoStack->push(new WebUndoCommandQt(command));
> 
> Who are freeing these?
The free is taken care of by undoStack when it pops up the item.
> 
> > Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:234
> > +    undoStack->clear();
> 
> Will this free everything in the stack? That is not normal behavior in webcore afaik so we might need a comment if that is Qt behavior

This is what "WebKit1" does. See Source/WebKit/WebCoreSupport/EditorClientQt.cpp
> 
> > Source/WebKit2/UIProcess/qt/WebUndoCommandQt.h:49
> > +        bool m_inUndoRedo; // our undo stack works differently - don't re-enter!
> 
> Our*
> 
> Maybe a better variable name can be found.
Will clean up.

[Chang Shu]

Created attachment 97338 [details]
fix patch 3

[Andreas Kling]

Comment on attachment 97338 [details]
fix patch 3

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.

[Chang Shu]

Created attachment 97449 [details]
fix patch 4

[Sam Weinig]

Comment on attachment 97449 [details]
fix patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=97449&action=review

> Source/WebKit2/UIProcess/qt/WebUndoCommandQt.h:36
> +    WebUndoCommandQt(WTF::PassRefPtr<WebKit::WebEditCommandProxy>, QUndoCommand* parent = 0);

WTF:: not needed here.

> Source/WebKit2/UIProcess/qt/WebUndoCommandQt.h:38
> +    WebUndoCommandQt(WTF::PassRefPtr<WebKit::WebEditCommandProxy>);

WTF:: not needed here.

> Source/WebKit2/UIProcess/qt/WebUndoCommandQt.h:45
> +    bool inUndoRedo() const { return m_inUndoRedo; };

Missing newline after this function.

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

WTF:: not needed here.

[Chang Shu]

Created attachment 97457 [details]
fix patch 5

update r+ patch with minor fixes.

[WebKit Review Bot]

Comment on attachment 97457 [details]
fix patch 5

Clearing flags on attachment: 97457

Committed r89050: <http://trac.webkit.org/changeset/89050>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andreas Kling]

Comment on attachment 97457 [details]
fix patch 5

View in context: https://bugs.webkit.org/attachment.cgi?id=97457&action=review

> Source/WebKit2/UIProcess/qt/WebUndoCommandQt.cpp:51
> +    // Ignore the first redo called from QUndoStack::push().

I still don't know *why* this is needed. What is the problem you are solving by ignoring the first redo()?

[Chang Shu]

> > Source/WebKit2/UIProcess/qt/WebUndoCommandQt.cpp:51
> > +    // Ignore the first redo called from QUndoStack::push().
> I still don't know *why* this is needed. What is the problem you are solving by ignoring the first redo()?

My investigation shows the code path looks like this:
registerEditCommand --> undoStack->push(new WebUndoCommandQt) --> redo()
So once WebUndoCommandQt is created, a redo() is called before any undo happens. I guess this is why the first redo should be ignored.
However, I am not the original author of the code. The first version of the code is made by ddkilzer:
http://trac.webkit.org/browser/trunk/WebKitQt/WebCoreSupport/EditCommandQt.cpp?rev=25754