[webkit-reviews] review granted: [Bug 51389] Selection becomes stale when CharacterData is manipulated directly : [Attachment 77367] new approach without splitText fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 51389: Selection becomes stale when CharacterData is manipulated directly
https://bugs.webkit.org/show_bug.cgi?id=51389

Attachment 77367: new approach without splitText fix
https://bugs.webkit.org/attachment.cgi?id=77367&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=77367&action=review

> WebCore/dom/CharacterData.cpp:154
> +    if (document() && document()->frame() &&
document()->frame()->selection())
> +	   document()->frame()->selection()->textWillBeReplaced(this,
offsetOfReplacedData, oldLength, newLength);

We do need to null-check document()->frame(), but we do not need to null-check
document() or selection().

> WebCore/editing/SelectionController.cpp:263
> +    if (position.offsetInContainerNode() > static_cast<int>(offset) &&
position.offsetInContainerNode() < static_cast<int>(offset + oldLength))

I suggest putting the typecast on the other side. It’s easy to be sure that
casting a non-negative int to unsigned is safe. Harder to know that casting an
unsigned to an int is safe.

> WebCore/editing/SelectionController.cpp:276
> +    if (isNone() || !node || highestAncestor(node)->nodeType() ==
Node::DOCUMENT_FRAGMENT_NODE)
> +	   return;

I think it might be better to check highestAncestor(node) != node->document()
rather than checking the type of the highest ancestor. But is that check
needed? Won’t the logic do the right thing without a special case for that?

> WebCore/editing/SelectionController.h:181
> +    void respondToNodeModification(Node* node, bool baseRemoved, bool
extentRemoved, bool startRemoved, bool endRemoved);

I suggest omitting the argument name “node” here.