Do not convert from RectF to Rect by flooring.

Fixes cases where we incorrectly convert from RectF to Rect by flooring. In all cases we should be taking the enclosing or enclosed int rect as appropriate.

This mainly affects bits of code using the old Rect Rect::Scale(float) function. There are, thankfully, not too many. I've replaced this legacy function with Rect Rect::ScaleUnsafe(float) and when this lands, I will open a bug for switching from ScaleUnsafe to a Scale followed by a ToEnclosedRect or ToEnclosingRect.

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

[sky, 2012-09-27 17:48:30]

https://codereview.chromium.org/10996037/diff/8001/ui/gfx/rect.h
File ui/gfx/rect.h (right):

https://codereview.chromium.org/10996037/diff/8001/ui/gfx/rect.h#newcode78
ui/gfx/rect.h:78: Rect ScaleUnsafe(float scale) const WARN_UNUSED_RESULT;
Why do we need these? Can't we pick one and make Scale use it? This is
especially confusing when you've updated BackingStoreAura to use the Unsafe one.

https://codereview.chromium.org/10996037/diff/8001/ui/gfx/size_base.h
File ui/gfx/size_base.h (right):

https://codereview.chromium.org/10996037/diff/8001/ui/gfx/size_base.h#newcode74
ui/gfx/size_base.h:74: bool allow_negative_size_;
Having special cases where Size is some times negative and some times not is
error prone. It doesn't seem like you're even using this. Remove it.

[Ian Vollick, 2012-09-27 18:22:53]

On 2012/09/27 17:48:30, sky wrote:
> https://codereview.chromium.org/10996037/diff/8001/ui/gfx/rect.h
> File ui/gfx/rect.h (right):
> 
> https://codereview.chromium.org/10996037/diff/8001/ui/gfx/rect.h#newcode78
> ui/gfx/rect.h:78: Rect ScaleUnsafe(float scale) const WARN_UNUSED_RESULT;
> Why do we need these? Can't we pick one and make Scale use it? This is
> especially confusing when you've updated BackingStoreAura to use the Unsafe
one.
Done. When converting the resulting RectF's to Rects, I've used ToEnclosingRect.

> 
> https://codereview.chromium.org/10996037/diff/8001/ui/gfx/size_base.h
> File ui/gfx/size_base.h (right):
> 
>
https://codereview.chromium.org/10996037/diff/8001/ui/gfx/size_base.h#newcode74
> ui/gfx/size_base.h:74: bool allow_negative_size_;
> Having special cases where Size is some times negative and some times not is
> error prone. It doesn't seem like you're even using this. Remove it.
Done.

I have added a ClampToNonNegative function for use in places where we may
encounter a negative Size (e.g., Scale). Since these checks were almost entirely
the point of scale_base_impl.h, it is no longer necessary and I've removed it.

[sky, 2012-09-27 20:33:55]

webkit/plugins/ppapi/ppb_graphics_2d_impl could likely use ths new code too.

https://codereview.chromium.org/10996037/diff/11001/ui/gfx/rect.cc
File ui/gfx/rect.cc (right):

https://codereview.chromium.org/10996037/diff/11001/ui/gfx/rect.cc#newcode17
ui/gfx/rect.cc:17: #include "ui/gfx/rect_conversions.h"
Do you need this include?

https://codereview.chromium.org/10996037/diff/11001/ui/gfx/rect_conversions.h
File ui/gfx/rect_conversions.h (right):

https://codereview.chromium.org/10996037/diff/11001/ui/gfx/rect_conversions.h...
ui/gfx/rect_conversions.h:13: UI_EXPORT Rect ToEnclosingRect(const RectF& rect);
Document what these mean.

https://codereview.chromium.org/10996037/diff/11001/ui/gfx/rect_f.cc
File ui/gfx/rect_f.cc (right):

https://codereview.chromium.org/10996037/diff/11001/ui/gfx/rect_f.cc#newcode11
ui/gfx/rect_f.cc:11: #include "ui/gfx/safe_floor_ceil.h"
Is this needed?

https://codereview.chromium.org/10996037/diff/11001/ui/gfx/safe_floor_ceil.cc
File ui/gfx/safe_floor_ceil.cc (right):

https://codereview.chromium.org/10996037/diff/11001/ui/gfx/safe_floor_ceil.cc...
ui/gfx/safe_floor_ceil.cc:10: int ClampToInt(float value) {
Do we really need all this? Are we really passing around max/nan?

https://codereview.chromium.org/10996037/diff/11001/ui/gfx/size_base.h
File ui/gfx/size_base.h (right):

https://codereview.chromium.org/10996037/diff/11001/ui/gfx/size_base.h#newcode70
ui/gfx/size_base.h:70: { }
{} on previous line

https://codereview.chromium.org/10996037/diff/11001/ui/gfx/size_base.h#newcode74
ui/gfx/size_base.h:74: ~SizeBase() { }
{}

[Ian Vollick, 2012-09-27 22:02:39]

On 2012/09/27 20:33:55, sky wrote:
> webkit/plugins/ppapi/ppb_graphics_2d_impl could likely use ths new code too.
> 
> https://codereview.chromium.org/10996037/diff/11001/ui/gfx/rect.cc
> File ui/gfx/rect.cc (right):
> 
> https://codereview.chromium.org/10996037/diff/11001/ui/gfx/rect.cc#newcode17
> ui/gfx/rect.cc:17: #include "ui/gfx/rect_conversions.h"
> Do you need this include?
Nope, removed.
> 
> https://codereview.chromium.org/10996037/diff/11001/ui/gfx/rect_conversions.h
> File ui/gfx/rect_conversions.h (right):
> 
>
https://codereview.chromium.org/10996037/diff/11001/ui/gfx/rect_conversions.h...
> ui/gfx/rect_conversions.h:13: UI_EXPORT Rect ToEnclosingRect(const RectF&
rect);
> Document what these mean.
Done.
> 
> https://codereview.chromium.org/10996037/diff/11001/ui/gfx/rect_f.cc
> File ui/gfx/rect_f.cc (right):
> 
> https://codereview.chromium.org/10996037/diff/11001/ui/gfx/rect_f.cc#newcode11
> ui/gfx/rect_f.cc:11: #include "ui/gfx/safe_floor_ceil.h"
> Is this needed?
Nope, removed.
> 
> https://codereview.chromium.org/10996037/diff/11001/ui/gfx/safe_floor_ceil.cc
> File ui/gfx/safe_floor_ceil.cc (right):
> 
>
https://codereview.chromium.org/10996037/diff/11001/ui/gfx/safe_floor_ceil.cc...
> ui/gfx/safe_floor_ceil.cc:10: int ClampToInt(float value) {
> Do we really need all this? Are we really passing around max/nan?
I think so. We're hoping to use these types in the chromium compositor code in
cc/, and given arbitrary user input, it's possible we could have a float rect
that can't be represented as an int rect.
> 
> https://codereview.chromium.org/10996037/diff/11001/ui/gfx/size_base.h
> File ui/gfx/size_base.h (right):
> 
>
https://codereview.chromium.org/10996037/diff/11001/ui/gfx/size_base.h#newcode70
> ui/gfx/size_base.h:70: { }
> {} on previous line
Done.
> 
>
https://codereview.chromium.org/10996037/diff/11001/ui/gfx/size_base.h#newcode74
> ui/gfx/size_base.h:74: ~SizeBase() { }
> {}
Done.

[sky, 2012-09-27 23:39:24]

Search for ScaleRectBounds here:
http://src.chromium.org/viewvc/chrome/trunk/src/webkit/plugins/ppapi/ppb_grap...
and spe . That's the one I can think you can now remove.

That said, if you want to split the webkit changes into a separate patch that'll
make your life easier since I'm not a reviewer there.

I'm LGTMing what you have.

[jamesr, 2012-09-27 23:42:36]

lgtm (i have webkit/ owners)

[commit-bot: I haz the power, 2012-09-28 01:47:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/10996037/7005

[commit-bot: I haz the power, 2012-09-28 02:20:55]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-28 11:02:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/10996037/9010

[commit-bot: I haz the power, 2012-09-28 14:32:39]

Change committed as 159256