Bug 118714 – Use toHTMLSelectElement and dismiss isHTMLSelectElement

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

[Kangil Han]

Use toHTMLSelectElement and dismiss isHTMLSelectElement

[Kangil Han]

Created attachment 206731 [details]
Patch

[Kangil Han]

After having time for thought on isFooElement, I've reached a conclusion that using isFooElement would bring us more drawbacks than advantages.

For example, as Ryosuke concerned, all elements aren't presented by tag names.
Moreover, it will bring unnecessary header includes, it actually is refactoring though.

Therefore, from now on, I will introduce or use toFooElement and dismiss isFooElement step by step. :)

[Ryosuke Niwa]

Comment on attachment 206731 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Additionally, this patch dismisses isHTMLSelectElement because it doesn't cover all element and basically accompanies unnecessary header includes.

I don't really understand what you mean here.

[EFL EWS Bot]

Comment on attachment 206731 [details]
Patch

Attachment 206731 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1090104

[Kangil Han]

(In reply to comment #3)
> (From update of attachment 206731 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206731&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Additionally, this patch dismisses isHTMLSelectElement because it doesn't cover all element and basically accompanies unnecessary header includes.
> 
> I don't really understand what you mean here.

How about this? :)

Additionally, this patch dismisses isHTMLSelectElement because all elements aren't presented by tag names and it will bring unnecessary header includes; it actually is refactoring though.

[Kangil Han]

(In reply to comment #4)
> (From update of attachment 206731 [details])
> Attachment 206731 [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/1090104

Below error is not related to this patch. :)

*** Error during phase checkout of libseccomp: ########## Error running git remote update origin ***

[Ryosuke Niwa]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 206731 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=206731&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        Additionally, this patch dismisses isHTMLSelectElement because it doesn't cover all element and basically accompanies unnecessary header includes.
> > 
> > I don't really understand what you mean here.
> 
> How about this? :)
> 
> Additionally, this patch dismisses isHTMLSelectElement because all elements aren't presented by tag names and it will bring unnecessary header includes; it actually is refactoring though.

How about /dismisses/removes/ and mention that "not all element subclasses can be checked by a combination of tag names". I don't think unnecessary header includes is applicable here. I don't think you have to say this is refactoring since the entire patch is refactoring.

[Kangil Han]

Created attachment 206738 [details]
Patch

[Kangil Han]

(In reply to comment #7)
> 
> How about /dismisses/removes/ and mention that "not all element subclasses can be checked by a combination of tag names". I don't think unnecessary header includes is applicable here. I don't think you have to say this is refactoring since the entire patch is refactoring.

Thx Ryosuke! :)
Took Ryosuke's comment into consideration.

[WebKit Commit Bot]

Comment on attachment 206738 [details]
Patch

Clearing flags on attachment: 206738

Committed r152709: <http://trac.webkit.org/changeset/152709>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.