[webkit-reviews] review denied: [Bug 79197] Implement ReifiedDOMTree traversal APIs for Shadow DOM. : [Attachment 133178] fix win build hopefully.

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Hayato Ito
<hayato at chromium.org>'s request for review:
Bug 79197: Implement ReifiedDOMTree traversal APIs for Shadow DOM.
https://bugs.webkit.org/show_bug.cgi?id=79197

Attachment 133178: fix win build hopefully.
https://bugs.webkit.org/attachment.cgi?id=133178&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133178&action=review


This is really close to being done. A lot of awesome work, Hayato-san. I spent
a lot of time staring at this code, trying to make sure you're doing the right
thing. Overall, the machinery looks good. Pending a few naming/stylistic
comments and build fixes, we can land this.

One question: would ReifiedTreeTraversal be better viewed as a cursor pattern
device, like a Walker? Instead of all public functions being static, we give it
a node in constructor, then operate on that node with the functions. WDYT? Will
this be useful?

> Source/WebCore/dom/ReifiedTreeTraversal.cpp:83
> +    // FIXME: Add an assertion once InsertionPoint have isActive() function.


URL of the bug?

> Source/WebCore/dom/ReifiedTreeTraversal.cpp:90
> +    Node* child = traverseLightChildren(node, direction);
> +    return child;

Can just be return traverseLightChildren?

> Source/WebCore/dom/ReifiedTreeTraversal.cpp:101
> +Node* ReifiedTreeTraversal::traverseMe(const Node* node,
ReifiedTreeTraversalDirection direction)

traverseMe -> traverseNode?

> Source/WebCore/dom/ReifiedTreeTraversal.cpp:131
> +Node* ReifiedTreeTraversal::traverseSiblingMaybeBackToInsertionPoint(const
Node* node, ReifiedTreeTraversalDirection direction)

Maybe -> Or

> Source/WebCore/dom/ReifiedTreeTraversal.cpp:155
> +Node* ReifiedTreeTraversal::traverseBackingToYoungerShadowRoot(const Node*
node, ReifiedTreeTraversalDirection direction)

BackingTo -> BackTo? or BackInto?

> Source/WebCore/dom/ReifiedTreeTraversal.cpp:177
> +Node* ReifiedTreeTraversal::traverseMeEscapingFallbackContents(const Node*
node, CrossedUpperBoundary& crossed)

Me -> Node

> Source/WebCore/dom/ReifiedTreeTraversal.cpp:197
> +    // FIXME: Do not use 'unsed' parameter.

UNUSED_PARAM?

> Source/WebCore/dom/ReifiedTreeTraversal.cpp:216
> +Node* ReifiedTreeTraversal::parentNodeMaybeBackToInsertionPoint(const Node*
node, CrossedUpperBoundary& crossed)

Should you set crossed = Crossed at the start?

> Source/WebCore/dom/ReifiedTreeTraversal.cpp:227
> +    if (Node* parent = node->parentNode()) {

Ditto?

> Source/WebCore/dom/ReifiedTreeTraversal.h:40
> +// FIXME: Make some functions inline to optimise the performance.

File a bug and put URL here?

> Source/WebCore/dom/ReifiedTreeTraversal.h:62
> +    // FIXME: An 'adjusted' is not good name for general use case. Rename
this.

File a bug and put URL here?