[webkit-reviews] review denied: [Bug 104191] Implement matching cue by the class name with ::cue pseudo element : [Attachment 177912] Proposed fix 0.2

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Dima Gorbik <dgorbik at apple.com>'s
request for review:
Bug 104191: Implement matching cue by the class name with ::cue pseudo element
https://bugs.webkit.org/show_bug.cgi?id=104191

Attachment 177912: Proposed fix 0.2
https://bugs.webkit.org/attachment.cgi?id=177912&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=177912&action=review


> Source/WebCore/css/CSSParser.cpp:10104
> +#if ENABLE(VIDEO_TRACK)
> +	   // Don't set the tag for the PseudoCue element because the matching
will not work when the tags are being compared.
> +	   if (!(specifiers->pseudoType() == CSSSelector::PseudoCue))
> +#endif
> +	       specifiers->setTag(tag);

I don't quite understand this. The comment is not very helpful.

> Source/WebCore/css/SelectorChecker.cpp:1160
> +	   subContext.isMatchingCueNodes = true;

I don't see you ever reading this bit. Why does it exist?

> Source/WebCore/dom/Element.cpp:2482
> +#if ENABLE(VIDEO_TRACK)
> +    if (other.isWebVTTNode())
> +	   setIsWebVTTNode(true);
> +#endif
>      cloneAttributesFromElement(other);
>      copyNonAttributePropertiesFromElement(other);

I think this can be handled in a cleaner way. See the comment below.

> Source/WebCore/html/track/WebVTTParser.cpp:367
>	       child = HTMLElement::create(qTag, document);
>  
>	   if (child) {
> +	       child->setIsWebVTTNode(true);

I think you will want to add new private HTMLElement subclass (WebVTTElement or
similar) and build the tree out of those instead of generic HTMLElements. That
class will have virtual copyNonAttributePropertiesFromElement() function that
will copy the isWebVTTNode bit and any other metadata you might want. Or maybe
just make isWebVTTNode itself virtual, it is not called much I think.