[webkit-reviews] review denied: [Bug 89439] [Forms] Make step action of SpinButtonElement replaceable : [Attachment 148278] Patch 1

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 89439: [Forms] Make step action of SpinButtonElement replaceable
https://bugs.webkit.org/show_bug.cgi?id=89439

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=148278&action=review


> Source/WebCore/html/TextFieldInputType.h:87
>  
> +    virtual void spinButtonStepDown() OVERRIDE;
> +    virtual void spinButtonStepUp() OVERRIDE;

nit: Add a comment like "// SpinButtonElement::Callback functions."

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:301
>		   if (m_upDownState != Indeterminate) {
> -		       input->stepUpFromRenderer(m_upDownState == Up ? 1 : -1);

> +		       if (m_upDownState == Up)
> +			   m_callback.spinButtonStepUp();
> +		       else
> +			   m_callback.spinButtonStepDown();

How do you make sure m_callback object is live after spinButtonStepUp/Down
calls, which dispatch 'change' event and JavaScript code change the input type.


> Source/WebCore/html/shadow/TextControlInnerElements.h:99
> +    class Callback {

The name 'Callback' sounds too generic.
It should be more specific.  e.g. ValueUpdater, ValueChangeDelegate, ...

> Source/WebCore/html/shadow/TextControlInnerElements.h:102
> +	   virtual void spinButtonStepDown() = 0;
> +	   virtual void spinButtonStepUp() = 0;

Why do you separate the functionality of stepUpFromRenderer() into two
functions? It makes the patch larger.