[webkit-reviews] review denied: [Bug 87463] VisibleSelection::validate() sometimes crosses editing boundaries when there is a shadow DOM : [Attachment 144519] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Shinya Kawanaka
<shinyak at chromium.org>'s request for review:
Bug 87463: VisibleSelection::validate() sometimes crosses editing boundaries
when there is a shadow DOM
https://bugs.webkit.org/show_bug.cgi?id=87463

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

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


> Source/WebCore/dom/Position.cpp:80
> +    Node* node = position.deprecatedNode();

Please try not to use deprecatedNode and/or anchorNode in new code unless
you're explicitly checking the position's type.
See https://rniwa.com/2011-06-26/position-and-anchor-types/

> Source/WebCore/dom/Position.cpp:85
> +    if (node->parentNode() && node->parentNode()->rendererIsEditable() !=
node->parentNode()->rendererIsEditable())

When could node->parentNode()->rendererIsEditable() ever be different from
node->parentNode()->rendererIsEditable()?
We definitely need a test case that executes this line. r- because of this.

> Source/WebCore/dom/Position.cpp:91
> +static PositionIterator visiblePositionIterator(const Position& position)

I don't understand what this function does. Please make the function name more
descriptive of what it does.

> Source/WebCore/dom/Position.cpp:96
> +	       return createLegacyEditingPosition(position.anchorNode(),
caretMaxOffset(position.anchorNode()));

Please don't instantiate legacy positions. I think you're not fixing the bug
properly if you have to instantiate legacy position here.

> Source/WebCore/dom/Position.cpp:109
> +    default:

In most cases, we prefer listing all values of enum in switch statements
instead of using the default label.