Bug 38381 - [Windows] Implement <input type=number> UI
Summary: [Windows] Implement <input type=number> UI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 41924 42260
Blocks: 27968
  Show dependency tree / graph
 
Reported: 2010-04-30 02:01 PDT by Kent Tamura
Modified: 2010-09-30 22:09 PDT (History)
1 user (show)

See Also:

Attachments
Patch (9.91 KB, patch)
2010-07-11 08:09 PDT, Kent Tamura 		no flags Details | Formatted Diff | Diff
Patch 2 (9.82 KB, patch)
2010-09-30 00:18 PDT, Kent Tamura 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2010-04-30 02:01:45 PDT 
Visit https://bugs.webkit.org/show_bug.cgi?id=27968 for a screenshot.
Comment 1 Kent Tamura 2010-07-11 08:09:53 PDT 
Created attachment 61177 [details]
Patch
Comment 2 chris fleizach 2010-09-22 00:30:12 PDT 
Comment on attachment 61177 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=61177&action=review

mostly minor comments

> WebCore/rendering/RenderThemeWin.cpp:98
> +// Spin button staes

states spelled incorrectly

> WebCore/rendering/RenderThemeWin.cpp:500
> +}

extra new line that's not needed

> WebCore/rendering/RenderThemeWin.cpp:516
> +            result.m_state = determineClassicState(o, false);

instead of using a bool here, a simple ENUM would be helpful so that when looking at lines like this, it's clear that you're passing in a down button

> WebCore/rendering/RenderThemeWin.cpp:680
> +        width = 17; // Vista's default.

can you retrieve this from some system setting if it's a default?

> WebCore/rendering/RenderThemeWin.cpp:688
> +        return false;

why less than 2 and not 1 or 3?
Comment 3 Kent Tamura 2010-09-30 00:18:11 PDT 
Created attachment 69317 [details]
Patch 2
Comment 4 Kent Tamura 2010-09-30 00:20:19 PDT 
Comment on attachment 61177 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=61177&action=review

>> WebCore/rendering/RenderThemeWin.cpp:98
>> +// Spin button staes
> 
> states spelled incorrectly

Fixed.

>> WebCore/rendering/RenderThemeWin.cpp:500
>> +}
> 
> extra new line that's not needed

Fixed.

>> WebCore/rendering/RenderThemeWin.cpp:516
>> +            result.m_state = determineClassicState(o, false);
> 
> instead of using a bool here, a simple ENUM would be helpful so that when looking at lines like this, it's clear that you're passing in a down button

Fixed.  I introduced ControlSubPart enum, which is the same as RenderThemeChromiumWin.

>> WebCore/rendering/RenderThemeWin.cpp:680
>> +        width = 17; // Vista's default.
> 
> can you retrieve this from some system setting if it's a default?

I don't think we can.

>> WebCore/rendering/RenderThemeWin.cpp:688
>> +        return false;
> 
> why less than 2 and not 1 or 3?

I added a comment.
Comment 5 chris fleizach 2010-09-30 09:13:09 PDT 
Comment on attachment 69317 [details]
Patch 2

r=me thanks
Comment 6 Kent Tamura 2010-09-30 22:09:19 PDT 
Comment on attachment 69317 [details]
Patch 2

Clearing flags on attachment: 69317

Committed r68864: <http://trac.webkit.org/changeset/68864>
Comment 7 Kent Tamura 2010-09-30 22:09:28 PDT 
All reviewed patches have been landed.  Closing bug.