WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
61727
Add a utility function which dumps a tree for the Node, including a document of a frame.
https://bugs.webkit.org/show_bug.cgi?id=61727
Summary
Add a utility function which dumps a tree for the Node, including a document ...
Hayato Ito
Reported
2011-05-30 06:03:13 PDT
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.
Attachments
dump also a document of a frame
(4.52 KB, patch)
2011-05-30 06:10 PDT
,
Hayato Ito
no flags
Details
Formatted Diff
Diff
Addressed the review comments.
(4.13 KB, patch)
2011-05-30 20:51 PDT
,
Hayato Ito
no flags
Details
Formatted Diff
Diff
Addressed the review comments.
(3.81 KB, patch)
2011-05-30 23:01 PDT
,
Hayato Ito
rniwa
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Hayato Ito
Comment 1
2011-05-30 06:10:11 PDT
Created
attachment 95342
[details]
dump also a document of a frame
Ryosuke Niwa
Comment 2
2011-05-30 12:00:15 PDT
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 3
2011-05-30 20:50:15 PDT
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
Comment 4
2011-05-30 20:51:25 PDT
Created
attachment 95392
[details]
Addressed the review comments.
Ryosuke Niwa
Comment 5
2011-05-30 21:08:51 PDT
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 6
2011-05-30 22:59:35 PDT
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
Comment 7
2011-05-30 23:01:44 PDT
Created
attachment 95398
[details]
Addressed the review comments.
Ryosuke Niwa
Comment 8
2011-05-30 23:14:20 PDT
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
Comment 9
2011-05-30 23:25:29 PDT
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
Comment 10
2011-05-31 00:40:03 PDT
Committed
r87716
: <
http://trac.webkit.org/changeset/87716
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug