Bug 118235 - Adopt is/toHTMLMapElement for code cleanup
Summary: Adopt is/toHTMLMapElement for code cleanup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kangil Han
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-01 04:12 PDT by Kangil Han
Modified: 2013-07-03 21:32 PDT (History)
10 users (show)

See Also:


Attachments
Patch (6.74 KB, patch)
2013-07-01 04:15 PDT, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (6.79 KB, patch)
2013-07-03 19:36 PDT, Kangil Han
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kangil Han 2013-07-01 04:12:40 PDT
Adopt is/toHTMLMapElement for code cleanup
Comment 1 Kangil Han 2013-07-01 04:15:03 PDT
Created attachment 205796 [details]
Patch
Comment 2 Andreas Kling 2013-07-01 04:40:57 PDT
Comment on attachment 205796 [details]
Patch

r=me

It's a little bit unfortunate that the isFooElement(Node*) helpers incur the cost of an isElementNode() check that wouldn't be necessary if the argument were an Element*. Perhaps we could templatize the method so the compiler would skip the check when the argument is of a narrower type. Food for thought.
Comment 3 Kangil Han 2013-07-01 07:01:28 PDT
(In reply to comment #2)
> 
> It's a little bit unfortunate that the isFooElement(Node*) helpers incur the cost of an isElementNode() check that wouldn't be necessary if the argument were an Element*. Perhaps we could templatize the method so the compiler would skip the check when the argument is of a narrower type. Food for thought.

Oops, you are right. :(
IMHO, thinking about node-element inheritance has led a choice to use Node* as an argument of isFooElement. Though hasTagName() is not a virtual function. :(

How about to take Element* as an argument of is/toFooElement?
Plus why don't we make Element::hasTagName an inline function?
Comment 4 Kangil Han 2013-07-01 07:13:45 PDT
(In reply to comment #3)
> Plus why don't we make Element::hasTagName an inline function?

My bad. :(
We can't make this function an inline.
Sorry to confuse you. :)
Comment 5 Andreas Kling 2013-07-01 07:25:50 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Plus why don't we make Element::hasTagName an inline function?
> 
> My bad. :(
> We can't make this function an inline.
> Sorry to confuse you. :)

Element::hasTagName() is already an inline function, defined in Element.h

It could be nice to use Element* arguments instead, though I suspect that would require a lot of type checking at various call sites.
Comment 6 Kangil Han 2013-07-01 07:35:20 PDT
(In reply to comment #5)
> 
> It could be nice to use Element* arguments instead, though I suspect that would require a lot of type checking at various call sites.

I guess so. :(
Anyway, I will try it and let you know. :)
Comment 7 Kangil Han 2013-07-03 19:36:03 PDT
Created attachment 206047 [details]
Patch
Comment 8 Kangil Han 2013-07-03 19:37:57 PDT
(In reply to comment #2)
> (From update of attachment 205796 [details])
> r=me
> 
> It's a little bit unfortunate that the isFooElement(Node*) helpers incur the cost of an isElementNode() check that wouldn't be necessary if the argument were an Element*. Perhaps we could templatize the method so the compiler would skip the check when the argument is of a narrower type. Food for thought.

Done by appending overloading isFooElement(Element*) inline function. :)
Thx!
Comment 9 WebKit Commit Bot 2013-07-03 21:32:15 PDT
Comment on attachment 206047 [details]
Patch

Clearing flags on attachment: 206047

Committed r152387: <http://trac.webkit.org/changeset/152387>
Comment 10 WebKit Commit Bot 2013-07-03 21:32:19 PDT
All reviewed patches have been landed.  Closing bug.