[Webkit-unassigned] [Bug 105480] Implement element type selectors for the WebVTT ::cue pseudo class

[bugzilla-daemon at webkit.org]

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.