Bug 107488

<rdar://problem/13057269>

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.