[webkit-reviews] review requested: [Bug 32813] Type=number UI part 2: Mac implementation of spin-button : [Attachment 45318] Proposed patch

[bugzilla-daemon at webkit.org]

TAMURA, Kent <tkent at chromium.org> has asked  for review:
Bug 32813: Type=number UI part 2: Mac implementation of spin-button
https://bugs.webkit.org/show_bug.cgi?id=32813

Attachment 45318: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=45318&action=review

------- Additional Comments from TAMURA, Kent <tkent at chromium.org>
(In reply to comment #0)
> Why have this big function if you're only going to return 0?
> +const int* stepperMargins(NSControlSize controlSize)
> +{
> +    static const int margins[3][4] = {
> +	   {0, 0, 0, 0},
> +	   {0, 0, 0, 0},
> +	   {0, 0, 0, 0}

I followed other similar functions in this file. I removed it in the updated
patch.

> No need:
> +    static NSStepperCell* cell;
> +    if (!cell) {
> +	   cell = [[NSStepperCell alloc] init];
> +    }
> Just do:
> static NSStepperCell* cell = [[NSStepperCell alloc] init];

Fixed.

> Needs { } due to the comment:
> +    if (states & PressedState && states & SpinUpState)
> +	   // FIXME: There is no way to draw a NSSteperCell with the up button
> hilighted.
> +	   // Disables the hilight of the down button if the up button is
> pressed.
> +	   [cell setHighlighted:NO];

Fixed.