Bug 38381

Summary:

[Windows] Implement <input type=number> UI

Product:

WebKit

Reporter:

Kent Tamura <tkent>

Component:

Forms

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cfleizach

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Windows Vista

Bug Depends on:

41924, 42260

Bug Blocks:

27968

Attachments:

Description	Flags
Patch	none
Patch 2	none

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.