Implement ::cue() pseudo element property whitelist

• Component: Media
• Status: RESOLVED FIXED

[Dima Gorbik]

Specs require that only certain properties could be applied with the ::cue pseudo-element.
http://dev.w3.org/html5/webvtt/#the-'::cue'-pseudo-element

[Radar WebKit Bug Importer]

<rdar://problem/12914495>

[Dima Gorbik]

Created attachment 183095 [details]
Proposed fix 0.1

[Silvia Pfeiffer]

Nice! Could you add to your test a CSS setting that is not supported by WebVTT and show that it is not being executed?

[Dima Gorbik]

(In reply to comment #3)
> Nice! Could you add to your test a CSS setting that is not supported by WebVTT and show that it is not being executed?

This is what padding is for :-)
This whitelisting patch doesn't work for the ::cue without an argument yet though. I will fix this soon later.

[Dima Gorbik]

Also properties with the background shorthand should be applied to the cueDisplayBox instead of WebVTT properties. I haven't yet find a way to do this without touching too much css resolving code.

[WebKit Review Bot]

Comment on attachment 183095 [details]
Proposed fix 0.1

Attachment 183095 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15910715

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Dima Gorbik]

(In reply to comment #6)
> (From update of attachment 183095 [details])
> Attachment 183095 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/15910715
> 
> New failing tests:
> inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

This test runs locally perfectly.

[Antti Koivisto]

Comment on attachment 183095 [details]
Proposed fix 0.1

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

[Dima Gorbik]

Created attachment 183367 [details]
Proposed fix 0.2

[Antti Koivisto]

Comment on attachment 183367 [details]
Proposed fix 0.2

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

> Source/WebCore/css/RuleSet.h:42
> +    PropertyWhitelistNone   = 0,

= 0 is unnecessary (and there is extra space)

[Antti Koivisto]

r=me, nice

[WebKit Review Bot]

Comment on attachment 183367 [details]
Proposed fix 0.2

Clearing flags on attachment: 183367

Committed r140173: <http://trac.webkit.org/changeset/140173>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

Comment on attachment 183367 [details]
Proposed fix 0.2

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

> LayoutTests/media/track/track-css-property-whitelist.html:21
> +            while (nextElementSibling.nodeType != 1) {

Nit: Node.ELEMENT_NODE would be clearer.

> LayoutTests/media/track/track-css-property-whitelist.html:23
> +                nextElementSibling = nextElementSibling.nextSibling;
> +            }

Nit: braces not needed.

> LayoutTests/media/track/track-css-property-whitelist.html:36
> +            skipNonElements(cueNode);

Does this line do anything?

> Source/WebCore/css/RuleSet.cpp:111
> +static inline PropertyWhitelistType determinePropertyWhitelistType(const AddRuleFlags addRuleFlags, const CSSSelector* selector)

This should have UNUSED_PARAM for platforms that don't ENABLE(VIDEO_TRACK)

[Dima Gorbik]

(In reply to comment #14)
> (From update of attachment 183367 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183367&action=review
> 
> > LayoutTests/media/track/track-css-property-whitelist.html:21
> > +            while (nextElementSibling.nodeType != 1) {
> 
> Nit: Node.ELEMENT_NODE would be clearer.
> 
> > LayoutTests/media/track/track-css-property-whitelist.html:23
> > +                nextElementSibling = nextElementSibling.nextSibling;
> > +            }
> 
> Nit: braces not needed.
> 
> > LayoutTests/media/track/track-css-property-whitelist.html:36
> > +            skipNonElements(cueNode);
> 
> Does this line do anything?
> 
> > Source/WebCore/css/RuleSet.cpp:111
> > +static inline PropertyWhitelistType determinePropertyWhitelistType(const AddRuleFlags addRuleFlags, const CSSSelector* selector)
> 
> This should have UNUSED_PARAM for platforms that don't ENABLE(VIDEO_TRACK)

Thanks, I will fix those when I touch the whitelisting code soon.