Bug 107080 – Remove a TextTrack.h include from the Element.h and move WebVTT related stuff outside the Element

• Component: Media
• Status: RESOLVED FIXED

[Dima Gorbik]

We should try to keep hot include files light in terms of sub includes to reduce build times.

[Dima Gorbik]

Created attachment 183349 [details]
Proposed fix 0.1

[Elliott Sprehn]

Comment on attachment 183349 [details]
Proposed fix 0.1

View in context: https://bugs.webkit.org/attachment.cgi?id=183349&action=review

> Source/WebCore/css/SelectorChecker.cpp:948
> +            return (element->isWebVTTElement() && toWebVTTElement(element)->webVTTNodeType() == WebVTTNodeTypeFuture);

Instead of this incantation in multiple places it would be nicer to hide this in a method on Element, Element::webVTTNodeType() const { return isWebVTTElement(this) ? toWebVTTElement(this)->type() ? WebVTTNodeTypeNone; };

There's no reason for the method on WebVTTElement to be called webVTTNodeType, it should just be called type() or something.

> Source/WebCore/html/track/WebVTTElement.h:32
> +enum WebVTTNodeType {

WebVTTElementType ? And also fix the setters and such to be setWebVTTType() I don't really see a reason to repeat Node and Element so many times.

> Source/WebCore/html/track/WebVTTElement.h:58
> +    ASSERT(tagName.localName().impl());

This is a really weird assert to make. Shouldn't Element already assert about this?

[Dima Gorbik]

(In reply to comment #2)
> (From update of attachment 183349 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183349&action=review
> 
> > Source/WebCore/css/SelectorChecker.cpp:948
> > +            return (element->isWebVTTElement() && toWebVTTElement(element)->webVTTNodeType() == WebVTTNodeTypeFuture);
> 
> Instead of this incantation in multiple places it would be nicer to hide this in a method on Element, Element::webVTTNodeType() const { return isWebVTTElement(this) ? toWebVTTElement(this)->type() ? WebVTTNodeTypeNone; };

I will end up with an include in Element.h again, what's the purpose of the patch then?

[Build Bot]

Comment on attachment 183349 [details]
Proposed fix 0.1

Attachment 183349 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15947246

[Elliott Sprehn]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 183349 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=183349&action=review
> > 
> > > Source/WebCore/css/SelectorChecker.cpp:948
> > > +            return (element->isWebVTTElement() && toWebVTTElement(element)->webVTTNodeType() == WebVTTNodeTypeFuture);
> > 
> > Instead of this incantation in multiple places it would be nicer to hide this in a method on Element, Element::webVTTNodeType() const { return isWebVTTElement(this) ? toWebVTTElement(this)->type() ? WebVTTNodeTypeNone; };
> 
> I will end up with an include in Element.h again, what's the purpose of the patch then?

Not if you make it non-inline, or if you place the inline version in the WebVTTElement.h, it's not that big a deal. :)

It does occur to me that you're putting multiple virtual method calls in the middle of selector matching which is rather hot, you'll want to cache that value like we do for isFormControlElement() in that method.

[Elliott Sprehn]

(In reply to comment #5)
> ...
> Not if you make it non-inline, or if you place the inline version in the WebVTTElement.h, it's not that big a deal. :)
> 

Discussed this on IRC, I think we should just leave the patch the way it is in terms of the methods. Ignore my suggestion in this bug.

[Dima Gorbik]

Created attachment 183590 [details]
Proposed fix 0.2

[Dima Gorbik]

(In reply to comment #5)
> It does occur to me that you're putting multiple virtual method calls in the middle of selector matching which is rather hot, you'll want to cache that value like we do for isFormControlElement() in that method.

This code is located in the function that checks if we may share styles. It's not being called too often, so probably we don't have to do that kind of caching.

[Sam Weinig]

Comment on attachment 183590 [details]
Proposed fix 0.2

View in context: https://bugs.webkit.org/attachment.cgi?id=183590&action=review

> Source/WebCore/html/track/WebVTTElement.h:32
> +enum WebVTTNodeType {

This should probably be WebVTTElementType, no?

> Source/WebCore/html/track/WebVTTElement.h:48
> +    WebVTTElement(const QualifiedName& tagName, Document*);

No need for tagName here.  This should probably be private.

> Source/WebCore/html/track/WebVTTElement.h:56
> +: HTMLElement(tagName, document)
> +, m_webVTTNodeType(WebVTTNodeTypeNone)

These should be indented.  Does this need to be in the header?

> Source/WebCore/html/track/WebVTTElement.h:60
> +inline WebVTTElement* toWebVTTElement(Node* node)

You should probably also add a 

// This will catch anyone doing an unnecessary cast.
void toWebVTTElement(const WebVTTElement*); 

without an implementation.

[Dima Gorbik]

Created attachment 183593 [details]
Proposed fix 0.3

[Radar WebKit Bug Importer]

<rdar://problem/13047516>

[WebKit Review Bot]

Comment on attachment 183593 [details]
Proposed fix 0.3

Clearing flags on attachment: 183593

Committed r140231: <http://trac.webkit.org/changeset/140231>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.