Bug 61727 – Add a utility function which dumps a tree for the Node, including a document of a frame.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hayato Ito]

This is a separated patch from bug 59812.

https://bugs.webkit.org/show_bug.cgi?id=59812#c11

I don't need a public Node::parentOrHostOrFrameOwner function anymore, which is now a private function.
This patch now includes only one public member function, Node::showTreeForThisAcrossFrame, which is useful for debugging a complex document which contains iframe elements.
All other member functions are now private functions.

All functions are available only in debug build.

[Hayato Ito]

Created attachment 95342 [details]
dump also a document of a frame

[Ryosuke Niwa]

Comment on attachment 95342 [details]
dump also a document of a frame

View in context: https://bugs.webkit.org/attachment.cgi?id=95342&action=review

> Source/WebCore/dom/Node.cpp:851
> +inline static ShadowRoot* shadowRoot(const Node* node)

We don't normally use const Node* because constness goes away as soon as we walk the tree.

> Source/WebCore/dom/Node.cpp:2383
> +    if (!parent && document()->frame())
> +        parent = document()->frame()->ownerElement();

Shouldn't you be checking whether document() is null or not?

> Source/WebCore/dom/Node.cpp:2415
> +        if (node == markedNode)
> +            fprintf(stderr, "%s", markedLabel);

It seems like you can just do fputs("*", stderr);

> Source/WebCore/dom/Node.h:504
> +    // Like showTreeForThis, but also shows a document of a frame element.

Nit: I don't think this comment is necessary.  It repeats what the function name tells me.

[Hayato Ito]

Comment on attachment 95342 [details]
dump also a document of a frame

Thank you for the review.

View in context: https://bugs.webkit.org/attachment.cgi?id=95342&action=review

>> Source/WebCore/dom/Node.cpp:851
>> +inline static ShadowRoot* shadowRoot(const Node* node)
> 
> We don't normally use const Node* because constness goes away as soon as we walk the tree.

Done. I reverted this change.

>> Source/WebCore/dom/Node.cpp:2383
>> +        parent = document()->frame()->ownerElement();
> 
> Shouldn't you be checking whether document() is null or not?

Done. Added the null check.
It's possible that document() is null according to Node::document()'s comment.

>> Source/WebCore/dom/Node.cpp:2415
>> +            fprintf(stderr, "%s", markedLabel);
> 
> It seems like you can just do fputs("*", stderr);

Done. I removed a local variable, markedLabel.

>> Source/WebCore/dom/Node.h:504
>> +    // Like showTreeForThis, but also shows a document of a frame element.
> 
> Nit: I don't think this comment is necessary.  It repeats what the function name tells me.

Done. Removed.

[Hayato Ito]

Created attachment 95392 [details]
Addressed the review comments.

[Ryosuke Niwa]

Comment on attachment 95392 [details]
Addressed the review comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=95392&action=review

> Source/WebCore/ChangeLog:8
> +        https://bugs.webkit.org/show_bug.cgi?id=61727

Missing bug title.  Also, the bug URL + bug title need to appear before the description.

> Source/WebCore/dom/Node.cpp:2411
> +    const Node* node = this;
> +    while (node->parentOrHostOrFrameOwner())
> +        node = node->parentOrHostOrFrameOwner();
> +    const Node* rootNode = node;

Can you declare rootNode before the loop and walk up the tree using rootNode instead of node?
That way, you can declare node in the for loop as in:
for (Node* node = rootNode; node; node = node->traverseNextNodeAcrossFrame())

> Source/WebCore/dom/Node.cpp:2413
> +        if (node == markedNode)

You can just compare node with this here instead of declaring markedNode.

> Source/WebCore/dom/Node.cpp:2418
> +        fprintf(stderr, "%s", indent.utf8().data());

Come to think of it, there's no reason to use fprintf here.  You can just do fputs(indent.utf8().data(), stderr).

> Source/WebCore/dom/Node.h:722
> +    // In addition to parentOrHost, also up to a FrameOwnerElement of the current frame.
> +    ContainerNode* parentOrHostOrFrameOwner() const;
> +    // Like traversalNextNode, but traverses also into a document of a frame element.

Again, these comments don't add any values.

[Hayato Ito]

Comment on attachment 95392 [details]
Addressed the review comments.

Hi Ryosuke,
Thank you for the review, again.

View in context: https://bugs.webkit.org/attachment.cgi?id=95392&action=review

>> Source/WebCore/ChangeLog:8
>> +        https://bugs.webkit.org/show_bug.cgi?id=61727
> 
> Missing bug title.  Also, the bug URL + bug title need to appear before the description.

Done.
I am assuming that we cannot have an empty line between a bug title (one line is preferred?) and bug URL.
I omitted the description because this is trivial change and the bug title tells well.

>> Source/WebCore/dom/Node.cpp:2411
>> +    const Node* rootNode = node;
> 
> Can you declare rootNode before the loop and walk up the tree using rootNode instead of node?
> That way, you can declare node in the for loop as in:
> for (Node* node = rootNode; node; node = node->traverseNextNodeAcrossFrame())

Sure. That's better.

>> Source/WebCore/dom/Node.cpp:2413
>> +        if (node == markedNode)
> 
> You can just compare node with this here instead of declaring markedNode.

Done

>> Source/WebCore/dom/Node.cpp:2418
>> +        fprintf(stderr, "%s", indent.utf8().data());
> 
> Come to think of it, there's no reason to use fprintf here.  You can just do fputs(indent.utf8().data(), stderr).

Done.

>> Source/WebCore/dom/Node.h:722
>> +    // Like traversalNextNode, but traverses also into a document of a frame element.
> 
> Again, these comments don't add any values.

Okay. I just tried to follow the convention of this file, which has a lot of comments.
I agree with you. The comments don't add any values here, and they are private member fucntions. I removed the comments.

[Hayato Ito]

Created attachment 95398 [details]
Addressed the review comments.

[Ryosuke Niwa]

Comment on attachment 95398 [details]
Addressed the review comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=95398&action=review

> Source/WebCore/dom/Node.cpp:2387
> +ContainerNode* Node::parentOrHostOrFrameOwner() const
> +{
> +    ContainerNode* parent = parentOrHostNode();
> +    if (!parent && document() && document()->frame())
> +        parent = document()->frame()->ownerElement();
> +    return parent;
> +}
> +
> +Node* Node::traverseNextNodeAcrossFrame() const

On my second thought, these functions don't even need to be member functions since they don't access any private functions / variables and are only used by showTreeForThisAcrossFrame.  Please make them static local functions before you land.

[Hayato Ito]

Thank you for the review again.

(In reply to comment #8)
> (From update of attachment 95398 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95398&action=review
> 
> > Source/WebCore/dom/Node.cpp:2387
> > +ContainerNode* Node::parentOrHostOrFrameOwner() const
> > +{
> > +    ContainerNode* parent = parentOrHostNode();
> > +    if (!parent && document() && document()->frame())
> > +        parent = document()->frame()->ownerElement();
> > +    return parent;
> > +}
> > +
> > +Node* Node::traverseNextNodeAcrossFrame() const
> 
> On my second thought, these functions don't even need to be member functions since they don't access any private functions / variables and are only used by showTreeForThisAcrossFrame.  Please make them static local functions before you land.

Okay. I'll land this patch after I make them static local functions.

[Hayato Ito]

Committed r87716: <http://trac.webkit.org/changeset/87716>