Refactor HTMLInputElement: Move a part of HTMLInputElement::defaultEventHandler() to InputTypes
Created attachment 76626 [details] Patch
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 :)
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.
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.