WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(62.38 KB, patch)
2010-11-17 16:01 PST
,
Dimitri Glazkov (Google)
no flags
Details
Formatted Diff
Diff
Patch
(1.38 KB, patch)
2010-11-17 16:45 PST
,
Dimitri Glazkov (Google)
no flags
Details
Formatted Diff
Diff
Patch
(5.45 KB, patch)
2010-11-29 12:04 PST
,
Dimitri Glazkov (Google)
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Dimitri Glazkov (Google)
Comment 1
2010-11-17 14:04:38 PST
Created
attachment 74153
[details]
Patch
Early Warning System Bot
Comment 2
2010-11-17 14:25:04 PST
Attachment 74153
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/6159039
WebKit Review Bot
Comment 3
2010-11-17 14:27:22 PST
Attachment 74153
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/6190026
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
Created
attachment 74171
[details]
Patch
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
Created
attachment 74178
[details]
Patch
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
Attachment 74178
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/6193031
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
Created
attachment 75050
[details]
Patch
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
Committed
r72825
: <
http://trac.webkit.org/changeset/72825
>
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