[webkit-reviews] review denied: [Bug 82021] Position should be able to have ShadowRoot as a container. : [Attachment 140698] 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 82021: Position should be able to have ShadowRoot as a container.
https://bugs.webkit.org/show_bug.cgi?id=82021

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

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


Please fix all the nits below and builds before you land.

> Source/WebCore/dom/Position.cpp:114
> +    ASSERT(!m_anchorNode || !editingIgnoresContent(m_anchorNode.get()));

If RuntimeEnabledFeatures::shadowDOMEnabled() is false, then m_anchorNode
should not be a shadow root.

>> Source/WebCore/dom/Position.h:221
>> +inline ContainerNode* findParent(const Node* node)
> 
> A free-standing function with a generic name like this seems like a bad
thing. Can it be a static member of Position?

Yes, please make this a private static member of Position. I would also rename
it to something like parent(). I don't think we want to prefix it with "find"
because it doesn't really search for the parent.

> Source/WebCore/dom/Position.h:227
> +    // FIXME: Since enabling Position to have ShadowRoot as a container is
not proved not to
> +    // break editing code. However, this is really required for editing in
Shadow DOM.
> +    // So we implement this behind the flag. After writing a lot of tests
for them to confirm
> +    // this is safe, we will remove the flag for this purpose.
> +    // See also this umbrella bugs. http://wkb.ug/82697

This comment just repeats what the code says. You should just say "See
http://webkit.org/b/82697" IMO.