Bug 105477 - Implement ::cue() pseudo element property whitelist
Summary: Implement ::cue() pseudo element property whitelist
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 43668
  Show dependency treegraph
 
Reported: 2012-12-19 16:41 PST by Dima Gorbik
Modified: 2013-01-19 23:26 PST (History)
13 users (show)

See Also:


Attachments
Proposed fix 0.1 (18.73 KB, patch)
2013-01-16 18:59 PST, Dima Gorbik
koivisto: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Proposed fix 0.2 (18.64 KB, patch)
2013-01-17 21:16 PST, Dima Gorbik
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dima Gorbik 2012-12-19 16:41:33 PST
Specs require that only certain properties could be applied with the ::cue pseudo-element.
http://dev.w3.org/html5/webvtt/#the-'::cue'-pseudo-element
Comment 1 Radar WebKit Bug Importer 2012-12-19 16:42:39 PST
<rdar://problem/12914495>
Comment 2 Dima Gorbik 2013-01-16 18:59:49 PST
Created attachment 183095 [details]
Proposed fix 0.1
Comment 3 Silvia Pfeiffer 2013-01-16 19:09:20 PST
Nice! Could you add to your test a CSS setting that is not supported by WebVTT and show that it is not being executed?
Comment 4 Dima Gorbik 2013-01-16 19:22:16 PST
(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.
Comment 5 Dima Gorbik 2013-01-16 19:33:02 PST
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.
Comment 6 WebKit Review Bot 2013-01-16 19:46:32 PST
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
Comment 7 Dima Gorbik 2013-01-16 21:01:28 PST
(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.
Comment 8 Antti Koivisto 2013-01-17 15:02:32 PST
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
Comment 9 Dima Gorbik 2013-01-17 21:16:23 PST
Created attachment 183367 [details]
Proposed fix 0.2
Comment 10 Antti Koivisto 2013-01-18 10:18:25 PST
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)
Comment 11 Antti Koivisto 2013-01-18 10:19:08 PST
r=me, nice
Comment 12 WebKit Review Bot 2013-01-18 10:24:14 PST
Comment on attachment 183367 [details]
Proposed fix 0.2

Clearing flags on attachment: 183367

Committed r140173: <http://trac.webkit.org/changeset/140173>
Comment 13 WebKit Review Bot 2013-01-18 10:24:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Eric Carlson 2013-01-19 11:24:48 PST
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)
Comment 15 Dima Gorbik 2013-01-19 23:26:46 PST
(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.