Bug 70223 - CSSStyleSheet: finding the owner node should be in its own method
Summary: CSSStyleSheet: finding the owner node should be in its own method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Roland Steiner
URL:
Keywords:
Depends on:
Blocks: 67720
  Show dependency treegraph
 
Reported: 2011-10-17 03:13 PDT by Roland Steiner
Modified: 2011-11-07 21:47 PST (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Steiner 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.
Comment 1 Roland Steiner 2011-10-17 03:28:32 PDT
Created attachment 111235 [details]
Patch
Comment 2 Andreas Kling 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.
Comment 3 Roland Steiner 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.
Comment 4 Dimitri Glazkov (Google) 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?
Comment 5 Andreas Kling 2011-11-06 01:40:25 PST
Comment on attachment 111235 [details]
Patch

Removing from review queue since this patch doesn't apply anymore.
Comment 6 Roland Steiner 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'.
Comment 7 WebKit Review Bot 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>
Comment 8 WebKit Review Bot 2011-11-07 08:32:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Darin Adler 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!
Comment 10 Andreas Kling 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" :-)
Comment 11 Darin Adler 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.
Comment 12 Roland Steiner 2011-11-07 18:19:24 PST
Want me to rename it to findStyleSheetOwnerNode?
Comment 13 Darin Adler 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.
Comment 14 Roland Steiner 2011-11-07 21:47:00 PST
Ask, and ye shall receive! ;) -> https://bugs.webkit.org/show_bug.cgi?id=71764