[Editing][BugFix] Check editability in InserListCommand::listifyParagraph

[Editing][BugFix] Check editability in InserListCommand::listifyParagraph

The listifyParagraph(pos,listtag) function roughly does:
 1. Insert a listtag(ol or ul) element.
 2. Move a paragraph over pos into the list element.
Then the paragraph should be editable. Without that check, attached layout test failed
 in insertNodeAt at L414.

Main change is on L365 and other are about renaming

BUG=574228

[yoichio, 2016-01-12 05:41:15]

Description was changed from

==========
Check editability in InserListCommand::listifyParagraph

BUG=574228
==========

to

==========
[Editing][BugFix] Check editability in InserListCommand::listifyParagraph

The listifyParagraph(pos,listtag) function roughly does:
 1. Insert a listtag(ol or ul) element.
 2. Move a paragraph over pos into the list element.
Then the paragraph should be editable. Without that check, attached layout test
failed
 in insertNodeAt at L414.

Main change is on L365 and other are about renaming.

BUG=574228
==========

[yoichio, 2016-01-12 05:41:16]

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

[yosin_UTC9, 2016-01-12 05:51:44]

https://codereview.chromium.org/1570733002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/editing/execCommand/insertunorderedlist-crash-in-domsubtreemodified-event.html
(right):

https://codereview.chromium.org/1570733002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/editing/execCommand/insertunorderedlist-crash-in-domsubtreemodified-event.html:10:
document.addEventListener("DOMSubtreeModified", function (event) {
Can we repro this w/o mutation event handler?

https://codereview.chromium.org/1570733002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/editing/execCommand/insertunorderedlist-crash-in-domsubtreemodified-event.html:11:
document.execCommand('InsertUnorderedList');
It seems this execCommand apples to non-editable element.

https://codereview.chromium.org/1570733002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp
(right):

https://codereview.chromium.org/1570733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp:367:
Position startPosition = start.toParentAnchoredPosition();
nit: |const Position&|

https://codereview.chromium.org/1570733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp:368:
Position endPosition = end.toParentAnchoredPosition();
nit: |const Position&|

https://codereview.chromium.org/1570733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp:384:
RefPtrWillBeRawPtr<HTMLElement> newListElementToBeInserted = nullptr;
It seems renaming |listElement| to |lnewListElementToBeInserted| doesn't relate
to fix the bug.
Please have separate CL for renaming.

https://codereview.chromium.org/1570733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp:427: const
VisiblePosition paragraphStartToInsert = startOfParagraph(start,
CanSkipOverEditingBoundary);
nit: |const VisiblePosition&|
This is kind of renaming variables and not relate to fix the bug, please have a
separate CL.

[yoichio, 2016-01-12 07:06:58]

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

[commit-bot: I haz the power, 2016-01-12 07:07:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1570733002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1570733002/20001

[yosin_UTC9, 2016-01-12 08:44:49]

C++ changes seems good.
I would like to get replies for comments about layout test in Patch 1.

[commit-bot: I haz the power, 2016-01-12 08:52:01]

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

[commit-bot: I haz the power, 2016-01-12 08:52:02]

Dry run: This issue passed the CQ dry run.

[yoichio, 2016-01-13 02:11:50]

Description was changed from

==========
[Editing][BugFix] Check editability in InserListCommand::listifyParagraph

The listifyParagraph(pos,listtag) function roughly does:
 1. Insert a listtag(ol or ul) element.
 2. Move a paragraph over pos into the list element.
Then the paragraph should be editable. Without that check, attached layout test
failed
 in insertNodeAt at L414.

Main change is on L365 and other are about renaming.

BUG=574228
==========

to

==========
[Editing][BugFix] Check editability in InserListCommand::listifyParagraph

The listifyParagraph(pos,listtag) function roughly does:
 1. Insert a listtag(ol or ul) element.
 2. Move a paragraph over pos into the list element.
Then the paragraph should be editable. Without that check, attached layout test
failed
 in insertNodeAt at L414.

Main change is on L365 and other are about renaming

BUG=574228
==========

[yoichio, 2016-01-13 06:56:59]

The CQ bit was checked by yoichio@chromium.org

[yosin_UTC9, 2016-01-28 04:38:39]

It seems crbug.com/574228 no more crash, according to both CF and my local
desktop execution.

Since, crrev.com/1612983003 passes updated selection to execCommand(),
InsertListCommand doesn't fall into situation describe in a patch description.