[webkit-reviews] review denied: [Bug 48317] Refactor HTMLInputElement: Move a part of HTMLInputElement::defaultEventhandler() to InputTypes : [Attachment 71860] Patch

[bugzilla-daemon at webkit.org]

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

Attachment 71860: Patch
https://bugs.webkit.org/attachment.cgi?id=71860&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=71860&action=review

Great work on the refactoring.

I’m going to say review- because I find the boolean results of these various
functions a bit too confusing. We should choose a single meaning for the
boolean result from the event function and stick to that. That probably means
reversing the logic for DOMActivateEvent.

I’m not sure that the “return immediately if form is 0” behavior from
DOMActivate is even important. Maybe the handleDOMActivateEvent function
doesn’t need a return value. These confusing bool return values are poison!

> WebCore/ChangeLog:16
> +	   Move m_xPos and m_yPos from HTMLInputElement to ImageInputType. It's

> +	   safe to move them because they have valid values just after click
event
> +	   and form submission by the click.

Could you create a test case where we change the type in response to the click
event before the form submission happens?

> WebCore/html/BaseDateAndTimeInputType.cpp:116
> +bool BaseDateAndTimeInputType::handleKeydownEvent(KeyboardEvent* event)
> +{
> +    if (handleKeydownEventForSpinButton(event))
> +	   return true;
> +    return TextFieldInputType::handleKeydownEventForSpinButton(event);
> +}

This is a very surprising function. It’s not clear at all why this calls
handleKeydownEventForSpinButton first, and then calls
TextFieldInputType::handleKeydownEventForSpinButton! Maybe the second call was
supposed to be a call to handleKeydownEvent?

> WebCore/html/FileInputType.cpp:83
> +    if (element()->renderer())
> +	   toRenderFileUploadControl(element()->renderer())->click();
> +    return false;

I would use && here instead of an if statement.

> WebCore/html/HTMLInputElement.cpp:1238
> +    if (evt->isKeyboardEvent() && evt->type() == eventNames().keydownEvent
> +	   &&
m_inputType->handleKeydownEvent(static_cast<KeyboardEvent*>(evt)))
>	   return;

I hate how the formatting looks in cases like this. Maybe we could put this all
on one long line?

> WebCore/html/HTMLInputElement.cpp:1256
> +    if (evt->type() == eventNames().DOMActivateEvent && !disabled()
> +	   && m_inputType->handleDOMActivateEvent(evt))
> +	   return;

Same thought here.

I’m not sure why the !disabled() check is being done here in the base class.
It’s an unclear separation of responsibilities. Nothing in the name
handleDOMActivateEvent makes it clear that disabled is already checked. It
seems a surprising difference from the handleKeydownEvent function.

> WebCore/html/ImageInputType.cpp:33
> +ALWAYS_INLINE ImageInputType::ImageInputType(HTMLInputElement* element)

Isn’t a plain old inline sufficient? Why was ALWAYS_INLINE needed?

> WebCore/html/ImageInputType.cpp:72
> +bool ImageInputType::handleClickEvent(MouseEvent* mouseEvent)

You’re moving enough code here that you may have to update copyright.

> WebCore/html/ImageInputType.h:53
> +    int m_xPosition;
> +    int m_yPosition;

These names aren’t so great. How about:

    IntPoint m_clickLocation;

Or something like that?

> WebCore/html/InputType.cpp:276
> +bool InputType::handleKeydownEventForSpinButton(KeyboardEvent* event)

Why is this spin-button-specific event handling in the InputType base class?
There should at least be a comment explaining this.

> WebCore/html/InputType.cpp:278
> +    String key = static_cast<KeyboardEvent*>(event)->keyIdentifier();

It would be better if keyIdentifier returned a const String& and this local
variable was also const String&. There’s no need to churn the reference count
just to check string equality.

> WebCore/html/InputType.cpp:285
> +    int step = 0;
> +    if (key == "Up")
> +	   step = 1;
> +    else if (key == "Down")
> +	   step = -1;
> +    if (!step)
> +	   return false;

I think "else return false" would be clearer.

> WebCore/html/InputType.h:105
> +    virtual bool handleClickEvent(MouseEvent*);
> +    virtual bool handleDOMActivateEvent(Event*);
> +    virtual bool handleKeydownEvent(KeyboardEvent*);

The meanings of these bool return values are unclear to me. What does true and
false mean?

I also think that we should provide a basic handleEvent function here, and put
the dispatching to the individual handleEvent functions inside that, but maybe
you can do that later. Eventually I’d hope these individual functions could
become protected or even private.

> WebCore/html/RangeInputType.cpp:131
> +bool RangeInputType::handleKeydownEvent(KeyboardEvent* event)

You’re moving enough code here that you may have to update copyright.

> WebCore/html/RangeInputType.cpp:141
> +	   // FIXME: Is 1/100 reasonable?

This is a mysterious comment. Rewrite for clarity?

> WebCore/html/ResetInputType.cpp:60
> +bool ResetInputType::handleDOMActivateEvent(Event*)
> +{
> +    if (!element()->form())
> +	   return true;
> +    element()->form()->reset();
> +    return false;
> +}

This seems backwards. What does the boolean return value from this function
means? It seems to be returning true when the function does nothing and false
when the function does something. Maybe we need a different return type if the
boolean result is unclear.

> WebCore/html/TextFieldInputType.cpp:59
> +    Document* document = element()->document();
> +    if (!document->frame())
> +	   return false;
> +    if (!document->frame()->editor()->doTextFieldCommandFromEvent(element(),
event))

The only way document is ever used is to call document->frame() so clearly the
local variable should be the Frame* instead.

> WebCore/html/TextFieldInputType.cpp:62
> +    event->setDefaultHandled();
> +    return true;

Why do some of these functions call setDefaultedHandled, but not others. I
think there is something wrong with the idiom here. I know you are only
refactoring, but this refactoring seems to be exposing inconsistencies that may
be bugs, so we should at least add FIXME.