Upstream: ui implementation in Android

Upstream: ui implementation in Android


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109681

[michaelbai, 2011-11-09 23:35:03]

please help to review this

Thanks

[sky, 2011-11-10 00:41:01]

Mostly just style stuff.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/canvas_skia_android.cc
File ui/gfx/canvas_skia_android.cc (right):

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/canvas_skia_android.c...
ui/gfx/canvas_skia_android.cc:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
2011

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android.cc
File ui/gfx/native_theme_android.cc (right):

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:19: unsigned int
NativeThemeAndroid::button_length_ = 14;
prefix all of these with static.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:41: static SkColor BrightenColor(const
color_utils::HSL& hsl, SkAlpha alpha,
each param on its own line.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:60: NativeThemeAndroid::NativeThemeAndroid() {
Remember to make sure position in .cc matches position in .h.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:94: const gfx::Rect& rect, Part direction, State
state) {
each param on its own line.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:257: }
default: NOTREACHED?

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:312: const int kRight = rect.right();
don't use constant naming style for this. We generally don't use const like this
in methods too.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:717: const SkBitmap& bitmap,
spacing if off.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:762: // GTK Theme engines have way too much
control over the layout of
Is this comment really applicable here?

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android.h
File ui/gfx/native_theme_android.h (right):

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:20: kScrollbarDownArrow,
Use CAP_STYLE. The chromium style guide calls this out specifically.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:96: virtual gfx::Size GetPartSize(Part part)
const;
Are there subclasses that warrant this being virtual?

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:97: // Paint the part to the canvas.
newline between 96 and 97.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:114: // Draw the checkbox.
newline between methods.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:115: virtual void PaintCheckbox(SkCanvas* canvas,
If you can't nicely align all params with the '(', then wrap them all (including
the first).

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:160: protected:
remove this since you're already in the protected section.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:161: bool IntersectsClipRectInt(SkCanvas* canvas,
Add descriptions of all your methods.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:164: void DrawBitmapInt(SkCanvas* canvas, const
SkBitmap& bitmap,
wrap bitmap to the next line.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/platform_font_android.cc
File ui/gfx/platform_font_android.cc (right):

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/platform_font_android...
ui/gfx/platform_font_android.cc:6: #include "ui/gfx/platform_font.h"
platform_font should be the first include.

[michaelbai, 2011-11-10 22:51:36]

Addressed almost all comments, only one need answered by original author.

PTAL

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/canvas_skia_android.cc
File ui/gfx/canvas_skia_android.cc (right):

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/canvas_skia_android.c...
ui/gfx/canvas_skia_android.cc:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
On 2011/11/10 00:41:01, sky wrote:
> 2011

Done.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android.cc
File ui/gfx/native_theme_android.cc (right):

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:19: unsigned int
NativeThemeAndroid::button_length_ = 14;
On 2011/11/10 00:41:01, sky wrote:
> prefix all of these with static.

These are class static members, 'static' can't be added here.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:41: static SkColor BrightenColor(const
color_utils::HSL& hsl, SkAlpha alpha,
On 2011/11/10 00:41:01, sky wrote:
> each param on its own line.

Done.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:60: NativeThemeAndroid::NativeThemeAndroid() {
On 2011/11/10 00:41:01, sky wrote:
> Remember to make sure position in .cc matches position in .h.

Did you mean the this was in wrong place? I think its location is correct. Would
the order be

static
constructor/deconstructor
rest of methods whose position matches the position in .h.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:94: const gfx::Rect& rect, Part direction, State
state) {
On 2011/11/10 00:41:01, sky wrote:
> each param on its own line.

Done.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:257: }
On 2011/11/10 00:41:01, sky wrote:
> default: NOTREACHED?

Done.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:312: const int kRight = rect.right();
On 2011/11/10 00:41:01, sky wrote:
> don't use constant naming style for this. We generally don't use const like
this
> in methods too.

Done.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:717: const SkBitmap& bitmap,
On 2011/11/10 00:41:01, sky wrote:
> spacing if off.

Done.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:762: // GTK Theme engines have way too much
control over the layout of
On 2011/11/10 00:41:01, sky wrote:
> Is this comment really applicable here?

I have ping the author to answer this.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android.h
File ui/gfx/native_theme_android.h (right):

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:20: kScrollbarDownArrow,
On 2011/11/10 00:41:01, sky wrote:
> Use CAP_STYLE. The chromium style guide calls this out specifically.

These styles were used in all native_theme_{platform}, I'd keep it as it and
make them consistent

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:96: virtual gfx::Size GetPartSize(Part part)
const;
On 2011/11/10 00:41:01, sky wrote:
> Are there subclasses that warrant this being virtual?

Removed 'virtual', thanks

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:97: // Paint the part to the canvas.
On 2011/11/10 00:41:01, sky wrote:
> newline between 96 and 97.

Done.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:114: // Draw the checkbox.
On 2011/11/10 00:41:01, sky wrote:
> newline between methods.

Done.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:115: virtual void PaintCheckbox(SkCanvas* canvas,
On 2011/11/10 00:41:01, sky wrote:
> If you can't nicely align all params with the '(', then wrap them all
(including
> the first).

Done.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:160: protected:
On 2011/11/10 00:41:01, sky wrote:
> remove this since you're already in the protected section.

Done.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:161: bool IntersectsClipRectInt(SkCanvas* canvas,
On 2011/11/10 00:41:01, sky wrote:
> Add descriptions of all your methods.

Done.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:164: void DrawBitmapInt(SkCanvas* canvas, const
SkBitmap& bitmap,
On 2011/11/10 00:41:01, sky wrote:
> wrap bitmap to the next line.

Done.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/platform_font_android.cc
File ui/gfx/platform_font_android.cc (right):

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/platform_font_android...
ui/gfx/platform_font_android.cc:6: #include "ui/gfx/platform_font.h"
On 2011/11/10 00:41:01, sky wrote:
> platform_font should be the first include.

Done.

[sky, 2011-11-10 23:21:28]

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android.cc
File ui/gfx/native_theme_android.cc (right):

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:60: NativeThemeAndroid::NativeThemeAndroid() {
On 2011/11/10 22:51:36, michaelbai wrote:
> On 2011/11/10 00:41:01, sky wrote:
> > Remember to make sure position in .cc matches position in .h.
> 
> Did you mean the this was in wrong place?

Yes.
> I think its location is correct. Would
> the order be
> 
> static
> constructor/deconstructor
> rest of methods whose position matches the position in .h.

Here's what the style guide says:
"Method definitions in the corresponding .cc file should be the same as the
declaration order, as much as possible."
So, this isn't the right place.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android.h
File ui/gfx/native_theme_android.h (right):

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:20: kScrollbarDownArrow,
On 2011/11/10 22:51:36, michaelbai wrote:
> On 2011/11/10 00:41:01, sky wrote:
> > Use CAP_STYLE. The chromium style guide calls this out specifically.
> 
> These styles were used in all native_theme_{platform}, I'd keep it as it and
> make them consistent

See native_theme_win.h

[michaelbai, 2011-11-11 00:11:47]

PTAL

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android.cc
File ui/gfx/native_theme_android.cc (right):

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:60: NativeThemeAndroid::NativeThemeAndroid() {
On 2011/11/10 23:21:28, sky wrote:
> On 2011/11/10 22:51:36, michaelbai wrote:
> > On 2011/11/10 00:41:01, sky wrote:
> > > Remember to make sure position in .cc matches position in .h.
> > 
> > Did you mean the this was in wrong place?
> 
> Yes.
> > I think its location is correct. Would
> > the order be
> > 
> > static
> > constructor/deconstructor
> > rest of methods whose position matches the position in .h.
> 
> Here's what the style guide says:
> "Method definitions in the corresponding .cc file should be the same as the
> declaration order, as much as possible."
> So, this isn't the right place.

Done.

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android.h
File ui/gfx/native_theme_android.h (right):

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.h:20: kScrollbarDownArrow,
On 2011/11/10 23:21:28, sky wrote:
> On 2011/11/10 22:51:36, michaelbai wrote:
> > On 2011/11/10 00:41:01, sky wrote:
> > > Use CAP_STYLE. The chromium style guide calls this out specifically.
> > 
> > These styles were used in all native_theme_{platform}, I'd keep it as it and
> > make them consistent
> 
> See native_theme_win.h

Done.

[sky, 2011-11-11 00:14:15]

http://codereview.chromium.org/8497054/diff/10001/ui/gfx/native_theme_android.h
File ui/gfx/native_theme_android.h (right):

http://codereview.chromium.org/8497054/diff/10001/ui/gfx/native_theme_android...
ui/gfx/native_theme_android.h:37: kDisabled,
This should be ALL_CAPS too.

http://codereview.chromium.org/8497054/diff/10001/ui/gfx/native_theme_android...
ui/gfx/native_theme_android.h:110: virtual void PaintArrowButton(SkCanvas* gc,
Same comment on virtual as earlier. If it's not needed remove it.

http://codereview.chromium.org/8497054/diff/10001/ui/gfx/native_theme_android...
ui/gfx/native_theme_android.h:236: static unsigned int scrollbar_width_;
Is there a reason these aren't const too? It doesn't look like you change any of
them.

[michaelbai, 2011-11-11 00:33:28]

http://codereview.chromium.org/8497054/diff/10001/ui/gfx/native_theme_android.h
File ui/gfx/native_theme_android.h (right):

http://codereview.chromium.org/8497054/diff/10001/ui/gfx/native_theme_android...
ui/gfx/native_theme_android.h:37: kDisabled,
On 2011/11/11 00:14:16, sky wrote:
> This should be ALL_CAPS too.

Done.

http://codereview.chromium.org/8497054/diff/10001/ui/gfx/native_theme_android...
ui/gfx/native_theme_android.h:110: virtual void PaintArrowButton(SkCanvas* gc,
On 2011/11/11 00:14:16, sky wrote:
> Same comment on virtual as earlier. If it's not needed remove it.

Done.

http://codereview.chromium.org/8497054/diff/10001/ui/gfx/native_theme_android...
ui/gfx/native_theme_android.h:236: static unsigned int scrollbar_width_;
On 2011/11/11 00:14:16, sky wrote:
> Is there a reason these aren't const too? It doesn't look like you change any
of
> them.

Done.

[michaelbai, 2011-11-11 00:33:41]

PTAL

[sky, 2011-11-11 02:41:39]

LGTM with the following change

http://codereview.chromium.org/8497054/diff/10003/ui/gfx/native_theme_android.cc
File ui/gfx/native_theme_android.cc (right):

http://codereview.chromium.org/8497054/diff/10003/ui/gfx/native_theme_android...
ui/gfx/native_theme_android.cc:19: static const unsigned int button_length_ =
14;
These should now be kConstant style.

[bulach, 2011-11-11 11:49:24]

replying to one question below:

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android.cc
File ui/gfx/native_theme_android.cc (right):

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:762: // GTK Theme engines have way too much
control over the layout of
On 2011/11/10 22:51:36, michaelbai wrote:
> On 2011/11/10 00:41:01, sky wrote:
> > Is this comment really applicable here?
> 
> I have ping the author to answer this. 

that was originally intended to minimize our diffs to the original file :) we
can remove this comment now, it makes no sense here..

[michaelbai, 2011-11-11 18:16:26]

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android.cc
File ui/gfx/native_theme_android.cc (right):

http://codereview.chromium.org/8497054/diff/1008/ui/gfx/native_theme_android....
ui/gfx/native_theme_android.cc:762: // GTK Theme engines have way too much
control over the layout of
On 2011/11/11 11:49:25, bulach wrote:
> On 2011/11/10 22:51:36, michaelbai wrote:
> > On 2011/11/10 00:41:01, sky wrote:
> > > Is this comment really applicable here?
> > 
> > I have ping the author to answer this. 
> 
> that was originally intended to minimize our diffs to the original file :) we
> can remove this comment now, it makes no sense here..

Done.

http://codereview.chromium.org/8497054/diff/10003/ui/gfx/native_theme_android.cc
File ui/gfx/native_theme_android.cc (right):

http://codereview.chromium.org/8497054/diff/10003/ui/gfx/native_theme_android...
ui/gfx/native_theme_android.cc:19: static const unsigned int button_length_ =
14;
On 2011/11/11 02:41:39, sky wrote:
> These should now be kConstant style.

Done.

[commit-bot: I haz the power, 2011-11-11 18:16:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/8497054/15001

[commit-bot: I haz the power, 2011-11-11 20:38:39]

Change committed as 109681