Bug 136784 – Make all CSSSelector data members private

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

[Chris Dumez]

Make all CSSSelector data members private. Previously, some of the data members such as m_relation / m_match / m_pseudoType were public and accessed directly from outside the class. The new approach is better because:
- Those members are bit fields so by using getters, we can hide the casts inside the getters. The setters can now also check that the bitfield is big enough to actually store the enum value.
- When using those in switch() statements, the compiler now complains if we fail to test some of the enum values as the value is now an enum, and not merely an unsigned integer.
- Some of these members already has getters (e.g. relation(), pseudoType()).
- Better encapsulation.

[Chris Dumez]

Created attachment 238036 [details]
Patch

[Benjamin Poulain]

Comment on attachment 238036 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238036&action=review

Really neat. That is something I wanted for a while but I was too lazy to fix everything :)

It is a little sad we need the public setters for the parser. I guess in the future we could refactor those out.

> Source/WebCore/css/CSSSelector.cpp:87
>      // FIXME: Pseudo-elements and pseudo-classes do not have the same specificity. This function
>      // isn't quite correct.

Hum, we should fix that at some point.

> Source/WebCore/css/CSSSelector.h:211
> +            ASSERT(m_pseudoType == pseudoElementType);

I assume you are asserting in case the type become larger than the bit field?

> Source/WebCore/css/CSSSelector.h:214
> +        void setPagePseudoType(PagePseudoClassType pagePseudoType)

You could move this one next to its getter.

[Chris Dumez]

Comment on attachment 238036 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238036&action=review

>> Source/WebCore/css/CSSSelector.h:211
>> +            ASSERT(m_pseudoType == pseudoElementType);
> 
> I assume you are asserting in case the type become larger than the bit field?

Yes, I explained it in the Changelog.

>> Source/WebCore/css/CSSSelector.h:214
>> +        void setPagePseudoType(PagePseudoClassType pagePseudoType)
> 
> You could move this one next to its getter.

Will do.

[Chris Dumez]

Created attachment 238041 [details]
Patch

[Benjamin Poulain]

(In reply to comment #3)
> (From update of attachment 238036 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238036&action=review
> 
> >> Source/WebCore/css/CSSSelector.h:211
> >> +            ASSERT(m_pseudoType == pseudoElementType);
> > 
> > I assume you are asserting in case the type become larger than the bit field?
> 
> Yes, I explained it in the Changelog.

You are assuming I can read properly ;)

[WebKit Commit Bot]

Comment on attachment 238041 [details]
Patch

Clearing flags on attachment: 238041

Committed r173569: <http://trac.webkit.org/changeset/173569>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.