[webkit-reviews] review denied: [Bug 63816] Remove calls to deprecatedNode in EventHandler.cpp : [Attachment 99484] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 63816: Remove calls to deprecatedNode in EventHandler.cpp
https://bugs.webkit.org/show_bug.cgi?id=63816

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

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

> Source/WebCore/ChangeLog:14
> +	   (WebCore::EventHandler::updateSelectionForMouseDrag): Replaced the
call to deprecatedNode by a call to
> +	   containerNode because the node is only used to find the containing
block via its renderer.

I don’t understand why this explanation makes the change OK. Couldn’t the
container node have a different containing block than the anchor node?

> Source/WebCore/page/EventHandler.cpp:339
> -	   if (pos.isNotNull() &&
pos.deepEquivalent().deprecatedNode()->isDescendantOf(URLElement))
> +	   if (pos.isNotNull() &&
pos.deepEquivalent().containerNode()->isDescendantOf(URLElement))

I haven’t carefully watched other deprecatedNode/containerNode conversions in
the past, but this one changes behavior.

This will now give the wrong answer for a position that is a BeforeAnchor or
AfterAnchor of a node that is child of the URL element. The containerNode
function will return the URL element itself, and isDescandantOf will return
false.

The old expression would have been true in that case.

The old code already seems wrong for BeforeChildren and AfterChildren
positions.

The change in behavior may be harmless because positionForPoint never returns a
BeforeAnchor or AfterAnchor position.

The code doesn’t make much sense. Why is it interesting if the container node
is a descendant of the URL element? I think the real question is whether the
position is inside the URL element, and we should have a function that
correctly asks that question.

We’d probably need to change this from
deprecatedNode->isDescendantOf(URLElement) to
URLElement->contains(containerNode) to make it make sense.