[webkit-reviews] review granted: [Bug 87428] Indenting a paragraph that begins with a link 3 times breaks the paragraph into two paragraphs : [Attachment 145317] Patch (with change from comment 2)

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Shezan Baig
<shezbaig.wk at gmail.com>'s request for review:
Bug 87428: Indenting a paragraph that begins with a link 3 times breaks the
paragraph into two paragraphs
https://bugs.webkit.org/show_bug.cgi?id=87428

Attachment 145317: Patch (with change from comment 2)
https://bugs.webkit.org/attachment.cgi?id=145317&action=review

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


Please consider addressing the comments below before landing it.

> Source/WebCore/ChangeLog:11
> +	   Fix the way lastNode (our insertion point) is updated whenever
> +	   traverseNextSibling moves up to a new parent, so that the relative
> +	   depth between the next sibling and the original start node is
> +	   maintained in the clone.

Thanks for the revision. This description is much more helpful. However, it
would be ideal if you could also explain why the divergence in the depth leads
to breaking a single paragraph into two paragraphs.

> Source/WebCore/editing/CompositeEditCommand.cpp:1019
> +	       // If traverseNextSibling required moving up to a new parent,
this
> +	       // means that node is not a true sibling of startNode, i.e.
> +	       // node->parent() is not the same as startNode->parent(). We
need
> +	       // to adjust lastNode (our insertion point) so that the relative

> +	       // depth between clonedNode (below) and the clone of
> +	       // start->deprecatedNode() will be the same as the relative
depth
> +	       // between node and start->deprecatedNode(). We do this by
moving
> +	       // lastNode up the same number of times that traverseNextSibling

> +	       // moved up, i.e. until node->parent() is the same as
> +	       // startNode->parent().

This comment is very verbose. A single line comment like "Move lastNode up in
the tree as much as node was moved up in the tree by traverseNextSibling" would
do.