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

ui/gfx/render_text.cc

Index: ui/gfx/render_text.cc
diff --git a/ui/gfx/render_text.cc b/ui/gfx/render_text.cc
index a8246c2d6b1f83905b7092cd9266f2547a179964..25c8e7b000cd06bfa37e150d49aa7e81611be9e1 100644
--- a/ui/gfx/render_text.cc
+++ b/ui/gfx/render_text.cc
@@ -406,7 +406,7 @@ void RenderText::SetText(const string16& text) {
 void RenderText::SetHorizontalAlignment(HorizontalAlignment alignment) {
   if (horizontal_alignment_ != alignment) {
     horizontal_alignment_ = alignment;
-    display_offset_ = Point();
+    display_offset_ = Vector2d();
     cached_bounds_and_offset_valid_ = false;
   }
 }
@@ -765,7 +765,7 @@ RenderText::RenderText()
       cached_bounds_and_offset_valid_(false) {
 }
 
-const Point& RenderText::GetUpdatedDisplayOffset() {
+const Vector2d& RenderText::GetUpdatedDisplayOffset() {
   UpdateCachedBoundsAndOffset();
   return display_offset_;
 }
@@ -839,41 +839,41 @@ void RenderText::ApplyCompositionAndSelectionStyles(
   }
 }
 
-Point RenderText::GetTextOrigin() {
-  Point origin = display_rect().origin();
-  origin = origin.Add(GetUpdatedDisplayOffset());
-  origin = origin.Add(GetAlignmentOffset());
-  return origin;
+Vector2d RenderText::GetTextOffset() {
+  Vector2d offset = display_rect().OffsetFromOrigin();
+  offset.Add(GetUpdatedDisplayOffset());
+  offset.Add(GetAlignmentOffset());
+  return offset;
 }
 
 Point RenderText::ToTextPoint(const Point& point) {
-  return point.Subtract(GetTextOrigin());
+  return point.Subtract(GetTextOffset());
 }
 
 Point RenderText::ToViewPoint(const Point& point) {
-  return point.Add(GetTextOrigin());
+  return point.Add(GetTextOffset());
 }
 
 int RenderText::GetContentWidth() {
   return GetStringSize().width() + (cursor_enabled_ ? 1 : 0);
 }
 
-Point RenderText::GetAlignmentOffset() {
+Vector2d RenderText::GetAlignmentOffset() {
   if (horizontal_alignment() != ALIGN_LEFT) {

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

Nit: Can eliminate {} by reversing condition and early-returning

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

yay early-outs. excellent.

     int x_offset = display_rect().width() - GetContentWidth();
     if (horizontal_alignment() == ALIGN_CENTER)
       x_offset /= 2;
-    return Point(x_offset, 0);
+    return Vector2d(x_offset, 0);
   }
-  return Point();
+  return Vector2d();
 }
 
 Point RenderText::GetOriginForDrawing() {
-  Point origin(GetTextOrigin());
+  Vector2d offset(GetTextOffset());
   const int height = GetStringSize().height();

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

Nit: Inline

   // Center the text vertically in the display area.
-  origin.Offset(0, (display_rect().height() - height) / 2);
-  return origin;
+  offset.Grow(0, (display_rect().height() - height) / 2);

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

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.

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

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.

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

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.

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

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

+  return gfx::PointAtOffsetFromOrigin(offset);
 }
 
 void RenderText::ApplyFadeEffects(internal::SkiaTextRenderer* renderer) {
@@ -1001,7 +1001,7 @@ void RenderText::UpdateCachedBoundsAndOffset() {
       delta_offset = negate_rtl * (display_width - (content_width + offset));
   }
 
-  display_offset_.Offset(delta_offset, 0);
+  display_offset_.Grow(delta_offset, 0);

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

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);

   cursor_bounds_.Offset(delta_offset, 0);
 }