RESOLVED FIXED 136784
Make all CSSSelector data members private
https://bugs.webkit.org/show_bug.cgi?id=136784
Summary Make all CSSSelector data members private
Chris Dumez
Reported 2014-09-12 09:52:55 PDT
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.
Attachments
Patch (41.18 KB, patch)
2014-09-12 10:06 PDT, Chris Dumez
no flags
Patch (41.27 KB, patch)
2014-09-12 11:42 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2014-09-12 10:06:53 PDT
Benjamin Poulain
Comment 2 2014-09-12 11:24:52 PDT
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 3 2014-09-12 11:32:03 PDT
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
Comment 4 2014-09-12 11:42:52 PDT
Benjamin Poulain
Comment 5 2014-09-12 11:44:07 PDT
(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 6 2014-09-12 12:24:19 PDT
Comment on attachment 238041 [details] Patch Clearing flags on attachment: 238041 Committed r173569: <http://trac.webkit.org/changeset/173569>
WebKit Commit Bot
Comment 7 2014-09-12 12:24:23 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.