[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


Antti Koivisto <koivisto at iki.fi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #181974|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #19 from Antti Koivisto <koivisto at iki.fi>  2013-01-09 14:20:02 PST ---
(From update of attachment 181974)
View in context: https://bugs.webkit.org/attachment.cgi?id=181974&action=review

> LayoutTests/media/track/captions-webvtt/styling.vtt:19
>  \ No newline at end of file

You could fix that.

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

> Source/WebCore/html/HTMLTagNames.in:26
> +c interfaceName=HTMLElement

This is bit misleading as these are not really HTML names (we don't expose HTML elements with these names) and so should not be in HTMLTagNames. I think you should just construct and expose QualifiedNames for them manually in TextTrackCue.h/cpp (see anyName in QualifiedName.h for example).

> Source/WebCore/html/HTMLTagNames.in:139
> +v interfaceName=HTMLQuoteElement

Same thing.

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

> Source/WebCore/html/track/TextTrackCue.cpp:490
> -
>          if (!m_documentFragment)
> -          return 0;
> +            return;
> +    }
> +}

This return branch looks unnecessary.

> Source/WebCore/html/track/TextTrackCue.cpp:492
> +void TextTrackCue::copyWebVTTNodeToDOMTree(PassRefPtr<Node> WebVTTNode, PassRefPtr<Node> root)

'root' is not root here. The variable should be called 'parent'. It could also be tightened to be ContainerNode.

PassRefPtr's are only needed when transferring ownership. Please use plain pointers.

In WebKit coding style, variable names start with lowercase letter (webVTTNode).

This could be a standalone function if you passed in the Document*.

> Source/WebCore/html/track/TextTrackCue.cpp:496
> +    for (Node* n = WebVTTNode->firstChild(); n && !ec; n = n->nextSibling()) {

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

Test with node->hasTagName(vTag), no need of element test or toElement.

> Source/WebCore/html/track/TextTrackCue.cpp:500
> +            clonedNode = HTMLElement::create(spanTag, static_cast<Document*>(m_scriptExecutionContext));

You should construct correct subclasses, HTMLSpanElement here...

Please save Document* to a variable for resuse.

> Source/WebCore/html/track/TextTrackCue.cpp:501
> +        else if (n->isElementNode() && toElement(n)->localName() == HTMLNames::vTag.localName())

hasTagName

> Source/WebCore/html/track/TextTrackCue.cpp:502
> +            clonedNode = HTMLElement::create(qTag, static_cast<Document*>(m_scriptExecutionContext));

... HTMLQuoteElement here.

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

> Source/WebCore/html/track/TextTrackCue.cpp:510
> +        root->appendChild(clonedNode, ec);

This should never fail. Get rid of the ec variable, do appendChild(clonedNode, ASSERT_NO_EXCEPTION) instead.

> Source/WebCore/html/track/TextTrackCue.cpp:511
> +        copyWebVTTNodeToDOMTree(n, clonedNode);

You shouldn't recurse if the node is not a ContainerNode.

> Source/WebCore/html/track/TextTrackCue.cpp:530
> +PassRefPtr<DocumentFragment> TextTrackCue::getCueRenderingTree()
> +{

We don't use get* naming like this, createCueRenderingTree.

> Source/WebCore/html/track/TextTrackCue.cpp:537
> +    Document* document;
> +    RefPtr<DocumentFragment> clonedFragment;
> +    
> +    createWebVTTNodeTree();
> +    document = static_cast<Document*>(m_scriptExecutionContext);
> +    
> +    clonedFragment = DocumentFragment::create(document);

Please define the variable and set its value on a single line.

> Source/WebCore/html/track/TextTrackCue.cpp:-745
> -    if (m_hasInnerTimestamps)
> -        updateDisplayTree(track()->mediaElement()->currentTime());

Why don't we need to call updateDisplayTree anymore?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.