Bug 49686

Created attachment 74153 [details] Patch

Attachment 74153 [details] did not build on qt: Build output: http://queues.webkit.org/results/6159039

Attachment 74153 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6190026

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.

Comment on attachment 74153 [details] Patch r=me as long as you don’t break the build

(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.

Created attachment 74171 [details] Patch

Comment on attachment 74171 [details] Patch parent to parentNode conversion landed as http://trac.webkit.org/changeset/72259.

Created attachment 74178 [details] Patch

Comment on attachment 74178 [details] Patch Letting the EWS bots cook it.

They can cook r+ patches now too. :)

Attachment 74178 [details] did not build on qt: Build output: http://queues.webkit.org/results/6193031

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.

Created attachment 75050 [details] Patch

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.

(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.

Committed r72825: <http://trac.webkit.org/changeset/72825>