[webkit-reviews] review granted: [Bug 50097] Refactor HTMLInputElement: Move a part of HTMLInputElement::defaultEventHandler() to InputTypes : [Attachment 74992] Patch 3 (fix Qt build)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kent Tamura <tkent at chromium.org>'s
request for review:
Bug 50097: Refactor HTMLInputElement: Move a part of
HTMLInputElement::defaultEventHandler() to InputTypes
https://bugs.webkit.org/show_bug.cgi?id=50097

Attachment 74992: Patch 3 (fix Qt build)
https://bugs.webkit.org/attachment.cgi?id=74992&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.