[webkit-reviews] review granted: [Bug 113007] Unable to insert a paragraph in between some text whose previous sibling is a non-editable block. : [Attachment 194489] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Arpita Bahuguna
<arpitabahuguna at gmail.com>'s request for review:
Bug 113007: Unable to insert a paragraph in between some text whose previous
sibling is a non-editable block.
https://bugs.webkit.org/show_bug.cgi?id=113007

Attachment 194489: Patch
https://bugs.webkit.org/attachment.cgi?id=194489&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=194489&action=review


> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:375
> +		   visiblePos = positionBeforeNode(n);

I would have preferred to declare another local variable rather than re-using
visiblePos.
As confusingly named as visiblePos is, visiblePos is supposed to be
VisiblePosition of the insertion point.

> LayoutTests/editing/inserting/insert-paragraph-between-text-expected.txt:1
> +Testcase for bug <https://bugs.webkit.org/show_bug.cgi?id=113007> 113007:
Unable to insert a paragraph in between some text whose previous sibling is a
non-editable block.

We can use shorthand URL https://webkit.org/b/113007. Also, We probably don't
have both bug URL and the bug id.

> LayoutTests/editing/inserting/insert-paragraph-between-text.html:12
> +var sel = window.getSelection();

Please don't use abbreviations like sel. Spell out the whole word.

> LayoutTests/editing/inserting/insert-paragraph-between-text.html:13
> +sel.setPosition(test, 3);

More standardized way to do this is to use collapse instead. setPosition is a
WebKit extension.
Instead of saying 3, maybe we should just use test.childNodes.length?