Summary: | [Chromium] <input type=number> UI implementation for Windows | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||
Component: | Forms | Assignee: | 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
Kent Tamura
2010-07-14 07:14:44 PDT
Created attachment 61616 [details]
Patch
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).
Created attachment 61753 [details]
Patch 2
(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 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 on attachment 61753 [details] Patch 2 Clearing flags on attachment: 61753 Committed r63589: <http://trac.webkit.org/changeset/63589> All reviewed patches have been landed. Closing bug. (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. |