WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2012-12-19 16:42:39 PST
<
rdar://problem/12914495
>
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.
Top of Page
Format For Printing
XML
Clone This Bug