Bug 70223 – CSSStyleSheet: finding the owner node should be in its own method

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Roland Steiner]

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.

[Roland Steiner]

Created attachment 111235 [details]
Patch

[Andreas Kling]

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]

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

Removing from review queue since this patch doesn't apply anymore.

[Roland Steiner]

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 on attachment 113827 [details]
de-conflicted patch

Clearing flags on attachment: 113827

Committed r99426: <http://trac.webkit.org/changeset/99426>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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

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

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

Want me to rename it to findStyleSheetOwnerNode?

[Darin Adler]

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

Ask, and ye shall receive! ;) -> https://bugs.webkit.org/show_bug.cgi?id=71764