Make adding and subtracting gfx:: point types simpler.

Make adding and subtracting gfx:: point types simpler.

1) Removed iffy flooring conversion from Float types to Int types.
2) Added methods to convert from Int to Float types so you can do things like
add a Point to a PointF and viceversa.
3) Added operator+ and operator- for point types. This had to be done in both
Point and PointF headers to allow (Point+PointF) to compile.
4) Move the PointF::~PointF() destructor to the .cc file to be consistent with
other types.
5) Remove the unimplemented PointF::PointF(Point) constructor from the header.

BUG=152473
R=sky,jamesr

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=160426

[sky, 2012-09-28 23:05:49]

https://codereview.chromium.org/10993094/diff/3001/ui/gfx/point.h
File ui/gfx/point.h (right):

https://codereview.chromium.org/10993094/diff/3001/ui/gfx/point.h#newcode28
ui/gfx/point.h:28: Point(const Point& other);
explicit

https://codereview.chromium.org/10993094/diff/3001/ui/gfx/point.h#newcode51
ui/gfx/point.h:51: operator PointF() const WARN_UNUSED_RESULT {
Why the WARN_UNUSED_RESULT?

https://codereview.chromium.org/10993094/diff/3001/ui/gfx/point_f.h
File ui/gfx/point_f.h (right):

https://codereview.chromium.org/10993094/diff/3001/ui/gfx/point_f.h#newcode20
ui/gfx/point_f.h:20: PointF(const PointF& other);
Why remove the explicit?

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

https://codereview.chromium.org/10993094/diff/3001/ui/gfx/rect.h#newcode81
ui/gfx/rect.h:81: operator RectF() const WARN_UNUSED_RESULT {
Why WARN_UNUSED_RESULT?

[danakj, 2012-09-28 23:19:14]

Thanks, PTAL.

https://codereview.chromium.org/10993094/diff/3001/ui/gfx/point.h
File ui/gfx/point.h (right):

https://codereview.chromium.org/10993094/diff/3001/ui/gfx/point.h#newcode28
ui/gfx/point.h:28: Point(const Point& other);
On 2012/09/28 23:05:49, sky wrote:
> explicit

thanks. i was able to remove the copy constructors once i removed the undefined
PointF(Point) constructor, so these are gone.

https://codereview.chromium.org/10993094/diff/3001/ui/gfx/point.h#newcode51
ui/gfx/point.h:51: operator PointF() const WARN_UNUSED_RESULT {
On 2012/09/28 23:05:49, sky wrote:
> Why the WARN_UNUSED_RESULT?

removed.

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

https://codereview.chromium.org/10993094/diff/3001/ui/gfx/rect.h#newcode81
ui/gfx/rect.h:81: operator RectF() const WARN_UNUSED_RESULT {
On 2012/09/28 23:05:49, sky wrote:
> Why WARN_UNUSED_RESULT?

removed.

[sky, 2012-10-01 13:42:03]

I have mixed feelings about all this. From the style guide: "Do not overload
operators except in rare, special circumstances". Is there a compelling reason
to add operator overloading here? Similarly arguments apply to for keeping
ToRectF and the like.

[danakj, 2012-10-01 15:22:38]

On 2012/10/01 13:42:03, sky wrote:
> I have mixed feelings about all this. From the style guide: "Do not overload
> operators except in rare, special circumstances". Is there a compelling reason
> to add operator overloading here? Similarly arguments apply to for keeping
> ToRectF and the like.

Regarding operator+ and operator-: Adding two points together is a well defined
mathematical operation. I agree adding operators for many classes would be
confusing. However, this is adding two numbers together, and it becomes more
clear, not less, by using operators.

Compare:

gfx::PointF textureOffset = quad->textureOffset() + clampOffset +
                            (tileRect.origin() - quad->quadRect().origin());

gfx::PointF textureOffset = quad->textureOffset().Add(clampOffset).Add(
                           
tileRect.origin().Subtract(quad->quadRect().origin())));

The first is clear what is going on, the latter is closer to alphabet soup. I
think "well defined/commonly understood math operations" should be an acceptable
circumstance.

As to operator RectF() it is the same as a RectF(Rect) constructor, but allows
us to keep the include headers going the other way. A RectF can hold every value
that a Rect can, so this is not lossy, and I don't see the value-add in being
explicit in casting up to a RectF for a function that is going to return a RectF
also. The above then becomes

gfx::PointF textureOffset =
quad->textureOffset().ToPointF().Add(clampOffset).Add(
                           
tileRect.ToRectF().origin().Subtract(quad->quadRect().ToRectF().origin())));

IMHO this is the worst-case scenario for doing geometric math.

[sky, 2012-10-01 17:08:29]

Here's more from the section suggesting no operators:

It can fool our intuition into thinking that expensive operations are
cheap, built-in operations.
It is much harder to find the call sites for overloaded operators.
Searching for Equals() is much easier than searching for relevant
invocations of ==.
Some operators work on pointers too, making it easy to introduce bugs.
Foo + 4 may do one thing, while &Foo + 4 does something totally
different. The compiler does not complain for either of these, making
this very hard to debug.

I agree that +/- are well understood, but these points are valid. That
said, we do allow operators for a handful of classes so I'm not sure
where to draw the line. I'm inclined to see them added here. Brett, do
you have any recommendations for adding the +/- operators to
Point/Rect?

  -Scott

On Mon, Oct 1, 2012 at 8:22 AM,  <danakj@chromium.org> wrote:
> On 2012/10/01 13:42:03, sky wrote:
>>
>> I have mixed feelings about all this. From the style guide: "Do not
>> overload
>> operators except in rare, special circumstances". Is there a compelling
>> reason
>> to add operator overloading here? Similarly arguments apply to for keeping
>> ToRectF and the like.
>
>
> Regarding operator+ and operator-: Adding two points together is a well
> defined
> mathematical operation. I agree adding operators for many classes would be
> confusing. However, this is adding two numbers together, and it becomes more
> clear, not less, by using operators.
>
> Compare:
>
> gfx::PointF textureOffset = quad->textureOffset() + clampOffset +
>                             (tileRect.origin() - quad->quadRect().origin());
>
> gfx::PointF textureOffset = quad->textureOffset().Add(clampOffset).Add(
>
> tileRect.origin().Subtract(quad->quadRect().origin())));
>
> The first is clear what is going on, the latter is closer to alphabet soup.
> I
> think "well defined/commonly understood math operations" should be an
> acceptable
> circumstance.
>
> As to operator RectF() it is the same as a RectF(Rect) constructor, but
> allows
> us to keep the include headers going the other way. A RectF can hold every
> value
> that a Rect can, so this is not lossy, and I don't see the value-add in
> being
> explicit in casting up to a RectF for a function that is going to return a
> RectF
> also. The above then becomes
>
> gfx::PointF textureOffset =
> quad->textureOffset().ToPointF().Add(clampOffset).Add(
>
> tileRect.ToRectF().origin().Subtract(quad->quadRect().ToRectF().origin())));
>
> IMHO this is the worst-case scenario for doing geometric math.
>
> https://codereview.chromium.org/10993094/

[brettw, 2012-10-01 17:23:29]

I think simple geometric base types are the main place where operator
overloading makes sense, so I'm OK with this change.

[jamesr, 2012-10-01 18:17:17]

On 2012/10/01 15:22:38, danakj wrote:
> As to operator RectF() it is the same as a RectF(Rect) constructor, but allows
> us to keep the include headers going the other way. A RectF can hold every
value
> that a Rect can, so this is not lossy

It can't exactly for absolute magnitudes > 2^22

[danakj, 2012-10-01 18:20:10]

On 2012/10/01 18:17:17, jamesr wrote:
> On 2012/10/01 15:22:38, danakj wrote:
> > As to operator RectF() it is the same as a RectF(Rect) constructor, but
allows
> > us to keep the include headers going the other way. A RectF can hold every
> value
> > that a Rect can, so this is not lossy
> 
> It can't exactly for absolute magnitudes > 2^22

Oh ok fair enough :) I don't think we'd be checking for that with explicit
casting though either? So I still think the implicit conversion makes sense to
let us to operations on int and float types together, similar to adding an int
and a float directly is viable.

[sky, 2012-10-01 21:02:20]

https://codereview.chromium.org/10993094/diff/2010/ui/gfx/point.h
File ui/gfx/point.h (right):

https://codereview.chromium.org/10993094/diff/2010/ui/gfx/point.h#newcode55
ui/gfx/point.h:55: inline Point operator+(Point lhs, Point rhs) {
const Point& here (and 59).

https://codereview.chromium.org/10993094/diff/2010/ui/gfx/point_f.h
File ui/gfx/point_f.h (right):

https://codereview.chromium.org/10993094/diff/2010/ui/gfx/point_f.h#newcode26
ui/gfx/point_f.h:26: inline PointF operator+(PointF lhs, PointF rhs) {
const Point& here (and 30).

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

https://codereview.chromium.org/10993094/diff/2010/ui/gfx/rect.h#newcode81
ui/gfx/rect.h:81: operator RectF() const {
Why is this better than ToRectF?

https://codereview.chromium.org/10993094/diff/2010/ui/gfx/size.h
File ui/gfx/size.h (right):

https://codereview.chromium.org/10993094/diff/2010/ui/gfx/size.h#newcode48
ui/gfx/size.h:48: operator SizeF() const {
Why is this better than ToSizeF?

[sky, 2012-10-01 21:02:28]

Thanks Brett.

  -Scott

On Mon, Oct 1, 2012 at 10:23 AM,  <brettw@chromium.org> wrote:
> I think simple geometric base types are the main place where operator
> overloading makes sense, so I'm OK with this change.
>
> https://codereview.chromium.org/10993094/

[danakj, 2012-10-01 21:55:34]

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

https://codereview.chromium.org/10993094/diff/2010/ui/gfx/rect.h#newcode81
ui/gfx/rect.h:81: operator RectF() const {
On 2012/10/01 21:02:20, sky wrote:
> Why is this better than ToRectF?

So you don't have to cast all the ints to floats explicitly every time you have
some floating point values in the equation.

float f; int i;
float f2 = f + i; // does not need static_cast<float>(i).

RectF f; Rect i;
PointF f2 = f.origin() + i.origin(); // would need i.ToRectF().origin()

If I want to add something to a PointF, I don't care if it is a Point or a
PointF. I feel ToRectF() just decreases the signal to noise ratio of the code.

[danakj, 2012-10-01 22:03:37]

https://codereview.chromium.org/10993094/diff/2010/ui/gfx/point.h
File ui/gfx/point.h (right):

https://codereview.chromium.org/10993094/diff/2010/ui/gfx/point.h#newcode55
ui/gfx/point.h:55: inline Point operator+(Point lhs, Point rhs) {
On 2012/10/01 21:02:20, sky wrote:
> const Point& here (and 59).

Done.

https://codereview.chromium.org/10993094/diff/2010/ui/gfx/point_f.h
File ui/gfx/point_f.h (right):

https://codereview.chromium.org/10993094/diff/2010/ui/gfx/point_f.h#newcode26
ui/gfx/point_f.h:26: inline PointF operator+(PointF lhs, PointF rhs) {
On 2012/10/01 21:02:20, sky wrote:
> const Point& here (and 30).

Done.

[sky, 2012-10-01 22:59:21]

It's obscure and not immediately obvious what is going on. Which is
why the style guide says no to operator overloading. An explicit ToXXX
makes it clear what is going on.

  -Scott

On Mon, Oct 1, 2012 at 2:55 PM,  <danakj@chromium.org> wrote:
>
> https://codereview.chromium.org/10993094/diff/2010/ui/gfx/rect.h
> File ui/gfx/rect.h (right):
>
> https://codereview.chromium.org/10993094/diff/2010/ui/gfx/rect.h#newcode81
> ui/gfx/rect.h:81: operator RectF() const {
> On 2012/10/01 21:02:20, sky wrote:
>>
>> Why is this better than ToRectF?
>
>
> So you don't have to cast all the ints to floats explicitly every time
> you have some floating point values in the equation.
>
> float f; int i;
> float f2 = f + i; // does not need static_cast<float>(i).
>
> RectF f; Rect i;
> PointF f2 = f.origin() + i.origin(); // would need i.ToRectF().origin()
>
> If I want to add something to a PointF, I don't care if it is a Point or
> a PointF. I feel ToRectF() just decreases the signal to noise ratio of
> the code.
>
> https://codereview.chromium.org/10993094/

[danakj, 2012-10-02 18:36:51]

ok. ptal

[enne (OOO), 2012-10-02 19:29:22]

On 2012/10/01 22:59:21, sky wrote:
> It's obscure and not immediately obvious what is going on. Which is
> why the style guide says no to operator overloading. An explicit ToXXX
> makes it clear what is going on.

I don't understand how it's obscure or not immediately obvious.  As Dana points
out, when adding a float and an int, C++ doesn't require you to write this
cumbersome line:

    float x = 3.0 + static_cast<float>(4);

The compiler does the obvious thing and casts the int to a float implicitly.  I
don't think doing this with a group of two or four ints changes the obviousness
here.

Is the problem here just the implicit conversion operator overloading? Could we
instead add a RectF(Rect) constructor? Or, operator+(RectF, Rect) variations?

[sky, 2012-10-02 20:04:32]

In my experience folks are less surprised by implicit conversion of
primitive types then they are by operator overloading of non-primitive
types.

  -Scott

On Tue, Oct 2, 2012 at 12:29 PM,  <enne@chromium.org> wrote:
> On 2012/10/01 22:59:21, sky wrote:
>>
>> It's obscure and not immediately obvious what is going on. Which is
>> why the style guide says no to operator overloading. An explicit ToXXX
>> makes it clear what is going on.
>
>
> I don't understand how it's obscure or not immediately obvious.  As Dana
> points
> out, when adding a float and an int, C++ doesn't require you to write this
> cumbersome line:
>
>     float x = 3.0 + static_cast<float>(4);
>
> The compiler does the obvious thing and casts the int to a float implicitly.
> I
> don't think doing this with a group of two or four ints changes the
> obviousness
> here.
>
> Is the problem here just the implicit conversion operator overloading? Could
> we
> instead add a RectF(Rect) constructor? Or, operator+(RectF, Rect)
> variations?
>
> http://codereview.chromium.org/10993094/

[sky, 2012-10-02 20:05:38]

LGTM

http://codereview.chromium.org/10993094/diff/25001/ui/gfx/size.h
File ui/gfx/size.h (right):

http://codereview.chromium.org/10993094/diff/25001/ui/gfx/size.h#newcode46
ui/gfx/size.h:46: SizeF ToSizeF() const WARN_UNUSED_RESULT {
Remove the WARN_UNUSED_RESULT. We're only using that on functions where it isn't
clear if it mutates the object you're invoking it on (like Scale).

[danakj, 2012-10-02 20:06:33]

http://codereview.chromium.org/10993094/diff/25001/ui/gfx/size.h
File ui/gfx/size.h (right):

http://codereview.chromium.org/10993094/diff/25001/ui/gfx/size.h#newcode46
ui/gfx/size.h:46: SizeF ToSizeF() const WARN_UNUSED_RESULT {
On 2012/10/02 20:05:38, sky wrote:
> Remove the WARN_UNUSED_RESULT. We're only using that on functions where it
isn't
> clear if it mutates the object you're invoking it on (like Scale).

OK. I will remove it from the already-present ToRectF() then too.

[enne (OOO), 2012-10-02 20:33:21]

On 2012/10/02 20:04:32, sky wrote:
> In my experience folks are less surprised by implicit conversion of
> primitive types then they are by operator overloading of non-primitive
> type

In 99% of classes, I would agree with that statement.  I think classes that are
clearly simple "math" or "geometry" classes like point, size, and rect that have
well-defined operators are the exception to the rule.

Folks using these classes will be far more surprised by having to explicitly
convert ints to floats than the other way around.

[commit-bot: I haz the power, 2012-10-04 22:19:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/10993094/17002

[commit-bot: I haz the power, 2012-10-04 22:46:30]

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-10-04 22:46:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/10993094/34001

[commit-bot: I haz the power, 2012-10-05 01:04:57]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-05 01:05:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/10993094/33002

[commit-bot: I haz the power, 2012-10-05 01:17:57]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[tfarina, 2012-10-05 01:49:48]

On 2012/10/02 20:33:21, enne wrote:
> On 2012/10/02 20:04:32, sky wrote:
> > In my experience folks are less surprised by implicit conversion of
> > primitive types then they are by operator overloading of non-primitive
> > type
> 
> In 99% of classes, I would agree with that statement.  I think classes that
are
> clearly simple "math" or "geometry" classes like point, size, and rect that
have
> well-defined operators are the exception to the rule.
> 
> Folks using these classes will be far more surprised by having to explicitly
> convert ints to floats than the other way around.

Scott, if we can't do this conversion implicitly, then migrating cc/ to
gfx::Point/Size/Rect will be a *big* pain. Because if I'm not wrong
IntPoint/Size/Rect support this and it's being used there.

[enne (OOO), 2012-10-05 16:11:26]

On 2012/10/05 01:49:48, tfarina wrote:
> Scott, if we can't do this conversion implicitly, then migrating cc/ to
> gfx::Point/Size/Rect will be a *big* pain. Because if I'm not wrong
> IntPoint/Size/Rect support this and it's being used there.

Exactly.  If we can't implicitly convert gfx geometry ints to floats, then my
inclination would be that cc should just not use gfx geometry classes at all and
instead use its own internal point/size/rect classes that are less cumbersome to
use.

[piman, 2012-10-05 16:29:37]

On Fri, Oct 5, 2012 at 9:11 AM, <enne@chromium.org> wrote:

> On 2012/10/05 01:49:48, tfarina wrote:
>
>> Scott, if we can't do this conversion implicitly, then migrating cc/ to
>> gfx::Point/Size/Rect will be a *big* pain. Because if I'm not wrong
>> IntPoint/Size/Rect support this and it's being used there.
>>
>
> Exactly.  If we can't implicitly convert gfx geometry ints to floats, then
> my
> inclination would be that cc should just not use gfx geometry classes at
> all and
> instead use its own internal point/size/rect classes that are less
> cumbersome to
> use.
>

Before we go to extremes, it's possible to add conversion operators in cc/.

Antoine


>
>
https://chromiumcodereview.**appspot.com/10993094/<https://chromiumcodereview...
>

[commit-bot: I haz the power, 2012-10-05 16:49:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/10993094/36001

[danakj, 2012-10-05 17:20:33]

On Fri, Oct 5, 2012 at 9:29 AM, Antoine Labour <piman@chromium.org> wrote:
>
>
> On Fri, Oct 5, 2012 at 9:11 AM, <enne@chromium.org> wrote:
>>
>> On 2012/10/05 01:49:48, tfarina wrote:
>>>
>>> Scott, if we can't do this conversion implicitly, then migrating cc/ to
>>> gfx::Point/Size/Rect will be a *big* pain. Because if I'm not wrong
>>> IntPoint/Size/Rect support this and it's being used there.
>>
>>
>> Exactly.  If we can't implicitly convert gfx geometry ints to floats, then
>> my
>> inclination would be that cc should just not use gfx geometry classes at
>> all and
>> instead use its own internal point/size/rect classes that are less
>> cumbersome to
>> use.
>
>
> Before we go to extremes, it's possible to add conversion operators in cc/.

Can we? I'm attempting to but the compiler isn't finding the
conversion, such as:

class ConvertingPoint : public Point {
 ConvertingPoint(Point p)
     : Point(p)
  { }
  operator PointF() {
    return PointF(x(), y());
  }
};

And I don't think we can just write an implicit conversion operator
outside of the class.

[piman, 2012-10-05 17:27:31]

On Fri, Oct 5, 2012 at 10:20 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Fri, Oct 5, 2012 at 9:29 AM, Antoine Labour <piman@chromium.org> wrote:
> >
> >
> > On Fri, Oct 5, 2012 at 9:11 AM, <enne@chromium.org> wrote:
> >>
> >> On 2012/10/05 01:49:48, tfarina wrote:
> >>>
> >>> Scott, if we can't do this conversion implicitly, then migrating cc/ to
> >>> gfx::Point/Size/Rect will be a *big* pain. Because if I'm not wrong
> >>> IntPoint/Size/Rect support this and it's being used there.
> >>
> >>
> >> Exactly.  If we can't implicitly convert gfx geometry ints to floats,
> then
> >> my
> >> inclination would be that cc should just not use gfx geometry classes at
> >> all and
> >> instead use its own internal point/size/rect classes that are less
> >> cumbersome to
> >> use.
> >
> >
> > Before we go to extremes, it's possible to add conversion operators in
> cc/.
>
> Can we? I'm attempting to but the compiler isn't finding the
> conversion, such as:
>
> class ConvertingPoint : public Point {
>  ConvertingPoint(Point p)
>      : Point(p)
>   { }
>   operator PointF() {
>     return PointF(x(), y());
>   }
> };
>
> And I don't think we can just write an implicit conversion operator
> outside of the class.
>

How about making cc's IntPoint etc. extend gfx types and put operators on
them?

Antoine

[commit-bot: I haz the power, 2012-10-05 18:24:29]

Retried try job too often for step(s) interactive_ui_tests