Bug 50097 – 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 74902 [details]
Patch

[Dimitri Glazkov (Google)]

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.

[Kent Tamura]

(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.

[Kent Tamura]

Created attachment 74991 [details]
Patch 2

[Early Warning System Bot]

Attachment 74991 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6450013

[Kent Tamura]

Created attachment 74992 [details]
Patch 3 (fix Qt build)

[Darin Adler]

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.

[Kent Tamura]

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.

[Kent Tamura]

(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.

[Kent Tamura]

Landed: http://trac.webkit.org/changeset/73008