[webkit-reviews] review granted: [Bug 113007] Unable to insert a paragraph in between some text whose previous sibling is a non-editable block. : [Attachment 194800] 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 194800: Patch
https://bugs.webkit.org/attachment.cgi?id=194800&action=review

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


> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:374
> +	       VisiblePosition posBeforeNode;

Please spell out position instead of using abbreviations like "pos". I know the
existing code uses it but we're trying to remove that usage.

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

Can't we declare VisiblePosition here?

> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:378
> +		   if (!posBeforeNode.isNull()) {
> +		       if (comparePositions(VisiblePosition(insertionPosition),
posBeforeNode) <= 0)

These two if-statements could be combined.

> LayoutTests/editing/inserting/insert-paragraph-between-text.html:18
> +Markup.description('Testcase for bug https://webkit.org/b/113007: Unable to
insert a paragraph in between some text whose previous sibling is a
non-editable block.\n'+
> +'The test has passed if three lines are displayed instead of two, with the
last line consisting of the letter "e".');

I would prefer seeing this description before the test code so that I can read
it before reading the test code.