SkRect to gfx::Rect type conversions.

SkRect to gfx::Rect type conversions.

This adds more Skia rect conversions, and removes the lossy SkRectToRect
conversion (making all its users invoke ToFlooredRectDeprecated explicitly).

BUG=147395

[aelias_OOO_until_Jul13, 2012-11-01 00:28:39]

Hi sky@, this is another small gfx type conversion patch for CC.

[danakj, 2012-11-01 00:36:59]

https://codereview.chromium.org/11275089/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/11275089/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:584: clip_rect =
gfx::ToEnclosingRect(gfx::SkRectToRectF(sk_clip_rect));
Maybe you want to use gfx::ToFlooredRectDeprecated() throughout to preserve the
current behaviour, and add these cases to
http://code.google.com/p/chromium/issues/detail?id=158106

https://codereview.chromium.org/11275089/diff/1/ui/gfx/transform.h
File ui/gfx/transform.h (right):

https://codereview.chromium.org/11275089/diff/1/ui/gfx/transform.h#newcode106
ui/gfx/transform.h:106: void TransformRect(RectF* rect) const;
Why the transform changes in this CL? I think they should be separate. As we're
not using ui::transform in cc/ at all right now, it seems like a separate
concern?

https://codereview.chromium.org/11275089/diff/1/ui/gfx/transform.h#newcode108
ui/gfx/transform.h:108: // Same as above, but expands to enclosing int rect.
Not sure if implicit enclosing is great either. I like the forced-to-be-explicit
nature of the other geometry things when going from float to int.

[aelias_OOO_until_Jul13, 2012-11-01 00:49:30]

OK, switched to ToFlooredRectDeprecated so that this patch is a no-op and
removed the extra Transform changes.

[danakj, 2012-11-01 00:59:23]

Thanks, I like this better as a single change.

https://codereview.chromium.org/11275089/diff/3002/ash/wm/image_grid.cc
File ash/wm/image_grid.cc (right):

https://codereview.chromium.org/11275089/diff/3002/ash/wm/image_grid.cc#newco...
ash/wm/image_grid.cc:24: gfx::RectF bounds = layer.bounds();
I think this change isn't needed anymore?

https://codereview.chromium.org/11275089/diff/3002/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

https://codereview.chromium.org/11275089/diff/3002/ui/aura/root_window.cc#new...
ui/aura/root_window.cc:42: #include "ui/gfx/rect_f.h"
These aren't needed now?

[jamesr, 2012-11-01 01:15:46]

lgtm for cc/ modulo one thing

https://codereview.chromium.org/11275089/diff/3002/cc/software_renderer.cc
File cc/software_renderer.cc (right):

https://codereview.chromium.org/11275089/diff/3002/cc/software_renderer.cc#ne...
cc/software_renderer.cc:221:
gfx::RectFToSkRect(quadVertexRect()).toQuad(vertices);
this line doesn't appear to be doing anything useful with or without this patch

[aelias_OOO_until_Jul13, 2012-11-01 05:00:18]

OK, I removed those extra lines.

https://codereview.chromium.org/11275089/diff/3002/cc/software_renderer.cc
File cc/software_renderer.cc (right):

https://codereview.chromium.org/11275089/diff/3002/cc/software_renderer.cc#ne...
cc/software_renderer.cc:221:
gfx::RectFToSkRect(quadVertexRect()).toQuad(vertices);
On 2012/11/01 01:15:46, jamesr wrote:
> this line doesn't appear to be doing anything useful with or without this
patch

This line populates "vertices" with the four corners of quadVertexRect.

[danakj, 2012-11-01 05:12:27]

On 2012/11/01 05:00:18, aelias wrote:
> OK, I removed those extra lines.

Cool lgtm!

> https://codereview.chromium.org/11275089/diff/3002/cc/software_renderer.cc
> File cc/software_renderer.cc (right):
> 
>
https://codereview.chromium.org/11275089/diff/3002/cc/software_renderer.cc#ne...
> cc/software_renderer.cc:221:
> gfx::RectFToSkRect(quadVertexRect()).toQuad(vertices);
> On 2012/11/01 01:15:46, jamesr wrote:
> > this line doesn't appear to be doing anything useful with or without this
> patch
> 
> This line populates "vertices" with the four corners of quadVertexRect.

[sky, 2012-11-01 16:10:24]

https://codereview.chromium.org/11275089/diff/10001/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

https://codereview.chromium.org/11275089/diff/10001/ui/gfx/transform.cc#newco...
ui/gfx/transform.cc:159: *rect =
gfx::ToFlooredRectDeprecated(SkRectToRectF(src));
The plan was to remove ToFlooredRectDeprecated. If we really need the function
it should not be deprecated.

https://codereview.chromium.org/11275089/diff/10001/ui/views/controls/tree/tr...
File ui/views/controls/tree/tree_view_views.cc (right):

https://codereview.chromium.org/11275089/diff/10001/ui/views/controls/tree/tr...
ui/views/controls/tree/tree_view_views.cc:522:
gfx::SkRectToRectF(sk_clip_rect));
Why does this need to use RectF and then floor it? Same comment for
table_view_views.

[danakj, 2012-11-01 16:38:21]

https://codereview.chromium.org/11275089/diff/10001/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

https://codereview.chromium.org/11275089/diff/10001/ui/gfx/transform.cc#newco...
ui/gfx/transform.cc:159: *rect =
gfx::ToFlooredRectDeprecated(SkRectToRectF(src));
On 2012/11/01 16:10:24, sky wrote:
> The plan was to remove ToFlooredRectDeprecated. If we really need the function
> it should not be deprecated.

IMO it's more a matter of making existing code that does this operation
implicitly into explicit calls to this function. Then we can see where we need
to evaluate and probably fix with a call to enclosed/enclosing, or whatnot.

[sky, 2012-11-01 17:28:46]

Shouldn't this call one of enclosed/enclosing rather than the deprecated method?

[danakj, 2012-11-01 17:38:27]

I just prefer CLs to not change behaviour when they are making API changes, and
do behaviour changes separately. I took a look at each call site in the latest
CL and gave my thought on what it should do instead of flooring.

https://codereview.chromium.org/11275089/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/11275089/diff/10001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:584: clip_rect =
gfx::ToFlooredRectDeprecated(
It seems that the integer rect is being used to schedule a paint, so it should
include any pixels partly-included in the float rect. So I'd think that
ToEnclosingRect is the right one here.

https://codereview.chromium.org/11275089/diff/10001/ui/compositor/layer_unitt...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/11275089/diff/10001/ui/compositor/layer_unitt...
ui/compositor/layer_unittest.cc:1053: last_clip_rect_ =
gfx::ToFlooredRectDeprecated(
Tests check that the integer rect contains things, so rounding out might be
kinda lossy. Can we make last_clip_rect_ a RectF here?

https://codereview.chromium.org/11275089/diff/10001/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

https://codereview.chromium.org/11275089/diff/10001/ui/gfx/transform.cc#newco...
ui/gfx/transform.cc:159: *rect =
gfx::ToFlooredRectDeprecated(SkRectToRectF(src));
I guess these methods need to take a RectF then, and each transform callsite
needs to decide enclosed/enclosing/use float. Sorry for making you undo that
first.

https://codereview.chromium.org/11275089/diff/10001/ui/views/controls/table/t...
File ui/views/controls/table/table_view_views.cc (right):

https://codereview.chromium.org/11275089/diff/10001/ui/views/controls/table/t...
ui/views/controls/table/table_view_views.cc:230: gfx::Rect clip_rect =
gfx::ToFlooredRectDeprecated(
This is iterating over rows that are contained in the clip rect, so
ToEnclosingRect sounds right here to me, to avoid skipping rows partly-included
in the clip.

https://codereview.chromium.org/11275089/diff/10001/ui/views/controls/tree/tr...
File ui/views/controls/tree/tree_view_views.cc (right):

https://codereview.chromium.org/11275089/diff/10001/ui/views/controls/tree/tr...
ui/views/controls/tree/tree_view_views.cc:522:
gfx::SkRectToRectF(sk_clip_rect));
On 2012/11/01 16:10:24, sky wrote:
> Why does this need to use RectF and then floor it? Same comment for
> table_view_views.

This is trying to paint everything touched by the clip, so ToEnclosingRect
sounds right, to paint pixels that are party inside the clip.

[danakj, 2012-11-01 17:48:28]

https://codereview.chromium.org/11275089/diff/10001/ui/views/controls/tree/tr...
File ui/views/controls/tree/tree_view_views.cc (right):

https://codereview.chromium.org/11275089/diff/10001/ui/views/controls/tree/tr...
ui/views/controls/tree/tree_view_views.cc:522:
gfx::SkRectToRectF(sk_clip_rect));
On 2012/11/01 16:10:24, sky wrote:
> Why does this need to use RectF and then floor it? Same comment for
> table_view_views.

The clip rect given by skia is a float rect, but we're using integer rects in
ui/ which is why there is conversion happening here. It used to be implicit, so
easily forgotten. SkRect is float, SkIRect is integers.

[danakj, 2012-11-01 17:50:18]

https://codereview.chromium.org/11275089/diff/10001/ui/views/controls/tree/tr...
File ui/views/controls/tree/tree_view_views.cc (right):

https://codereview.chromium.org/11275089/diff/10001/ui/views/controls/tree/tr...
ui/views/controls/tree/tree_view_views.cc:522:
gfx::SkRectToRectF(sk_clip_rect));
On 2012/11/01 17:48:28, danakj wrote:
> On 2012/11/01 16:10:24, sky wrote:
> > Why does this need to use RectF and then floor it? Same comment for
> > table_view_views.
> 
> The clip rect given by skia is a float rect, but we're using integer rects in
> ui/ which is why there is conversion happening here. It used to be implicit,
so
> easily forgotten. SkRect is float, SkIRect is integers.

I guess my question is why we use getClipBounds() instead of
getClipDeviceBounds(), which would then give us pixel (integer) results. I am
curious if that was intentional.

[tfarina, 2012-11-10 20:23:07]

https://codereview.chromium.org/11275089/diff/10001/ui/gfx/skia_util.cc
File ui/gfx/skia_util.cc (right):

https://codereview.chromium.org/11275089/diff/10001/ui/gfx/skia_util.cc#newco...
ui/gfx/skia_util.cc:23: return SkRect::MakeXYWH(rect.x(), rect.y(),
rect.width(), rect.height());
I think you can't guarantee that SkScalar is int?

The type of SkRect::MakeXYWH parameters is SkScalar not int.

Hence I have used iset() here, which precisely takes integers in its parameters.

[aelias_OOO_until_Jul13, 2012-11-12 19:50:39]

Ping; sky@, could you clarify what your primary concern is with this patch
preventing it from landing?  It's a net improvement to remove the lossy
conversion function and do the equivalent at the callsites.

[danakj, 2012-11-12 20:01:53]

https://codereview.chromium.org/11275089/diff/10001/ui/gfx/skia_util.cc
File ui/gfx/skia_util.cc (right):

https://codereview.chromium.org/11275089/diff/10001/ui/gfx/skia_util.cc#newco...
ui/gfx/skia_util.cc:23: return SkRect::MakeXYWH(rect.x(), rect.y(),
rect.width(), rect.height());
On 2012/11/10 20:23:08, tfarina wrote:
> I think you can't guarantee that SkScalar is int?
> 
> The type of SkRect::MakeXYWH parameters is SkScalar not int.
> 
> Hence I have used iset() here, which precisely takes integers in its
parameters.

Yeh, these parameters need SkIntToScalar()

https://codereview.chromium.org/11275089/diff/10001/ui/gfx/skia_util.cc#newco...
ui/gfx/skia_util.cc:27: return SkRect::MakeXYWH(rect.x(), rect.y(),
rect.width(), rect.height());
SkIntToScalar() here also.

https://codereview.chromium.org/11275089/diff/10001/ui/gfx/skia_util.cc#newco...
ui/gfx/skia_util.cc:43: return gfx::RectF(rect.x(), rect.y(), rect.width(),
rect.height());
SkScalarToFloat for these parameters.

[tfarina, 2012-11-12 21:36:14]

On Mon, Nov 12, 2012 at 6:01 PM,  <danakj@chromium.org> wrote:
>
https://codereview.chromium.org/11275089/diff/10001/ui/gfx/skia_util.cc#newco...
> ui/gfx/skia_util.cc:23: return SkRect::MakeXYWH(rect.x(), rect.y(),
> rect.width(), rect.height());
> On 2012/11/10 20:23:08, tfarina wrote:
>>
>> I think you can't guarantee that SkScalar is int?
>
>
>> The type of SkRect::MakeXYWH parameters is SkScalar not int.
>
>
>> Hence I have used iset() here, which precisely takes integers in its
>
> parameters.
>
> Yeh, these parameters need SkIntToScalar()
>
So he doesn't have to touch this function, just leave it as is (i.e,
using iset).

Thanks.

-- 
Thiago

[sky, 2012-11-12 22:50:10]

The point of deprecating a method is that it shouldn't be used and
we're in the process of migrating code. You shouldn't add new
dependencies to a deprecated method.

  -Scott

On Mon, Nov 12, 2012 at 11:50 AM,  <aelias@chromium.org> wrote:
> Ping; sky@, could you clarify what your primary concern is with this patch
> preventing it from landing?  It's a net improvement to remove the lossy
> conversion function and do the equivalent at the callsites.
>
> https://codereview.chromium.org/11275089/

[danakj, 2012-11-13 00:20:10]

Moving to https://codereview.chromium.org/11359172