Bug 105477

<rdar://problem/12914495>

Created attachment 183095 [details] Proposed fix 0.1

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

(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.

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 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

(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 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

Created attachment 183367 [details] Proposed fix 0.2

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)

r=me, nice

Comment on attachment 183367 [details] Proposed fix 0.2 Clearing flags on attachment: 183367 Committed r140173: <http://trac.webkit.org/changeset/140173>

All reviewed patches have been landed. Closing bug.

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)

(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.