Bug 70223

Summary: CSSStyleSheet: finding the owner node should be in its own method
Product: WebKit Reporter: Roland Steiner <rolandsteiner>
Component: CSSAssignee: Roland Steiner <rolandsteiner>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dglazkov, kling, koivisto, macpherson, morrita, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 67720    
Attachments:
Description Flags
Patch
none
de-conflicted patch none

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
DetailsFormatted DiffDiff
de-conflicted patch (2.37 KB, patch)
2011-11-06 23:33 PST, Roland Steiner 		no flags
DetailsFormatted DiffDiff
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.