Bug 62536 – [EFL] Supports to execute "redo" command.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Jaehun Lim]

This is an initial implementaion to support execCommand("undo") in Javascript.

[Jaehun Lim]

Created attachment 96942 [details]
Proposed patch

[Raphael Kubo da Costa (:rakuco)]

The code looks like the one in the GTK+ port but without some additional checks (for m_isInRedo, for example). I suggest porting those checks as well.

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:150
> +    redoStack.clear();

Perhaps it might be good to check if you're not inside a redo operation?

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:194
> +    RefPtr<WebCore::EditCommand> command(*(--redoStack.end()));

What if the stack is empty? You should probably enclose this block of code inside an if (canRedo())

[Eric Seidel (no email)]

Comment on attachment 96942 [details]
Proposed patch

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

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:194
> +    RefPtr<WebCore::EditCommand> command(*(--redoStack.end()));

So this will never be called when canRedo is false I assume?

[WebKit Review Bot]

Comment on attachment 96942 [details]
Proposed patch

Clearing flags on attachment: 96942

Committed r88723: <http://trac.webkit.org/changeset/88723>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jaehun Lim]

(In reply to comment #2)
> The code looks like the one in the GTK+ port but without some additional checks (for m_isInRedo, for example). I suggest porting those checks as well.
> 
> > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:150
> > +    redoStack.clear();
> 
> Perhaps it might be good to check if you're not inside a redo operation?
> 
> > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:194
> > +    RefPtr<WebCore::EditCommand> command(*(--redoStack.end()));
> 
> What if the stack is empty? You should probably enclose this block of code inside an if (canRedo())

I missed some checking codes as you said.
I'll make another patch soon. Thanks.

[Lucas De Marchi]

Re-opening this bug. Please address the comments #2 and #3.

[Jaehun Lim]

Created attachment 97375 [details]
Modified patch

[Jaehun Lim]

(In reply to comment #2)
> The code looks like the one in the GTK+ port but without some additional checks (for m_isInRedo, for example). I suggest porting those checks as well.
> 
> > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:150
> > +    redoStack.clear();
> 
> Perhaps it might be good to check if you're not inside a redo operation?
> 
> > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:194
> > +    RefPtr<WebCore::EditCommand> command(*(--redoStack.end()));
> 
> What if the stack is empty? You should probably enclose this block of code inside an if (canRedo())

I added some codes for status checking using m_isInRedo or canRedo() as you said.

[Raphael Kubo da Costa (:rakuco)]

It looks OK to me now. Informal r+. Please mark the previous patch as obsolete.

[Jaehun Lim]

(In reply to comment #10)
> It looks OK to me now. Informal r+. Please mark the previous patch as obsolete.

Sorry, I missed it.

[Gyuyoung Kim]

Informal review r+ on my side.

[Ryosuke Niwa]

Comment on attachment 97375 [details]
Modified patch

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

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:157
> -    notImplemented();
> +    redoStack.append(command);

Why don't you prepend here instead?

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:190
> +        RefPtr<WebCore::EditCommand> command(*(--undoStack.end()));
> +        undoStack.remove(--undoStack.end());

so that you can use undoStack.first() and undoStack.removeFirst() here instead.

[Jaehun Lim]

Created attachment 100248 [details]
Modified patch

Use prepend() instead of append().

[Jaehun Lim]

(In reply to comment #13)
> (From update of attachment 97375 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97375&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:157
> > -    notImplemented();
> > +    redoStack.append(command);
> 
> Why don't you prepend here instead?
> 
> > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:190
> > +        RefPtr<WebCore::EditCommand> command(*(--undoStack.end()));
> > +        undoStack.remove(--undoStack.end());
> 
> so that you can use undoStack.first() and undoStack.removeFirst() here instead.

I applied your comments. Thanks.

[Jaehun Lim]

Created attachment 100249 [details]
Fix a trivial mistake.

[Ryosuke Niwa]

Comment on attachment 100249 [details]
Fix a trivial mistake.

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

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:203
> +        RefPtr<WebCore::EditCommand> command(undoStack.takeFirst());

Nit: I think we normally use assignment operator here.

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:211
> +        RefPtr<WebCore::EditCommand> command(redoStack.takeFirst());

Ditto.

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:214
> +        ASSERT(!m_isInRedo);
> +        m_isInRedo = true;

Do we really need to do this?  We don't do this for undo, right?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #17)
> > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:214
> > +        ASSERT(!m_isInRedo);
> > +        m_isInRedo = true;
> 
> Do we really need to do this?  We don't do this for undo, right?

This one was probably my suggestion, as this is exactly what the gtk port does. It shouldn't hurt IMO.

[Jaehun Lim]

Created attachment 100375 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 100375 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:214
> +        ASSERT(!m_isInRedo);
> +        m_isInRedo = true;

I'm still not happy about the fact there's asymmetry between undo and redo.

[Gyuyoung Kim]

(In reply to comment #20)
> (From update of attachment 100375 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100375&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:214
> > +        ASSERT(!m_isInRedo);
> > +        m_isInRedo = true;
> 
> I'm still not happy about the fact there's asymmetry between undo and redo.

It looks the m_isInRedo flag is used by registerCommandForUndo() in GTK port.

http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp#L354

So, if we don't know why redoStack is cleared when m_isInRedo is false, we should not add the m_isInRedo to redo().

[Jaehun Lim]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 100375 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=100375&action=review
> > 
> > > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:214
> > > +        ASSERT(!m_isInRedo);
> > > +        m_isInRedo = true;
> > 
> > I'm still not happy about the fact there's asymmetry between undo and redo.
> 
> It looks the m_isInRedo flag is used by registerCommandForUndo() in GTK port.
> 
> http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp#L354
> 
> So, if we don't know why redoStack is cleared when m_isInRedo is false, we should not add the m_isInRedo to redo().

I think we should check redo status when a command is added into undoStack.

In general, when a new command is added into undoStack, commands in redoStack are useless, So redoStack should be cleared.
But when a command is moved from redoStack to undoStack by redo function, we should not clear the redoStack, because user could redo next commands in redoStack continuously.

But current registerCommandForUndo() has no redostack clearing codes.
I'll make a new patch.

[Jaehun Lim]

Created attachment 100436 [details]
New patch

[Gyuyoung Kim]

Comment on attachment 100436 [details]
New patch

LGTM. Ryuske, could you review this patch again ?

[Ryosuke Niwa]

Comment on attachment 100436 [details]
New patch

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

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:167
> +    if (!m_isInRedo)
> +        redoStack.clear();

Now, this makes a lot of sense!

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:218
> +        m_isInRedo = true;
> +        command->reapply();
> +        m_isInRedo = false;

I must say that this is likely broken if scripts run execCommand in mutation events but I suppose all other ports have the same issue ? :(

[WebKit Review Bot]

Comment on attachment 100436 [details]
New patch

Clearing flags on attachment: 100436

Committed r90808: <http://trac.webkit.org/changeset/90808>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.