Undo of delete/forward-delete of text should not select the deleted text on non-mac platforms

Undo of delete/forward-delete of a word should not select the deleted word on non-mac platforms

Currently on deleting a word(character granularity or by ctrl+backspace)
and undoing it,the deleted word gets selected.
Instead it should not be selected and the caret should be placed at the
position from where the deletion was actually started.
This patch introduces this behavior change for non-mac platforms.

This is achieved by not setting a new starting selection after every
delete.

This aligns chrome behavior with that of IE and FF wrt selection of
deleted words.

R=ojan, yosin

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

[vanihegde, 2013-10-25 04:09:57]

I am little confused as to how to go about layout tests for this change.
There are already enough tests which check this scenario. Nevertheless, these
tests check mac editing behavior alone. Now that the behavior differs for
non-mac platforms I thought of replicating the existing tests for
any one(as all non-mac platforms are expected to have same behavior) non-mac
platform.

i.e. I would rename undo-combined-delete-boundary* as
undo-combined-delete-boundary-mac-behavior* and add similar test for say linux
as
undo-combined-delete-boundary-linux-behavior.html to test non-mac behavior.

In this patch, I have done this for
editing/undo/undo-combined-delete-boundary.html

If this approach is fine, I will go ahead and do the same for following tests as
well.
editing/undo/undo-combined-delete-boundary.html
editing/undo/undo-combined-delete.html
editing/undo/undo-delete-boundary.html
editing/undo/undo-delete.html
editing/undo/undo-forward-delete-boundary.html
editing/undo/undo-forward-delete.html

We can as well leave these tests as they are and add a new test to verify the
behavior change for other platforms.
But since the existing cases cover a good deal of scenarios it would be good
to test them for non-mac behavior as well. Hence choosing this approach.

I look forward to suggestions on the same. Thanks.

[yosin_UTC9, 2013-10-25 04:44:48]

ACK for C++

For layout tests, please use fast/js/resrouces/js-test-pre.js framework to use
platform independent tests, e.g. avoid to use  render tree dump, pixel dump,
etc.

We can use windows.internals.setEditingBehavior() to test "mac" and "win"
behavior in one platform.

[yosin_UTC9, 2013-10-25 04:47:33]

So, I suggest rewrite affected tests into platform independent in another patch.
Then, update expected text in this patch.

[vanihegde, 2013-10-25 04:53:47]

On 2013/10/25 04:47:33, Yoshifumi Inoue wrote:
> So, I suggest rewrite affected tests into platform independent in another
patch.
> Then, update expected text in this patch.

Thanks for the quick review.
Just to confirm that I have got it right, now we convert all affected currently
pixel based tests to text based.
This test conversion should go as the first patch and then update the expected
text based on new change in this patch.

[yosin_UTC9, 2013-10-25 05:28:59]

On 2013/10/25 04:53:47, vanihegde wrote:
> On 2013/10/25 04:47:33, Yoshifumi Inoue wrote:
> > So, I suggest rewrite affected tests into platform independent in another
> patch.
> > Then, update expected text in this patch.
> 
> Thanks for the quick review.
> Just to confirm that I have got it right, now we convert all affected
currently
> pixel based tests to text based.
> This test conversion should go as the first patch and then update the expected
> text based on new change in this patch.

Yes. Sorry for multiple steps. However, these steps makes review easier.

I did
 - https://codereview.chromium.org/18884002/ Make
editing/deleting/delete-br-00[1-7].html platform independent 
 - https://codereview.chromium.org/18799004 Don't insert redundant BR for
merging

[vanihegde, 2013-10-25 05:49:16]

On 2013/10/25 05:28:59, Yoshifumi Inoue wrote:
> On 2013/10/25 04:53:47, vanihegde wrote:
> > On 2013/10/25 04:47:33, Yoshifumi Inoue wrote:
> > > So, I suggest rewrite affected tests into platform independent in another
> > patch.
> > > Then, update expected text in this patch.
> > 
> > Thanks for the quick review.
> > Just to confirm that I have got it right, now we convert all affected
> currently
> > pixel based tests to text based.
> > This test conversion should go as the first patch and then update the
expected
> > text based on new change in this patch.
> 
> Yes. Sorry for multiple steps. However, these steps makes review easier.
> 
> I did
>  - https://codereview.chromium.org/18884002/ Make
> editing/deleting/delete-br-00[1-7].html platform independent 
>  - https://codereview.chromium.org/18799004 Don't insert redundant BR for
> merging

No issues. Thanks for the reference.

[ojan, 2013-10-26 01:36:00]

https://codereview.chromium.org/43143003/diff/1/Source/core/editing/EditingBe...
File Source/core/editing/EditingBehavior.h (right):

https://codereview.chromium.org/43143003/diff/1/Source/core/editing/EditingBe...
Source/core/editing/EditingBehavior.h:84: bool
shouldUndoOfWordDeletionSelectWord() const { return m_type ==
EditingMacBehavior; }
This name isn't quite right. It's not just word selections. How about
shouldUndoOfDeleteSelectText?

[vanihegde, 2013-10-28 04:27:38]

On 2013/10/26 01:36:00, ojan wrote:
>
https://codereview.chromium.org/43143003/diff/1/Source/core/editing/EditingBe...
> File Source/core/editing/EditingBehavior.h (right):
> 
>
https://codereview.chromium.org/43143003/diff/1/Source/core/editing/EditingBe...
> Source/core/editing/EditingBehavior.h:84: bool
> shouldUndoOfWordDeletionSelectWord() const { return m_type ==
> EditingMacBehavior; }
> This name isn't quite right. It's not just word selections. How about
> shouldUndoOfDeleteSelectText?

Sorry for the delayed response. shouldUndoOfDeleteSelectText sounds good.

[vanihegde, 2013-10-29 04:38:01]

Changed function name to 'shouldUndoOfDeleteSelectText' and updated the tests &
expectations as per changed behavior for non-mac platforms. Please have a look.
Thanks.

[yosin_UTC9, 2013-10-29 05:00:01]

ACK
Thanks!

[vanihegde, 2013-10-29 12:02:57]

If the changes are OK, can someone LGTM this? Thanks in advance.

[ojan, 2013-10-29 18:35:25]

lgtm

[commit-bot: I haz the power, 2013-10-29 18:56:29]

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

[commit-bot: I haz the power, 2013-10-29 22:22:43]

Change committed as 160868