Add Vector2d classes that represent offsets, instead of using Point.

Add Vector2d classes that represent offsets, instead of using Point.

Previously Point served two purposes, to be a position in 2d space, and also
an offset from the origin. This introduces a Vector2d class to represent an
offset, allowing typesafety checks for geometric operations.

The following are now the case:

Point +/- Vector = Point
- A point plus/minus an offset gives you a point at a position offset by the
vector.

Vector +/- Vector = Vector
- Two offsets can be added together to make a new offset.

Point - Point = Vector
- Subtracting one point from another gives you the offset between the two
points.

We add some new methods to perform these operations:

Rect::OffsetFromOrigin() gives the offset between the position of the rect
and the origin.

Point::OffsetFromOrigin() gives the offset between the point and the origin.

PointAtOffsetFromOrigin(Vector2d) converts a Vector2d to a Point at the given
offset away from the origin.

Rect::Offset(), Point::Add(), and Point::Subtract() now receive a Vector2d
instead of a point.

BUG=147395
R=sky

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

[danakj, 2012-10-25 00:32:12]

Hi Sky,

Please take a look! Thanks.

The interesting parts are, of course:
ui/gfx/vector*
ui/gfx/point*
ui/gfx/rect*

[tfarina, 2012-10-25 00:40:20]

https://codereview.chromium.org/11269022/diff/2001/ui/gfx/vector2d.h
File ui/gfx/vector2d.h (right):

https://codereview.chromium.org/11269022/diff/2001/ui/gfx/vector2d.h#newcode46
ui/gfx/vector2d.h:46: int m_x;
nit: please remember that we are in chromium code, not WebKit land :)

s/m_x/x_

https://codereview.chromium.org/11269022/diff/2001/ui/gfx/vector2d_f.h
File ui/gfx/vector2d_f.h (right):

https://codereview.chromium.org/11269022/diff/2001/ui/gfx/vector2d_f.h#newcode43
ui/gfx/vector2d_f.h:43: float m_x;
nit: please remember that we are in chromium code, not WebKit land :)

s/m_x/x_

[tfarina, 2012-10-25 00:41:52]

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

https://codereview.chromium.org/11269022/diff/2001/ui/gfx/point.h#newcode25
ui/gfx/point.h:25: class UI_EXPORT Point : public PointBase<Point, int,
Vector2d> {
nit: indent less two spaces back! somehow you messed here ;)

[danakj, 2012-10-25 00:59:08]

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

https://codereview.chromium.org/11269022/diff/2001/ui/gfx/point.h#newcode25
ui/gfx/point.h:25: class UI_EXPORT Point : public PointBase<Point, int,
Vector2d> {
On 2012/10/25 00:41:53, tfarina wrote:
> nit: indent less two spaces back! somehow you messed here ;)

Done.

https://codereview.chromium.org/11269022/diff/2001/ui/gfx/vector2d.h
File ui/gfx/vector2d.h (right):

https://codereview.chromium.org/11269022/diff/2001/ui/gfx/vector2d.h#newcode46
ui/gfx/vector2d.h:46: int m_x;
On 2012/10/25 00:40:20, tfarina wrote:
> nit: please remember that we are in chromium code, not WebKit land :)
> 
> s/m_x/x_

oh, shoot. thanks.

[danakj, 2012-10-25 02:38:00]

I started making use of the Vector2d to replace IntSize in cc/ here
https://codereview.chromium.org/11266030/ and added some stuff that is being
used there to the CL.

Also, added some unittests!

[sky, 2012-10-25 15:30:39]

Before reviewing this I want to resolve whether we we should go with
returning new values always. I've sent you an email on that.

  -Scott

On Wed, Oct 24, 2012 at 7:38 PM,  <danakj@chromium.org> wrote:
> I started making use of the Vector2d to replace IntSize in cc/ here
> https://codereview.chromium.org/11266030/ and added some stuff that is being
> used there to the CL.
>
> Also, added some unittests!
>
> https://codereview.chromium.org/11269022/

[danakj, 2012-10-26 02:48:56]

I used this for a bunch more porting found these missing, so I added them here
with tests.

- Vector2dF::scale(s)
- Vector2dF::scale(x, y)
- Rect::Rect(point, vector2d)
- RectF::RectF(pointF, vector2dF)

[danakj, 2012-10-26 04:36:53]

On Oct 25, 2012 10:48 PM, <danakj@chromium.org> wrote:
>
> I used this for a bunch more porting found these missing, so I added them
here
> with tests.
>
> - Vector2dF::scale(s)
> - Vector2dF::scale(x, y)

> - Rect::Rect(point, vector2d)
> - RectF::RectF(pointF, vector2dF)

On second thought these constructors aren't a good idea since vectors can
be negative. I'll make a vector->size conversion instead that does abs().

[Ian Vollick, 2012-10-26 12:45:02]

On 2012/10/26 04:36:53, danakj wrote:
> On Oct 25, 2012 10:48 PM, <mailto:danakj@chromium.org> wrote:
> >
> > I used this for a bunch more porting found these missing, so I added them
> here
> > with tests.
> >
> > - Vector2dF::scale(s)
> > - Vector2dF::scale(x, y)
> 
> > - Rect::Rect(point, vector2d)
> > - RectF::RectF(pointF, vector2dF)
> 
> On second thought these constructors aren't a good idea since vectors can
> be negative. I'll make a vector->size conversion instead that does abs().

Perhaps Rect::Rect({5, 5}, {-2, -2}) = { origin={3, 3}, bounds={2, 2} }?

[danakj, 2012-10-26 15:53:03]

On Fri, Oct 26, 2012 at 8:45 AM,  <vollick@chromium.org> wrote:
> On 2012/10/26 04:36:53, danakj wrote:
>
>> On Oct 25, 2012 10:48 PM, <mailto:danakj@chromium.org> wrote:
>> >
>> > I used this for a bunch more porting found these missing, so I added
>> > them
>> here
>> > with tests.
>> >
>> > - Vector2dF::scale(s)
>> > - Vector2dF::scale(x, y)
>
>
>> > - Rect::Rect(point, vector2d)
>> > - RectF::RectF(pointF, vector2dF)
>
>
>> On second thought these constructors aren't a good idea since vectors can
>> be negative. I'll make a vector->size conversion instead that does abs().
>
>
> Perhaps Rect::Rect({5, 5}, {-2, -2}) = { origin={3, 3}, bounds={2, 2} }?

What I wanted was a way to take a distance, and make a rect with
width/height equal to the length of the x/y components of the vector.
I think SizeOfVector2d(Vector2d) does this in a clear way, and
requires no changing of Rect.

Updated CL with this, removed the Rect constructors with vector, and
added Vector2d::Length methods, with unit tests.

[sky, 2012-10-26 20:22:17]

I'm adding Peter as a reviewer on this one.

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

https://codereview.chromium.org/11269022/diff/16001/ui/gfx/point.h#newcode72
ui/gfx/point.h:72: inline Point operator+(Point lhs, Vector2d rhs) {
const& on both

https://codereview.chromium.org/11269022/diff/16001/ui/gfx/point.h#newcode76
ui/gfx/point.h:76: inline Point operator-(Point lhs, Vector2d rhs) {
const& on both

https://codereview.chromium.org/11269022/diff/16001/ui/gfx/point.h#newcode80
ui/gfx/point.h:80: inline Vector2d operator-(Point lhs, Point rhs) {
const& on both

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

https://codereview.chromium.org/11269022/diff/16001/ui/gfx/point_f.h#newcode43
ui/gfx/point_f.h:43: inline PointF operator+(PointF lhs, Vector2dF rhs) {
const& on both

https://codereview.chromium.org/11269022/diff/16001/ui/gfx/point_f.h#newcode47
ui/gfx/point_f.h:47: inline PointF operator-(PointF lhs, Vector2dF rhs) {
const& on both

https://codereview.chromium.org/11269022/diff/16001/ui/gfx/point_f.h#newcode51
ui/gfx/point_f.h:51: inline Vector2dF operator-(PointF lhs, PointF rhs) {
const& on both

[tfarina, 2012-10-26 20:26:13]

On Fri, Oct 26, 2012 at 1:52 PM, Dana Jansens <danakj@chromium.org> wrote:
> On Fri, Oct 26, 2012 at 8:45 AM,  <vollick@chromium.org> wrote:
>> On 2012/10/26 04:36:53, danakj wrote:
>>
>>> On Oct 25, 2012 10:48 PM, <mailto:danakj@chromium.org> wrote:
>>> >
>>> > I used this for a bunch more porting found these missing, so I added
>>> > them
>>> here
>>> > with tests.
>>> >
>>> > - Vector2dF::scale(s)
>>> > - Vector2dF::scale(x, y)
>>
>>
>>> > - Rect::Rect(point, vector2d)
>>> > - RectF::RectF(pointF, vector2dF)
>>
>>
>>> On second thought these constructors aren't a good idea since vectors can
>>> be negative. I'll make a vector->size conversion instead that does abs().
>>
>>
>> Perhaps Rect::Rect({5, 5}, {-2, -2}) = { origin={3, 3}, bounds={2, 2} }?
>
> What I wanted was a way to take a distance, and make a rect with
> width/height equal to the length of the x/y components of the vector.
> I think SizeOfVector2d(Vector2d) does this in a clear way, and
> requires no changing of Rect.
>
> Updated CL with this, removed the Rect constructors with vector, and
> added Vector2d::Length methods, with unit tests.

Dana, I've seem you doing a lot of stuff in a single CL. Sometimes is
just better to do just ONE thing than a bunch, one small patch is much
easier to review and to get landed.

-- 
Thiago

[Peter Kasting, 2012-10-26 20:36:18]

Talked with Scott.  The concept of a vector is something I'm fine with, and the
couple of uses I glanced at here seem appropriate.

One of Scott's fears was that we might convert everything now but have someone
else come along in the future and start (mis-)using Point or Size for something.
 My counterargument was that as long as we remove all the methods that would
allow this -- e.g. you can't add a Point to any type except a Vector -- we'd
prevent this from happening because it would be impossible to actually use Point
or Size for any other purpose than what they're named for.

[danakj, 2012-10-26 20:41:43]

On Fri, Oct 26, 2012 at 4:36 PM,  <pkasting@chromium.org> wrote:
> Talked with Scott.  The concept of a vector is something I'm fine with, and
> the
> couple of uses I glanced at here seem appropriate.
>
> One of Scott's fears was that we might convert everything now but have
> someone
> else come along in the future and start (mis-)using Point or Size for
> something.
>  My counterargument was that as long as we remove all the methods that would
> allow this -- e.g. you can't add a Point to any type except a Vector -- we'd
> prevent this from happening because it would be impossible to actually use
> Point
> or Size for any other purpose than what they're named for.

I agree with that, finding all these uses of Point as a distance was
quite easy since they no longer compiled. I think we should be careful
about new methods, cuz they might allow misuse, but if we identify
that we can remove/change those methods and uses of them will have to
fix themselves accordingly.

[danakj, 2012-10-26 20:42:56]

Thanks Sky, consts addressed.

vollick@ had suggested to me that "Rect BoundingRect(Point, Point)" might make
more sense than "Size SizeOfVector(Vector)", but they are about equivalent so I
don't feel strongly either way. I leave that to reviewers.

On 2012/10/26 20:26:13, tfarina wrote:
> Dana, I've seem you doing a lot of stuff in a single CL. Sometimes is
> just better to do just ONE thing than a bunch, one small patch is much
> easier to review and to get landed.

Sure Thiago, thanks. I think you'll find the changes to ui/gfx quite small, but
the fallout from making API changes used so broadly makes the patches become
much larger. I think that trying to roll out an API change like this across
multiple CLs would be pretty difficult however.

[Peter Kasting, 2012-10-26 20:50:46]

On 2012/10/26 20:42:56, danakj wrote:
> Thanks Sky, consts addressed.
> 
> vollick@ had suggested to me that "Rect BoundingRect(Point, Point)" might make
> more sense than "Size SizeOfVector(Vector)", but they are about equivalent so
I
> don't feel strongly either way. I leave that to reviewers.

I am not a fan of SizeOfVector().  The only meaningful "size" for a vector is an
integral length.  This function seems more like a fancy name for a reinterpret
cast, and I fear it will allow people to write code that uses the wrong
conceptual types for what they want.

OTOH, BoundingRect(Point, Point) seems like a perfectly reasonable operation to
perform.  If adding this allows us to avoid the other, my preference is for
that.

[danakj, 2012-10-26 21:03:04]

On 2012/10/26 20:50:46, Peter Kasting wrote:
> On 2012/10/26 20:42:56, danakj wrote:
> > Thanks Sky, consts addressed.
> > 
> > vollick@ had suggested to me that "Rect BoundingRect(Point, Point)" might
make
> > more sense than "Size SizeOfVector(Vector)", but they are about equivalent
so
> I
> > don't feel strongly either way. I leave that to reviewers.
> 
> I am not a fan of SizeOfVector().  The only meaningful "size" for a vector is
an
> integral length.  This function seems more like a fancy name for a reinterpret
> cast, and I fear it will allow people to write code that uses the wrong
> conceptual types for what they want.
> 
> OTOH, BoundingRect(Point, Point) seems like a perfectly reasonable operation
to
> perform.  If adding this allows us to avoid the other, my preference is for
> that.

Sounds good, thanks. There's nothing vector-specific about BoundingRect, I'll do
that separately. I've removed SizeOfVector().

[sky, 2012-10-26 21:37:49]

I didn't make it all the way through this review. I feel like there are plenty
of cases where we really want to add two points or subtract two points and get a
point (think conversion between two coordinate systems). Having to convert
to/from a vector2d feels cumbersome. And having to know about the extra class
adds to the cognitive load.

For example:

void RunMoveLoop(const gfx::Point& drag_offset);

to

void RunMoveLoop(const gfx::Vector2d& drag_offset);

This is an example of where the offset is relative to a particular coordinate
system. I don't feel Vector2d is a better usage here.

Here's another example. We're going from Canvas::Translate(Point) to
Canvas::Translate(Vector). This means I can't do something as simple as:

canvas->Translate(child_view->bounds().origin();

I have to instead remember that Translate is really about a distance and so I
need to switch to a Vector2d:

canvas->Translate(child_view->bounds().DistanceFromOrigin());

Whether a Vector2d or Point makes sense for Translate (and many of these other
methods being changed) really depends upon how the caller is representing its
data. If it's something like Views, than Points make more sense since each view
is represented as a Rect.

https://codereview.chromium.org/11269022/diff/15002/ash/tooltips/tooltip_cont...
File ash/tooltips/tooltip_controller.cc (right):

https://codereview.chromium.org/11269022/diff/15002/ash/tooltips/tooltip_cont...
ash/tooltips/tooltip_controller.cc:455:
tooltip_window_->GetBoundsInScreen().DistanceFromOrigin());
This feels unnatural. In this case we really want to add two points and use the
resulting point.

https://codereview.chromium.org/11269022/diff/15002/ash/wm/gestures/long_pres...
File ash/wm/gestures/long_press_affordance_handler.cc (right):

https://codereview.chromium.org/11269022/diff/15002/ash/wm/gestures/long_pres...
ash/wm/gestures/long_press_affordance_handler.cc:206:
canvas->Translate(gfx::Vector2d(center.x(), center.y()));
This too feels cumbersome. If we're saying a distance is represented by a Vector
shouldn't |center| be a vector?

https://codereview.chromium.org/11269022/diff/15002/chrome/browser/ui/gtk/tab...
File chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc (right):

https://codereview.chromium.org/11269022/diff/15002/chrome/browser/ui/gtk/tab...
chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc:510: gfx::Vector2d
tabstrip_offset =
This results in a lot harder to read code.

[Peter Kasting, 2012-10-26 21:50:11]

I think we need to be careful to be as simple and clear as possible when
translating.  Vector2d _can_ result in less-clear code, but IMO it doesn't have
to.  I also just have a different take on this than Scott sometimes.

On 2012/10/26 21:37:49, sky wrote:
> For example:
> 
> void RunMoveLoop(const gfx::Point& drag_offset);
> 
> to
> 
> void RunMoveLoop(const gfx::Vector2d& drag_offset);
> 
> This is an example of where the offset is relative to a particular coordinate
> system. I don't feel Vector2d is a better usage here.

Without additional context, it's hard for me to see why a Point is better here.

> Here's another example. We're going from Canvas::Translate(Point) to
> Canvas::Translate(Vector). This means I can't do something as simple as:
> 
> canvas->Translate(child_view->bounds().origin();
> 
> I have to instead remember that Translate is really about a distance and so I
> need to switch to a Vector2d:
> 
> canvas->Translate(child_view->bounds().DistanceFromOrigin());

This seems clearly better to me.

https://codereview.chromium.org/11269022/diff/15002/ash/tooltips/tooltip_cont...
File ash/tooltips/tooltip_controller.cc (right):

https://codereview.chromium.org/11269022/diff/15002/ash/tooltips/tooltip_cont...
ash/tooltips/tooltip_controller.cc:455:
tooltip_window_->GetBoundsInScreen().DistanceFromOrigin());
On 2012/10/26 21:37:50, sky wrote:
> This feels unnatural. In this case we really want to add two points and use
the
> resulting point.

I disagree.  The new code seems better.

https://codereview.chromium.org/11269022/diff/15002/ash/wm/gestures/long_pres...
File ash/wm/gestures/long_press_affordance_handler.cc (right):

https://codereview.chromium.org/11269022/diff/15002/ash/wm/gestures/long_pres...
ash/wm/gestures/long_press_affordance_handler.cc:206:
canvas->Translate(gfx::Vector2d(center.x(), center.y()));
On 2012/10/26 21:37:50, sky wrote:
> This too feels cumbersome. If we're saying a distance is represented by a
Vector
> shouldn't |center| be a vector?

Seems like this should be:

  canvas->Translate(center.DistanceFromOrigin());

https://codereview.chromium.org/11269022/diff/15002/chrome/browser/ui/gtk/tab...
File chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc (right):

https://codereview.chromium.org/11269022/diff/15002/chrome/browser/ui/gtk/tab...
chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc:510: gfx::Vector2d
tabstrip_offset =
On 2012/10/26 21:37:50, sky wrote:
> This results in a lot harder to read code.

I agree, but not because of Vector2d, just because of clumsy conversion.  IMO
this should have been written as:

  gfx::Point tabstrip_screen_point(
      ui::GetWidgetScreenPosition(tabstrip->tabstrip_.get()));
  return screen_point - tabstrip_screen_point.DistanceFromOrigin();

(Optionally, one could inline everything into the return statement.)

This becomes even simpler if we change GetWidgetScreenPosition() to
GetWidgetScreenOffset() and have it return a Vector2d -- which, I suspect, is
what every caller is really going to want.

[Peter Kasting, 2012-10-26 21:51:59]

Oh, one other thing.  "Distance" may be a bad word to use with arguments or
return values that are vectors, as it sounds like a scalar (a la "length").  You
may want something like "offset" instead that doesn't sound as misleading.

[danakj, 2012-10-26 21:59:43]

Agree about Offset for the name, thanks. I'll switch that and make these two
changes.

As to needing to call "OffsetFromOrigin()" vs "Origin()" I'm not sure it's more
cognitive load so much as it's just different from how you'd do it now..? Right
now you have to switch from a Rect to a Point. After, you'd switch from a Rect
to a Vector, either way you can't just Add(the rect itself).

I don't feel religious about using Vectors instead of Points, but it seemed like
it would make more robust code.

https://codereview.chromium.org/11269022/diff/15002/ash/wm/gestures/long_pres...
File ash/wm/gestures/long_press_affordance_handler.cc (right):

https://codereview.chromium.org/11269022/diff/15002/ash/wm/gestures/long_pres...
ash/wm/gestures/long_press_affordance_handler.cc:206:
canvas->Translate(gfx::Vector2d(center.x(), center.y()));
On 2012/10/26 21:50:12, Peter Kasting wrote:
> On 2012/10/26 21:37:50, sky wrote:
> > This too feels cumbersome. If we're saying a distance is represented by a
> Vector
> > shouldn't |center| be a vector?
> 
> Seems like this should be:
> 
>   canvas->Translate(center.DistanceFromOrigin());

Ya certainly, or just have

gfx::Vector2d centerOffset(width / 2, height / 2); and then pass that directly.

https://codereview.chromium.org/11269022/diff/15002/chrome/browser/ui/gtk/tab...
File chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc (right):

https://codereview.chromium.org/11269022/diff/15002/chrome/browser/ui/gtk/tab...
chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc:510: gfx::Vector2d
tabstrip_offset =
On 2012/10/26 21:50:12, Peter Kasting wrote:
> On 2012/10/26 21:37:50, sky wrote:
> > This results in a lot harder to read code.
> 
> I agree, but not because of Vector2d, just because of clumsy conversion.  IMO
> this should have been written as:
> 
>   gfx::Point tabstrip_screen_point(
>       ui::GetWidgetScreenPosition(tabstrip->tabstrip_.get()));
>   return screen_point - tabstrip_screen_point.DistanceFromOrigin();
> 
> (Optionally, one could inline everything into the return statement.)
> 
> This becomes even simpler if we change GetWidgetScreenPosition() to
> GetWidgetScreenOffset() and have it return a Vector2d -- which, I suspect, is
> what every caller is really going to want.

I'll look into that, thanks.

There's probably a few clumsy conversions in here.. I tried to be
careful/meticulous but clearly it can be improved.

[Peter Kasting, 2012-10-26 22:04:05]

On 2012/10/26 21:59:43, danakj wrote:
> I don't feel religious about using Vectors instead of Points, but it seemed
like
> it would make more robust code.

I don't like the world where we continue to allow all the existing operations
(e.g. Point + Point).  This opens us to wildly inconsistent use of vectors and
points everywhere.

It seems much better to be strict about this conversion.  I really don't think
most places are more confusing -- in fact some I find less so -- once you get
past the habit of having used Points for this sort of thing in the past.  In
some sense, as the primary views maintainer, Scott is more affected by this than
any other Chromium contributor would be.

https://codereview.chromium.org/11269022/diff/15002/ash/wm/gestures/long_pres...
File ash/wm/gestures/long_press_affordance_handler.cc (right):

https://codereview.chromium.org/11269022/diff/15002/ash/wm/gestures/long_pres...
ash/wm/gestures/long_press_affordance_handler.cc:206:
canvas->Translate(gfx::Vector2d(center.x(), center.y()));
On 2012/10/26 21:59:43, danakj wrote:
> On 2012/10/26 21:50:12, Peter Kasting wrote:
> > On 2012/10/26 21:37:50, sky wrote:
> > > This too feels cumbersome. If we're saying a distance is represented by a
> > Vector
> > > shouldn't |center| be a vector?
> > 
> > Seems like this should be:
> > 
> >   canvas->Translate(center.DistanceFromOrigin());
> 
> Ya certainly, or just have
> 
> gfx::Vector2d centerOffset(width / 2, height / 2); and then pass that
directly.

The reason I didn't do this is because |center| is also used below.  Of course,
perhaps those uses would also prefer vectors, I don't know.

[tfarina, 2012-10-26 22:04:18]

http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_unittest.cc
File ui/gfx/vector2d_unittest.cc (right):

http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_unittest.c...
ui/gfx/vector2d_unittest.cc:12: namespace ui {
Looks like only the unittests you added are wrapped in 'ui' namespace:
$ for f in $(find ui/gfx/ -name "*unittest.cc*"); do git grep -n "namespace ui
{" $f; done

ui/gfx/safe_integer_conversions_unittest.cc:11:namespace ui {
ui/gfx/point_unittest.cc:11:namespace ui {
ui/gfx/rect_unittest.cc:13:namespace ui {
ui/gfx/size_unittest.cc:12:namespace ui {

But instead they should be in 'gfx' namespace:

$ for f in $(find ui/gfx/ -name "*unittest.cc*"); do git grep -n "namespace gfx
{" $f; done                                                                     
                               
ui/gfx/font_list_unittest.cc:30:namespace gfx {
ui/gfx/shadow_value_unittest.cc:10:namespace gfx {
ui/gfx/canvas_unittest.cc:10:namespace gfx {
ui/gfx/font_fallback_win_unittest.cc:8:namespace gfx {
ui/gfx/render_text_unittest.cc:21:namespace gfx {
ui/gfx/image/image_skia_unittest.cc:24:namespace gfx {
ui/gfx/platform_font_win_unittest.cc:12:namespace gfx {
ui/gfx/codec/jpeg_codec_unittest.cc:59:namespace gfx {
ui/gfx/codec/png_codec_unittest.cc:23:namespace gfx {
ui/gfx/platform_font_pango_unittest.cc:13:namespace gfx {

To be consistent with the rest of the unittests in this directory.

You may fix this one, and let the others for a follow up CL.

[danakj, 2012-10-26 22:07:58]

http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_unittest.cc
File ui/gfx/vector2d_unittest.cc (right):

http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_unittest.c...
ui/gfx/vector2d_unittest.cc:12: namespace ui {
On 2012/10/26 22:04:19, tfarina wrote:
> Looks like only the unittests you added are wrapped in 'ui' namespace:

Oh, thanks. I was assuming all tests were in ui since the ones I'd looked at
were.

[tfarina, 2012-10-26 22:22:30]

http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h
File ui/gfx/vector2d_f.h (right):

http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h#newcode5
ui/gfx/vector2d_f.h:5: // Defines a simple float vector class.  This class is
used to indicate a
I generally prefer to put this kind of comment right before the class
declaration. I don't know if Scott or Peter has similar preferences, nor if they
care about or not.

http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h#newcode56
ui/gfx/vector2d_f.h:56: float Length() const {
Could you deinline all the implementations, i.e, move all the
definitions/implementations of these "may be" not trivial functions to the
source file instead?

http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h#newcode63
ui/gfx/vector2d_f.h:63: private:
Do we allow copy and assign, right? Just checking, I think this is similar to
what done with Point class?

[danakj, 2012-10-26 22:24:51]

http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h
File ui/gfx/vector2d_f.h (right):

http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h#newcode56
ui/gfx/vector2d_f.h:56: float Length() const {
On 2012/10/26 22:22:30, tfarina wrote:
> Could you deinline all the implementations, i.e, move all the
> definitions/implementations of these "may be" not trivial functions to the
> source file instead?

I can, though I'm following the lead of point_base.h for instance, which has
them inline. So, should this differ?

http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h#newcode63
ui/gfx/vector2d_f.h:63: private:
On 2012/10/26 22:22:30, tfarina wrote:
> Do we allow copy and assign, right? Just checking, I think this is similar to
> what done with Point class?

Right, we do.

[tfarina, 2012-10-26 22:26:59]

On Fri, Oct 26, 2012 at 8:24 PM,  <danakj@chromium.org> wrote:
> I can, though I'm following the lead of point_base.h for instance, which
> has them inline. So, should this differ?
>
PointBase is a template hence the definition, there, should be in the
header file.

-- 
Thiago

[sky, 2012-10-26 22:37:00]

Old cranky stick in the mud here. For the record I prefer adding more methods to
Point than this. That said, you and Peter feel strongly about this so I'm will
to go with it. I'm signing off on this and letting Peter finish the review.

[Peter Kasting, 2012-10-26 22:44:49]

After talking with Scott some more, I suspect some of the friction here is
because, while this change fixes all the spots that don't compile any more, it
doesn't fix deeper-seated API design issues like "functions that take or return
the origin of a coordinate system should probably use vectors instead of points"
and thus we have some extra conversion friction.

The difficulty is that I don't know of an easy way to locate all such places for
fixing.  Scott, do you have any ideas for who/how we could audit the existing
design?  Perhaps in looking through this change there are functions that stand
out as examples of what we should look for specifically?

http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h
File ui/gfx/vector2d_f.h (right):

http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h#newcode5
ui/gfx/vector2d_f.h:5: // Defines a simple float vector class.  This class is
used to indicate a
On 2012/10/26 22:22:30, tfarina wrote:
> I generally prefer to put this kind of comment right before the class
> declaration. I don't know if Scott or Peter has similar preferences, nor if
they
> care about or not.

Google style actually talks about both file- and class-level comments.  In a
case like this, most people end up just writing a class-level comment that says
much of the same thing.  It's not wrong to do it this way.  I would probably
very very weakly prefer the other way just because it allows the comment to be
slightly shorter (e.g. the beginning can be condensed to "A distance in two
dimensions between points." or something).

Though to be totally honest, the meaning of "vector" is so clear that this
comment could probably be omitted entirely...

http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h#newcode56
ui/gfx/vector2d_f.h:56: float Length() const {
On 2012/10/26 22:24:52, danakj wrote:
> On 2012/10/26 22:22:30, tfarina wrote:
> > Could you deinline all the implementations, i.e, move all the
> > definitions/implementations of these "may be" not trivial functions to the
> > source file instead?
> 
> I can, though I'm following the lead of point_base.h for instance, which has
> them inline. So, should this differ?

As Thiago noted, we're forced to inline there due to templates.  When you don't
have that constraint, you should move all the non-unix_hacker()-style-accessors
to a .cc file.

[danakj, 2012-10-26 22:55:49]

I think one thing that helps a lot is renaming distance to offset. Now you do:

rect.Offset(thing.OffsetFromOrigin())

so it kind of falls out from the names.

The other thing is I had:
point - point = vector
but
point.OffsetFrom(point) = vector

Overloading point.Subtract(point) = vector like the operators makes a
number of code changes unneeded.

On Fri, Oct 26, 2012 at 6:44 PM,  <pkasting@chromium.org> wrote:
> After talking with Scott some more, I suspect some of the friction here is
> because, while this change fixes all the spots that don't compile any more,
> it
> doesn't fix deeper-seated API design issues like "functions that take or
> return
> the origin of a coordinate system should probably use vectors instead of
> points"
> and thus we have some extra conversion friction.
>
> The difficulty is that I don't know of an easy way to locate all such places
> for
> fixing.  Scott, do you have any ideas for who/how we could audit the
> existing
> design?  Perhaps in looking through this change there are functions that
> stand
> out as examples of what we should look for specifically?
>
>
>
> http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h
> File ui/gfx/vector2d_f.h (right):
>
>
http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h#newcode5
> ui/gfx/vector2d_f.h:5: // Defines a simple float vector class.  This
> class is used to indicate a
> On 2012/10/26 22:22:30, tfarina wrote:
>>
>> I generally prefer to put this kind of comment right before the class
>> declaration. I don't know if Scott or Peter has similar preferences,
>
> nor if they
>>
>> care about or not.
>
>
> Google style actually talks about both file- and class-level comments.
> In a case like this, most people end up just writing a class-level
> comment that says much of the same thing.  It's not wrong to do it this
> way.  I would probably very very weakly prefer the other way just
> because it allows the comment to be slightly shorter (e.g. the beginning
> can be condensed to "A distance in two dimensions between points." or
> something).
>
> Though to be totally honest, the meaning of "vector" is so clear that
> this comment could probably be omitted entirely...
>
>
>
http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h#newcode56
> ui/gfx/vector2d_f.h:56: float Length() const {
> On 2012/10/26 22:24:52, danakj wrote:
>>
>> On 2012/10/26 22:22:30, tfarina wrote:
>> > Could you deinline all the implementations, i.e, move all the
>> > definitions/implementations of these "may be" not trivial functions
>
> to the
>>
>> > source file instead?
>
>
>> I can, though I'm following the lead of point_base.h for instance,
>
> which has
>>
>> them inline. So, should this differ?
>
>
> As Thiago noted, we're forced to inline there due to templates.  When
> you don't have that constraint, you should move all the
> non-unix_hacker()-style-accessors to a .cc file.
>
> http://codereview.chromium.org/11269022/
>
> --
>
>

[sky, 2012-10-26 23:20:49]

On Fri, Oct 26, 2012 at 3:44 PM,  <pkasting@chromium.org> wrote:
> After talking with Scott some more, I suspect some of the friction here is
> because, while this change fixes all the spots that don't compile any more,
> it
> doesn't fix deeper-seated API design issues like "functions that take or
> return
> the origin of a coordinate system should probably use vectors instead of
> points"
> and thus we have some extra conversion friction.
>
> The difficulty is that I don't know of an easy way to locate all such places
> for
> fixing.  Scott, do you have any ideas for who/how we could audit the
> existing
> design?

I can't think of anyone that has the time now.

  -Scott

> Perhaps in looking through this change there are functions that
> stand
> out as examples of what we should look for specifically?
>
>
>
> http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h
> File ui/gfx/vector2d_f.h (right):
>
>
http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h#newcode5
> ui/gfx/vector2d_f.h:5: // Defines a simple float vector class.  This
> class is used to indicate a
> On 2012/10/26 22:22:30, tfarina wrote:
>>
>> I generally prefer to put this kind of comment right before the class
>> declaration. I don't know if Scott or Peter has similar preferences,
>
> nor if they
>>
>> care about or not.
>
>
> Google style actually talks about both file- and class-level comments.
> In a case like this, most people end up just writing a class-level
> comment that says much of the same thing.  It's not wrong to do it this
> way.  I would probably very very weakly prefer the other way just
> because it allows the comment to be slightly shorter (e.g. the beginning
> can be condensed to "A distance in two dimensions between points." or
> something).
>
> Though to be totally honest, the meaning of "vector" is so clear that
> this comment could probably be omitted entirely...
>
>
>
http://codereview.chromium.org/11269022/diff/16002/ui/gfx/vector2d_f.h#newcode56
> ui/gfx/vector2d_f.h:56: float Length() const {
> On 2012/10/26 22:24:52, danakj wrote:
>>
>> On 2012/10/26 22:22:30, tfarina wrote:
>> > Could you deinline all the implementations, i.e, move all the
>> > definitions/implementations of these "may be" not trivial functions
>
> to the
>>
>> > source file instead?
>
>
>> I can, though I'm following the lead of point_base.h for instance,
>
> which has
>>
>> them inline. So, should this differ?
>
>
> As Thiago noted, we're forced to inline there due to templates.  When
> you don't have that constraint, you should move all the
> non-unix_hacker()-style-accessors to a .cc file.
>
> http://codereview.chromium.org/11269022/

[danakj, 2012-10-26 23:30:33]

PTAL

http://codereview.chromium.org/11269022/diff/15002/chrome/browser/ui/gtk/tabs...
File chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc (right):

http://codereview.chromium.org/11269022/diff/15002/chrome/browser/ui/gtk/tabs...
chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc:510: gfx::Vector2d
tabstrip_offset =
On 2012/10/26 21:50:12, Peter Kasting wrote:
> This becomes even simpler if we change GetWidgetScreenPosition() to
> GetWidgetScreenOffset() and have it return a Vector2d -- which, I suspect, is
> what every caller is really going to want.

This certainly does clean up things, such as ConvertWidgetPointToScreen in
gtk_util.cc.

The one place that actually wants a point, is duplicating code from
GetWidgetScreenRect, so switched it to that.

http://codereview.chromium.org/11269022/diff/16002/chrome/browser/ui/window_s...
File chrome/browser/ui/window_snapshot/window_snapshot_aura.cc (right):

http://codereview.chromium.org/11269022/diff/16002/chrome/browser/ui/window_s...
chrome/browser/ui/window_snapshot/window_snapshot_aura.cc:29:
snapshot_bounds.origin().Add(window->bounds().DistanceFromOrigin()));
This was a naive conversion, made this more clear.

http://codereview.chromium.org/11269022/diff/16002/ui/base/dragdrop/drag_utils.h
File ui/base/dragdrop/drag_utils.h (right):

http://codereview.chromium.org/11269022/diff/16002/ui/base/dragdrop/drag_util...
ui/base/dragdrop/drag_utils.h:42: const gfx::Vector2d& cursor_offset,
I think this change was maybe a bad idea, this should be a point inside the
image. The variable name should change instead.

Actually there's no callers of cursor_offset() so renaming it to cursor_pos()
and leaving it alone type-wise.

http://codereview.chromium.org/11269022/diff/16002/ui/views/view.cc
File ui/views/view.cc (right):

http://codereview.chromium.org/11269022/diff/16002/ui/views/view.cc#newcode626
ui/views/view.cc:626:
root->GetWidget()->GetClientAreaBoundsInScreen().DistanceFromOrigin());
We probably want something like GetClientAreaOffsetInScreen() on Widget..

[Peter Kasting, 2012-10-26 23:50:15]

http://codereview.chromium.org/11269022/diff/16002/ui/views/view.cc
File ui/views/view.cc (right):

http://codereview.chromium.org/11269022/diff/16002/ui/views/view.cc#newcode626
ui/views/view.cc:626:
root->GetWidget()->GetClientAreaBoundsInScreen().DistanceFromOrigin());
On 2012/10/26 23:30:34, danakj wrote:
> We probably want something like GetClientAreaOffsetInScreen() on Widget..

Yes.  I think pretty much any function which has "InScreen" or "ToScreen" or
"FromScreen" anywhere are probably all candidates to convert.  For Widget, if we
can access the client area offset and the client bounds size (or the client
bounds rect in client coordinates, i.e. starting at the origin), we can compute
the client bounds rect in screen coordinates where needed.

[danakj, 2012-10-27 00:35:32]

Rebased.

http://codereview.chromium.org/11269022/diff/23302/ash/display/multi_display_...
File ash/display/multi_display_manager.cc (left):

http://codereview.chromium.org/11269022/diff/23302/ash/display/multi_display_...
ash/display/multi_display_manager.cc:521: int64 distance = diff.x() * diff.x() +
diff.y() * diff.y();
Was able to make use of LengthSquared() here.

[Peter Kasting, 2012-10-27 01:01:39]

Had to run before I made it any further than
chrome/browser/ui/gtk/download/download_shelf_gtk.cc .

Feel free to look for more opportunities to inline and otherwise
simplify/shorten code.

Otherwise I will pick this back up Monday.

http://codereview.chromium.org/11269022/diff/23302/ash/display/multi_display_...
File ash/display/multi_display_manager.cc (right):

http://codereview.chromium.org/11269022/diff/23302/ash/display/multi_display_...
ash/display/multi_display_manager.cc:517: gfx::Vector2d diff =
center.Subtract(location_in_screen);
Nit: Inline into next statement

http://codereview.chromium.org/11269022/diff/23302/ash/launcher/overflow_bubb...
File ash/launcher/overflow_bubble.cc (right):

http://codereview.chromium.org/11269022/diff/23302/ash/launcher/overflow_bubb...
ash/launcher/overflow_bubble.cc:169: const gfx::Point
origin(gfx::PointAtOffsetFromOrigin(-scroll_offset_));
Nit: Inline into next statement

http://codereview.chromium.org/11269022/diff/23302/ash/tooltips/tooltip_contr...
File ash/tooltips/tooltip_controller.cc (right):

http://codereview.chromium.org/11269022/diff/23302/ash/tooltips/tooltip_contr...
ash/tooltips/tooltip_controller.cc:453: gfx::Point widget_loc = curr_mouse_loc_;
Nit: Inline both of these statements into the SetText() call

http://codereview.chromium.org/11269022/diff/23302/ash/tooltips/tooltip_contr...
ash/tooltips/tooltip_controller.cc:455:
tooltip_window_->GetBoundsInScreen().OffsetFromOrigin());
Can we replace GetBoundsInScreen() with a function that returns a vector
directly?

http://codereview.chromium.org/11269022/diff/23302/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

http://codereview.chromium.org/11269022/diff/23302/cc/gl_renderer.cc#newcode696
cc/gl_renderer.cc:696: gfx::Vector2dF clampOffset = clampRect.origin() -
tileRect.origin();
Nit: If you inline into next statement, the tileRect.origin()s will cancel.

http://codereview.chromium.org/11269022/diff/23302/chrome/browser/automation/...
File chrome/browser/automation/testing_automation_provider_gtk.cc (right):

http://codereview.chromium.org/11269022/diff/23302/chrome/browser/automation/...
chrome/browser/automation/testing_automation_provider_gtk.cc:42: y = offset.y();
Nit: This causes us to translate back and forth from gint.  How about:

    if (screen_coordinates) {
      bounds->set_origin(gfx::Point() + ui::GetWidgetScreenOffset(widget));
    } else {
      gint x, y;
      gtk_widget_translate_coordinates(widget, GTK_WIDGET(window), 0, 0, &x,
                                       &y);
      bounds->set_origin(gfx::Point(x, y));
    }

http://codereview.chromium.org/11269022/diff/23302/chrome/browser/notificatio...
File chrome/browser/notifications/balloon.h (right):

http://codereview.chromium.org/11269022/diff/23302/chrome/browser/notificatio...
chrome/browser/notifications/balloon.h:66: const gfx::Vector2d& offset() {
return offset_;}
Nit: method is const

http://codereview.chromium.org/11269022/diff/23302/chrome/browser/notificatio...
File chrome/browser/notifications/balloon_collection_impl.cc (right):

http://codereview.chromium.org/11269022/diff/23302/chrome/browser/notificatio...
chrome/browser/notifications/balloon_collection_impl.cc:252:
(*it)->set_offset(gfx::Vector2d(0, 0));
Nit: 0, 0 can be removed

[danakj, 2012-10-29 19:17:19]

I think our best API change would be to add a GetOffsetInScreen() method and
CenterOffset() method.

Then go through all uses of the current methods, and audit if they should switch
or not. Personally, I think this CL is huge enough and I'd rather do that
separately though.

http://codereview.chromium.org/11269022/diff/23302/ash/display/multi_display_...
File ash/display/multi_display_manager.cc (right):

http://codereview.chromium.org/11269022/diff/23302/ash/display/multi_display_...
ash/display/multi_display_manager.cc:517: gfx::Vector2d diff =
center.Subtract(location_in_screen);
On 2012/10/27 01:01:39, Peter Kasting wrote:
> Nit: Inline into next statement

Done.

http://codereview.chromium.org/11269022/diff/23302/ash/launcher/overflow_bubb...
File ash/launcher/overflow_bubble.cc (right):

http://codereview.chromium.org/11269022/diff/23302/ash/launcher/overflow_bubb...
ash/launcher/overflow_bubble.cc:169: const gfx::Point
origin(gfx::PointAtOffsetFromOrigin(-scroll_offset_));
On 2012/10/27 01:01:39, Peter Kasting wrote:
> Nit: Inline into next statement

Done.

http://codereview.chromium.org/11269022/diff/23302/ash/tooltips/tooltip_contr...
File ash/tooltips/tooltip_controller.cc (right):

http://codereview.chromium.org/11269022/diff/23302/ash/tooltips/tooltip_contr...
ash/tooltips/tooltip_controller.cc:453: gfx::Point widget_loc = curr_mouse_loc_;
On 2012/10/27 01:01:39, Peter Kasting wrote:
> Nit: Inline both of these statements into the SetText() call

Done.

http://codereview.chromium.org/11269022/diff/23302/ash/tooltips/tooltip_contr...
ash/tooltips/tooltip_controller.cc:455:
tooltip_window_->GetBoundsInScreen().OffsetFromOrigin());
On 2012/10/27 01:01:39, Peter Kasting wrote:
> Can we replace GetBoundsInScreen() with a function that returns a vector
> directly?

I don't think we want to replace the method. There's a lot of callers of things
like:
GetBoundsInScreen().CenterPoint()
GetBoundsInScreen().Contains(...)
Intersects(GetBoundsInScreen())

We could add a method along side it, however. Shall we do that in this CL
though? It doesn't seem intrinsic to the change to me, since there are so many
callers of this, and there's no GetPositionInScreen() at the moment to convert.

http://codereview.chromium.org/11269022/diff/23302/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

http://codereview.chromium.org/11269022/diff/23302/cc/gl_renderer.cc#newcode696
cc/gl_renderer.cc:696: gfx::Vector2dF clampOffset = clampRect.origin() -
tileRect.origin();
On 2012/10/27 01:01:39, Peter Kasting wrote:
> Nit: If you inline into next statement, the tileRect.origin()s will cancel.

Done.

http://codereview.chromium.org/11269022/diff/23302/chrome/browser/automation/...
File chrome/browser/automation/testing_automation_provider_gtk.cc (right):

http://codereview.chromium.org/11269022/diff/23302/chrome/browser/automation/...
chrome/browser/automation/testing_automation_provider_gtk.cc:42: y = offset.y();
On 2012/10/27 01:01:39, Peter Kasting wrote:
> Nit: This causes us to translate back and forth from gint.  How about:
> 
>     if (screen_coordinates) {
>       bounds->set_origin(gfx::Point() + ui::GetWidgetScreenOffset(widget));
>     } else {
>       gint x, y;
>       gtk_widget_translate_coordinates(widget, GTK_WIDGET(window), 0, 0, &x,
>                                        &y);
>       bounds->set_origin(gfx::Point(x, y));
>     }

Done.

http://codereview.chromium.org/11269022/diff/23302/chrome/browser/notificatio...
File chrome/browser/notifications/balloon.h (right):

http://codereview.chromium.org/11269022/diff/23302/chrome/browser/notificatio...
chrome/browser/notifications/balloon.h:66: const gfx::Vector2d& offset() {
return offset_;}
On 2012/10/27 01:01:39, Peter Kasting wrote:
> Nit: method is const

Done.

http://codereview.chromium.org/11269022/diff/23302/chrome/browser/notificatio...
File chrome/browser/notifications/balloon_collection_impl.cc (right):

http://codereview.chromium.org/11269022/diff/23302/chrome/browser/notificatio...
chrome/browser/notifications/balloon_collection_impl.cc:252:
(*it)->set_offset(gfx::Vector2d(0, 0));
On 2012/10/27 01:01:39, Peter Kasting wrote:
> Nit: 0, 0 can be removed

Done.

http://codereview.chromium.org/11269022/diff/29001/chrome/browser/ui/gtk/gtk_...
File chrome/browser/ui/gtk/gtk_util.cc (right):

http://codereview.chromium.org/11269022/diff/29001/chrome/browser/ui/gtk/gtk_...
chrome/browser/ui/gtk/gtk_util.cc:488: *p = p->Add(offset);
inlined.

http://codereview.chromium.org/11269022/diff/29001/chrome/browser/ui/gtk/tabs...
File chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc (right):

http://codereview.chromium.org/11269022/diff/29001/chrome/browser/ui/gtk/tabs...
chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc:820: gfx::Vector2d
offset = ui::GetWidgetScreenOffset(parent);
inlined

http://codereview.chromium.org/11269022/diff/29001/content/browser/accessibil...
File content/browser/accessibility/browser_accessibility.cc (right):

http://codereview.chromium.org/11269022/diff/29001/content/browser/accessibil...
content/browser/accessibility/browser_accessibility.cc:154: gfx::Vector2d
view_offset = manager_->GetViewBounds().OffsetFromOrigin();
inlined.

http://codereview.chromium.org/11269022/diff/29001/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/11269022/diff/29001/content/renderer/render_wi...
content/renderer/render_widget.cc:1613: return gfx::Vector2d(0, 0);
dropped the 0,0.

http://codereview.chromium.org/11269022/diff/29001/ui/base/dragdrop/drag_util...
File ui/base/dragdrop/drag_utils.cc (right):

http://codereview.chromium.org/11269022/diff/29001/ui/base/dragdrop/drag_util...
ui/base/dragdrop/drag_utils.cc:96: gfx::Vector2d cursor_offset(size.width() / 2,
kLinkDragImageVPadding);
Left this, inlining it makes it into a 4-line function all, and having a
variable name here helps document what the vector is representing.

http://codereview.chromium.org/11269022/diff/29001/webkit/plugins/ppapi/ppb_g...
File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right):

http://codereview.chromium.org/11269022/diff/29001/webkit/plugins/ppapi/ppb_g...
webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:711: clip, gfx::Vector2d(dx, dy));
fits on one line

[Peter Kasting, 2012-10-29 23:06:32]

(Sending comments up to but not including browser_view.cc since you're uploading
a new patch)

http://codereview.chromium.org/11269022/diff/32016/ash/display/multi_display_...
File ash/display/multi_display_manager.cc (right):

http://codereview.chromium.org/11269022/diff/32016/ash/display/multi_display_...
ash/display/multi_display_manager.cc:529: int64 distance = (center -
location_in_screen).LengthSquared();
Nit: Rename |distance_squared| (and maybe also |closest_distance_squared|). 
Maybe the comment should note "Note: Using LengthSquared() is cheaper than using
Length(), and fine since we only care about relative distances, not absolute
ones."

http://codereview.chromium.org/11269022/diff/32016/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

http://codereview.chromium.org/11269022/diff/32016/cc/gl_renderer.cc#newcode696
cc/gl_renderer.cc:696: gfx::PointF textureOffset = quad->textureOffset() +
clampRect.OffsetFromOrigin() - quad->quadRect().OffsetFromOrigin();
Nit: 80 columns

http://codereview.chromium.org/11269022/diff/32016/chrome/browser/ui/gtk/down...
File chrome/browser/ui/gtk/download/download_shelf_gtk.cc (right):

http://codereview.chromium.org/11269022/diff/32016/chrome/browser/ui/gtk/down...
chrome/browser/ui/gtk/download/download_shelf_gtk.cc:386: gfx::Rect bounds =
ui::GetWidgetScreenBounds(shelf_.get());
I don't know Gtk enough to ensure this does the same thing as the old code.

http://codereview.chromium.org/11269022/diff/32016/chrome/browser/ui/gtk/tabs...
File chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc (right):

http://codereview.chromium.org/11269022/diff/32016/chrome/browser/ui/gtk/tabs...
chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc:509:
ui::GetWidgetScreenOffset(tabstrip->tabstrip_.get());
Nit: Inline into next statement

http://codereview.chromium.org/11269022/diff/32016/chrome/browser/ui/gtk/tabs...
File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right):

http://codereview.chromium.org/11269022/diff/32016/chrome/browser/ui/gtk/tabs...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:104: gfx::Size(allocation.width,
allocation.height));
Nit: Assuming your earlier use of GetWidgetScreenBounds works correctly, seems
like this could use a similar trick to be condensed to something like:

  gfx::Rect bounds(ui::GetWidgetScreenBounds(widget));
  bounds.Offset(-ui::GetWidgetScreenOffset(parent));
  return bounds;

(Collapses to one line once we have Rect:operator+())

[danakj, 2012-10-29 23:21:40]

http://codereview.chromium.org/11269022/diff/32016/ash/display/multi_display_...
File ash/display/multi_display_manager.cc (right):

http://codereview.chromium.org/11269022/diff/32016/ash/display/multi_display_...
ash/display/multi_display_manager.cc:529: int64 distance = (center -
location_in_screen).LengthSquared();
On 2012/10/29 23:06:33, Peter Kasting wrote:
> Nit: Rename |distance_squared| (and maybe also |closest_distance_squared|). 
> Maybe the comment should note "Note: Using LengthSquared() is cheaper than
using
> Length(), and fine since we only care about relative distances, not absolute
> ones."

Done.

http://codereview.chromium.org/11269022/diff/32016/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

http://codereview.chromium.org/11269022/diff/32016/cc/gl_renderer.cc#newcode696
cc/gl_renderer.cc:696: gfx::PointF textureOffset = quad->textureOffset() +
clampRect.OffsetFromOrigin() - quad->quadRect().OffsetFromOrigin();
On 2012/10/29 23:06:33, Peter Kasting wrote:
> Nit: 80 columns

Heh, we're not doing that in cc/ yet, it was pretty lucky that the old code
decided to split before 80 columns. Think this should remain one line to match
the dir, as we'll batch convert cc/ later.

http://codereview.chromium.org/11269022/diff/32016/chrome/browser/ui/gtk/down...
File chrome/browser/ui/gtk/download/download_shelf_gtk.cc (right):

http://codereview.chromium.org/11269022/diff/32016/chrome/browser/ui/gtk/down...
chrome/browser/ui/gtk/download/download_shelf_gtk.cc:386: gfx::Rect bounds =
ui::GetWidgetScreenBounds(shelf_.get());
On 2012/10/29 23:06:33, Peter Kasting wrote:
> I don't know Gtk enough to ensure this does the same thing as the old code.

New code calls this:

gfx::Rect GetWidgetScreenBounds(GtkWidget* widget) {
  gfx::Point position = PointAtOffsetFromOrigin(GetWidgetScreenOffset(widget));

  GtkAllocation allocation;
  gtk_widget_get_allocation(widget, &allocation);

  return gfx::Rect(position.x(), position.y(),
                   allocation.width, allocation.height);
}

This is the same as what we're deleting here % s/position/offset/

http://codereview.chromium.org/11269022/diff/32016/chrome/browser/ui/gtk/tabs...
File chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc (right):

http://codereview.chromium.org/11269022/diff/32016/chrome/browser/ui/gtk/tabs...
chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc:509:
ui::GetWidgetScreenOffset(tabstrip->tabstrip_.get());
On 2012/10/29 23:06:33, Peter Kasting wrote:
> Nit: Inline into next statement

Done.

http://codereview.chromium.org/11269022/diff/32016/chrome/browser/ui/gtk/tabs...
File chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc (right):

http://codereview.chromium.org/11269022/diff/32016/chrome/browser/ui/gtk/tabs...
chrome/browser/ui/gtk/tabs/tab_renderer_gtk.cc:104: gfx::Size(allocation.width,
allocation.height));
On 2012/10/29 23:06:33, Peter Kasting wrote:
> Nit: Assuming your earlier use of GetWidgetScreenBounds works correctly, seems
> like this could use a similar trick to be condensed to something like:
> 
>   gfx::Rect bounds(ui::GetWidgetScreenBounds(widget));
>   bounds.Offset(-ui::GetWidgetScreenOffset(parent));
>   return bounds;
> 
> (Collapses to one line once we have Rect:operator+())

Done.

[danakj, 2012-10-29 23:26:05]

The other changes were:

content/browser/accessibility/browser_accessibility_win.cc
BrowserAccessibilityWin::scrollToPoint

content/browser/renderer_host/render_widget_host_view_gtk.cc
RenderWidgetHostViewGtk::CopyFromCompositingSurface

content/browser/renderer_host/render_widget_host_view_win.cc
RenderWidgetHostViewWin::Redraw

ui/views/view.cc
View::ConvertPointToScreen
View::ConvertPointFromScreen

[Peter Kasting, 2012-10-30 01:14:14]

http://codereview.chromium.org/11269022/diff/5049/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

http://codereview.chromium.org/11269022/diff/5049/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:505: gfx::Vector2d
dragged_view_offset = GetWindowOffset(point_in_screen);
Nit: Inline

http://codereview.chromium.org/11269022/diff/5049/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:1300: gfx::Vector2d
dragged_view_offset = GetWindowOffset(point_in_screen);
Nit: Inline

http://codereview.chromium.org/11269022/diff/5049/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:2038: return
point.OffsetFromOrigin();
I wonder if view.h should change from having all the current
ConvertPoint{From,To}XXX() functions to simply having some kind of
GetOffsetInScreen() function.  Then code like this would just be "return
point_in_screen - toplevel_view->GetOffsetInScreen();".  And most other places
we convert could be changed similarly.

We might need to have a GetOffsetFromWidget() function too. 
(GetOffsetFromParent() wouldn't be necessary because that's just
bounds().OffsetFromOrigin().)

If this makes sense it would probably be better as a separate change.

http://codereview.chromium.org/11269022/diff/5049/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/11269022/diff/5049/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:797: } else if
(coordinate_type == IA2_COORDTYPE_PARENT_RELATIVE) {
Nit: Can save one line by reversing this condition and returning, then combining
the subsequent "else" and "if (parent_)" into one "else if"

http://codereview.chromium.org/11269022/diff/5049/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_embedder_helper.cc (right):

http://codereview.chromium.org/11269022/diff/5049/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_embedder_helper.cc:95: if
(rvh->GetView())
Nit: {}

http://codereview.chromium.org/11269022/diff/5049/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/11269022/diff/5049/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:467:
clip.Offset(moves[i].window_rect.OffsetFromOrigin());
Nit: Combine these two Offset() calls (2 places)

http://codereview.chromium.org/11269022/diff/5049/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:470:
clip.Offset(-moves[i].window_rect.x(), -moves[i].window_rect.y());
Nit: -moves[i].window_rect.OffsetFromOrigin()

http://codereview.chromium.org/11269022/diff/5049/content/browser/web_content...
File content/browser/web_contents/web_contents_view_mac.mm (right):

http://codereview.chromium.org/11269022/diff/5049/content/browser/web_content...
content/browser/web_contents/web_contents_view_mac.mm:189: gfx::Point
image_point = gfx::PointAtOffsetFromOrigin(image_offset);
Nit: Inline

http://codereview.chromium.org/11269022/diff/5049/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/11269022/diff/5049/content/renderer/render_vie...
content/renderer/render_view_impl.cc:2168: end_rect.bottom());
Nit: Can inline these two points

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
File ui/aura/test/event_generator.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
ui/aura/test/event_generator.cc:132: gfx::Vector2d(diff.x() / count * i,
diff.y() / count * i));
Do you think this suggests we should support multiplication and division
operators on vectors to scale them?

http://codereview.chromium.org/11269022/diff/5049/ui/base/dragdrop/drag_utils.cc
File ui/base/dragdrop/drag_utils.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/base/dragdrop/drag_utils...
ui/base/dragdrop/drag_utils.cc:96: gfx::Vector2d cursor_offset(size.width() / 2,
kLinkDragImageVPadding);
Nit: Inline

http://codereview.chromium.org/11269022/diff/5049/ui/base/dragdrop/os_exchang...
File ui/base/dragdrop/os_exchange_data_provider_gtk.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/base/dragdrop/os_exchang...
ui/base/dragdrop/os_exchange_data_provider_gtk.cc:225: GdkPixbuf* drag_image,
const gfx::Vector2d& cursor_offset) {
Nit: One arg per line

http://codereview.chromium.org/11269022/diff/5049/ui/base/gestures/gesture_re...
File ui/base/gestures/gesture_recognizer_impl.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/base/gestures/gesture_re...
ui/base/gestures/gesture_recognizer_impl.cc:136: int distance = delta.x() *
delta.x() + delta.y() * delta.y();
Nit: Use LengthSquared() and add comments/change names as you did earlier in
this CL.

http://codereview.chromium.org/11269022/diff/5049/ui/base/gtk/gtk_screen_util.cc
File ui/base/gtk/gtk_screen_util.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/base/gtk/gtk_screen_util...
ui/base/gtk/gtk_screen_util.cc:51: gfx::Point position =
PointAtOffsetFromOrigin(GetWidgetScreenOffset(widget));
Nit: Seems like we don't need to convert to a point, we could just call
Vector::x() and y() below.

Or alternately, we could do something like:

  GtkAllocation allocation;
  gtk_widget_get_allocation(widget, &allocation);
  return gfx::Rect(PointAtOffsetFromOrigin(GetWidgetScreenOffset(widget)),
                   gfx::Size(allocation.width, allocation.height));

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/rect_base.h
File ui/gfx/rect_base.h (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/rect_base.h#newcode131
ui/gfx/rect_base.h:131: VectorClass CenterOffset() const {
I'm not convinced of the utility of this and the rest of your change doesn't
seem to use it.  Let's leave it out until a future changelist wants something
like this.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:862: if (horizontal_alignment() != ALIGN_LEFT) {
Nit: Can eliminate {} by reversing condition and early-returning

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:873: const int height = GetStringSize().height();
Nit: Inline

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:875: offset.Grow(0, (display_rect().height() - height) /
2);
Your CL uses Grow() in only three places besides the
definition/declaration/tests -- and only here if you follow my suggestions those
other two places.  What do you think about just eliminating Grow() entirely in
favor of vector addition?  It results in creating another temp, but it makes a
bit more sense to me mathematically.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:1004: display_offset_.Grow(delta_offset, 0);
Nit: I suggest declaring a temp vector to enforce that these both get offset by
the same amount:

  Vector2d delta(delta_offset, 0);
  display_offset_ += delta;
  cursor_bounds_.Offset(delta);

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text_mac.cc#n...
ui/gfx/render_text_mac.cc:237: offset.Grow(0, common_baseline_);
Nit: Inline this addition into the SkIntToScalar call below.

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

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/size.h#newcode14
ui/gfx/size.h:14: #include "ui/gfx/vector2d.h"
Nit: Revert?

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/size_f.h
File ui/gfx/size_f.h (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/size_f.h#newcode12
ui/gfx/size_f.h:12: #include "ui/gfx/vector2d_f.h"
Nit: Revert?

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/size_unittest.cc
File ui/gfx/size_unittest.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/size_unittest.cc#new...
ui/gfx/size_unittest.cc:7: #include "base/basictypes.h"
Nit: Surprised this is necessary

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.cc
File ui/gfx/vector2d.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.cc#newcode15
ui/gfx/vector2d.cc:15: y_(0) {
Nit: Could also put these first three lines all on one line (2 places)

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.cc#newcode48
ui/gfx/vector2d.cc:48: static_cast<double>(x_) * x_ + static_cast<double>(y_) *
y_));
Nit: Would this be more efficient as

      static_cast<double>(x_ * x_) + y_ * y_));

?  Then we'd do all but the sqrt() step as integer math...

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.h
File ui/gfx/vector2d.h (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.h#newcode27
ui/gfx/vector2d.h:27: void set_x(int x) { x_ = x; }
Only ParamTraits uses set_x() and set_y().  I wonder if we can nuke these in
favor of a friend declaration for the relevant ParamTraits function, for the
same reason I wouldn't mind nuking Grow().

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.h#newcode33
ui/gfx/vector2d.h:33: bool IsZero() const;
Nit: Nowhere besides tests seem to use this, remove?

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.h#newcode46
ui/gfx/vector2d.h:46: // Gives the square of the diagonal length of the vector.
Nit: Perhaps add note that this is cheaper than Length() and thus useful for
consumers who only need to compare relative lengths.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d_unittest.cc
File ui/gfx/vector2d_unittest.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d_unittest.cc...
ui/gfx/vector2d_unittest.cc:48: static const struct {
Nit: No need for these local consts to be marked static (several places)

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d_unittest.cc...
ui/gfx/vector2d_unittest.cc:57: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(int_tests); ++i)
Nit: Can you use arraysize() instead if you give the struct type itself a name? 
(several places)

http://codereview.chromium.org/11269022/diff/5049/ui/views/controls/textfield...
File ui/views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/views/controls/textfield...
ui/views/controls/textfield/native_textfield_views.cc:116: return
ExceededDragThreshold(location_delta.x(), location_delta.y());
This may be another function that makes sense to convert to taking a vector,
since we basically want to know "did this drag offset exceed the threshold" and
drag offsets are vectors.

http://codereview.chromium.org/11269022/diff/5049/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11269022/diff/5049/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1169: gfx::Point plugin_origin =
PP_ToGfxPoint(view_data_.rect.point);
Nit: Construct a Vector2d here instead.  Then you can pass its negation directly
to the Offset() call just below, and pass it directly to the two Offset() calls
at the bottom.

[danakj, 2012-10-30 19:21:21]

PTAL.

Avoiding done spam; anything without a reply comment I addressed without needing
to comment further.

http://codereview.chromium.org/11269022/diff/5049/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

http://codereview.chromium.org/11269022/diff/5049/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:2038: return
point.OffsetFromOrigin();
On 2012/10/30 01:14:14, Peter Kasting wrote:
> I wonder if view.h should change from having all the current
> ConvertPoint{From,To}XXX() functions to simply having some kind of
> GetOffsetInScreen() function.  Then code like this would just be "return
> point_in_screen - toplevel_view->GetOffsetInScreen();".  And most other places
> we convert could be changed similarly.
> 
> We might need to have a GetOffsetFromWidget() function too. 
> (GetOffsetFromParent() wouldn't be necessary because that's just
> bounds().OffsetFromOrigin().)
> 
> If this makes sense it would probably be better as a separate change.

That sounds pretty reasonable, and like it would reduce a bunch of API.

http://codereview.chromium.org/11269022/diff/5049/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/11269022/diff/5049/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:797: } else if
(coordinate_type == IA2_COORDTYPE_PARENT_RELATIVE) {
On 2012/10/30 01:14:14, Peter Kasting wrote:
> Nit: Can save one line by reversing this condition and returning, then
combining
> the subsequent "else" and "if (parent_)" into one "else if"

Idk, I find that version a lot harder to read and reason about, I don't think
it's worth saving a line.

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
File ui/aura/test/event_generator.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
ui/aura/test/event_generator.cc:132: gfx::Vector2d(diff.x() / count * i,
diff.y() / count * i));
On 2012/10/30 01:14:14, Peter Kasting wrote:
> Do you think this suggests we should support multiplication and division
> operators on vectors to scale them?

Yeh, that sounds nice. We have a Scale() method on them now. I wanted to change
this code minimally to avoid hard-to-spot bugs in such a large CL.

http://codereview.chromium.org/11269022/diff/5049/ui/base/gtk/gtk_screen_util.cc
File ui/base/gtk/gtk_screen_util.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/base/gtk/gtk_screen_util...
ui/base/gtk/gtk_screen_util.cc:51: gfx::Point position =
PointAtOffsetFromOrigin(GetWidgetScreenOffset(widget));
On 2012/10/30 01:14:14, Peter Kasting wrote:
> Nit: Seems like we don't need to convert to a point, we could just call
> Vector::x() and y() below.
> 
> Or alternately, we could do something like:
> 
>   GtkAllocation allocation;
>   gtk_widget_get_allocation(widget, &allocation);
>   return gfx::Rect(PointAtOffsetFromOrigin(GetWidgetScreenOffset(widget)),
>                    gfx::Size(allocation.width, allocation.height));

ya, the latter sounds nice. i had done that elsewhere too.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/rect_base.h
File ui/gfx/rect_base.h (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/rect_base.h#newcode131
ui/gfx/rect_base.h:131: VectorClass CenterOffset() const {
On 2012/10/30 01:14:14, Peter Kasting wrote:
> I'm not convinced of the utility of this and the rest of your change doesn't
> seem to use it.  Let's leave it out until a future changelist wants something
> like this.

k

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:862: if (horizontal_alignment() != ALIGN_LEFT) {
On 2012/10/30 01:14:14, Peter Kasting wrote:
> Nit: Can eliminate {} by reversing condition and early-returning

yay early-outs. excellent.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:875: offset.Grow(0, (display_rect().height() - height) /
2);
On 2012/10/30 01:14:14, Peter Kasting wrote:
> Your CL uses Grow() in only three places besides the
> definition/declaration/tests -- and only here if you follow my suggestions
those
> other two places.  What do you think about just eliminating Grow() entirely in
> favor of vector addition?  It results in creating another temp, but it makes a
> bit more sense to me mathematically.

I'm not sure. In cases like Vector.Add(Vector) it's a no-brainer to use
operators instead. In this case, where we have separate components, it makes for
more verbose code.

Do you like this idea because it lets you inline everything into the return
statement below? Maybe that's why I'm less convinced, as I don't like the
inline-everything pattern in general as it makes debugging harder and loses
documentation via viable names.

I'd prefer to keep Grow(x,y) for now, similar to Inset(l,t,r,b) on rect, and see
how it is/isn't used in the cc/ scrolling code.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text_mac.cc#n...
ui/gfx/render_text_mac.cc:237: offset.Grow(0, common_baseline_);
On 2012/10/30 01:14:14, Peter Kasting wrote:
> Nit: Inline this addition into the SkIntToScalar call below.

Then we need to inline it also into the run_origin line as well, duplicating
this logic. Prefer to keep this one as it is.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/size_unittest.cc
File ui/gfx/size_unittest.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/size_unittest.cc#new...
ui/gfx/size_unittest.cc:7: #include "base/basictypes.h"
On 2012/10/30 01:14:14, Peter Kasting wrote:
> Nit: Surprised this is necessary

Oh, this was for the macro to give the size of an array. But the test using it
was removed. Removing as well.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.cc
File ui/gfx/vector2d.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.cc#newcode48
ui/gfx/vector2d.cc:48: static_cast<double>(x_) * x_ + static_cast<double>(y_) *
y_));
On 2012/10/30 01:14:14, Peter Kasting wrote:
> Nit: Would this be more efficient as
> 
>       static_cast<double>(x_ * x_) + y_ * y_));
> 
> ?  Then we'd do all but the sqrt() step as integer math...

But we'd also lose some range, as x*x doesn't fit in the same integer size as x.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.h
File ui/gfx/vector2d.h (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.h#newcode27
ui/gfx/vector2d.h:27: void set_x(int x) { x_ = x; }
On 2012/10/30 01:14:14, Peter Kasting wrote:
> Only ParamTraits uses set_x() and set_y().  I wonder if we can nuke these in
> favor of a friend declaration for the relevant ParamTraits function, for the
> same reason I wouldn't mind nuking Grow().

I made the code in render_text use set_x() for the delta_offset before I got
here. Think we should keep these to keep the class flexible.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.h#newcode33
ui/gfx/vector2d.h:33: bool IsZero() const;
On 2012/10/30 01:14:14, Peter Kasting wrote:
> Nit: Nowhere besides tests seem to use this, remove?

This is used in the cc/ code. I added it here when I tried to port cc/ over to
using Vector2d and found it missing. Would you prefer a separate CL?

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d_unittest.cc
File ui/gfx/vector2d_unittest.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d_unittest.cc...
ui/gfx/vector2d_unittest.cc:57: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(int_tests); ++i)
On 2012/10/30 01:14:14, Peter Kasting wrote:
> Nit: Can you use arraysize() instead if you give the struct type itself a
name? 
> (several places)

Was following from other tests. Why is this preferable? It adds more code to
these tests and forces the structs to be defined outside of the test method.

http://codereview.chromium.org/11269022/diff/5049/ui/views/controls/textfield...
File ui/views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/views/controls/textfield...
ui/views/controls/textfield/native_textfield_views.cc:116: return
ExceededDragThreshold(location_delta.x(), location_delta.y());
On 2012/10/30 01:14:14, Peter Kasting wrote:
> This may be another function that makes sense to convert to taking a vector,
> since we basically want to know "did this drag offset exceed the threshold"
and
> drag offsets are vectors.

Ya for sure. Changed the method. This was a really nice cleanup!

[Peter Kasting, 2012-10-30 20:24:37]

http://codereview.chromium.org/11269022/diff/5049/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

http://codereview.chromium.org/11269022/diff/5049/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:2038: return
point.OffsetFromOrigin();
On 2012/10/30 19:21:21, danakj wrote:
> On 2012/10/30 01:14:14, Peter Kasting wrote:
> > I wonder if view.h should change from having all the current
> > ConvertPoint{From,To}XXX() functions to simply having some kind of
> > GetOffsetInScreen() function.  Then code like this would just be "return
> > point_in_screen - toplevel_view->GetOffsetInScreen();".  And most other
places
> > we convert could be changed similarly.
> > 
> > We might need to have a GetOffsetFromWidget() function too. 
> > (GetOffsetFromParent() wouldn't be necessary because that's just
> > bounds().OffsetFromOrigin().)
> > 
> > If this makes sense it would probably be better as a separate change.
> 
> That sounds pretty reasonable, and like it would reduce a bunch of API.

Should there be a bug for this, then, or are you keeping a list of the specific
API changes we want to follow up with?  (Seems like there were more than just
this, weren't there?)

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
File ui/aura/test/event_generator.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
ui/aura/test/event_generator.cc:132: gfx::Vector2d(diff.x() / count * i,
diff.y() / count * i));
On 2012/10/30 19:21:21, danakj wrote:
> On 2012/10/30 01:14:14, Peter Kasting wrote:
> > Do you think this suggests we should support multiplication and division
> > operators on vectors to scale them?
> 
> Yeh, that sounds nice. We have a Scale() method on them now. I wanted to
change
> this code minimally to avoid hard-to-spot bugs in such a large CL.

You're welcome to tack Scale() -> *,/ onto the list of operators to add on that
bug, then :)

As for the code here, it looks like it suffers from some pretty severe precision
issues.  It seems like we should at least upcast diff.x() and diff.y() to
floats.  If we're going to do that anyway, we might as well go ahead and just
call Scale().  Which points out that Scale() is missing on Vector2d.  Seems like
it should be there and should do the calculations in floating-point internally,
then round.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:875: offset.Grow(0, (display_rect().height() - height) /
2);
On 2012/10/30 19:21:21, danakj wrote:
> On 2012/10/30 01:14:14, Peter Kasting wrote:
> > What do you think about just eliminating Grow() entirely in
> > favor of vector addition?  It results in creating another temp, but it makes
a
> > bit more sense to me mathematically.
> 
> I'm not sure. In cases like Vector.Add(Vector) it's a no-brainer to use
> operators instead. In this case, where we have separate components, it makes
for
> more verbose code.
> 
> Do you like this idea because it lets you inline everything into the return
> statement below?

No -- I dislike Grow() solely because it doesn't match up with any mathematical
operation I'm familiar with.  I think of modifying a vector by adding another
vector to it.  Grow() just feels very strange and wrong.  You shouldn't be able
to just muck directly with the pieces inside it.  I don't know that I can
explain better than that.

I realize that Grow(x, y) is trivially different from "+= Vector(x, y)" but the
latter seems clearer to me.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text_mac.cc#n...
ui/gfx/render_text_mac.cc:237: offset.Grow(0, common_baseline_);
On 2012/10/30 19:21:21, danakj wrote:
> On 2012/10/30 01:14:14, Peter Kasting wrote:
> > Nit: Inline this addition into the SkIntToScalar call below.
> 
> Then we need to inline it also into the run_origin line as well, duplicating
> this logic. Prefer to keep this one as it is.

No, I forgot to mention.  We should also have run_origin do:

  SkPoint run_origin = {x, y};

(Or optionally SkPoint::Make(x, y);, but that's less efficient and more
verbose.)

It wants SkScalars anyway, so it makes sense to supply the ones we already
calculated.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.cc
File ui/gfx/vector2d.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.cc#newcode48
ui/gfx/vector2d.cc:48: static_cast<double>(x_) * x_ + static_cast<double>(y_) *
y_));
On 2012/10/30 19:21:21, danakj wrote:
> On 2012/10/30 01:14:14, Peter Kasting wrote:
> > Nit: Would this be more efficient as
> > 
> >       static_cast<double>(x_ * x_) + y_ * y_));
> > 
> > ?  Then we'd do all but the sqrt() step as integer math...
> 
> But we'd also lose some range, as x*x doesn't fit in the same integer size as
x.

What about this then:

  return static_cast<float>(std::sqrt(static_cast<double>(LengthSquared())));

And actually, now that I think about it -- doesn't LengthSquared() maybe need to
explicitly upcast in the multiplication, for the same reason you complained
about here?  Otherwise it does everything as int and then promotes the final
result, doesn't it?

(I never remember the fine details of the promotion/conversion rules, so I could
be wrong.  But an explicit cast like that would be obvious and safe.)

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.h
File ui/gfx/vector2d.h (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.h#newcode27
ui/gfx/vector2d.h:27: void set_x(int x) { x_ = x; }
On 2012/10/30 19:21:21, danakj wrote:
> On 2012/10/30 01:14:14, Peter Kasting wrote:
> > Only ParamTraits uses set_x() and set_y().  I wonder if we can nuke these in
> > favor of a friend declaration for the relevant ParamTraits function, for the
> > same reason I wouldn't mind nuking Grow().
> 
> I made the code in render_text use set_x() for the delta_offset before I got
> here. Think we should keep these to keep the class flexible.

Oh.  I'm not sure I actually like that change.  As with Grow(), I feel wrong
about letting people muck with the components inside a vector.  I'd rather it be
immutable except by, say, adding other vectors.  The old code there that
calculated an offset first did this.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.h#newcode33
ui/gfx/vector2d.h:33: bool IsZero() const;
On 2012/10/30 19:21:21, danakj wrote:
> On 2012/10/30 01:14:14, Peter Kasting wrote:
> > Nit: Nowhere besides tests seem to use this, remove?
> 
> This is used in the cc/ code. I added it here when I tried to port cc/ over to
> using Vector2d and found it missing. Would you prefer a separate CL?

No, it's fine.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d_unittest.cc
File ui/gfx/vector2d_unittest.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d_unittest.cc...
ui/gfx/vector2d_unittest.cc:57: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(int_tests); ++i)
On 2012/10/30 19:21:21, danakj wrote:
> On 2012/10/30 01:14:14, Peter Kasting wrote:
> > Nit: Can you use arraysize() instead if you give the struct type itself a
> name? 
> > (several places)
> 
> Was following from other tests. Why is this preferable? It adds more code to
> these tests and forces the structs to be defined outside of the test method.

Does it?  My hope was that simply changing from:

struct {
  ...
} foo[] = ...

to

struct Foo {
  ...
} foo[] = ...

...would allow you to use arraysize().  If that's not true, then don't worry
about it.

http://codereview.chromium.org/11269022/diff/39006/ui/gfx/vector2d.h
File ui/gfx/vector2d.h (right):

http://codereview.chromium.org/11269022/diff/39006/ui/gfx/vector2d.h#newcode15
ui/gfx/vector2d.h:15: #include "base/basictypes.h"
How come this file includes this but vector2d_f.h doesn't?

[danakj, 2012-10-30 21:14:52]

There's some set_x() use elsewhere in the code still, that used to set_x() on a
Point. So I'll add removing set_x()/set_y() to potential next steps on a bug.

Comments addressed PTAL.

http://codereview.chromium.org/11269022/diff/5049/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

http://codereview.chromium.org/11269022/diff/5049/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:2038: return
point.OffsetFromOrigin();
On 2012/10/30 20:24:37, Peter Kasting wrote:
> On 2012/10/30 19:21:21, danakj wrote:
> > On 2012/10/30 01:14:14, Peter Kasting wrote:
> > > I wonder if view.h should change from having all the current
> > > ConvertPoint{From,To}XXX() functions to simply having some kind of
> > > GetOffsetInScreen() function.  Then code like this would just be "return
> > > point_in_screen - toplevel_view->GetOffsetInScreen();".  And most other
> places
> > > we convert could be changed similarly.
> > > 
> > > We might need to have a GetOffsetFromWidget() function too. 
> > > (GetOffsetFromParent() wouldn't be necessary because that's just
> > > bounds().OffsetFromOrigin().)
> > > 
> > > If this makes sense it would probably be better as a separate change.
> > 
> > That sounds pretty reasonable, and like it would reduce a bunch of API.
> 
> Should there be a bug for this, then, or are you keeping a list of the
specific
> API changes we want to follow up with?  (Seems like there were more than just
> this, weren't there?)

Ya, /me makes bug.

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
File ui/aura/test/event_generator.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
ui/aura/test/event_generator.cc:132: gfx::Vector2d(diff.x() / count * i,
diff.y() / count * i));
On 2012/10/30 20:24:37, Peter Kasting wrote:
> On 2012/10/30 19:21:21, danakj wrote:
> > On 2012/10/30 01:14:14, Peter Kasting wrote:
> > > Do you think this suggests we should support multiplication and division
> > > operators on vectors to scale them?
> > 
> > Yeh, that sounds nice. We have a Scale() method on them now. I wanted to
> change
> > this code minimally to avoid hard-to-spot bugs in such a large CL.
> 
> You're welcome to tack Scale() -> *,/ onto the list of operators to add on
that
> bug, then :)

Actually, I'm less sure about this after trying it out. Suddenly you have points
and scalars all appearing in the same statement and it gets very unclear.

Vector2d v = (point1 - point2) * scalar1 / scalar2;

I was going to do here, but I think I'll use the Scale() method, and we can
revisit this later. Also, there is Scale(float) and Scale(float, float) so at
least one of the Scale() methods needs to stay, leading to inconsistency.

> As for the code here, it looks like it suffers from some pretty severe
precision
> issues.  It seems like we should at least upcast diff.x() and diff.y() to
> floats.  If we're going to do that anyway, we might as well go ahead and just
> call Scale().  Which points out that Scale() is missing on Vector2d.  Seems
like
> it should be there and should do the calculations in floating-point
internally,
> then round.

I strongly dislike implicit rounding from Float to Int. You should work with a
Vector2dF if you want to scale by a float. Then do an explicit round/floor/ceil
to get back an int.

I've switched this code to scaling vectors, and added vector2d_conversions.h/cc
to support it.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:875: offset.Grow(0, (display_rect().height() - height) /
2);
On 2012/10/30 20:24:37, Peter Kasting wrote:
> On 2012/10/30 19:21:21, danakj wrote:
> > On 2012/10/30 01:14:14, Peter Kasting wrote:
> > > What do you think about just eliminating Grow() entirely in
> > > favor of vector addition?  It results in creating another temp, but it
makes
> a
> > > bit more sense to me mathematically.
> > 
> > I'm not sure. In cases like Vector.Add(Vector) it's a no-brainer to use
> > operators instead. In this case, where we have separate components, it makes
> for
> > more verbose code.
> > 
> > Do you like this idea because it lets you inline everything into the return
> > statement below?
> 
> No -- I dislike Grow() solely because it doesn't match up with any
mathematical
> operation I'm familiar with.  I think of modifying a vector by adding another
> vector to it.  Grow() just feels very strange and wrong.  You shouldn't be
able
> to just muck directly with the pieces inside it.  I don't know that I can
> explain better than that.
> 
> I realize that Grow(x, y) is trivially different from "+= Vector(x, y)" but
the
> latter seems clearer to me.

Ok, since it sounds like feel strongly, I will remove it.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/render_text_mac.cc#n...
ui/gfx/render_text_mac.cc:237: offset.Grow(0, common_baseline_);
On 2012/10/30 20:24:37, Peter Kasting wrote:
> On 2012/10/30 19:21:21, danakj wrote:
> > On 2012/10/30 01:14:14, Peter Kasting wrote:
> > > Nit: Inline this addition into the SkIntToScalar call below.
> > 
> > Then we need to inline it also into the run_origin line as well, duplicating
> > this logic. Prefer to keep this one as it is.
> 
> No, I forgot to mention.  We should also have run_origin do:
> 
>   SkPoint run_origin = {x, y};
> 
> (Or optionally SkPoint::Make(x, y);, but that's less efficient and more
> verbose.)
> 
> It wants SkScalars anyway, so it makes sense to supply the ones we already
> calculated.

Oh I see, yes.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.cc
File ui/gfx/vector2d.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.cc#newcode48
ui/gfx/vector2d.cc:48: static_cast<double>(x_) * x_ + static_cast<double>(y_) *
y_));
On 2012/10/30 20:24:37, Peter Kasting wrote:
> On 2012/10/30 19:21:21, danakj wrote:
> > On 2012/10/30 01:14:14, Peter Kasting wrote:
> > > Nit: Would this be more efficient as
> > > 
> > >       static_cast<double>(x_ * x_) + y_ * y_));
> > > 
> > > ?  Then we'd do all but the sqrt() step as integer math...
> > 
> > But we'd also lose some range, as x*x doesn't fit in the same integer size
as
> x.
> 
> What about this then:
> 
>   return static_cast<float>(std::sqrt(static_cast<double>(LengthSquared())));

Ya, I like that!

> And actually, now that I think about it -- doesn't LengthSquared() maybe need
to
> explicitly upcast in the multiplication, for the same reason you complained
> about here?

Yep, good catch.

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.h
File ui/gfx/vector2d.h (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d.h#newcode27
ui/gfx/vector2d.h:27: void set_x(int x) { x_ = x; }
On 2012/10/30 20:24:37, Peter Kasting wrote:
> On 2012/10/30 19:21:21, danakj wrote:
> > On 2012/10/30 01:14:14, Peter Kasting wrote:
> > > Only ParamTraits uses set_x() and set_y().  I wonder if we can nuke these
in
> > > favor of a friend declaration for the relevant ParamTraits function, for
the
> > > same reason I wouldn't mind nuking Grow().
> > 
> > I made the code in render_text use set_x() for the delta_offset before I got
> > here. Think we should keep these to keep the class flexible.
> 
> Oh.  I'm not sure I actually like that change.  As with Grow(), I feel wrong
> about letting people muck with the components inside a vector.  I'd rather it
be
> immutable except by, say, adding other vectors.  The old code there that
> calculated an offset first did this.

Ok, I've undone that to not use set_x().

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d_unittest.cc
File ui/gfx/vector2d_unittest.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/gfx/vector2d_unittest.cc...
ui/gfx/vector2d_unittest.cc:57: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(int_tests); ++i)
On 2012/10/30 20:24:37, Peter Kasting wrote:
> On 2012/10/30 19:21:21, danakj wrote:
> > On 2012/10/30 01:14:14, Peter Kasting wrote:
> > > Nit: Can you use arraysize() instead if you give the struct type itself a
> > name? 
> > > (several places)
> > 
> > Was following from other tests. Why is this preferable? It adds more code to
> > these tests and forces the structs to be defined outside of the test method.
> 
> Does it?  My hope was that simply changing from:
> 
> struct {
>   ...
> } foo[] = ...
> 
> to
> 
> struct Foo {
>   ...
> } foo[] = ...
> 
> ...would allow you to use arraysize().  If that's not true, then don't worry
> about it.

// One caveat is that arraysize() doesn't accept any array of an
// anonymous type or a type defined inside a function.

Ya, leaving it as is.

http://codereview.chromium.org/11269022/diff/39006/ui/gfx/vector2d.h
File ui/gfx/vector2d.h (right):

http://codereview.chromium.org/11269022/diff/39006/ui/gfx/vector2d.h#newcode15
ui/gfx/vector2d.h:15: #include "base/basictypes.h"
On 2012/10/30 20:24:37, Peter Kasting wrote:
> How come this file includes this but vector2d_f.h doesn't?

For LengthSquared to return int64.

[Peter Kasting, 2012-10-30 21:41:23]

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
File ui/aura/test/event_generator.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
ui/aura/test/event_generator.cc:132: gfx::Vector2d(diff.x() / count * i,
diff.y() / count * i));
On 2012/10/30 21:14:52, danakj wrote:
> I was going to do here, but I think I'll use the Scale() method, and we can
> revisit this later. Also, there is Scale(float) and Scale(float, float) so at
> least one of the Scale() methods needs to stay, leading to inconsistency.

Commented on bug.  I'll note that having operator* for a uniform scaling and
Scale(float, float) for if you need to scale differently in different dimensions
seems like an OK disparity to me.  Although, much like with the setters and
Grow(), I'm a bit leery of allowing nonuniform scaling between dimensions. 
However, unlike with Grow(), I'm not sure how to fall back on something else if
code really needs to do that.  Does the compositor need this sort of nonuniform
scaling?

http://codereview.chromium.org/11269022/diff/45334/ui/aura/test/event_generat...
File ui/aura/test/event_generator.cc (right):

http://codereview.chromium.org/11269022/diff/45334/ui/aura/test/event_generat...
ui/aura/test/event_generator.cc:131: gfx::Vector2dF diff = point -
current_location_;
This is wrong if one of the mouse dispatch event processing calls ends up
changing current_location_.  I don't know if that can happen but I'd rather not
risk it.  How about:

  gfx::Vector2d diff(point - current_location_);
  for (float i = 1; i <= count; ++i) {
    gfx::Vector2dF step(diff);
    step.Scale(i / count);
    gfx::Point event_point(current_location_ + gfx::ToRoundedVector2d(step));
    Dispatch(ui::MouseEvent(event_type, event_point, event_point, flags));
  }

[danakj, 2012-10-30 21:49:09]

Done.

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
File ui/aura/test/event_generator.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
ui/aura/test/event_generator.cc:132: gfx::Vector2d(diff.x() / count * i,
diff.y() / count * i));
On 2012/10/30 21:41:23, Peter Kasting wrote:
> On 2012/10/30 21:14:52, danakj wrote:
> > I was going to do here, but I think I'll use the Scale() method, and we can
> > revisit this later. Also, there is Scale(float) and Scale(float, float) so
at
> > least one of the Scale() methods needs to stay, leading to inconsistency.
> 
> Commented on bug.  I'll note that having operator* for a uniform scaling and
> Scale(float, float) for if you need to scale differently in different
dimensions
> seems like an OK disparity to me.  Although, much like with the setters and
> Grow(), I'm a bit leery of allowing nonuniform scaling between dimensions. 
> However, unlike with Grow(), I'm not sure how to fall back on something else
if
> code really needs to do that.  Does the compositor need this sort of
nonuniform
> scaling?

We do non-uniform scaling between different spaces, yes. For instance in
layer_tree_host_impl.cc:

localStartPoint.scale(widthScale, heightScale);

For image layers that are XxY and drawing into a space MxN, we need different
scales for each axis.

http://codereview.chromium.org/11269022/diff/45334/ui/aura/test/event_generat...
File ui/aura/test/event_generator.cc (right):

http://codereview.chromium.org/11269022/diff/45334/ui/aura/test/event_generat...
ui/aura/test/event_generator.cc:131: gfx::Vector2dF diff = point -
current_location_;
On 2012/10/30 21:41:23, Peter Kasting wrote:
> This is wrong if one of the mouse dispatch event processing calls ends up
> changing current_location_.  I don't know if that can happen but I'd rather
not
> risk it.  How about:
> 
>   gfx::Vector2d diff(point - current_location_);
>   for (float i = 1; i <= count; ++i) {
>     gfx::Vector2dF step(diff);
>     step.Scale(i / count);
>     gfx::Point event_point(current_location_ + gfx::ToRoundedVector2d(step));
>     Dispatch(ui::MouseEvent(event_type, event_point, event_point, flags));
>   }

ya, ok!

[Peter Kasting, 2012-10-30 21:56:35]

Will re-review

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
File ui/aura/test/event_generator.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
ui/aura/test/event_generator.cc:132: gfx::Vector2d(diff.x() / count * i,
diff.y() / count * i));
On 2012/10/30 21:49:10, danakj wrote:
> On 2012/10/30 21:41:23, Peter Kasting wrote:
> > On 2012/10/30 21:14:52, danakj wrote:
> > > I was going to do here, but I think I'll use the Scale() method, and we
can
> > > revisit this later. Also, there is Scale(float) and Scale(float, float) so
> at
> > > least one of the Scale() methods needs to stay, leading to inconsistency.
> > 
> > Commented on bug.  I'll note that having operator* for a uniform scaling and
> > Scale(float, float) for if you need to scale differently in different
> dimensions
> > seems like an OK disparity to me.  Although, much like with the setters and
> > Grow(), I'm a bit leery of allowing nonuniform scaling between dimensions. 
> > However, unlike with Grow(), I'm not sure how to fall back on something else
> if
> > code really needs to do that.  Does the compositor need this sort of
> nonuniform
> > scaling?
> 
> We do non-uniform scaling between different spaces, yes. For instance in
> layer_tree_host_impl.cc:
> 
> localStartPoint.scale(widthScale, heightScale);
> 
> For image layers that are XxY and drawing into a space MxN, we need different
> scales for each axis.

Sure, but is it vectors you need to scale, rather than points or rects?

Ah well, if you need it, keep it, and whether to use the operator for uniform
scaling is up to you.

[danakj, 2012-10-30 21:59:34]

I messed up the upload and then one blew up, but should be good to go now.

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
File ui/aura/test/event_generator.cc (right):

http://codereview.chromium.org/11269022/diff/5049/ui/aura/test/event_generato...
ui/aura/test/event_generator.cc:132: gfx::Vector2d(diff.x() / count * i,
diff.y() / count * i));
On 2012/10/30 21:56:35, Peter Kasting wrote:
> On 2012/10/30 21:49:10, danakj wrote:
> > On 2012/10/30 21:41:23, Peter Kasting wrote:
> > > On 2012/10/30 21:14:52, danakj wrote:
> > > > I was going to do here, but I think I'll use the Scale() method, and we
> can
> > > > revisit this later. Also, there is Scale(float) and Scale(float, float)
so
> > at
> > > > least one of the Scale() methods needs to stay, leading to
inconsistency.
> > > 
> > > Commented on bug.  I'll note that having operator* for a uniform scaling
and
> > > Scale(float, float) for if you need to scale differently in different
> > dimensions
> > > seems like an OK disparity to me.  Although, much like with the setters
and
> > > Grow(), I'm a bit leery of allowing nonuniform scaling between dimensions.

> > > However, unlike with Grow(), I'm not sure how to fall back on something
else
> > if
> > > code really needs to do that.  Does the compositor need this sort of
> > nonuniform
> > > scaling?
> > 
> > We do non-uniform scaling between different spaces, yes. For instance in
> > layer_tree_host_impl.cc:
> > 
> > localStartPoint.scale(widthScale, heightScale);
> > 
> > For image layers that are XxY and drawing into a space MxN, we need
different
> > scales for each axis.
> 
> Sure, but is it vectors you need to scale, rather than points or rects?
> 
> Ah well, if you need it, keep it, and whether to use the operator for uniform
> scaling is up to you.

I'm not entirely sure, since these types are going to all change. If we need to
scale a scroll delta between spaces then it'll be a vector. There is one such
scale in pinchGestureUpdate() in the same file for an IntSize, so I'd say it
seems likely. Maybe refactoring could make us only need to move end-points
around though?

[Peter Kasting, 2012-10-31 01:04:40]

LGTM

http://codereview.chromium.org/11269022/diff/42207/ash/wm/workspace/frame_max...
File ash/wm/workspace/frame_maximize_button.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:453: else if (delta.x() < 0 &&
delta.y() > delta.x() && delta.y() < -delta.x())
Nit: While here, remove "else"s (no else after return)

http://codereview.chromium.org/11269022/diff/42207/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/11269022/diff/42207/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:467:
clip.Offset(moves[i].window_rect.OffsetFromOrigin() + scroll_offset);
One other possible way to do this I didn't think of:

    const gfx::Vector2d offset(
        moves[i].window_rect.OffsetFromOrigin() + scroll_offset);
    clip.Offset(offset);
    ...
    clip.Offset(-offset);

This is two lines longer but possibly clearer and less error-prone.  I'm
ambivalent, do whichever you like better.

http://codereview.chromium.org/11269022/diff/42207/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/11269022/diff/42207/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:1329:
canvas.Translate(gfx::Point() - dirty_area.origin());
Nit: Simpler:

    canvas.Translate(-dirty_area.OffsetFromOrigin());

http://codereview.chromium.org/11269022/diff/42207/ui/aura/test/event_generat...
File ui/aura/test/event_generator.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/aura/test/event_generat...
ui/aura/test/event_generator.cc:134: gfx::Point move_point = current_location_ +
gfx::ToFlooredVector2d(step);
How come you ignored three points of my proposed code?:

(1) Make |i| float to remove static_cast
(2) Constructor-style init for non-POD types (Vector2dF)
(3) Use ToRoundedVector2d() to reduce error in individual moves

http://codereview.chromium.org/11269022/diff/42207/ui/base/dragdrop/drag_util...
File ui/base/dragdrop/drag_utils.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/base/dragdrop/drag_util...
ui/base/dragdrop/drag_utils.cc:97: image, size, gfx::Vector2d(size.width() / 2,
kLinkDragImageVPadding),
Gah, didn't realize inlining this would actually make everything longer overall.
 Go back to how it used to be :)

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:875: Vector2d(0, (display_rect().height() -
GetStringSize().height()) / 2));
Nit: Technically this line would need to be indented by 4 more spaces.

I'd fix this by instead putting "GetTextOffset() +" on the end of the previous
line and leaving this line alone.

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:236: const SkScalar x =
SkIntToScalar(GetTextOffset().x());
Nit: FWIW, I had intended to still have a separate Vector2d temp, I only meant
for the Grow() call to be inlined.  GetTextOffset() may not be cheap.

http://codereview.chromium.org/11269022/diff/42207/ui/views/controls/table/ta...
File ui/views/controls/table/table_view_win.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/views/controls/table/ta...
ui/views/controls/table/table_view_win.cc:713: gfx::Point
mouse_down_pos(mouse_down_x, mouse_down_y);
Nit: Convert mouse_down_x/mouse_down_y from a pair of ints to a point to
eliminate this explicit construction.

You can use CR_DEFINE_STATIC_LOCAL to declare this as a function-level static
without causing any extra static destruction.

[danakj, 2012-10-31 16:55:46]

Thanks so much for the very thorough review on such a large change!

http://codereview.chromium.org/11269022/diff/42207/ash/wm/workspace/frame_max...
File ash/wm/workspace/frame_maximize_button.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ash/wm/workspace/frame_max...
ash/wm/workspace/frame_maximize_button.cc:453: else if (delta.x() < 0 &&
delta.y() > delta.x() && delta.y() < -delta.x())
On 2012/10/31 01:04:41, Peter Kasting wrote:
> Nit: While here, remove "else"s (no else after return)

Done.

http://codereview.chromium.org/11269022/diff/42207/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/11269022/diff/42207/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:467:
clip.Offset(moves[i].window_rect.OffsetFromOrigin() + scroll_offset);
On 2012/10/31 01:04:41, Peter Kasting wrote:
> One other possible way to do this I didn't think of:
> 
>     const gfx::Vector2d offset(
>         moves[i].window_rect.OffsetFromOrigin() + scroll_offset);
>     clip.Offset(offset);
>     ...
>     clip.Offset(-offset);
> 
> This is two lines longer but possibly clearer and less error-prone.  I'm
> ambivalent, do whichever you like better.

I like temp vars :) I'll do this.

http://codereview.chromium.org/11269022/diff/42207/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/11269022/diff/42207/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_win.cc:1329:
canvas.Translate(gfx::Point() - dirty_area.origin());
On 2012/10/31 01:04:41, Peter Kasting wrote:
> Nit: Simpler:
> 
>     canvas.Translate(-dirty_area.OffsetFromOrigin());

Done.

http://codereview.chromium.org/11269022/diff/42207/ui/aura/test/event_generat...
File ui/aura/test/event_generator.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/aura/test/event_generat...
ui/aura/test/event_generator.cc:134: gfx::Point move_point = current_location_ +
gfx::ToFlooredVector2d(step);
On 2012/10/31 01:04:41, Peter Kasting wrote:
> How come you ignored three points of my proposed code?:
> 
> (1) Make |i| float to remove static_cast
Oh, I didn't see that, assumed your lack of static cast was pseudocode.

> (2) Constructor-style init for non-POD types (Vector2dF)
Ah, okay.

> (3) Use ToRoundedVector2d() to reduce error in individual moves
I saw that, but didn't want to introduce behaviour changes in this CL. If you're
certain about this, then okay.

http://codereview.chromium.org/11269022/diff/42207/ui/base/dragdrop/drag_util...
File ui/base/dragdrop/drag_utils.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/base/dragdrop/drag_util...
ui/base/dragdrop/drag_utils.cc:97: image, size, gfx::Vector2d(size.width() / 2,
kLinkDragImageVPadding),
On 2012/10/31 01:04:41, Peter Kasting wrote:
> Gah, didn't realize inlining this would actually make everything longer
overall.
>  Go back to how it used to be :)

Done.

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:875: Vector2d(0, (display_rect().height() -
GetStringSize().height()) / 2));
On 2012/10/31 01:04:41, Peter Kasting wrote:
> Nit: Technically this line would need to be indented by 4 more spaces.
> 
> I'd fix this by instead putting "GetTextOffset() +" on the end of the previous
> line and leaving this line alone.

Then the Vector2d() line needs to line up below the open brace for
PointAtOffsetFromOrigin, right? And ends up spanning like 3 lines. I'll use a
temp var here.

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text_mac.cc#...
ui/gfx/render_text_mac.cc:236: const SkScalar x =
SkIntToScalar(GetTextOffset().x());
On 2012/10/31 01:04:41, Peter Kasting wrote:
> Nit: FWIW, I had intended to still have a separate Vector2d temp, I only meant
> for the Grow() call to be inlined.  GetTextOffset() may not be cheap.

Done.

http://codereview.chromium.org/11269022/diff/42207/ui/views/controls/table/ta...
File ui/views/controls/table/table_view_win.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/views/controls/table/ta...
ui/views/controls/table/table_view_win.cc:713: gfx::Point
mouse_down_pos(mouse_down_x, mouse_down_y);
On 2012/10/31 01:04:41, Peter Kasting wrote:
> Nit: Convert mouse_down_x/mouse_down_y from a pair of ints to a point to
> eliminate this explicit construction.
> 
> You can use CR_DEFINE_STATIC_LOCAL to declare this as a function-level static
> without causing any extra static destruction.

Oh, fancy.

[danakj, 2012-10-31 16:58:15]

+sky for OWNERS on ui/ ash/ chrome/
+piman for OWNERS on content/ webkit/plugins/ppapi/
+enne for OWNERS on cc/

[sky, 2012-10-31 17:13:03]

Rubber stamp LGTM

[Peter Kasting, 2012-10-31 17:13:40]

http://codereview.chromium.org/11269022/diff/42207/ui/aura/test/event_generat...
File ui/aura/test/event_generator.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/aura/test/event_generat...
ui/aura/test/event_generator.cc:134: gfx::Point move_point = current_location_ +
gfx::ToFlooredVector2d(step);
On 2012/10/31 16:55:46, danakj wrote:
> On 2012/10/31 01:04:41, Peter Kasting wrote:
> > (3) Use ToRoundedVector2d() to reduce error in individual moves
> I saw that, but didn't want to introduce behaviour changes in this CL. If
you're
> certain about this, then okay.

We're already introducing a behavior change because the old code did integer
division.  Given that lets's go all the way :)

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:875: Vector2d(0, (display_rect().height() -
GetStringSize().height()) / 2));
On 2012/10/31 16:55:46, danakj wrote:
> On 2012/10/31 01:04:41, Peter Kasting wrote:
> > Nit: Technically this line would need to be indented by 4 more spaces.
> > 
> > I'd fix this by instead putting "GetTextOffset() +" on the end of the
previous
> > line and leaving this line alone.
> 
> Then the Vector2d() line needs to line up below the open brace for
> PointAtOffsetFromOrigin, right? And ends up spanning like 3 lines. I'll use a
> temp var here.

No, it doesn't need to (and in fact shouldn't) line up.  Subsequent lines get
indented 4, only function args get indented even.  (And for loops, for some
reason.)

Or at least that's how I've understood our typical style...

[danakj, 2012-10-31 17:15:00]

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:875: Vector2d(0, (display_rect().height() -
GetStringSize().height()) / 2));
On 2012/10/31 17:13:41, Peter Kasting wrote:
> On 2012/10/31 16:55:46, danakj wrote:
> > On 2012/10/31 01:04:41, Peter Kasting wrote:
> > > Nit: Technically this line would need to be indented by 4 more spaces.
> > > 
> > > I'd fix this by instead putting "GetTextOffset() +" on the end of the
> previous
> > > line and leaving this line alone.
> > 
> > Then the Vector2d() line needs to line up below the open brace for
> > PointAtOffsetFromOrigin, right? And ends up spanning like 3 lines. I'll use
a
> > temp var here.
> 
> No, it doesn't need to (and in fact shouldn't) line up.  Subsequent lines get
> indented 4, only function args get indented even.  (And for loops, for some
> reason.)
> 
> Or at least that's how I've understood our typical style...

Oh! I was adding the two offsets, then making a point. You're thinking to make a
point then add the offset. I'll do that :)

[Peter Kasting, 2012-10-31 17:18:41]

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:875: Vector2d(0, (display_rect().height() -
GetStringSize().height()) / 2));
On 2012/10/31 17:15:00, danakj wrote:
> On 2012/10/31 17:13:41, Peter Kasting wrote:
> > On 2012/10/31 16:55:46, danakj wrote:
> > > On 2012/10/31 01:04:41, Peter Kasting wrote:
> > > > Nit: Technically this line would need to be indented by 4 more spaces.
> > > > 
> > > > I'd fix this by instead putting "GetTextOffset() +" on the end of the
> > previous
> > > > line and leaving this line alone.
> > > 
> > > Then the Vector2d() line needs to line up below the open brace for
> > > PointAtOffsetFromOrigin, right? And ends up spanning like 3 lines. I'll
use
> a
> > > temp var here.
> > 
> > No, it doesn't need to (and in fact shouldn't) line up.  Subsequent lines
get
> > indented 4, only function args get indented even.  (And for loops, for some
> > reason.)
> > 
> > Or at least that's how I've understood our typical style...
> 
> Oh! I was adding the two offsets, then making a point. You're thinking to make
a
> point then add the offset. I'll do that :)

Uh... I was?

I thought I was advocating for

  return gfx::PointAtOffsetFromOrigin(GetTextOffset() +
      Vector2d(0, (display_rect().height() - GetStringSize().height()) / 2));

But maybe you're saying we could do:

  gfx::Point p(0, (display_rect().height() - GetStringSize().height()) / 2);
  return p + GetTextOffset();

...which I hadn't thought of, but is also fine.

[Peter Kasting, 2012-10-31 17:19:53]

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:875: Vector2d(0, (display_rect().height() -
GetStringSize().height()) / 2));
Incidentally, I suspect that this code's callers might really want a vector
offset rather than a point.  But I haven't checked.

[danakj, 2012-10-31 17:29:31]

On Wed, Oct 31, 2012 at 1:18 PM,  <pkasting@chromium.org> wrote:
>
> http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text.cc
> File ui/gfx/render_text.cc (right):
>
>
http://codereview.chromium.org/11269022/diff/42207/ui/gfx/render_text.cc#newc...
> ui/gfx/render_text.cc:875: Vector2d(0, (display_rect().height() -
> GetStringSize().height()) / 2));
> On 2012/10/31 17:15:00, danakj wrote:
>>
>> On 2012/10/31 17:13:41, Peter Kasting wrote:
>> > On 2012/10/31 16:55:46, danakj wrote:
>> > > On 2012/10/31 01:04:41, Peter Kasting wrote:
>> > > > Nit: Technically this line would need to be indented by 4 more
>
> spaces.
>>
>> > > >
>> > > > I'd fix this by instead putting "GetTextOffset() +" on the end
>
> of the
>>
>> > previous
>> > > > line and leaving this line alone.
>> > >
>> > > Then the Vector2d() line needs to line up below the open brace for
>> > > PointAtOffsetFromOrigin, right? And ends up spanning like 3 lines.
>
> I'll use
>>
>> a
>> > > temp var here.
>> >
>> > No, it doesn't need to (and in fact shouldn't) line up.  Subsequent
>
> lines get
>>
>> > indented 4, only function args get indented even.  (And for loops,
>
> for some
>>
>> > reason.)
>> >
>> > Or at least that's how I've understood our typical style...
>
>
>> Oh! I was adding the two offsets, then making a point. You're thinking
>
> to make a
>>
>> point then add the offset. I'll do that :)
>
>
> Uh... I was?
>
> I thought I was advocating for
>
>   return gfx::PointAtOffsetFromOrigin(GetTextOffset() +
>
>       Vector2d(0, (display_rect().height() - GetStringSize().height()) /
> 2));

Oh, but then the Vector2d is still a parameter of the function. So if
you had a second parameter it would line up with the opening ( but
long wrapping lines don't?

Either way, this works well :)

return gfx::PointAtOffsetFromOrigin(GetTextOffset()) +
    Vector2d(0, (display_rect().height() - GetStringSize().height()) / 2);

[enne (OOO), 2012-10-31 17:51:52]

lgtm

[piman, 2012-10-31 18:07:23]

LGTM+nit

http://codereview.chromium.org/11269022/diff/62001/content/public/common/comm...
File content/public/common/common_param_traits.cc (right):

http://codereview.chromium.org/11269022/diff/62001/content/public/common/comm...
content/public/common/common_param_traits.cc:160: gfx::Vector2d* r) {
nit: indentation

[Peter Kasting, 2012-10-31 18:08:52]

On 2012/10/31 17:29:31, danakj wrote:
> Oh, but then the Vector2d is still a parameter of the function. So if
> you had a second parameter it would line up with the opening ( but
> long wrapping lines don't?

If you had a second arg, it would be weird to indent like this:

return LongFunctionName(long_arg_calculation +
    more_long_arg_calculation + etc.,
                        second_arg);

So in this case you'd want to go ahead and do:

return LongFunctionName(
    long_arg_calculation +
        more_long_arg_calculation + etc.,
    second_arg);

But if you don't have a second arg, I think wrapping by indenting 4 is OK.

[Peter Kasting, 2012-10-31 18:09:28]

http://codereview.chromium.org/11269022/diff/62001/content/public/common/comm...
File content/public/common/common_param_traits.cc (right):

http://codereview.chromium.org/11269022/diff/62001/content/public/common/comm...
content/public/common/common_param_traits.cc:160: gfx::Vector2d* r) {
On 2012/10/31 18:07:23, piman wrote:
> nit: indentation

Also, one arg per line if they don't all fit on one line.