Bug 104191 – Implement matching cue by the class name with ::cue pseudo element

• Component: Media
• Status: RESOLVED FIXED

[Dima Gorbik]

Implement a pseudo-element ::cue with a an argument of a simple selector list type that would match cues by a class name. 
Example: ::cue(.classname, .anothername)

[Dima Gorbik]

Created attachment 177869 [details]
Proposed fix 0.1 (test case and changelog missing)

[Dima Gorbik]

Comment on attachment 177869 [details]
Proposed fix 0.1 (test case and changelog missing)

View in context: https://bugs.webkit.org/attachment.cgi?id=177869&action=review

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

I will fix the typo in the comment.
I am not sure about this piece of code. Maybe I could use the tag somehow to find out that this is a webvtt node and let it set here?

> Source/WebCore/css/CSSParser.cpp:10142
> +#if ENABLE(VIDEO_TRACK)
> +    if (newSpecifier->isUnknownPseudoElement() || newSpecifier->pseudoType() == CSSSelector::PseudoCue) {
> +#else
>      if (newSpecifier->isUnknownPseudoElement()) {
> +#endif
>          // Unknown pseudo element always goes at the top of selector chain.
>          newSpecifier->appendTagHistory(CSSSelector::ShadowDescendant, sinkFloatingSelector(specifiers));
>          return newSpecifier;

Without this change the cue basically wouldn't work when the selector has a parent element matched by an ID (and other cases when updateSpecifiers in the parser is used). For example: #myvideo::cue(.test) wouldn't work because the pseudo element wouldn't get on top of the selector chain.

> Source/WebCore/css/SelectorChecker.cpp:462
> +            if (pseudoId != NOPSEUDO && m_mode != SharingRules && pseudoId != CUE)
>                  dynamicPseudo = pseudoId;

Not sure about this as well. Without this change the properties wouldn't be applied to the cue.

[Dima Gorbik]

Comment on attachment 177869 [details]
Proposed fix 0.1 (test case and changelog missing)

View in context: https://bugs.webkit.org/attachment.cgi?id=177869&action=review

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

I also don't like the fact that I have to copy this data manually. Is there a way to let it deal with this automatically somehow?

[Dima Gorbik]

Created attachment 177912 [details]
Proposed fix 0.2

[Eric Carlson]

Comment on attachment 177912 [details]
Proposed fix 0.2

View in context: https://bugs.webkit.org/attachment.cgi?id=177912&action=review

> LayoutTests/media/track/captions-webvtt/styling.vtt:10
> +00:01.000 --> 00:02.000
> +<c.red>dolor sit </c><c.green>amet, consectetur </c><c.red2>adipiscing elit</c>

Nit: you might as well add a newline at the end of the file to keep SVN from complaining.

> LayoutTests/media/track/track-css-matching-expected.txt:1
> +Test that cues are being matches properly by the class name

Nit: typo, matches -> matched

[Dima Gorbik]

(In reply to comment #5)
> (From update of attachment 177912 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177912&action=review
> 
> > LayoutTests/media/track/captions-webvtt/styling.vtt:10
> > +00:01.000 --> 00:02.000
> > +<c.red>dolor sit </c><c.green>amet, consectetur </c><c.red2>adipiscing elit</c>
> 
> Nit: you might as well add a newline at the end of the file to keep SVN from complaining.

I think this newline came from the previous version of the file. I should fix this in a previous bug. Thanks!

[Antti Koivisto]

Comment on attachment 177912 [details]
Proposed fix 0.2

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.

[Antti Koivisto]

Comment on attachment 177912 [details]
Proposed fix 0.2

View in context: https://bugs.webkit.org/attachment.cgi?id=177912&action=review

> Source/WebCore/css/CSSParser.cpp:10132
> +    if (newSpecifier->isUnknownPseudoElement() || newSpecifier->pseudoType() == CSSSelector::PseudoCue) {

The current ToT calls this isCustomPseudoElement(). It is not clear to me what the exact meaning of that is supposed to be but maybe PseudoCue just needs to have that set?

[Hajime Morrita]

Comment on attachment 177912 [details]
Proposed fix 0.2

View in context: https://bugs.webkit.org/attachment.cgi?id=177912&action=review

>> Source/WebCore/css/CSSParser.cpp:10132
>> +    if (newSpecifier->isUnknownPseudoElement() || newSpecifier->pseudoType() == CSSSelector::PseudoCue) {
> 
> The current ToT calls this isCustomPseudoElement(). It is not clear to me what the exact meaning of that is supposed to be but maybe PseudoCue just needs to have that set?

isUnknownPseudo() is used to represent -webkit-* pseudo ids. 
Since web components (cough) allows page author to specify their own pseudo, we needed to refine this "unknown" category into:

- A. -webkit-* -> PseudoWebKitCustomElement
- B. x-* (for web components) -> PseudoUserAgentCustomElement
- C. really unknown -> PseudoUnknown

isCustomPseudoElement() matches A and B and isUnknownPseudo() now matches only C.
Does this make sense?

It looks PseudoCue is usual pseudo id and good to have its own enum entry.

[Dima Gorbik]

(In reply to comment #9)
> It looks PseudoCue is usual pseudo id and good to have its own enum entry.

That sounds good, also we are unable to make a cue pseudo selector 'Custom' because it won't be matching any nodes since the code in SelectorChecker::checkSelector will work:
if (context.element->shadowPseudoId() != context.selector->value())
Here an element will now have shadowID. We could set the shadow id for all webvtt objects, but that would cause merging two code paths: one for built-in, compiled selector types and the other for the dynamic pseudo elements. That's probably not what we would like to do.

[Dima Gorbik]

(In reply to comment #7)
> (From update of attachment 177912 [details])
> 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.

If we allow the tag to be set here, it will be matched against webvtt objects' tags, and the selector will be declined since tags are different. I am not sure how QualifiedName is computed and if it may be still used for this.

[Dima Gorbik]

(In reply to comment #7)

> 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.

To make it virtual it will require storing the WebVTT information inside of QualifiedName so the object of the right type would be created in Document::createElement. I guess it's better to override copyNonAttributePropertiesFromElement for now.

[Dima Gorbik]

Antti, I think the subclassing will not help in this case. When the cloneChildNodes is called, document::create is then called to create the clone which gets a qTag as an argument. And it will not preserve the original class, it will create a regular Element instead of the WebVTTElement, and will call its' copyNonAttributeProperties which is empty.
There a type choosing code in document::create which is based on the namespace for the tag. May this be used to create a WebVTTElement instead of Element?
The other idea I have is to implement a virtual function in Element "createElementOfSameType". And this function could be used instead of document::create in cloneChildNodes. This could slow things down, but how often does cloning happens?

[Antti Koivisto]

Since cloning is really only needed for TextTrackCue::getCueAsHTML which clones m_documentFragment I think the easiest solution is simply set the bit for the nodes in the cloned tree there. We know they are all WebVTT nodes.

[Dima Gorbik]

Created attachment 178723 [details]
Proposed fix 0.3

[Antti Koivisto]

Comment on attachment 178723 [details]
Proposed fix 0.3

View in context: https://bugs.webkit.org/attachment.cgi?id=178723&action=review

r- for unsafe cast and many style errors. Please check http://www.webkit.org/coding/coding-style.html

> Source/WebCore/dom/Element.h:432
> +    virtual bool isWebVTTNode() const;

This doesn't need to be virtual since you are putting the bit to the rare data.

> Source/WebCore/html/track/TextTrackCue.cpp:490
> +void TextTrackCue::markNodesAsWebVTTNodes(Node *object)

Please use meaningful variable names. 'object' is as generic as it gets.

> Source/WebCore/html/track/TextTrackCue.cpp:492
> +    for (Node *child = object->firstChild(); child; child = child->nextSibling()) {

You should use NodeTraversal::next(current, root) instead of recursing.

* goes next to the type, Node* current

> Source/WebCore/html/track/TextTrackCue.cpp:493
> +        ((Element *) child)->setIsWebVTTNode(true);

You need to check with current->isElementNode() before casting.
Use C++ style static_cast
Remove unnecessary spaces, Element*

> Source/WebCore/html/track/TextTrackCue.cpp:494
> +        markNodesAsWebVTTNodes(child);

Use NodeTraversal  instead of recursing.

> Source/WebCore/html/track/TextTrackCue.h:128
> +    void markNodesAsWebVTTNodes(Node *);

* placement

[Dima Gorbik]

Created attachment 178925 [details]
Proposed fix 0.4

[Dima Gorbik]

Created attachment 178927 [details]
Proposed fix 0.5

[Silvia Pfeiffer]

FYI: You can also run Tools/Scripts/check-webkit-style to check if your patch satisfies the webkit style rules - it's not completely perfect but helps.

[Early Warning System Bot]

Comment on attachment 178927 [details]
Proposed fix 0.5

Attachment 178927 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15282257

[Early Warning System Bot]

Comment on attachment 178927 [details]
Proposed fix 0.5

Attachment 178927 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15284215

[Dima Gorbik]

Created attachment 178938 [details]
Proposed fix 0.6

[Build Bot]

Comment on attachment 178938 [details]
Proposed fix 0.6

Attachment 178938 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15296197

New failing tests:
fast/frames/sandboxed-iframe-attribute-parsing.html

[Antti Koivisto]

Comment on attachment 178938 [details]
Proposed fix 0.6

View in context: https://bugs.webkit.org/attachment.cgi?id=178938&action=review

Looks good otherwise.

> Source/WebCore/html/track/TextTrackCue.cpp:494
> +    for (Node* child = root->firstChild(); child; child = NodeTraversal::next(child, root))
> +        static_cast<Element*>(child)->setIsWebVTTNode(true);

This is still unsafe case. If someone adds a non Element (like Text) to the WebVTT tree you will have memory corruption. At minimum you should use toElement() casting function which includes assert. But really using the new ElementTraversal interface is the way to go.

[Antti Koivisto]

The test failure seems bogus though better at least try it out locally.

[Dima Gorbik]

Created attachment 179540 [details]
Proposed fix 0.7

[Radar WebKit Bug Importer]

<rdar://problem/12887267>

[WebKit Review Bot]

Comment on attachment 179540 [details]
Proposed fix 0.7

Attachment 179540 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15315853

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
WebCompositorInputHandlerImplTest.gestureFlingIgnoredTouchpad
WebCompositorInputHandlerImplTest.lastInputEventForVSync
WebCompositorInputHandlerImplTest.gestureFlingTransferResetsTouchpad
WebCompositorInputHandlerImplTest.gestureFlingAnimatesTouchpad
WebCompositorInputHandlerImplTest.gestureFlingIgnoredTouchscreen

[Antti Koivisto]

Comment on attachment 179540 [details]
Proposed fix 0.7

View in context: https://bugs.webkit.org/attachment.cgi?id=179540&action=review

r=me

> Source/WebCore/html/track/TextTrackCue.cpp:491
> +void TextTrackCue::markNodesAsWebVTTNodes(Node* root)

This could be a standalone functions instea of a member.

[WebKit Review Bot]

Comment on attachment 179540 [details]
Proposed fix 0.7

Clearing flags on attachment: 179540

Committed r137955: <http://trac.webkit.org/changeset/137955>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.