Bug 62536 - [EFL] Supports to execute "redo" command.
Summary: [EFL] Supports to execute "redo" command.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 62726
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-13 03:00 PDT by Jaehun Lim
Modified: 2011-07-11 22:58 PDT (History)
8 users (show)

See Also:


Attachments
Proposed patch (2.81 KB, patch)
2011-06-13 03:05 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff
Modified patch (3.59 KB, patch)
2011-06-15 16:47 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff
Modified patch (3.54 KB, patch)
2011-07-11 00:05 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff
Fix a trivial mistake. (3.54 KB, patch)
2011-07-11 00:08 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff
Patch (3.54 KB, patch)
2011-07-11 15:52 PDT, Jaehun Lim
rniwa: review+
Details | Formatted Diff | Diff
New patch (3.59 KB, patch)
2011-07-11 21:27 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaehun Lim 2011-06-13 03:00:21 PDT
This is an initial implementaion to support execCommand("undo") in Javascript.
Comment 1 Jaehun Lim 2011-06-13 03:05:30 PDT
Created attachment 96942 [details]
Proposed patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2011-06-13 06:44:25 PDT
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())
Comment 3 Eric Seidel (no email) 2011-06-13 14:40:34 PDT
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?
Comment 4 WebKit Review Bot 2011-06-13 15:45:38 PDT
Comment on attachment 96942 [details]
Proposed patch

Clearing flags on attachment: 96942

Committed r88723: <http://trac.webkit.org/changeset/88723>
Comment 5 WebKit Review Bot 2011-06-13 15:45:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Jaehun Lim 2011-06-13 16:03:13 PDT
(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.
Comment 7 Lucas De Marchi 2011-06-15 08:13:20 PDT
Re-opening this bug. Please address the comments #2 and #3.
Comment 8 Jaehun Lim 2011-06-15 16:47:05 PDT
Created attachment 97375 [details]
Modified patch
Comment 9 Jaehun Lim 2011-06-15 16:58:07 PDT
(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.
Comment 10 Raphael Kubo da Costa (:rakuco) 2011-06-16 07:12:32 PDT
It looks OK to me now. Informal r+. Please mark the previous patch as obsolete.
Comment 11 Jaehun Lim 2011-06-16 15:27:40 PDT
(In reply to comment #10)
> It looks OK to me now. Informal r+. Please mark the previous patch as obsolete.

Sorry, I missed it.
Comment 12 Gyuyoung Kim 2011-06-16 20:55:40 PDT
Informal review r+ on my side.
Comment 13 Ryosuke Niwa 2011-07-05 09:43:24 PDT
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.
Comment 14 Jaehun Lim 2011-07-11 00:05:04 PDT
Created attachment 100248 [details]
Modified patch

Use prepend() instead of append().
Comment 15 Jaehun Lim 2011-07-11 00:05:55 PDT
(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.
Comment 16 Jaehun Lim 2011-07-11 00:08:02 PDT
Created attachment 100249 [details]
Fix a trivial mistake.
Comment 17 Ryosuke Niwa 2011-07-11 09:22:33 PDT
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?
Comment 18 Raphael Kubo da Costa (:rakuco) 2011-07-11 09:52:41 PDT
(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.
Comment 19 Jaehun Lim 2011-07-11 15:52:16 PDT
Created attachment 100375 [details]
Patch
Comment 20 Ryosuke Niwa 2011-07-11 15:59:40 PDT
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.
Comment 21 Gyuyoung Kim 2011-07-11 20:33:29 PDT
(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().
Comment 22 Jaehun Lim 2011-07-11 21:17:52 PDT
(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.
Comment 23 Jaehun Lim 2011-07-11 21:27:56 PDT
Created attachment 100436 [details]
New patch
Comment 24 Gyuyoung Kim 2011-07-11 21:40:30 PDT
Comment on attachment 100436 [details]
New patch

LGTM. Ryuske, could you review this patch again ?
Comment 25 Ryosuke Niwa 2011-07-11 22:17:09 PDT
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 ? :(
Comment 26 WebKit Review Bot 2011-07-11 22:58:33 PDT
Comment on attachment 100436 [details]
New patch

Clearing flags on attachment: 100436

Committed r90808: <http://trac.webkit.org/changeset/90808>
Comment 27 WebKit Review Bot 2011-07-11 22:58:40 PDT
All reviewed patches have been landed.  Closing bug.