Summary: | Whitelist should also work for the WebVTT ::cue element without an argument | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dima Gorbik <dgorbik> | ||||||||||||
Component: | Media | Assignee: | Dima Gorbik <dgorbik> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cmarcelo, eric.carlson, feature-media-reviews, kling, koivisto, macpherson, menard, ojan.autocc, webkit-bug-importer, webkit-ews, webkit.review.bot | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 43668 | ||||||||||||||
Attachments: |
|
Description
Dima Gorbik
2013-01-21 17:04:00 PST
Created attachment 183857 [details]
Proposed fix 0.1
Created attachment 183863 [details]
Proposed fix 0.2
Comment on attachment 183863 [details] Proposed fix 0.2 Attachment 183863 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16038274 Comment on attachment 183863 [details] Proposed fix 0.2 View in context: https://bugs.webkit.org/attachment.cgi?id=183863&action=review > LayoutTests/media/track/track-css-property-whitelist.html:35 > cueNode = skipNonElements(textTrackDisplayElement(video, 'all-nodes').firstChild); This should just use firstElementChild instead. > Source/WebCore/css/RuleSet.cpp:49 > #include <wtf/MemoryInstrumentationVector.h> > +#if ENABLE(VIDEO_TRACK) Need a space before this #if line. > Source/WebCore/css/RuleSet.cpp:129 > +>>>>>>> whitelist2 Need to delete this. Comment on attachment 183863 [details] Proposed fix 0.2 Attachment 183863 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16042283 Created attachment 183865 [details]
Proposed fix 0.3
Comment on attachment 183865 [details] Proposed fix 0.3 View in context: https://bugs.webkit.org/attachment.cgi?id=183865&action=review > LayoutTests/media/track/track-css-property-whitelist.html:19 > function skipNonElements(root) Can probably delete this function if it’s not used any more. Anyone using it can probably use firstElementChild or nextElementSibling instead. Created attachment 183867 [details]
Proposed fix 0.4
Nice to have both firstElementChild and nextElementSibling functions.
Thanks for a review!
Comment on attachment 183867 [details] Proposed fix 0.4 View in context: https://bugs.webkit.org/attachment.cgi?id=183867&action=review > Source/WebCore/css/RuleSet.cpp:125 > - if (component->pseudoType() == CSSSelector::PseudoCue) > + if (component->pseudoType() == CSSSelector::PseudoCue || selector->value() == TextTrackCue::cueShadowPseudoId()) This looks wrong. If 'component' has (m_match == CSSSelector::Tag), value() should not be called as it will perform an incorrect cast. In fact, CSSSelector::value() should ASSERT(m_match != Tag), just like CSSSelector::tagQName() asserts the opposite. (In reply to comment #10) > (From update of attachment 183867 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183867&action=review > > > Source/WebCore/css/RuleSet.cpp:125 > > - if (component->pseudoType() == CSSSelector::PseudoCue) > > + if (component->pseudoType() == CSSSelector::PseudoCue || selector->value() == TextTrackCue::cueShadowPseudoId()) > > This looks wrong. If 'component' has (m_match == CSSSelector::Tag), value() should not be called as it will perform an incorrect cast. > In fact, CSSSelector::value() should ASSERT(m_match != Tag), just like CSSSelector::tagQName() asserts the opposite. Could you elaborate please. Right now I see that m_data.m_tagQName is used when (m_match == Tag). Even if RareData exists (in the constructor). How is that related to the value which is either m_data.m_rareData->m_value if RareData exists either m_data.m_value if it doesn't... (In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 183867 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=183867&action=review > > > > > Source/WebCore/css/RuleSet.cpp:125 > > > - if (component->pseudoType() == CSSSelector::PseudoCue) > > > + if (component->pseudoType() == CSSSelector::PseudoCue || selector->value() == TextTrackCue::cueShadowPseudoId()) > > > > This looks wrong. If 'component' has (m_match == CSSSelector::Tag), value() should not be called as it will perform an incorrect cast. > > In fact, CSSSelector::value() should ASSERT(m_match != Tag), just like CSSSelector::tagQName() asserts the opposite. > > Could you elaborate please. Right now I see that m_data.m_tagQName is used when (m_match == Tag). Even if RareData exists (in the constructor). How is that related to the value which is either m_data.m_rareData->m_value if RareData exists either m_data.m_value if it doesn't... First of all, it's calling value() on 'selector' in every iteration, not on 'component' as it should. With that fixed, the problem is that m_data's m_value/m_rareData/m_tagQName all share the same memory location. (m_match == Tag) means that it's QualifiedNameImpl* is stored there, otherwise it's either a RareData* or an AtomicStringImpl*. Calling value() will treat whatever is in the m_data union as if it's either AtomicStringImpl* or RareData*, even though it could also be a QualifiedNameImpl*. Basically, the check should look like this: if (component->pseudoType() == CSSSelector::PseudoCue || (component->m_match != Tag && component->value() == TextTrackCue::cueShadowPseudoId())) (In reply to comment #12) Oh, it's a union. My bad, never used them before. We should definitely add an ASSERT to getters. Created attachment 184066 [details]
Proposed fix 0.5
Comment on attachment 184066 [details] Proposed fix 0.5 Clearing flags on attachment: 184066 Committed r140505: <http://trac.webkit.org/changeset/140505> All reviewed patches have been landed. Closing bug. |