Bug 42259 – [Chromium] <input type=number> UI implementation for Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

[Chromium] <input type=number> UI implementation for Windows

[Kent Tamura]

Created attachment 61616 [details]
Patch

[Peter Kasting]

Comment on attachment 61616 [details]
Patch

WebCore/rendering/RenderThemeChromiumWin.cpp:498
 +      else if (isReadOnlyControl(o))
I think these two conditions might be clearer joined together as:

else if (isReadOnlyControl(o))
  result = (appearance == TextFieldPart || appearance == TextAreaPart || appearance == SearchFieldPart) ? ETS_READONLY : TS_DISABLED;

WebCore/rendering/RenderThemeChromiumWin.h:104
 +              NoneOrDownButton,
Nit: I don't see any reason not to do
    None,
    SpinButtonDown,
    SpinButtonUp,
...which seems clearer to me (even though I know the logic you have works correctly).

[Kent Tamura]

Created attachment 61753 [details]
Patch 2

[Kent Tamura]

(In reply to comment #2)
> (From update of attachment 61616 [details])
> WebCore/rendering/RenderThemeChromiumWin.cpp:498
>  +      else if (isReadOnlyControl(o))
> I think these two conditions might be clearer joined together as:
> 
> else if (isReadOnlyControl(o))
>   result = (appearance == TextFieldPart || appearance == TextAreaPart || appearance == SearchFieldPart) ? ETS_READONLY : TS_DISABLED;
> 
> WebCore/rendering/RenderThemeChromiumWin.h:104
>  +              NoneOrDownButton,
> Nit: I don't see any reason not to do
>     None,
>     SpinButtonDown,
>     SpinButtonUp,
> ...which seems clearer to me (even though I know the logic you have works correctly).

Peter, thank you for the comments.
They are reasonable.  I changed so.

[Darin Fisher (:fishd, Google)]

Comment on attachment 61753 [details]
Patch 2

WebCore/rendering/RenderThemeChromiumWin.cpp:685
 +      WebCore::ThemePainter upPainter(info.context, half);
nit: no need for the WebCore:: prefix, right?

WebCore/rendering/RenderThemeChromiumWin.cpp:694
 +      WebCore::ThemePainter downPainter(info.context, half);
ditto


R=me

[Kent Tamura]

Comment on attachment 61753 [details]
Patch 2

Clearing flags on attachment: 61753

Committed r63589: <http://trac.webkit.org/changeset/63589>

[Kent Tamura]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

(In reply to comment #5)
> (From update of attachment 61753 [details])
> WebCore/rendering/RenderThemeChromiumWin.cpp:685
>  +      WebCore::ThemePainter upPainter(info.context, half);
> nit: no need for the WebCore:: prefix, right?

Right.  Other functions have the same issues and I don't have a Windows develop machine at this moment.  So I committed this patch as is and will fix them later.