ui: Prefer +/- operators to apply offsets.

ui: Prefer +/- operators to apply offsets.

Adds +/- operators to gfx::Rect that are applied to its origin Point.
Removes gfx::Point method a.OffsetFrom(b) in favor of (b - a).

Begin use +/- instead of Offset() in ui/gfx/ and cc/

New tests:
ui_unittests:RectTest.Offset

R=pkasting,enne
BUG=158416
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=167012

[danakj, 2012-11-09 03:44:18]

pkasting please review ui/gfx/
enne please review cc/

[danakj, 2012-11-09 03:44:18]

pkasting please review ui/gfx/
enne please review cc/

[enne (OOO), 2012-11-09 06:00:55]

cc lgtm

[tfarina, 2012-11-09 14:29:52]

https://codereview.chromium.org/11293194/diff/1014/ui/gfx/rect_unittest.cc
File ui/gfx/rect_unittest.cc (right):

https://codereview.chromium.org/11293194/diff/1014/ui/gfx/rect_unittest.cc#ne...
ui/gfx/rect_unittest.cc:674: EXPECT_TRUE(gfx::RectF().IsExpressibleAsRect());
nit: I though I had removed the gfx:: prefix when inside gfx namespace, can you
fix this while you are here?

[danakj, 2012-11-09 16:15:11]

https://codereview.chromium.org/11293194/diff/1014/ui/gfx/rect_unittest.cc
File ui/gfx/rect_unittest.cc (right):

https://codereview.chromium.org/11293194/diff/1014/ui/gfx/rect_unittest.cc#ne...
ui/gfx/rect_unittest.cc:674: EXPECT_TRUE(gfx::RectF().IsExpressibleAsRect());
On 2012/11/09 14:29:52, tfarina wrote:
> nit: I though I had removed the gfx:: prefix when inside gfx namespace, can
you
> fix this while you are here?

Done.

[Peter Kasting, 2012-11-09 22:02:36]

LGTM

https://codereview.chromium.org/11293194/diff/9001/ui/gfx/point_base.h
File ui/gfx/point_base.h (right):

https://codereview.chromium.org/11293194/diff/9001/ui/gfx/point_base.h#newcode31
ui/gfx/point_base.h:31: void Offset(Type delta_x, Type delta_y) {
Are we planning on nuking this in favor of the operators eventually?

https://codereview.chromium.org/11293194/diff/9001/ui/gfx/point_base.h#newcode38
ui/gfx/point_base.h:38: y_ += vector.y();
I don't know whether these changes are much of a win...

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

https://codereview.chromium.org/11293194/diff/9001/ui/gfx/rect.h#newcode81
ui/gfx/rect.h:81: UI_EXPORT Rect operator-(const Rect& lhs, const Vector2d&
rhs);
Nit: Probably want to define operators for the opposite argument order too?

https://codereview.chromium.org/11293194/diff/9001/ui/gfx/rect_f.h
File ui/gfx/rect_f.h (right):

https://codereview.chromium.org/11293194/diff/9001/ui/gfx/rect_f.h#newcode59
ui/gfx/rect_f.h:59: UI_EXPORT RectF operator-(const RectF& lhs, const Vector2dF&
rhs);
Same comment

[danakj, 2012-11-09 22:10:10]

https://codereview.chromium.org/11293194/diff/9001/ui/gfx/point_base.h
File ui/gfx/point_base.h (right):

https://codereview.chromium.org/11293194/diff/9001/ui/gfx/point_base.h#newcode31
ui/gfx/point_base.h:31: void Offset(Type delta_x, Type delta_y) {
On 2012/11/09 22:02:36, Peter Kasting wrote:
> Are we planning on nuking this in favor of the operators eventually?

At first I was thinking yes definitely. But in my dreams last night I wondered
if sometimes using a mutating method fits better with the use case/surrounding
code, and sometimes operators fit better.

Either way, there are a million callsites, so it'll take some time. I started to
do that conversion but it ended up way too big to do at once.

https://codereview.chromium.org/11293194/diff/9001/ui/gfx/point_base.h#newcode38
ui/gfx/point_base.h:38: y_ += vector.y();
On 2012/11/09 22:02:36, Peter Kasting wrote:
> I don't know whether these changes are much of a win...

Yeh, not super amazing, I agree.

Basically, this lets you comment out/remove the Offset() methods, and find
callsites easily via compiler. I can undo this if you like, but would rather
keep them for that reason.

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

https://codereview.chromium.org/11293194/diff/9001/ui/gfx/rect.h#newcode81
ui/gfx/rect.h:81: UI_EXPORT Rect operator-(const Rect& lhs, const Vector2d&
rhs);
On 2012/11/09 22:02:36, Peter Kasting wrote:
> Nit: Probably want to define operators for the opposite argument order too?

I was wondering about that as well.

Is it as easy to see that "Vector - Rect" is going to move the rect though? I
feel like this way is a lot clearer, and I'd rather avoid the other one if we
can.

[Peter Kasting, 2012-11-09 22:12:10]

On 2012/11/09 22:10:10, danakj wrote:
> https://codereview.chromium.org/11293194/diff/9001/ui/gfx/rect.h#newcode81
> ui/gfx/rect.h:81: UI_EXPORT Rect operator-(const Rect& lhs, const Vector2d&
> rhs);
> On 2012/11/09 22:02:36, Peter Kasting wrote:
> > Nit: Probably want to define operators for the opposite argument order too?
> 
> I was wondering about that as well.
> 
> Is it as easy to see that "Vector - Rect" is going to move the rect though? I
> feel like this way is a lot clearer, and I'd rather avoid the other one if we
> can.

I would maybe define "Vector + Rect" but definitely not "Vector - Rect".

[danakj, 2012-11-09 22:24:50]

On 2012/11/09 22:12:10, Peter Kasting wrote:
> On 2012/11/09 22:10:10, danakj wrote:
> > https://codereview.chromium.org/11293194/diff/9001/ui/gfx/rect.h#newcode81
> > ui/gfx/rect.h:81: UI_EXPORT Rect operator-(const Rect& lhs, const Vector2d&
> > rhs);
> > On 2012/11/09 22:02:36, Peter Kasting wrote:
> > > Nit: Probably want to define operators for the opposite argument order
too?
> > 
> > I was wondering about that as well.
> > 
> > Is it as easy to see that "Vector - Rect" is going to move the rect though?
I
> > feel like this way is a lot clearer, and I'd rather avoid the other one if
we
> > can.
> 
> I would maybe define "Vector + Rect" but definitely not "Vector - Rect".

Then we are left with

Rect + Vector  : yes
Rect += Vector : yes

Vector + Rect  : yes
Vector += Rect : no

If that makes sense to you, then alrighty! PTAL

[Peter Kasting, 2012-11-09 22:27:28]

LGTM

[commit-bot: I haz the power, 2012-11-09 22:29:54]

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

[commit-bot: I haz the power, 2012-11-09 22:30:06]

Presubmit check for 11293194-12001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui/gfx

Presubmit checks took 7.0s to calculate.

[danakj, 2012-11-09 22:36:46]

+Sky please take a look at ui/gfx/

[sky, 2012-11-09 23:03:45]

LGTM

[slavi, 2012-11-10 00:08:01]

https://codereview.chromium.org/11293194/diff/6021/.gitmodules
File .gitmodules (left):

https://codereview.chromium.org/11293194/diff/6021/.gitmodules#oldcode189
.gitmodules:189: [submodule "third_party/findbugs"]
Did you intentionally committed this .gitmodules?

[danakj, 2012-11-10 00:10:45]

https://codereview.chromium.org/11293194/diff/6021/.gitmodules
File .gitmodules (left):

https://codereview.chromium.org/11293194/diff/6021/.gitmodules#oldcode189
.gitmodules:189: [submodule "third_party/findbugs"]
On 2012/11/10 00:08:01, slavi wrote:
> Did you intentionally committed this .gitmodules?

Uhhh... I have no idea how that got there.

[danakj, 2012-11-10 00:12:24]

On 2012/11/10 00:10:45, danakj wrote:
> https://codereview.chromium.org/11293194/diff/6021/.gitmodules
> File .gitmodules (left):
> 
> https://codereview.chromium.org/11293194/diff/6021/.gitmodules#oldcode189
> .gitmodules:189: [submodule "third_party/findbugs"]
> On 2012/11/10 00:08:01, slavi wrote:
> > Did you intentionally committed this .gitmodules?
> 
> Uhhh... I have no idea how that got there.

It looks like the commit did the right thing regardless.