WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(41.27 KB, patch)
2014-09-12 11:42 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
2014-09-12 10:06:53 PDT
Created
attachment 238036
[details]
Patch
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
Created
attachment 238041
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug