RESOLVED FIXED Bug 49686
Converge means of querying a parent node into one way, which is Node::parentNode.
https://bugs.webkit.org/show_bug.cgi?id=49686
Summary Converge means of querying a parent node into one way, which is Node::parentN...
Dimitri Glazkov (Google)
Reported 2010-11-17 14:01:31 PST
Converge means of querying a parent node into one way, which is Node::parentNode.
Attachments
Patch (54.13 KB, patch)
2010-11-17 14:04 PST, Dimitri Glazkov (Google)
no flags
Patch (62.38 KB, patch)
2010-11-17 16:01 PST, Dimitri Glazkov (Google)
no flags
Patch (1.38 KB, patch)
2010-11-17 16:45 PST, Dimitri Glazkov (Google)
no flags
Patch (5.45 KB, patch)
2010-11-29 12:04 PST, Dimitri Glazkov (Google)
darin: review+
Dimitri Glazkov (Google)
Comment 1 2010-11-17 14:04:38 PST
Early Warning System Bot
Comment 2 2010-11-17 14:25:04 PST
WebKit Review Bot
Comment 3 2010-11-17 14:27:22 PST
Darin Adler
Comment 4 2010-11-17 14:52:58 PST
Comment on attachment 74153 [details] Patch EWS says you missed some cases. I suggest landing most of the changes to call parentNode instead of parent first, and then land the patch that makes parent private separately along with the last few stragglers.
Darin Adler
Comment 5 2010-11-17 14:53:17 PST
Comment on attachment 74153 [details] Patch r=me as long as you don’t break the build
Dimitri Glazkov (Google)
Comment 6 2010-11-17 14:54:42 PST
(In reply to comment #4) > (From update of attachment 74153 [details]) > EWS says you missed some cases. I suggest landing most of the changes to call parentNode instead of parent first, and then land the patch that makes parent private separately along with the last few stragglers. Will do.
Dimitri Glazkov (Google)
Comment 7 2010-11-17 16:01:38 PST
Dimitri Glazkov (Google)
Comment 8 2010-11-17 16:23:22 PST
Comment on attachment 74171 [details] Patch parent to parentNode conversion landed as http://trac.webkit.org/changeset/72259.
Dimitri Glazkov (Google)
Comment 9 2010-11-17 16:45:34 PST
Dimitri Glazkov (Google)
Comment 10 2010-11-17 16:47:34 PST
Comment on attachment 74178 [details] Patch Letting the EWS bots cook it.
Adam Barth
Comment 11 2010-11-17 17:17:40 PST
They can cook r+ patches now too. :)
Early Warning System Bot
Comment 12 2010-11-17 17:21:32 PST
Darin Adler
Comment 13 2010-11-17 22:21:56 PST
Comment on attachment 74178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74178&action=review review- because EWS says XSLTProcessorQt.cpp still has some calls to parent in it and ... > WebCore/dom/Node.h:658 > + ContainerNode* parent() const { return TreeShared<ContainerNode>::parent(); } A cleaner way of doing this is with a using declaration: using TreeShared<ContainerNode>::parent; That way you don’t introduce a new inline function, just change the visibility of the existing function. I think this also deserves a comment. It’s not clear why the function is private without a comment.
Dimitri Glazkov (Google)
Comment 14 2010-11-29 12:04:31 PST
Darin Adler
Comment 15 2010-11-29 12:37:37 PST
Comment on attachment 75050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75050&action=review > WebCore/dom/Node.h:659 > Element* ancestorElement() const; > + // Use Node::parentNode as the consistent way of querying a parent node. > + using TreeShared<ContainerNode>::parent; I suggest paragraphing this comment and function separately rather than grouping with ancestorElement. This comment is slightly confusing. What it implies but does not say is that we make the inherited parent function private to make sure we get a compile error if we forget to follow that rule outside Node member functions. Many readers might not be able to figure that out.
Dimitri Glazkov (Google)
Comment 16 2010-11-29 13:25:42 PST
(In reply to comment #15) > (From update of attachment 75050 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75050&action=review > > > WebCore/dom/Node.h:659 > > Element* ancestorElement() const; > > + // Use Node::parentNode as the consistent way of querying a parent node. > > + using TreeShared<ContainerNode>::parent; > > I suggest paragraphing this comment and function separately rather than grouping with ancestorElement. > > This comment is slightly confusing. What it implies but does not say is that we make the inherited parent function private to make sure we get a compile error if we forget to follow that rule outside Node member functions. Many readers might not be able to figure that out. Thanks! Landing with an improved, paragraphed comment/decl.
Dimitri Glazkov (Google)
Comment 17 2010-11-29 13:27:55 PST
Note You need to log in before you can comment on or make changes to this bug.