The center of a rect is x+width/2, y+height/2

The center of a rect is x+width/2, y+height/2

If a rect is (0, 0, 5, 5) the center should be 2.5, 2.5, not 2, 2.

Tests: 
ui_unittests:RectTest.CenterPoint
ui_unittests:RectTest.CenterPointF

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

[sky, 2012-10-05 21:07:03]

Add a test otherwise with justification as to why it is the way it is otherwise
I guarantee someone will want to change it.

[oshima, 2012-10-05 21:07:23]

Peter should review this as he made this change in
http://codereview.chromium.org/6474016/

[Peter Kasting, 2012-10-05 21:11:05]

On 2012/10/05 21:07:23, oshima wrote:
> Peter should review this as he made this change in
> http://codereview.chromium.org/6474016/

Obviously, you should test the infobar antispoof arrow to ensure it still points
to the center pixel of the star after this change.

Frankly, your code looks more correct than mine, even accounting for the fact
that we didn't use to have fractional value support for rect/point.

[tfarina, 2012-10-05 21:25:23]

http://codereview.chromium.org/11065050/diff/1/ash/tooltips/tooltip_controlle...
File ash/tooltips/tooltip_controller_unittest.cc (right):

http://codereview.chromium.org/11065050/diff/1/ash/tooltips/tooltip_controlle...
ash/tooltips/tooltip_controller_unittest.cc:197: for (int i = 0; i < 49; i++) {
++i, while you are touching this.

[danakj, 2012-10-05 21:35:36]

fix aura_unittests

[danakj, 2012-10-05 21:40:13]

On 2012/10/05 21:11:05, Peter Kasting wrote:
> On 2012/10/05 21:07:23, oshima wrote:
> > Peter should review this as he made this change in
> > http://codereview.chromium.org/6474016/
> 
> Obviously, you should test the infobar antispoof arrow to ensure it still
points
> to the center pixel of the star after this change.

If the thing has an odd size, then it should be in the same place. If it has an
even size, then it should be in the left-of-pixel position instead of
right-of-center. That said, what's the way to test this?

[danakj, 2012-10-05 21:40:48]

swap left and right in that last sentence.

[Peter Kasting, 2012-10-05 22:00:19]

On 2012/10/05 21:40:13, danakj wrote:
> On 2012/10/05 21:11:05, Peter Kasting wrote:
> > On 2012/10/05 21:07:23, oshima wrote:
> > > Peter should review this as he made this change in
> > > http://codereview.chromium.org/6474016/
> > 
> > Obviously, you should test the infobar antispoof arrow to ensure it still
> points
> > to the center pixel of the star after this change.
> 
> If the thing has an odd size, then it should be in the same place. If it has
an
> even size, then it should be in the left-of-pixel position instead of
> right-of-center. That said, what's the way to test this?

The way to test is that you build Chrome on Windows and open any infobar (e.g. a
geolocation infobar, visit http://benwerd.com/lab/geo.php ) and look at
precisely which pixel the infobar "anti-spoof" arrow is pointing up at before
and after the change.  Hopefully they're the same.

[sky, 2012-10-05 22:55:21]

Can't you write a unit test for this?

On Fri, Oct 5, 2012 at 2:40 PM,  <danakj@chromium.org> wrote:
> On 2012/10/05 21:11:05, Peter Kasting wrote:
>>
>> On 2012/10/05 21:07:23, oshima wrote:
>> > Peter should review this as he made this change in
>> > http://codereview.chromium.org/6474016/
>
>
>> Obviously, you should test the infobar antispoof arrow to ensure it still
>
> points
>>
>> to the center pixel of the star after this change.
>
>
> If the thing has an odd size, then it should be in the same place. If it has
> an
> even size, then it should be in the left-of-pixel position instead of
> right-of-center. That said, what's the way to test this?
>
> https://codereview.chromium.org/11065050/

[Peter Kasting, 2012-10-05 22:58:05]

On Fri, Oct 5, 2012 at 3:55 PM, Scott Violet <sky@chromium.org> wrote:

> Can't you write a unit test for this?


What, that the arrow looks right?  Not sure how you want to unit test that
in a non-fragile way.


> > https://codereview.chromium.org/11065050/
>

[sky, 2012-10-05 22:59:24]

I meant for rect_base. I agree that testing the arrow in the info bar
doesn't make sense.

  -Scott

On Fri, Oct 5, 2012 at 3:58 PM, Peter Kasting <pkasting@chromium.org> wrote:
> On Fri, Oct 5, 2012 at 3:55 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> Can't you write a unit test for this?
>
>
> What, that the arrow looks right?  Not sure how you want to unit test that
> in a non-fragile way.
>
>>
>> > https://codereview.chromium.org/11065050/
>
>

[danakj, 2012-10-06 17:08:50]

tests added

[danakj, 2012-10-06 17:20:22]

On 2012/10/05 22:00:19, Peter Kasting wrote:
> The way to test is that you build Chrome on Windows and open any infobar (e.g.
a
> geolocation infobar, visit http://benwerd.com/lab/geo.php ) and look at
> precisely which pixel the infobar "anti-spoof" arrow is pointing up at before
> and after the change.  Hopefully they're the same.

Thanks, but when I visit this site with Windows or Mac Canary, I get an infobar
but don't see any "anti-spoof" anything appearing. Does this still exist, or is
there some missing step?

[danakj, 2012-10-06 17:26:25]

On 2012/10/06 17:20:22, danakj wrote:
> On 2012/10/05 22:00:19, Peter Kasting wrote:
> > The way to test is that you build Chrome on Windows and open any infobar
(e.g.
> a
> > geolocation infobar, visit http://benwerd.com/lab/geo.php ) and look at
> > precisely which pixel the infobar "anti-spoof" arrow is pointing up at
before
> > and after the change.  Hopefully they're the same.
> 
> Thanks, but when I visit this site with Windows or Mac Canary, I get an
infobar
> but don't see any "anti-spoof" anything appearing. Does this still exist, or
is
> there some missing step?

Oh, I think you meant the arrow from the infobar pointing at the location bar's
favicon? If so, I see that.

[Peter Kasting, 2012-10-06 18:07:07]

On 2012/10/06 17:26:25, danakj wrote:
> On 2012/10/06 17:20:22, danakj wrote:
> > On 2012/10/05 22:00:19, Peter Kasting wrote:
> > > The way to test is that you build Chrome on Windows and open any infobar
> (e.g.
> > a
> > > geolocation infobar, visit http://benwerd.com/lab/geo.php ) and look at
> > > precisely which pixel the infobar "anti-spoof" arrow is pointing up at
> before
> > > and after the change.  Hopefully they're the same.
> > 
> > Thanks, but when I visit this site with Windows or Mac Canary, I get an
> infobar
> > but don't see any "anti-spoof" anything appearing. Does this still exist, or
> is
> > there some missing step?
> 
> Oh, I think you meant the arrow from the infobar pointing at the location
bar's
> favicon? If so, I see that.

Right, screenshot that on Windows before and after your change.

[danakj, 2012-10-06 18:12:58]

On Sat, Oct 6, 2012 at 2:07 PM,  <pkasting@chromium.org> wrote:
> Right, screenshot that on Windows before and after your change.

What about that is Windows specific? The bar also appears on linux and mac.

[Peter Kasting, 2012-10-06 18:24:45]

On Sat, Oct 6, 2012 at 11:12 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Sat, Oct 6, 2012 at 2:07 PM,  <pkasting@chromium.org> wrote:
> > Right, screenshot that on Windows before and after your change.
>
> What about that is Windows specific? The bar also appears on linux and mac.
>

I am not convinced either of them actually use Rect::Center() to position
the arrow tip (especially Mac).  Remember that our frontend is re-written
on each OS.

PK

[danakj, 2012-10-06 18:26:53]

On 2012/10/06 18:24:45, Peter Kasting wrote:
> On Sat, Oct 6, 2012 at 11:12 AM, Dana Jansens <mailto:danakj@chromium.org>
wrote:
> 
> > On Sat, Oct 6, 2012 at 2:07 PM,  <mailto:pkasting@chromium.org> wrote:
> > > Right, screenshot that on Windows before and after your change.
> >
> > What about that is Windows specific? The bar also appears on linux and mac.
> >
> 
> I am not convinced either of them actually use Rect::Center() to position
> the arrow tip (especially Mac).  Remember that our frontend is re-written
> on each OS.

I see. On Linux the arrow did not move anyhow:
http://www/~danakj/Before-Rect-Center.png
http://www/~danakj/After-Rect-Center.png

Installing visual studio.. Is this something you could easily test? I don't have
a windows environment atm.

[Peter Kasting, 2012-10-06 22:10:53]

I could probably test this next week sometime if you need.

[danakj, 2012-10-06 22:16:12]

On 2012/10/06 22:10:53, Peter Kasting wrote:
> I could probably test this next week sometime if you need.

I don't have a windows build set up yet, but if there are no tests for this, is
there a reason to need the arrow in the center-left pixel instead of the
center-right?

This change is something we need for cc/ to make use of these types. And if you
need a different behaviour we're going to have to make a duplicate method of
some sort with the other behaviour.

[sadrul, 2012-10-07 01:53:21]

On 2012/10/06 22:16:12, danakj wrote:
> On 2012/10/06 22:10:53, Peter Kasting wrote:
> > I could probably test this next week sometime if you need.
> 
> I don't have a windows build set up yet,

ChromeOS shares a lot of the ui code with windows (including views
implementation of various components). So you may be able to test this on a
linux_chromeos build.

> but if there are no tests for this, is
> there a reason to need the arrow in the center-left pixel instead of the
> center-right?
> 
> This change is something we need for cc/ to make use of these types. And if
you
> need a different behaviour we're going to have to make a duplicate method of
> some sort with the other behaviour.

[Peter Kasting, 2012-10-07 05:49:48]

On 2012/10/06 22:16:12, danakj wrote:
> On 2012/10/06 22:10:53, Peter Kasting wrote:
> > I could probably test this next week sometime if you need.
> 
> I don't have a windows build set up yet, but if there are no tests for this,
is
> there a reason to need the arrow in the center-left pixel instead of the
> center-right?

Dunno if it will even shift anything.  That's why I want to look at the
screenshot.  If it shifts something and the result doesn't look good, we can
probably work around some other way, but we'll handle that when we see what this
does.

[danakj, 2012-10-07 18:24:01]

On 2012/10/07 01:53:21, sadrul wrote:
> On 2012/10/06 22:16:12, danakj wrote:
> > On 2012/10/06 22:10:53, Peter Kasting wrote:
> > > I could probably test this next week sometime if you need.
> > 
> > I don't have a windows build set up yet,
> 
> ChromeOS shares a lot of the ui code with windows (including views
> implementation of various components). So you may be able to test this on a
> linux_chromeos build.

Sadrul, thank you, you are awesome! ChromeOS build does indeed use the
browser_view.cc to place the arrow. I verified by adding +100 and putting it in
a total wrong place.

The arrow doesn't move at all when I apply this patch, as the object it's taking
the center of has an odd size (width 19). Since it didn't move the pictures
aren't that interesting, but:

http://www/~danakj/ChromeOS-Before.png
http://www/~danakj/ChromeOS-After.png

[danakj, 2012-10-09 04:21:54]

Ping for review?

[sky, 2012-10-09 15:55:50]

LGTM - but you still need to make sure windows is ok.

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

https://codereview.chromium.org/11065050/diff/14002/ui/gfx/rect_unittest.cc#n...
ui/gfx/rect_unittest.cc:315: EXPECT_TRUE(center == gfx::Point(10, 10));
Use EXPECT_EQ("10,10", center) for these. Doing so makes for more readable error
messages when things go wrong.

[rjkroege, 2012-10-09 15:57:08]

fwiw: the gesture recognizer unit test lgtm

[sky, 2012-10-09 21:05:52]

Your test for ChromeOS should cover windows. So, no need to test there.

[danakj, 2012-10-09 21:06:58]

On 2012/10/09 21:05:52, sky wrote:
> Your test for ChromeOS should cover windows. So, no need to test there.

Great, thanks!

[commit-bot: I haz the power, 2012-10-09 21:07:25]

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

[commit-bot: I haz the power, 2012-10-09 23:22:11]

Change committed as 160976