Bug 41925 – [Chromium] Linux implementation of <input type=number> UI

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

[Chromium] Linux implementation of <input type=number> UI

[Kent Tamura]

Created attachment 60997 [details]
Patch

[Kent Tamura]

(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.

[Kent Tamura]

Created attachment 61008 [details]
Screenshot

[Evan Stade]

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.

[Tony Chang]

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?

[Kent Tamura]

Created attachment 61204 [details]
Patch 2

[Kent Tamura]

(In reply to comment #6)
> Created an attachment (id=61204) [details]
> Patch 2

Changed parameter type: SkScalar* -> const SkScalar[3]

[Darin Fisher (:fishd, Google)]

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.

[Kent Tamura]

(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?

[Darin Fisher (:fishd, Google)]

(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}?

[Kent Tamura]

Created attachment 61352 [details]
Patch 3

[Kent Tamura]

(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.

[Darin Fisher (:fishd, Google)]

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

[Kent Tamura]

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.