Bug 42259

Summary: [Chromium] <input type=number> UI implementation for Windows
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, fishd
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Windows XP   
Bug Depends on: 42260    
Bug Blocks: 38570    
Attachments:
Description Flags
Patch
none
Patch 2 none

Kent Tamura
Reported 2010-07-14 07:14:44 PDT
[Chromium] <input type=number> UI implementation for Windows
Attachments
Patch (15.59 KB, patch)
2010-07-15 00:56 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Patch 2 (15.64 KB, patch)
2010-07-15 17:36 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Kent Tamura
Comment 1 2010-07-15 00:56:06 PDT
Created attachment 61616 [details] Patch
Peter Kasting
Comment 2 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).
Kent Tamura
Comment 3 2010-07-15 17:36:18 PDT
Created attachment 61753 [details] Patch 2
Kent Tamura
Comment 4 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.
Darin Fisher (:fishd, Google)
Comment 5 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
Kent Tamura
Comment 6 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>
Kent Tamura
Comment 7 2010-07-16 15:06:05 PDT
All reviewed patches have been landed. Closing bug.
Kent Tamura
Comment 8 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.
Note You need to log in before you can comment on or make changes to this bug.