Bug 41925 - [Chromium] Linux implementation of <input type=number> UI
Summary: [Chromium] Linux implementation of <input type=number> UI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 38568 41924
Blocks: 38570
  Show dependency treegraph
 
Reported: 2010-07-08 19:13 PDT by Kent Tamura
Modified: 2010-07-13 21:49 PDT (History)
3 users (show)

See Also:


Attachments
Patch (21.21 KB, patch)
2010-07-08 19:23 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Screenshot (21.30 KB, image/png)
2010-07-09 00:49 PDT, Kent Tamura
no flags Details
Patch 2 (21.26 KB, patch)
2010-07-12 03:32 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (31.46 KB, patch)
2010-07-13 04:06 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-08 19:13:44 PDT
[Chromium] Linux implementation of <input type=number> UI
Comment 1 Kent Tamura 2010-07-08 19:23:22 PDT
Created attachment 60997 [details]
Patch
Comment 2 Kent Tamura 2010-07-08 19:26:21 PDT
(In reply to comment #1)
> Created an attachment (id=60997) [details]
> Patch

This patch contains no tests because nothing is drawn without the patch of Bug#41924.
If the patch of Bug#41924 is committed before this, I'll add a test to this.  If this patch is committed before Bug#41924, I'll add a test separately.
Comment 3 Kent Tamura 2010-07-09 00:49:24 PDT
Created attachment 61008 [details]
Screenshot
Comment 4 Evan Stade 2010-07-09 12:06:20 PDT
the code looks fine to me (I am not a reviewer though).

The screenshots look a little funky imo---I think it's because the up/down arrows are not symmetric (the bottom being slightly larger, especially noticeable because the arrows are not the same distance from the middle line). I'm not sure how to fix that and it's certianly pretty nitpicky. Shrug.
Comment 5 Tony Chang 2010-07-09 12:21:34 PDT
Comment on attachment 60997 [details]
Patch

LGTM (also not a reviewer), just some nits.

WebCore/rendering/RenderThemeChromiumLinux.cpp:187
 +      SkScalar* hsv, SkScalar saturateAmount, SkScalar brightenAmount)
Nit: can we make |hsv| const?

WebCore/rendering/RenderThemeChromiumLinux.cpp:196
 +  SkColor RenderThemeChromiumLinux::outlineColor(SkScalar* hsv1, SkScalar* hsv2)
Nit: Can we make these const too?
Comment 6 Kent Tamura 2010-07-12 03:32:55 PDT
Created attachment 61204 [details]
Patch 2
Comment 7 Kent Tamura 2010-07-12 03:47:26 PDT
(In reply to comment #6)
> Created an attachment (id=61204) [details]
> Patch 2

Changed parameter type: SkScalar* -> const SkScalar[3]
Comment 8 Darin Fisher (:fishd, Google) 2010-07-12 22:43:22 PDT
Comment on attachment 61204 [details]
Patch 2

WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:90
 +      paint.setColor(RenderThemeChromiumLinux::saturateAndBrighten(trackHSV, 0, 0));
code in platform/ should not depend on code in rendering/.
that's a layering violation.  it seems like you should
refactor this so that there is another file in platform/chromium/
that defines the shared theme code.
Comment 9 Kent Tamura 2010-07-12 22:49:18 PDT
(In reply to comment #8)
> (From update of attachment 61204 [details])
> WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:90
>  +      paint.setColor(RenderThemeChromiumLinux::saturateAndBrighten(trackHSV, 0, 0));
> code in platform/ should not depend on code in rendering/.
> that's a layering violation.  it seems like you should
> refactor this so that there is another file in platform/chromium/
> that defines the shared theme code.

I see.
Do you think it's acceptable to remain these function in ScrollbarThemeChromiumLinux and call them from RenderThemeChroimiumLinux?
Comment 10 Darin Fisher (:fishd, Google) 2010-07-12 23:37:34 PDT
(In reply to comment #9)
> Do you think it's acceptable to remain these function in 
> ScrollbarThemeChromiumLinux and call them from RenderThemeChroimiumLinux?

That seems suboptimal since you plan to use them for things that aren't related to scrollbars, right?

How about a new file:  PlatformThemeChromium{Linux}?
Comment 11 Kent Tamura 2010-07-13 04:06:10 PDT
Created attachment 61352 [details]
Patch 3
Comment 12 Kent Tamura 2010-07-13 04:08:22 PDT
(In reply to comment #10)
> > Do you think it's acceptable to remain these function in 
> > ScrollbarThemeChromiumLinux and call them from RenderThemeChroimiumLinux?
> 
> That seems suboptimal since you plan to use them for things that aren't related to scrollbars, right?

Right :-)

> How about a new file:  PlatformThemeChromium{Linux}?

ok, the updated patch introduced PlatformThemeChromiumGtk, and fixes other layer violations.
Comment 13 Darin Fisher (:fishd, Google) 2010-07-13 10:01:06 PDT
Comment on attachment 61352 [details]
Patch 3

WebCore/platform/chromium/PlatformThemeChromiumGtk.h:52
 +      enum ArrowDirection {
nit: usually ideal style-wise to put enum declarations at the top of the
public section with a new line separating it from the methods.

WebCore/platform/chromium/PlatformThemeChromiumGtk.h:68
 +  }
} // namespace WebCore

WebCore/platform/chromium/PlatformThemeChromiumGtk.cpp:220
 +  }
} // namespace WebCore
Comment 14 Kent Tamura 2010-07-13 21:49:04 PDT
Thank you for reviewing.

(In reply to comment #13)
> (From update of attachment 61352 [details])
> WebCore/platform/chromium/PlatformThemeChromiumGtk.h:52
>  +      enum ArrowDirection {
> nit: usually ideal style-wise to put enum declarations at the top of the
> public section with a new line separating it from the methods.

Done.

> WebCore/platform/chromium/PlatformThemeChromiumGtk.h:68
>  +  }
> } // namespace WebCore

Done.

> WebCore/platform/chromium/PlatformThemeChromiumGtk.cpp:220
>  +  }
> } // namespace WebCore

Done.

Landed as r63280.