Bug 118334 - Append overloading function for isFooElement
Summary: Append overloading function for isFooElement
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-02 19:04 PDT by Kangil Han
Modified: 2013-07-03 07:19 PDT (History)
4 users (show)

See Also:


Attachments
Patch (13.48 KB, patch)
2013-07-02 19:09 PDT, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (1.83 KB, patch)
2013-07-03 01:14 PDT, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (14.96 KB, patch)
2013-07-03 05:08 PDT, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (11.66 KB, patch)
2013-07-03 06:34 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-02 19:04:45 PDT
is/toHTMLFormElement should use Element* for its argument
Comment 1 Kangil Han 2013-07-02 19:09:50 PDT
Created attachment 205961 [details]
Patch
Comment 2 Kent Tamura 2013-07-02 22:46:49 PDT
Comment on attachment 205961 [details]
Patch

The code looks worse.
Why don't you provide both of isHTMLFormElement(Node*) and isHTMLFormElement(Element*).
Comment 3 Andreas Kling 2013-07-02 22:48:53 PDT
(In reply to comment #2)
> (From update of attachment 205961 [details])
> The code looks worse.
> Why don't you provide both of isHTMLFormElement(Node*) and isHTMLFormElement(Element*).

Or possibly:
template<class T> bool isHTMLFormElementNode(T*)
Comment 4 Kangil Han 2013-07-02 23:34:00 PDT
Thx tkent and kling for the comments!
I will investigate to make code better. :)
Comment 5 Kangil Han 2013-07-03 01:14:19 PDT
Created attachment 205977 [details]
Patch
Comment 6 Kangil Han 2013-07-03 03:57:58 PDT
kling, tkent: review please? :)
Comment 7 Antti Koivisto 2013-07-03 04:08:03 PDT
Comment on attachment 205977 [details]
Patch

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

I

> Source/WebCore/html/HTMLFormElement.h:165
> -inline bool isHTMLFormElement(Node* node)
> +template<class T>
> +inline bool isHTMLFormElement(T* t)

I think you should provide overloads instead of using templates since you only really need two cases. Error messages are clearer that way.
Comment 8 Kangil Han 2013-07-03 04:14:18 PDT
(In reply to comment #7)
> I think you should provide overloads instead of using templates since you only really need two cases. Error messages are clearer that way.

Thx! I will do that. :)
Comment 9 Kangil Han 2013-07-03 05:08:15 PDT
Created attachment 205994 [details]
Patch
Comment 10 Antti Koivisto 2013-07-03 05:29:10 PDT
Comment on attachment 205994 [details]
Patch

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

> Source/WebCore/html/HTMLOptionElement.h:119
> +inline HTMLOptionElement* toHTMLOptionElement(Element* element)
> +{
> +    ASSERT_WITH_SECURITY_IMPLICATION(!element || isHTMLOptionElement(element));
> +    return static_cast<HTMLOptionElement*>(element);
> +}
> +

You don't really need the to* overloads.
Comment 11 Kangil Han 2013-07-03 06:34:43 PDT
Created attachment 206000 [details]
Patch
Comment 12 Kangil Han 2013-07-03 06:36:37 PDT
(In reply to comment #10)
> (From update of attachment 205994 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205994&action=review
> 
> > Source/WebCore/html/HTMLOptionElement.h:119
> > +inline HTMLOptionElement* toHTMLOptionElement(Element* element)
> > +{
> > +    ASSERT_WITH_SECURITY_IMPLICATION(!element || isHTMLOptionElement(element));
> > +    return static_cast<HTMLOptionElement*>(element);
> > +}
> > +
> 
> You don't really need the to* overloads.

Yes, right! Thx! :)
Took anttik's comment into consideration.
Comment 13 Antti Koivisto 2013-07-03 06:38:37 PDT
Comment on attachment 206000 [details]
Patch

Actually maybe you need it. I guess ASSERT_WITH_SECURITY_IMPLICATION might get compiled in in release mode too?
Comment 14 Antti Koivisto 2013-07-03 06:42:55 PDT
Comment on attachment 206000 [details]
Patch

Never mind, apparently it is just a fuzzer hint.
Comment 15 WebKit Commit Bot 2013-07-03 07:19:19 PDT
Comment on attachment 206000 [details]
Patch

Clearing flags on attachment: 206000

Committed r152353: <http://trac.webkit.org/changeset/152353>
Comment 16 WebKit Commit Bot 2013-07-03 07:19:22 PDT
All reviewed patches have been landed.  Closing bug.