Bug 118582 – Introduce toHTMLVideoElement

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kangil Han]

Introduce isHTMLVideoElement and toHTMLVideoElement

[Kangil Han]

Created attachment 206502 [details]
Patch

[Build Bot]

Comment on attachment 206502 [details]
Patch

Attachment 206502 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/962237

[Build Bot]

Comment on attachment 206502 [details]
Patch

Attachment 206502 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/970277

[Kangil Han]

@kling: Would you please give me a hint how I can fix below build error? :)

[Build error log]
/Volumes/Data/EWS/WebKit/Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:71:9: fatal error: 'WebCore/HTMLVideoElement.h' file not found
#import <WebCore/HTMLVideoElement.h>
        ^
1 error generated.

[Ryosuke Niwa]

Comment on attachment 206502 [details]
Patch

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

> Source/WebCore/html/HTMLVideoElement.h:109
> +inline bool isHTMLVideoElement(const Node* node)
> +{
> +    return node->hasTagName(HTMLNames::videoTag);
> +}
> +
> +inline bool isHTMLVideoElement(const Element* element)
> +{
> +    return element->hasTagName(HTMLNames::videoTag);
> +}

I don't think adding these helper functions is a good idea.
We don't do that for most of other elements at least.

> Source/WebCore/html/HTMLVideoElement.h:115
> +inline HTMLVideoElement* toHTMLVideoElement(Node* node)
> +{
> +    ASSERT_WITH_SECURITY_IMPLICATION(!node || isHTMLVideoElement(node));
> +    return static_cast<HTMLVideoElement*>(node);
> +}

We certainly do this.

[Gyuyoung Kim]

Comment on attachment 206502 [details]
Patch

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

> Source/WebCore/html/HTMLVideoElement.h:106
> +inline bool isHTMLVideoElement(const Element* element)

When I tried to add this function to svg, Benjamin and I noticed that we don't need to add this function. Is Element a child class which inherits Node ?

[Kangil Han]

Hi rniwa!
FOA, thx for the review. :)

(In reply to comment #5)
> (From update of attachment 206502 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206502&action=review
> 
> > Source/WebCore/html/HTMLVideoElement.h:109
> > +inline bool isHTMLVideoElement(const Node* node)
> > +{
> > +    return node->hasTagName(HTMLNames::videoTag);
> > +}
> > +
> > +inline bool isHTMLVideoElement(const Element* element)
> > +{
> > +    return element->hasTagName(HTMLNames::videoTag);
> > +}
> 
> I don't think adding these helper functions is a good idea.
> We don't do that for most of other elements at least.
> 

I've adopted isFooElement for some time and several patches have been landed. Actually, introducing isFooElement and toFooElement are agreed with anttik and kling. Therefore, if you wouldn't have strong objection for this, I would like to proceed adoption in WebKit. :)

[Kangil Han]

Hi gyuyoung!

(In reply to comment #6)
> (From update of attachment 206502 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206502&action=review
> 
> > Source/WebCore/html/HTMLVideoElement.h:106
> > +inline bool isHTMLVideoElement(const Element* element)
> 
> When I tried to add this function to svg, Benjamin and I noticed that we don't need to add this function. Is Element a child class which inherits Node ?

Node::hasTagName includes Node::isElementNode check and this is not necessary for Element. This is why we need overloading isFooElement(Element*) function. :)

[Gyuyoung Kim]

(In reply to comment #8)
> Hi gyuyoung!
> 
> (In reply to comment #6)
> > (From update of attachment 206502 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=206502&action=review
> > 
> > > Source/WebCore/html/HTMLVideoElement.h:106
> > > +inline bool isHTMLVideoElement(const Element* element)
> > 
> > When I tried to add this function to svg, Benjamin and I noticed that we don't need to add this function. Is Element a child class which inherits Node ?
> 
> Node::hasTagName includes Node::isElementNode check and this is not necessary for Element. This is why we need overloading isFooElement(Element*) function. :)

Though I don't know how much we can get benefit from it, looks make sense to me. BTW, do you have any number about it ?

[Kangil Han]

(In reply to comment #9)
> 
> Though I don't know how much we can get benefit from it, looks make sense to me. BTW, do you have any number about it ?

I don't have any profile data. :)
But, I can tell you about review history on this.

During isFooElement(Node*) adoption, anttik, kling and I had found a regression in code.

[before]
element->hasTagName(fooTag)
[after]
isFooElement(element)
means,
element->isElementNode() && element->hasTagName(fooTag)
so redundant 'element->isElementNode()' call had happened.
then, we've concluded to use isFooElement(Node*) and overloading isFooElement(Element*).

That's where we are so far. ;)

[Ryosuke Niwa]

Can't we auto-generate these is<tagName> functions instead?

[Kangil Han]

(In reply to comment #11)
> Can't we auto-generate these is<tagName> functions instead?

Would you please point out an example for reference? :)

[Ryosuke Niwa]

(In reply to comment #12)
> (In reply to comment #11)
> > Can't we auto-generate these is<tagName> functions instead?
> 
> Would you please point out an example for reference? :)

http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLTagNames.in

[Kangil Han]

Alright. :)

Summing up,
  1. toFooElement is recommended
  2. isFooElement wouldn't be because every element cannot adopt this. Nevertheless, if we want, let's use automation like HTMLTagNames.in

and I would like to focus what I can do now. So I am going to introduce toHTMLVideoElement in this bug and will do the same for other elements. During these introductions, I will think about necessity of isFooElement. :)

[Kangil Han]

Created attachment 206625 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 206625 [details]
Patch

Clearing flags on attachment: 206625

Committed r152614: <http://trac.webkit.org/changeset/152614>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.