Bug 104191

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

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.

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?

Created attachment 177912 [details] Proposed fix 0.2

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

(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!

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.

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?

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.

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

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

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

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?

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.

Created attachment 178723 [details] Proposed fix 0.3

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

Created attachment 178925 [details] Proposed fix 0.4

Created attachment 178927 [details] Proposed fix 0.5

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.

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

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

Created attachment 178938 [details] Proposed fix 0.6

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

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.

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

Created attachment 179540 [details] Proposed fix 0.7

<rdar://problem/12887267>

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

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.

Comment on attachment 179540 [details] Proposed fix 0.7 Clearing flags on attachment: 179540 Committed r137955: <http://trac.webkit.org/changeset/137955>

All reviewed patches have been landed. Closing bug.