WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
70223
CSSStyleSheet: finding the owner node should be in its own method
https://bugs.webkit.org/show_bug.cgi?id=70223
Summary
CSSStyleSheet: finding the owner node should be in its own method
Roland Steiner
Reported
2011-10-17 03:13:13 PDT
CSSStyleSheet::document() tries to find the owner node and - if it's a document - return it. This should be in its own function, which will come in useful for
https://bugs.webkit.org/show_bug.cgi?id=67720
. Also, the function itself can be streamlined.
Attachments
Patch
(2.85 KB, patch)
2011-10-17 03:28 PDT
,
Roland Steiner
no flags
Details
Formatted Diff
Diff
de-conflicted patch
(2.37 KB, patch)
2011-11-06 23:33 PST
,
Roland Steiner
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Roland Steiner
Comment 1
2011-10-17 03:28:32 PDT
Created
attachment 111235
[details]
Patch
Andreas Kling
Comment 2
2011-10-17 05:01:37 PDT
Comment on
attachment 111235
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=111235&action=review
> Source/WebCore/css/CSSStyleSheet.cpp:285 > +
Unnecessary newline.
> Source/WebCore/css/CSSStyleSheet.h:91 > + Node* findOwnerNode() const;
I think this name is way too easily confused with StyleSheet::ownerNode() which doesn't climb.
Roland Steiner
Comment 3
2011-10-17 17:15:59 PDT
(In reply to
comment #2
)
> > Source/WebCore/css/CSSStyleSheet.h:91 > > + Node* findOwnerNode() const; > > I think this name is way too easily confused with StyleSheet::ownerNode() which doesn't climb.
Well, that's what 'find' is supposed to imply, but I'm of course open to suggestions.
Dimitri Glazkov (Google)
Comment 4
2011-10-18 09:02:27 PDT
Comment on
attachment 111235
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=111235&action=review
>> Source/WebCore/css/CSSStyleSheet.h:91 >> + Node* findOwnerNode() const; > > I think this name is way too easily confused with StyleSheet::ownerNode() which doesn't climb.
Kling-san is right. Are there any examples of similar searching in the code? Perhaps we could steal naming convention from there?
Andreas Kling
Comment 5
2011-11-06 01:40:25 PST
Comment on
attachment 111235
[details]
Patch Removing from review queue since this patch doesn't apply anymore.
Roland Steiner
Comment 6
2011-11-06 23:33:08 PST
Created
attachment 113827
[details]
de-conflicted patch AFAICT there's no naming convention for tree climbing - e.g., CSSRule's parentStyleSheet() implicitly climbs. Renamed the function to 'styleSheetOwnerNode'.
WebKit Review Bot
Comment 7
2011-11-07 08:32:08 PST
Comment on
attachment 113827
[details]
de-conflicted patch Clearing flags on attachment: 113827 Committed
r99426
: <
http://trac.webkit.org/changeset/99426
>
WebKit Review Bot
Comment 8
2011-11-07 08:32:13 PST
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 9
2011-11-07 17:26:48 PST
(In reply to
comment #6
)
> AFAICT there's no naming convention for tree climbing - e.g., CSSRule's parentStyleSheet() implicitly climbs. Renamed the function to 'styleSheetOwnerNode'.
In the past, we have sometimes used the word “find” as the prefix for “climbing”. Heh, now that I write this it seems that’s what you used and Andreas talked you out of it!
Andreas Kling
Comment 10
2011-11-07 17:35:29 PST
(In reply to
comment #9
)
> (In reply to
comment #6
) > > AFAICT there's no naming convention for tree climbing - e.g., CSSRule's parentStyleSheet() implicitly climbs. Renamed the function to 'styleSheetOwnerNode'. > > In the past, we have sometimes used the word “find” as the prefix for “climbing”. Heh, now that I write this it seems that’s what you used and Andreas talked you out of it!
D'oh! Well I'm certainly not opposed to the "find" prefix, especially if that's an existing convention. I was merely pointing out the possibly confusing similarity between "ownerNode" and "findOwnerNode" :-)
Darin Adler
Comment 11
2011-11-07 17:40:17 PST
(In reply to
comment #10
)
> I was merely pointing out the possibly confusing similarity between "ownerNode" and "findOwnerNode" :-)
Yes, I agree completely with that. We can use the “find” prefix to allude to the fact that we’re not just getting something we have directly, say in a data member, and instead having to do a bit of searching. But when it comes to the thing after “find” it should not just be the same as some other function in the same class.
Roland Steiner
Comment 12
2011-11-07 18:19:24 PST
Want me to rename it to findStyleSheetOwnerNode?
Darin Adler
Comment 13
2011-11-07 20:13:30 PST
(In reply to
comment #12
)
> Want me to rename it to findStyleSheetOwnerNode?
That might be good. On the other hand, since in turn it’s called in a function named just “document”, I am not sure that adding a find just to this function’s name pays off all that well in reminding people about performance cost.
Roland Steiner
Comment 14
2011-11-07 21:47:00 PST
Ask, and ye shall receive! ;) ->
https://bugs.webkit.org/show_bug.cgi?id=71764
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