Apply RelocatablePosition in DeleteSelectionCommand::mergeParagraphs

Apply RelocatablePosition in DeleteSelectionCommand::mergeParagraphs

The above mentioned function used a VisiblePosition to track
|startOfParagraphToMove|, which is wrong as VPs can't persist through
DOM mutations. This patch uses RelocatablePosition instead.

BUG=668765
TEST=webkit_unit_tests --gtest_filter=DeleteSelectionCommandTest.deleteListFromTable

Committed: https://crrev.com/e0c438edea7a5d6b057edff9a65d3d735c98b10e
Cr-Commit-Position: refs/heads/master@{#434976}

[Xiaocheng, 2016-11-28 07:28:22]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 07:28:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2016-11-28 07:30:59]

Description was changed from

==========
Stop DeleteSelection from setting ending selection to disconnected positions

BUG=668765
==========

to

==========
Apply RelocatablePosition in DeleteSelectionCommand::mergeParagraphs

The above mentioned function used a VisiblePosition to track
|startOfParagraphToMove|, which is wrong as VPs can't persist through
DOM mutations. This patch uses RelocatablePosition instead.

BUG=668765
TEST=webkit_unit_tests
--gtest_filter=DeleteSelectionCommandTest.deleteListFromTable
==========

[Xiaocheng, 2016-11-28 08:45:36]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 08:45:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2016-11-28 09:39:52]

xiaochengh@chromium.org changed reviewers:
+ yosin@chromium.org

[Xiaocheng, 2016-11-28 09:39:53]

PTAL.

[commit-bot: I haz the power, 2016-11-28 10:26:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 10:26:44]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2016-11-29 01:36:22]

yosin@chromium.org changed reviewers:
+ tkent@chromium.org

[yosin_UTC9, 2016-11-29 01:36:22]

lgtm w/ nits

+tkent@ for BUILD.gn change to add DeleteSelectionCommand.cpp

https://codereview.chromium.org/2534783002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommandTest.cpp
(right):

https://codereview.chromium.org/2534783002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommandTest.cpp:27:
"<div contenteditable=\"true\">"
nit: Let's use single-quote to avoid escaping double-quote.

https://codereview.chromium.org/2534783002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommandTest.cpp:55:
EXPECT_EQ("<div contenteditable=\"true\"><br></div>",
nit: Let's use single-quote to avoid escaping double-quote.

[tkent, 2016-11-29 01:43:19]

lgtm

https://codereview.chromium.org/2534783002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommandTest.cpp
(right):

https://codereview.chromium.org/2534783002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommandTest.cpp:50:
document(), kSmartDelete, kMergeBlocksAfterDelete,
|kSmartDelete| is confusing.  The code looks to enable smart-delete.
I recommend:
 - Rename kSmartDelete to kNoSmartDelete or something, or
 - Change kSmartDelete to true, and passing |!kSmartDelete| to
DeleteSelectionCommand::create.

https://codereview.chromium.org/2534783002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommandTest.cpp:51:
kExpandForSpecialElements, kSanitizeMarkup,
Ditto for kExpandForSpecialElements.

[Xiaocheng, 2016-11-29 02:00:34]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2016-11-29 02:00:35]

Thanks for the review!

https://codereview.chromium.org/2534783002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommandTest.cpp
(right):

https://codereview.chromium.org/2534783002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommandTest.cpp:27:
"<div contenteditable=\"true\">"
On 2016/11/29 at 01:36:22, Yosi_UTC9 wrote:
> nit: Let's use single-quote to avoid escaping double-quote.

Quotes removed.

https://codereview.chromium.org/2534783002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommandTest.cpp:50:
document(), kSmartDelete, kMergeBlocksAfterDelete,
On 2016/11/29 at 01:43:19, tkent wrote:
> |kSmartDelete| is confusing.  The code looks to enable smart-delete.
> I recommend:
>  - Rename kSmartDelete to kNoSmartDelete or something, or
>  - Change kSmartDelete to true, and passing |!kSmartDelete| to
DeleteSelectionCommand::create.

Renamed to |kNoSmartDelete|.

https://codereview.chromium.org/2534783002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommandTest.cpp:51:
kExpandForSpecialElements, kSanitizeMarkup,
On 2016/11/29 at 01:43:19, tkent wrote:
> Ditto for kExpandForSpecialElements.

Renamed to |kNoExpandForSpecialElements|.

https://codereview.chromium.org/2534783002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommandTest.cpp:55:
EXPECT_EQ("<div contenteditable=\"true\"><br></div>",
On 2016/11/29 at 01:36:22, Yosi_UTC9 wrote:
> nit: Let's use single-quote to avoid escaping double-quote.

Can't use single-quote for test expectation...

[Xiaocheng, 2016-11-29 02:00:35]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2534783002/#ps80001
(title: "fix nits")

[commit-bot: I haz the power, 2016-11-29 02:01:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 03:55:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 03:55:03]

Failed to apply patch for third_party/WebKit/Source/core/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: third_party/WebKit/Source/core/BUILD.gn:1159
  error: third_party/WebKit/Source/core/BUILD.gn: patch does not apply

Patch:       third_party/WebKit/Source/core/BUILD.gn
Index: third_party/WebKit/Source/core/BUILD.gn
diff --git a/third_party/WebKit/Source/core/BUILD.gn
b/third_party/WebKit/Source/core/BUILD.gn
index
de2b95e942a09619477e239d7032071842cddd1e..c7f097c28310e1704079bc5f856d7a6de0167c0e
100644
--- a/third_party/WebKit/Source/core/BUILD.gn
+++ b/third_party/WebKit/Source/core/BUILD.gn
@@ -1159,6 +1159,7 @@ source_set("unit_tests") {
     "editing/VisibleSelectionTest.cpp",
     "editing/VisibleUnitsTest.cpp",
     "editing/commands/ApplyBlockElementCommandTest.cpp",
+    "editing/commands/DeleteSelectionCommandTest.cpp",
     "editing/commands/InsertListCommandTest.cpp",
     "editing/commands/ReplaceSelectionCommandTest.cpp",
     "editing/iterators/BackwardsTextBufferTest.cpp",

[Xiaocheng, 2016-11-29 12:18:58]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2016-11-29 12:18:58]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2534783002/#ps90001
(title: "Tue Nov 29 21:12:11 JST 2016")

[commit-bot: I haz the power, 2016-11-29 12:19:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 13:50:37]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1480421938590000,
"parent_rev": "fe63afa268833e4c5eff827d4b0bce22bc180bf6", "commit_rev":
"b479f4eabfa1143bac21c927bce5662a78f2e003"}

[commit-bot: I haz the power, 2016-11-29 13:51:00]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-11-29 13:54:41]

Description was changed from

==========
Apply RelocatablePosition in DeleteSelectionCommand::mergeParagraphs

The above mentioned function used a VisiblePosition to track
|startOfParagraphToMove|, which is wrong as VPs can't persist through
DOM mutations. This patch uses RelocatablePosition instead.

BUG=668765
TEST=webkit_unit_tests
--gtest_filter=DeleteSelectionCommandTest.deleteListFromTable
==========

to

==========
Apply RelocatablePosition in DeleteSelectionCommand::mergeParagraphs

The above mentioned function used a VisiblePosition to track
|startOfParagraphToMove|, which is wrong as VPs can't persist through
DOM mutations. This patch uses RelocatablePosition instead.

BUG=668765
TEST=webkit_unit_tests
--gtest_filter=DeleteSelectionCommandTest.deleteListFromTable

Committed: https://crrev.com/e0c438edea7a5d6b057edff9a65d3d735c98b10e
Cr-Commit-Position: refs/heads/master@{#434976}
==========

[commit-bot: I haz the power, 2016-11-29 13:54:42]

Patchset 6 (id:??) landed as
https://crrev.com/e0c438edea7a5d6b057edff9a65d3d735c98b10e
Cr-Commit-Position: refs/heads/master@{#434976}