Implement the Web Theme Engine for Android

Implement the Web Theme Engine for Android

BUG=
TEST=


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

[Peter Beverloo, 2011-12-08 14:59:00]

Hi guys, could you have a look please?

sky: minor typo fix (SLIDER_TNUMB -> SLIDER_THUMB) in ui/gfx/, introduced by cl
8497054.

darin: the addition of webthemeengine_impl_android.{h,cc}.

[Peter Beverloo, 2011-12-08 15:00:53]

As a clarification: this code will be exercised by WebKit layout tests once
we're able to run these.

[sky, 2011-12-08 17:33:54]

ui/gfx LGTM

[Peter Beverloo, 2011-12-14 14:08:44]

On 2011/12/08 17:33:54, sky wrote:
> ui/gfx LGTM

Thanks! Darin, could you take a look at the WebThemeEngine file please?

[darin (slow to review), 2011-12-14 17:56:36]

sorry, i thought i sent these comments already.  wrote them a while back :-(

http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl...
File webkit/glue/webthemeengine_impl_android.cc (right):

http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl...
webkit/glue/webthemeengine_impl_android.cc:15: using WebKit::WebRect;
it seems like adding WebThemeEngine here would be nice :)

http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl...
webkit/glue/webthemeengine_impl_android.cc:19: static gfx::Rect
WebRectToRect(const WebRect& rect) {
Hmm, webthemeengine_impl_linux.cc defines this too.

http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl...
webkit/glue/webthemeengine_impl_android.cc:167: NativeThemePart(part));
nit: indentation

http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl...
File webkit/glue/webthemeengine_impl_android.h (right):

http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl...
webkit/glue/webthemeengine_impl_android.h:5: #ifndef
WEBTHEMEENGINE_IMPL_ANDROID_H_
nit: please fix your include guards.

http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl...
webkit/glue/webthemeengine_impl_android.h:16: virtual void paint(
nit: in Google C++ style, all parameters get named.  painful if you are used to
WebKit coding, but oh well.

[Peter Beverloo, 2011-12-14 18:58:41]

Thank you for the review!

http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl...
File webkit/glue/webthemeengine_impl_android.cc (right):

http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl...
webkit/glue/webthemeengine_impl_android.cc:15: using WebKit::WebRect;
On 2011/12/14 17:56:36, darin wrote:
> it seems like adding WebThemeEngine here would be nice :)

Good catch, fixed! The file was based on the webthemeengine_impl_linux.cc file
which also doesn't do this. Shall I address that in another CL?

http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl...
webkit/glue/webthemeengine_impl_android.cc:19: static gfx::Rect
WebRectToRect(const WebRect& rect) {
On 2011/12/14 17:56:36, darin wrote:
> Hmm, webthemeengine_impl_linux.cc defines this too.

Avoiding the duplicate function would be ideal, of course, but what file would
be a good location for it to be defined in?

http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl...
webkit/glue/webthemeengine_impl_android.cc:167: NativeThemePart(part));
On 2011/12/14 17:56:36, darin wrote:
> nit: indentation

Done.

http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl...
File webkit/glue/webthemeengine_impl_android.h (right):

http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl...
webkit/glue/webthemeengine_impl_android.h:5: #ifndef
WEBTHEMEENGINE_IMPL_ANDROID_H_
On 2011/12/14 17:56:36, darin wrote:
> nit: please fix your include guards.

Done.

http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl...
webkit/glue/webthemeengine_impl_android.h:16: virtual void paint(
On 2011/12/14 17:56:36, darin wrote:
> nit: in Google C++ style, all parameters get named.  painful if you are used
to
> WebKit coding, but oh well.

Done.

[darin (slow to review), 2011-12-15 23:17:16]

On Wed, Dec 14, 2011 at 10:58 AM, <peter@chromium.org> wrote:

> Thank you for the review!
>
>
>
> http://codereview.chromium.**org/8889013/diff/1/webkit/**
>
glue/webthemeengine_impl_**android.cc<http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl_android.cc>
> File webkit/glue/webthemeengine_**impl_android.cc (right):
>
> http://codereview.chromium.**org/8889013/diff/1/webkit/**
>
glue/webthemeengine_impl_**android.cc#newcode15<http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl_android.cc#newcode15>
> webkit/glue/webthemeengine_**impl_android.cc:15: using WebKit::WebRect;
> On 2011/12/14 17:56:36, darin wrote:
>
>> it seems like adding WebThemeEngine here would be nice :)
>>
>
> Good catch, fixed! The file was based on the
> webthemeengine_impl_linux.cc file which also doesn't do this. Shall I
> address that in another CL?
>
>
+1 for multiple CLs.  I would recommend making this cleanup for the new
file in this CL though.


>
> http://codereview.chromium.**org/8889013/diff/1/webkit/**
>
glue/webthemeengine_impl_**android.cc#newcode19<http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl_android.cc#newcode19>
> webkit/glue/webthemeengine_**impl_android.cc:19: static gfx::Rect
> WebRectToRect(const WebRect& rect) {
> On 2011/12/14 17:56:36, darin wrote:
>
>> Hmm, webthemeengine_impl_linux.cc defines this too.
>>
>
> Avoiding the duplicate function would be ideal, of course, but what file
> would be a good location for it to be defined in?


Good question.  However, this method is not needed!  See WebRect.h :-)
WebRect defines "operator gfx::Rect()", so you can just use that.


http://codereview.chromium.**org/8889013/<http://codereview.chromium.org/8889...
>

[Peter Beverloo, 2011-12-16 14:31:43]

On 2011/12/15 23:17:16, darin wrote:
> On Wed, Dec 14, 2011 at 10:58 AM, <mailto:peter@chromium.org> wrote:
> 
> > Thank you for the review!
> >
> >
> >
> > http://codereview.chromium.**org/8889013/diff/1/webkit/**
> >
>
glue/webthemeengine_impl_**android.cc<http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl_android.cc>
> > File webkit/glue/webthemeengine_**impl_android.cc (right):
> >
> > http://codereview.chromium.**org/8889013/diff/1/webkit/**
> >
>
glue/webthemeengine_impl_**android.cc#newcode15<http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl_android.cc#newcode15>
> > webkit/glue/webthemeengine_**impl_android.cc:15: using WebKit::WebRect;
> > On 2011/12/14 17:56:36, darin wrote:
> >
> >> it seems like adding WebThemeEngine here would be nice :)
> >>
> >
> > Good catch, fixed! The file was based on the
> > webthemeengine_impl_linux.cc file which also doesn't do this. Shall I
> > address that in another CL?
> >
> >
> +1 for multiple CLs.  I would recommend making this cleanup for the new
> file in this CL though.

Done. Only changes are visible here:
http://codereview.chromium.org/8889013/diff2/12001:14001/webkit/glue/webtheme...

I'll upload a CL cleaning up webthemeengine_impl_linux.cc shortly.
 
> >
> > http://codereview.chromium.**org/8889013/diff/1/webkit/**
> >
>
glue/webthemeengine_impl_**android.cc#newcode19<http://codereview.chromium.org/8889013/diff/1/webkit/glue/webthemeengine_impl_android.cc#newcode19>
> > webkit/glue/webthemeengine_**impl_android.cc:19: static gfx::Rect
> > WebRectToRect(const WebRect& rect) {
> > On 2011/12/14 17:56:36, darin wrote:
> >
> >> Hmm, webthemeengine_impl_linux.cc defines this too.
> >>
> >
> > Avoiding the duplicate function would be ideal, of course, but what file
> > would be a good location for it to be defined in?
> 
> 
> Good question.  However, this method is not needed!  See WebRect.h :-)
> WebRect defines "operator gfx::Rect()", so you can just use that.

Yes, that's perfect. Thank you!

> 
>
http://codereview.chromium.**org/8889013/%3Chttp://codereview.chromium.org/88...>
> >

[darin (slow to review), 2011-12-16 18:02:10]

LGTM

http://codereview.chromium.org/8889013/diff/14001/webkit/glue/webthemeengine_...
File webkit/glue/webthemeengine_impl_android.cc (right):

http://codereview.chromium.org/8889013/diff/14001/webkit/glue/webthemeengine_...
webkit/glue/webthemeengine_impl_android.cc:180: static_cast<gfx::Rect>(rect),
nit: gfx::Rect(rect) <- invoke the copy constructor (and the implicit casting
operator), which all gets optimized down to just one copy.

[Peter Beverloo, 2011-12-19 15:38:51]

On 2011/12/16 18:02:10, darin wrote:
> LGTM
> 
>
http://codereview.chromium.org/8889013/diff/14001/webkit/glue/webthemeengine_...
> File webkit/glue/webthemeengine_impl_android.cc (right):
> 
>
http://codereview.chromium.org/8889013/diff/14001/webkit/glue/webthemeengine_...
> webkit/glue/webthemeengine_impl_android.cc:180: static_cast<gfx::Rect>(rect),
> nit: gfx::Rect(rect) <- invoke the copy constructor (and the implicit casting
> operator), which all gets optimized down to just one copy.

Good to know, thank you! For the review as well. Awaiting tryjobs and will land
shortly.

[commit-bot: I haz the power, 2011-12-20 11:16:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@chromium.org/8889013/19001

[commit-bot: I haz the power, 2011-12-20 12:29:13]

Change committed as 115117