[webkit-reviews] review denied: [Bug 105480] Implement element type selectors for the WebVTT ::cue pseudo class : [Attachment 181974] Proposd fix 0.6

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Dima Gorbik <dgorbik at apple.com>'s
request for review:
Bug 105480: Implement element type selectors for the WebVTT ::cue pseudo class
https://bugs.webkit.org/show_bug.cgi?id=105480

Attachment 181974: Proposd fix 0.6
https://bugs.webkit.org/attachment.cgi?id=181974&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
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?