[Webkit-unassigned] [Bug 105477] Implement ::cue() pseudo element property whitelist

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=105477


Antti Koivisto <koivisto at iki.fi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #183095|review?                     |review+
               Flag|                            |




--- Comment #8 from Antti Koivisto <koivisto at iki.fi>  2013-01-17 15:02:32 PST ---
(From update of attachment 183095)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.