RESOLVED FIXED Bug 105477
Implement ::cue() pseudo element property whitelist
https://bugs.webkit.org/show_bug.cgi?id=105477
Summary Implement ::cue() pseudo element property whitelist
Dima Gorbik
Reported 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
Attachments
Proposed fix 0.1 (18.73 KB, patch)
2013-01-16 18:59 PST, Dima Gorbik
koivisto: review+
webkit.review.bot: commit-queue-
Proposed fix 0.2 (18.64 KB, patch)
2013-01-17 21:16 PST, Dima Gorbik
no flags
Radar WebKit Bug Importer
Comment 1 2012-12-19 16:42:39 PST
Dima Gorbik
Comment 2 2013-01-16 18:59:49 PST
Created attachment 183095 [details] Proposed fix 0.1
Silvia Pfeiffer
Comment 3 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?
Dima Gorbik
Comment 4 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.
Dima Gorbik
Comment 5 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.
WebKit Review Bot
Comment 6 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
Dima Gorbik
Comment 7 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.
Antti Koivisto
Comment 8 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
Dima Gorbik
Comment 9 2013-01-17 21:16:23 PST
Created attachment 183367 [details] Proposed fix 0.2
Antti Koivisto
Comment 10 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)
Antti Koivisto
Comment 11 2013-01-18 10:19:08 PST
r=me, nice
WebKit Review Bot
Comment 12 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>
WebKit Review Bot
Comment 13 2013-01-18 10:24:18 PST
All reviewed patches have been landed. Closing bug.
Eric Carlson
Comment 14 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)
Dima Gorbik
Comment 15 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.
Note You need to log in before you can comment on or make changes to this bug.