| Summary: | Add a utility function which dumps a tree for the Node, including a document of a frame. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Hayato Ito <hayato> | ||||||||
| Component: | DOM | Assignee: | Hayato Ito <hayato> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | rniwa | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Hayato Ito
2011-05-30 06:03:13 PDT
Created attachment 95342 [details]
dump also a document of a frame
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. 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. Created attachment 95392 [details]
Addressed the review comments.
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. 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. Created attachment 95398 [details]
Addressed the review comments.
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. 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. Committed r87716: <http://trac.webkit.org/changeset/87716> |