Bug 118235

Summary: Adopt is/toHTMLMapElement for code cleanup
Product: WebKit Reporter: Kangil Han <kangil.han>
Component: WebCore Misc.Assignee: Kangil Han <kangil.han>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, esprehn+autocc, jdiggs, kling, koivisto, mario
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Kangil Han
Reported 2013-07-01 04:12:40 PDT
Adopt is/toHTMLMapElement for code cleanup
Attachments
Patch (6.74 KB, patch)
2013-07-01 04:15 PDT, Kangil Han 		no flags
DetailsFormatted DiffDiff
Patch (6.79 KB, patch)
2013-07-03 19:36 PDT, Kangil Han 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Kangil Han
Comment 1 2013-07-01 04:15:03 PDT
Created attachment 205796 [details] Patch
Andreas Kling
Comment 2 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.
Kangil Han
Comment 3 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?
Kangil Han
Comment 4 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. :)
Andreas Kling
Comment 5 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.
Kangil Han
Comment 6 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. :)
Kangil Han
Comment 7 2013-07-03 19:36:03 PDT
Created attachment 206047 [details] Patch
Kangil Han
Comment 8 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!
WebKit Commit Bot
Comment 9 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>
WebKit Commit Bot
Comment 10 2013-07-03 21:32:19 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.