Fix for broken undo functionality

Fix for broken undo functionality

When delete(character granularity) is performed after few text insertions,
this delete command is added to already open typing command. This was
resulting in wrong 'undo' behaviors.

This can be fixed by keeping continuous delete operations alone as a group.
Hence whenever a delete command follows some other typing command say text
insertion, we should open a new typing command.

This fixes many erroneous behaviors related to undo operation.

TEST=LayoutTests/editing/undo/undo-insert-delete.html

BUG=319659
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162784

[vanihegde, 2013-11-15 08:30:14]

This fix is causing editing/undo/5378473.html to fail.

The test is as below
document.execCommand("InsertText", false, "foo");
document.execCommand("Delete");
document.execCommand("Undo");

Test as of now expects the text 'foo' to be deleted and it is wrong.
The actual output after all above operations should be 'foo'.

I have now added a new test(text based) and it covers whatever was being done
in 5378473.html(pixel test). So we can knock this off.
Let me know, I will update the patch accordingly.

[yosin_UTC9, 2013-11-15 08:58:30]

On 2013/11/15 08:30:14, vanihegde wrote:
> This fix is causing editing/undo/5378473.html to fail.
> 
> The test is as below
> document.execCommand("InsertText", false, "foo");
> document.execCommand("Delete");
> document.execCommand("Undo");
> 
> Test as of now expects the text 'foo' to be deleted and it is wrong.
> The actual output after all above operations should be 'foo'.
> 
> I have now added a new test(text based) and it covers whatever was being done
> in 5378473.html(pixel test). So we can knock this off.
> Let me know, I will update the patch accordingly.

ACK

Please update your patch to fix failure of existing tests and send to try bots.

[vanihegde, 2013-11-15 09:35:27]

On 2013/11/15 08:58:30, Yoshi wrote:
> On 2013/11/15 08:30:14, vanihegde wrote:
> > This fix is causing editing/undo/5378473.html to fail.
> > 
> > The test is as below
> > document.execCommand("InsertText", false, "foo");
> > document.execCommand("Delete");
> > document.execCommand("Undo");
> > 
> > Test as of now expects the text 'foo' to be deleted and it is wrong.
> > The actual output after all above operations should be 'foo'.
> > 
> > I have now added a new test(text based) and it covers whatever was being
done
> > in 5378473.html(pixel test). So we can knock this off.
> > Let me know, I will update the patch accordingly.
> 
> ACK
> 
> Please update your patch to fix failure of existing tests and send to try
bots.

As I said, this patch introduces a new test and we do not need the test
editing/undo/5378473.html(this is a pixel test). So shall I remove that?

[vanihegde, 2013-11-15 09:36:37]

By the way I don't see editing/undo/5378473.html testing anything significant.
The new test case covers it.

[yosin_UTC9, 2013-11-15 10:22:21]

It is better to introduce LayoutTests/editing/undo/undo-insert-delete.html with
removing 5378473.html.

https://codereview.chromium.org/65043014/diff/70001/LayoutTests/editing/undo/...
File LayoutTests/editing/undo/undo-insert-delete.html (right):

https://codereview.chromium.org/65043014/diff/70001/LayoutTests/editing/undo/...
LayoutTests/editing/undo/undo-insert-delete.html:25: selection.collapseToEnd();
//This is needed for this test to work on mac platform
nit: Should have one space after "//".

https://codereview.chromium.org/65043014/diff/70001/Source/core/editing/Typin...
File Source/core/editing/TypingCommand.cpp (right):

https://codereview.chromium.org/65043014/diff/70001/Source/core/editing/Typin...
Source/core/editing/TypingCommand.cpp:114: if (RefPtr<TypingCommand>
lastTypingCommand = lastTypingCommandIfStillOpenForTyping(frame)) {
 lastTypingCommandIfStillOpenForTyping() should check DeleteKey rather than
here.

https://codereview.chromium.org/65043014/diff/70001/Source/core/editing/Typin...
File Source/core/editing/TypingCommand.h (right):

https://codereview.chromium.org/65043014/diff/70001/Source/core/editing/Typin...
Source/core/editing/TypingCommand.h:121: ETypingCommand
commandTypeOfOpenCommand() { return m_commandType; }
nit: Should be have |const| qualifier.

[vanihegde, 2013-11-15 10:38:43]

On 2013/11/15 10:22:21, Yoshi wrote:
> It is better to introduce LayoutTests/editing/undo/undo-insert-delete.html
with
> removing 5378473.html.

Did you mean 'without' removing here?
If you feel the need of it, then I can convert it into text based test and later
update the behavior as per new patch.
But please note that it is not testing anything different than the new test is.
 
> 
>
https://codereview.chromium.org/65043014/diff/70001/LayoutTests/editing/undo/...
> File LayoutTests/editing/undo/undo-insert-delete.html (right):
> 
>
https://codereview.chromium.org/65043014/diff/70001/LayoutTests/editing/undo/...
> LayoutTests/editing/undo/undo-insert-delete.html:25:
selection.collapseToEnd();
> //This is needed for this test to work on mac platform
> nit: Should have one space after "//".
> Will do.
 
>
https://codereview.chromium.org/65043014/diff/70001/Source/core/editing/Typin...
> File Source/core/editing/TypingCommand.cpp (right):
> 
>
https://codereview.chromium.org/65043014/diff/70001/Source/core/editing/Typin...
> Source/core/editing/TypingCommand.cpp:114: if (RefPtr<TypingCommand>
> lastTypingCommand = lastTypingCommandIfStillOpenForTyping(frame)) {
>  lastTypingCommandIfStillOpenForTyping() should check DeleteKey rather than
> here.
Why should we check in lastTypingCommandIfStillOpenForTyping()? Wouldn't it be
wrong?
The command is still open when 'delete' was performed. We just have to choose
whether to
append to existing command or open a new one based on last command.
Since this is specific to 'DeleteKeyPressed' it would be wrong to add this check
elsewhere.

> 
>
https://codereview.chromium.org/65043014/diff/70001/Source/core/editing/Typin...
> File Source/core/editing/TypingCommand.h (right):
> 
>
https://codereview.chromium.org/65043014/diff/70001/Source/core/editing/Typin...
> Source/core/editing/TypingCommand.h:121: ETypingCommand
> commandTypeOfOpenCommand() { return m_commandType; }
> nit: Should be have |const| qualifier.
Will do

[yosin_UTC9, 2013-11-15 10:55:11]

https://codereview.chromium.org/65043014/diff/70001/Source/core/editing/Typin...
File Source/core/editing/TypingCommand.cpp (right):

https://codereview.chromium.org/65043014/diff/70001/Source/core/editing/Typin...
Source/core/editing/TypingCommand.cpp:114: if (RefPtr<TypingCommand>
lastTypingCommand = lastTypingCommandIfStillOpenForTyping(frame)) {
On 2013/11/15 10:22:22, Yoshi wrote:
>  lastTypingCommandIfStillOpenForTyping() should check DeleteKey rather than
> here.
I'm wrong. Please ignore this comment.

[vanihegde, 2013-11-15 11:14:28]

Added a patch with nits fixed. Please have a look.
It wasn't quite clear as what to do about the test 5378473.html. Please let me
know if you would like me to modify this to a text based test.

[yosin_UTC9, 2013-11-18 01:25:18]

ACK

Please add TEST=LayoutTests/editing/undo/undo-insert-delete.html in description.

[vanihegde, 2013-11-19 10:29:31]

LGTM please.

[ojan, 2013-11-27 23:09:22]

lgtm

[commit-bot: I haz the power, 2013-11-27 23:12:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vani.hegde@samsung.com/65043014/200001

[commit-bot: I haz the power, 2013-11-28 01:19:20]

Change committed as 162784