Bug 49686 – Converge means of querying a parent node into one way, which is Node::parentNode.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dimitri Glazkov (Google)]

Converge means of querying a parent node into one way, which is Node::parentNode.

[Dimitri Glazkov (Google)]

Created attachment 74153 [details]
Patch

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[Darin Adler]

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 on attachment 74153 [details]
Patch

r=me as long as you don’t break the build

[Dimitri Glazkov (Google)]

(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)]

Created attachment 74171 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 74171 [details]
Patch

parent to parentNode conversion landed  as http://trac.webkit.org/changeset/72259.

[Dimitri Glazkov (Google)]

Created attachment 74178 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 74178 [details]
Patch

Letting the EWS bots cook it.

[Adam Barth]

They can cook r+ patches now too.  :)

[Early Warning System Bot]

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

[Darin Adler]

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)]

Created attachment 75050 [details]
Patch

[Darin Adler]

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)]

(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)]

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