Bug 42259 - [Chromium] <input type=number> UI implementation for Windows
Summary: [Chromium] <input type=number> UI implementation for Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Windows XP
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on: 42260
Blocks: 38570
  Show dependency treegraph
 
Reported: 2010-07-14 07:14 PDT by Kent Tamura
Modified: 2010-07-16 15:09 PDT (History)
2 users (show)

See Also:


Attachments
Patch (15.59 KB, patch)
2010-07-15 00:56 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (15.64 KB, patch)
2010-07-15 17:36 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2010-07-14 07:14:44 PDT
[Chromium] <input type=number> UI implementation for Windows
Comment 1 Kent Tamura 2010-07-15 00:56:06 PDT
Created attachment 61616 [details]
Patch
Comment 2 Peter Kasting 2010-07-15 11:12:34 PDT
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).
Comment 3 Kent Tamura 2010-07-15 17:36:18 PDT
Created attachment 61753 [details]
Patch 2
Comment 4 Kent Tamura 2010-07-15 17:37:12 PDT
(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.
Comment 5 Darin Fisher (:fishd, Google) 2010-07-16 10:51:06 PDT
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
Comment 6 Kent Tamura 2010-07-16 15:05:58 PDT
Comment on attachment 61753 [details]
Patch 2

Clearing flags on attachment: 61753

Committed r63589: <http://trac.webkit.org/changeset/63589>
Comment 7 Kent Tamura 2010-07-16 15:06:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Kent Tamura 2010-07-16 15:09:26 PDT
(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.