Avoid adding placeholder when deleting last text in root

Avoid adding placeholder when deleting last text in root

Editing code uses placeholders for all manner of different functions including
holding open content editable blocks. When there's no content in the blocks except
text, these placeholders aren't necessary and is confusing Android's IME path.

Android's IME path should probably just be fixed, but in the meantime, this
creates fewer placeholders, which is definitely an improvement.

The previous version of this patch was rolled out due to crashes, see
https://codereview.chromium.org/14969020. This patch adds a check in
cleanupAfterDeletion to ensure we don't remove ancestors of our destination node,
and yosin@ landed another fix in https://codereview.chromium.org/16053005/.

BUG=222806, 236726
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154347

[ojan, 2013-07-11 23:37:23]

lgtm

[ojan, 2013-07-11 23:37:46]

https://codereview.chromium.org/18644008/diff/1/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/18644008/diff/1/LayoutTests/TestExpectations#...
LayoutTests/TestExpectations:1378: crbug.com/222806
editing/deleting/delete-block-contents-001.html [ Failure Pass ImageOnlyFailure
]
NeedsRebaseline?

[yosin_UTC9, 2013-07-12 01:55:28]

https://chromiumcodereview.appspot.com/18644008/diff/1/Source/core/editing/Co...
File Source/core/editing/CompositeEditCommand.cpp (right):

https://chromiumcodereview.appspot.com/18644008/diff/1/Source/core/editing/Co...
Source/core/editing/CompositeEditCommand.cpp:1054: // Bail if we'd remove an
ancestor of our destination.
Could you move this change in another CL? I think this change doesn't relate to
avoid adding placeholder. It seems more tests affected than original patch.

This change may change behavior, existing test failures reveal. I may try this,
but not sure...

[leviw_travelin_and_unemployed, 2013-07-12 21:12:40]

On 2013/07/12 01:55:28, Yoshifumi Inoue wrote:
>
https://chromiumcodereview.appspot.com/18644008/diff/1/Source/core/editing/Co...
> File Source/core/editing/CompositeEditCommand.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/18644008/diff/1/Source/core/editing/Co...
> Source/core/editing/CompositeEditCommand.cpp:1054: // Bail if we'd remove an
> ancestor of our destination.
> Could you move this change in another CL? I think this change doesn't relate
to
> avoid adding placeholder. It seems more tests affected than original patch.
> 
> This change may change behavior, existing test failures reveal. I may try
this,
> but not sure...

Without this change we crash the test you added. It can't be landed without it.

[leviw_travelin_and_unemployed, 2013-07-12 21:13:00]

On 2013/07/12 21:12:40, Levi wrote:
> On 2013/07/12 01:55:28, Yoshifumi Inoue wrote:
> >
>
https://chromiumcodereview.appspot.com/18644008/diff/1/Source/core/editing/Co...
> > File Source/core/editing/CompositeEditCommand.cpp (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/18644008/diff/1/Source/core/editing/Co...
> > Source/core/editing/CompositeEditCommand.cpp:1054: // Bail if we'd remove an
> > ancestor of our destination.
> > Could you move this change in another CL? I think this change doesn't relate
> to
> > avoid adding placeholder. It seems more tests affected than original patch.
> > 
> > This change may change behavior, existing test failures reveal. I may try
> this,
> > but not sure...
> 
> Without this change we crash the test you added. It can't be landed without
it.

It's also never the right behavior to remove an ancestor of the placeholder.

[leviw_travelin_and_unemployed, 2013-07-15 19:12:51]

Yosin, do you still object to this? Can you explain how you expect this to
change results?

[commit-bot: I haz the power, 2013-07-16 17:17:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leviw@chromium.org/18644008/1

[commit-bot: I haz the power, 2013-07-16 17:17:46]

Can't process patch for file
LayoutTests/platform/win/editing/deleting/delete-block-contents-001-expected.png.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[leviw_travelin_and_unemployed, 2013-07-16 17:17:55]

I'm moving forward for the time being. yosin@, let me know if you want to
discuss this.

[commit-bot: I haz the power, 2013-07-16 23:00:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leviw@chromium.org/18644008/20001

[commit-bot: I haz the power, 2013-07-17 01:42:48]

Change committed as 154347

[yosin_UTC9, 2013-07-17 02:14:49]

On 2013/07/15 19:12:51, Levi wrote:
> Yosin, do you still object to this? Can you explain how you expect this to
> change results?

Sorry for late response. I was vacation on July 16.
It is OK to commit as is. I'd just wanted to know which tests were affected by
changes in CompositeEditCommand.cpp.
Anyway, this isn't big matter people other than me. ;-)