Bug 51093 – Refactor HTMLInputElement: Move a part of HTMLInputElement::defaultEventHandler() to InputTypes

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Refactor HTMLInputElement: Move a part of HTMLInputElement::defaultEventHandler() to InputTypes

[Kent Tamura]

Created attachment 76626 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 76626 [details]
Patch

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

ok, with comments.

> WebCore/ChangeLog:10
> +        renders, and the isindex form submission quirk code to InputTypes.

renders->renderers

> WebCore/html/HTMLInputElement.h:213
> +    void handleBeforeTextInsertedEvent(Event* event) { InputElement::handleBeforeTextInsertedEvent(m_data, this, this, event); }

Darin would tell me to move this into HTMLInputElement.cpp :)

[Darin Adler]

Comment on attachment 76626 [details]
Patch

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

Great to see more progress on this. I am really looking forward to the day we delete DeprecatedInputType, InputTypeMap, createTypeMap, deprecatedInputType, and m_deprecatedTypeNumber.

>> WebCore/html/HTMLInputElement.h:213
>> +    void handleBeforeTextInsertedEvent(Event* event) { InputElement::handleBeforeTextInsertedEvent(m_data, this, this, event); }
> 
> Darin would tell me to move this into HTMLInputElement.cpp :)

I definitely would it it was a virtual function and so unlikely to get inlined or if the function body was big enough and called in more than one place so that inlining it actually makes the overall code size bigger, with no significant gain in performance.

But if neither of those is true, then having it in the header is OK with me. I do prefer the wordier idiom where the function definition is outside the class definition, because it keeps the class definition itself easier to read. See Node::parentNode for an example of this.

> WebCore/html/NumberInputType.cpp:56
> +    return ch == '+' || ch == '-' || ch == '.' || ch == 'e' || ch == 'E' || (ch >= '0' && ch <= '9');

For the last part of this expression, the isASCIIDigit is a good choice, perhaps slightly better than writing it out.

[Kent Tamura]

Comment on attachment 76626 [details]
Patch

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

Thank you for reviewing.
I have committed it as http://trac.webkit.org/changeset/74549

>> WebCore/ChangeLog:10
>> +        renders, and the isindex form submission quirk code to InputTypes.
> 
> renders->renderers

Fixed.

>>> WebCore/html/HTMLInputElement.h:213
>>> +    void handleBeforeTextInsertedEvent(Event* event) { InputElement::handleBeforeTextInsertedEvent(m_data, this, this, event); }
>> 
>> Darin would tell me to move this into HTMLInputElement.cpp :)
> 
> I definitely would it it was a virtual function and so unlikely to get inlined or if the function body was big enough and called in more than one place so that inlining it actually makes the overall code size bigger, with no significant gain in performance.
> 
> But if neither of those is true, then having it in the header is OK with me. I do prefer the wordier idiom where the function definition is outside the class definition, because it keeps the class definition itself easier to read. See Node::parentNode for an example of this.

Yeah, this function is not virtual and has 1-line content.
I moved the function definition to the outside of the class declaration.

>> WebCore/html/NumberInputType.cpp:56
>> +    return ch == '+' || ch == '-' || ch == '.' || ch == 'e' || ch == 'E' || (ch >= '0' && ch <= '9');
> 
> For the last part of this expression, the isASCIIDigit is a good choice, perhaps slightly better than writing it out.

Fixed.