Refactor HTMLInputElement: Move a part of HTMLInputElement::defaultEventHandler() to InputTypes
Created attachment 74902 [details] Patch
Comment on attachment 74902 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74902&action=review > WebCore/html/InputType.cpp:307 > + // Simulate mouse click on the default form button for enter for these types of elements. Which types of elements? :) The comment now looks out of place. Also, passing implicitSubmission bool seems awkward, especially considering you already have shouldSubmitImplicitly method on this class.
(In reply to comment #2) > (From update of attachment 74902 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74902&action=review > > > WebCore/html/InputType.cpp:307 > > + // Simulate mouse click on the default form button for enter for these types of elements. > > Which types of elements? :) The comment now looks out of place. Oops, I'll update it. > Also, passing implicitSubmission bool seems awkward, especially considering you already have shouldSubmitImplicitly method on this class. Yeah, we can check implicit submission at one place.
Created attachment 74991 [details] Patch 2
Attachment 74991 [details] did not build on qt: Build output: http://queues.webkit.org/results/6450013
Created attachment 74992 [details] Patch 3 (fix Qt build)
Comment on attachment 74992 [details] Patch 3 (fix Qt build) View in context: https://bugs.webkit.org/attachment.cgi?id=74992&action=review > WebCore/html/BaseButtonInputType.cpp:80 > + if (element()->active()) > + element()->dispatchSimulatedClick(event); We could make a protected helper function for this sequence, which occurs repeatedly in multiple classes. > WebCore/html/InputType.h:115 > // Event handlers > - // If the return value is true, do no further default event handling in the > - // default event handler. If an event handler calls Event::setDefaultHandled(), > - // its return value must be true. > + // If the return value of handleFooEvent() is true, do no further default > + // event handling in the default event handler. If an event handler calls > + // Event::setDefaultHandled(), its return value must be true. > > + virtual bool shouldSubmitImplicitly(Event*); > virtual bool handleClickEvent(MouseEvent*); > virtual bool handleDOMActivateEvent(Event*); > virtual bool handleKeydownEvent(KeyboardEvent*); > + virtual bool handleKeypressEvent(KeyboardEvent*); > + virtual bool handleKeyupEvent(KeyboardEvent*); I think it would be better to put shouldSubmitImplicitly into a separate paragraph. It is not like the other functions, and changing the comment to say handleFooEvent is a roundabout way to deal with that. I think in a future patch we should get rid of the bool return values and instead change the code in HTMLInputElement to check defaultHandled directly; that makes it impossible to make a mistake and eliminates the need for that last sentence of the comment.
Comment on attachment 74992 [details] Patch 3 (fix Qt build) View in context: https://bugs.webkit.org/attachment.cgi?id=74992&action=review >> WebCore/html/BaseButtonInputType.cpp:80 >> + element()->dispatchSimulatedClick(event); > > We could make a protected helper function for this sequence, which occurs repeatedly in multiple classes. I'll add InputType::dispatchSimulatedClickIfActive(KeyboardEvent*). >> WebCore/html/InputType.h:115 >> + virtual bool handleKeyupEvent(KeyboardEvent*); > > I think it would be better to put shouldSubmitImplicitly into a separate paragraph. It is not like the other functions, and changing the comment to say handleFooEvent is a roundabout way to deal with that. > > I think in a future patch we should get rid of the bool return values and instead change the code in HTMLInputElement to check defaultHandled directly; that makes it impossible to make a mistake and eliminates the need for that last sentence of the comment. ok, I'll move shouldSubmitImplicitly() declaration.
(In reply to comment #7) > I think in a future patch we should get rid of the bool return values and instead change the code in HTMLInputElement to check defaultHandled directly; that makes it impossible to make a mistake and eliminates the need for that last sentence of the comment. Yes. "return true" and "setDefaultHandled()" are matched exactly in the current code though they were not matched before the previous patch. I'll remove the bool return values.
Landed: http://trac.webkit.org/changeset/73008