[webkit-reviews] review denied: [Bug 61594] REGRESSION: Hitting enter in the middle of this span causes the cursor to go to the end of the span : [Attachment 97678] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Annie Sullivan
<sullivan at chromium.org>'s request for review:
Bug 61594: REGRESSION: Hitting enter in the middle of this span causes the
cursor to go to the end of the span
https://bugs.webkit.org/show_bug.cgi?id=61594

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

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

This patch looks great but I have a few questions and nits.  I'd say r- for now
since I'd like see change log and some comments being revised before this patch
is landed.

> Source/WebCore/ChangeLog:16
> +	   (WebCore::CompositeEditCommand::splitTree):
> +	       Implement new function. Like splitTreeToNode(), but picks start
node based on position so that text nodes are split correctly.

So we usually start sentences after "splitTree): " then wrap at wherever is
appropriate.  This line is probably too long.  You should also mention that
this new function replaces the old splitTreeToNode.

> Source/WebCore/editing/CompositeEditCommand.cpp:-1235
> -    ASSERT(start);

We should probably assert that start.containerNode is not null.

> Source/WebCore/editing/CompositeEditCommand.cpp:1243
> +    // For text nodes, handle the case where the offset is in the middle of
the node or at the end,
> +    // moving the start node to the beginning of the position, splitting the
node if necessary.

This comment repeats what code says.  You should either remove the comment or
explain why this is needed instead.

> Source/WebCore/editing/CompositeEditCommand.cpp:1245
> +	   // FIXME: startNode can be null if traverseNextNode returns null.
What should it be set to in that case?

If startNode ends up being null, then we're trying to split a node at the very
end of the tree.  In that case, we shouldn't be splitting the tree so this
should be fine.
I think you should remove this comment.

> Source/WebCore/editing/FormatBlockCommand.cpp:64
> -    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ?
start.deprecatedNode() : splitTreeToNode(start.deprecatedNode(),
nodeToSplitTo);
> +    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ?
start.deprecatedNode() : splitTree(firstPositionInNode(start.deprecatedNode()),
nodeToSplitTo);

So you can't just pass start here?  Also, you should probably call
firstPositionInOrBeforeNode instead.

> Source/WebCore/editing/InsertListCommand.cpp:-271
> -	   nextListChild = listChildNode->nextSibling();
> -	   previousListChild = listChildNode->previousSibling();
>      } else {
>	   // A paragraph is visually a list item minus a list marker.	The
paragraph will be moved.
>	   start = startOfParagraph(originalStart, CanSkipOverEditingBoundary);

>	   end = endOfParagraph(start, CanSkipOverEditingBoundary);
> -	   nextListChild =
enclosingListChild(end.next().deepEquivalent().deprecatedNode(), listNode);
> -	   ASSERT(nextListChild != listChildNode);
> -	   previousListChild =
enclosingListChild(start.previous().deepEquivalent().deprecatedNode(),
listNode);
> -	   ASSERT(previousListChild != listChildNode);

Nice to see code being removed!

> Source/WebCore/editing/InsertListCommand.cpp:277
> +    // Unless this is the first node in the list, the list needs to be
split.

Again, this comment seems to repeat code.  Can we explain why this needs to be
done instead?  If not, we should get rid of this comment.

> Source/WebCore/editing/InsertListCommand.cpp:282
> +    // If this is the only node in the list, insert the placeholder after
it, so that if splitAt
> +    // and the node before the placeholder are both text nodes, they will
not be merged. See
> +    // LayoutTests/editing/execCommand/switch-list-type.html

This comment seems a bit verbose.  I'd just say "insert a placeholder to avoid
texts nodes of splitAt and the node before the placeholder won't get merged"