[Webkit-unassigned] [Bug 105480] Implement element type selectors for the WebVTT ::cue pseudo class
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jan 9 14:40:37 PST 2013
https://bugs.webkit.org/show_bug.cgi?id=105480
--- Comment #20 from Dima Gorbik <dgorbik at apple.com> 2013-01-09 14:42:29 PST ---
(In reply to comment #19)
> > Source/WebCore/html/HTMLQuoteElement.cpp:37
> > inline HTMLQuoteElement::HTMLQuoteElement(const QualifiedName& tagName, Document* document)
> > : HTMLElement(tagName, document)
> > {
> > - ASSERT(hasTagName(qTag) || hasTagName(blockquoteTag));
> > + ASSERT(hasTagName(qTag) || hasTagName(blockquoteTag) || hasTagName(vTag));
>
> This seems unnecessary. You never construct HTMLQuoteElement with vTag. Is this just because you want to use cloneNode() for making the rendering tree?
>
This is for cloneNode() to construct the right element. Also I think this should be for consistency.
> > Source/WebCore/html/track/TextTrackCue.cpp:486
> > m_documentFragment = WebVTTParser::create(0, m_scriptExecutionContext)->createDocumentFragmentFromCueText(m_content);
>
> m_documentFragment should be called something more sensible like m_webVTTNodeTree.
I agree. This is an old code. Do you want me to refactor this in this patch?
> Please use complete words are variable names, n -> node.
>
> > Source/WebCore/html/track/TextTrackCue.cpp:499
> > + if (n->isElementNode() && toElement(n)->localName() == HTMLNames::cTag.localName())
>
This is how it is in the cloning code.
> > Source/WebCore/html/track/TextTrackCue.cpp:500
> > + clonedNode = HTMLElement::create(spanTag, static_cast<Document*>(m_scriptExecutionContext));
>
> You should construct correct subclasses, HTMLSpanElement here...
This is how this node is constructed in the WebVTT parser. Should it be changed there as well?
> > Source/WebCore/html/track/TextTrackCue.cpp:509
> > + if (!clonedNode)
> > + clonedNode = n->cloneNode(false);
> > + else {
> > + toElement(clonedNode.get())->setAttribute(classAttr, toElement(n)->getAttribute(classAttr));
> > + toElement(clonedNode.get())->setAttribute(titleAttr, toElement(n)->getAttribute(titleAttr));
> > }
>
> With this class and title attributes are only getting set for elements created from v and q nodes. This seems wrong as there are other node types mapping to Elements.
Why should I do this for other node types if I use cloneNode function that copies all the attributes automatically?
> > Source/WebCore/html/track/TextTrackCue.cpp:511
> > + copyWebVTTNodeToDOMTree(n, clonedNode);
>
> You shouldn't recurse if the node is not a ContainerNode.
Is this because it's unsafe to call firstChild for non ContainerNodes?
> > Source/WebCore/html/track/TextTrackCue.cpp:-745
> > - if (m_hasInnerTimestamps)
> > - updateDisplayTree(track()->mediaElement()->currentTime());
>
> Why don't we need to call updateDisplayTree anymore?
The tree doesn't change anymore, since I removed past/future containers in the previous patch. All nodes stay on their places throughout the cue lifetime.
Thanks for a review!
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list