[webkit-reviews] review granted: [Bug 105477] Implement ::cue() pseudo element property whitelist : [Attachment 183095] Proposed fix 0.1

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Dima Gorbik <dgorbik at apple.com>'s
request for review:
Bug 105477: Implement ::cue() pseudo element property whitelist
https://bugs.webkit.org/show_bug.cgi?id=105477

Attachment 183095: Proposed fix 0.1
https://bugs.webkit.org/attachment.cgi?id=183095&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=183095&action=review


Looks pretty ok, some comments to consider though.

> Source/WebCore/css/RuleSet.cpp:122
> -    , m_isInRegionRule(!!(addRuleFlags & RuleIsInRegionRule))
> +    , m_whitelistFlags((addRuleFlags & RuleIsInRegionRule) ?
static_cast<unsigned>(PropertyWhitelistRegion) :
static_cast<unsigned>(PropertyWhitelistNone))

It would be nicer to extract the m_whitelistFlag in RuleData constructor for
cues too. Please use inline function like some other bits. Then all this is in
one place.

> Source/WebCore/css/RuleSet.cpp:222
>      if (selector->pseudoType() == CSSSelector::PseudoCue) {
> +	   ruleData.setIsInCueRule();
>	   m_cuePseudoRules.append(ruleData);
>	   return;

...instead of randomly doing this one bit here.

> Source/WebCore/css/RuleSet.h:46
> +enum PropertyWhitelistFlags {
> +    PropertyWhitelistNone   = 0,
> +    PropertyWhitelistRegion = 1,
> +#if ENABLE(VIDEO_TRACK)
> +    PropertyWhitelistCue    = 1 << 1,
> +#endif

Whitelists are probably always mutually exclusive yet this looks like they are
not. You could just call it PropertyWhitelistType and use default enum values.
Using equality comparisons everywhere looks slightly nicer than bitops.

> Source/WebCore/css/RuleSet.h:76
> +    bool isInRegionRule() const { return m_whitelistFlags &
PropertyWhitelistRegion; }
> +    PropertyWhitelistFlags whitelistType() const {return
static_cast<PropertyWhitelistFlags>(m_whitelistFlags); }
> +#if ENABLE(VIDEO_TRACK)
> +    bool isInCueRule() const { return m_whitelistFlags &
PropertyWhitelistCue; }
> +    void setIsInCueRule()  { m_whitelistFlags |= PropertyWhitelistCue; }

propertyWhitelistType()

I don't think you need other accessors, they can be removed.

> Source/WebCore/css/RuleSet.h:97
> +    unsigned m_whitelistFlags : 2;

m_propertyWhitelistType