[webkit-reviews] review denied: [Bug 82698] ShadowRoot.selection should return the selection whose range is in a shadow tree. : [Attachment 141662] 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 82698: ShadowRoot.selection should return the selection whose range is in a
shadow tree.
https://bugs.webkit.org/show_bug.cgi?id=82698

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

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


> Source/WebCore/dom/TreeScope.cpp:145
> +    if (this != rootNode()->document())
> +	   return rootNode()->document()->getSelection();

Why are you making this change? You should at least explain why you're making
this change in the change log.

> Source/WebCore/dom/TreeScopeAdjuster.cpp:47
> -    do {
> +    while (node) {
>	   if (node->treeScope() == treeScope())
>	       return node;
>	   if (!node->isInShadowTree())
>	       return 0;
> -    } while ((node = node->shadowAncestorNode()));
> +	   node = node->shadowAncestorNode();
> +    }

Why are you re-writing this do-while loop to a while loop?
Again, this needs to be explained somewhere.

> Source/WebCore/page/DOMSelection.cpp:503
> +Node* DOMSelection::adjustedNode(const Position& position) const

I would call it shadowAdjustedNode so that I know with respect to what it is
adjusted.

> Source/WebCore/page/DOMSelection.cpp:509
> +    Node* adjustedNode =
TreeScopeAdjuster(m_treeScope).ancestorInThisScope(containerNode);

Why do we need a class for this? All these proliferations of tiny classes is
going to increase our build time.
It should have been much better if this was just a function in TreeScope,
VisibleSelection, or even in htmlediting.

> LayoutTests/editing/shadow/selection-of-shadowroot-expected.txt:4
> +PASS divs[0] is selection.anchorNode.parentNode
> +PASS anchorTreeScopeRoot is internals.treeScopeRootNode(selection.focusNode)

> +PASS anchorTreeScopeRoot is internals.treeScopeRootNode(selection.baseNode)
> +PASS anchorTreeScopeRoot is
internals.treeScopeRootNode(selection.extentNode)

These outputs don't tell me what this test is testing.
Ideal test outputs makes what and why we're asserting self-evident.