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

ui/gfx/render_text.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/render_text.h"
 
 #include <algorithm>
 
 #include "base/i18n/break_iterator.h"
 #include "base/logging.h"
@@ skipping 388 matching lines @@
   if (directionality_mode_ == DIRECTIONALITY_FROM_TEXT)
     text_direction_ = base::i18n::UNKNOWN_DIRECTION;
 
   UpdateObscuredText();
   ResetLayout();
 }
 
 void RenderText::SetHorizontalAlignment(HorizontalAlignment alignment) {
   if (horizontal_alignment_ != alignment) {
     horizontal_alignment_ = alignment;
-    display_offset_ = Point();
+    display_offset_ = Vector2d();
     cached_bounds_and_offset_valid_ = false;
   }
 }
 
 void RenderText::SetFontList(const FontList& font_list) {
   font_list_ = font_list;
   cached_bounds_and_offset_valid_ = false;
   ResetLayout();
 }
 
@@ skipping 338 matching lines @@
       focused_(false),
       composition_range_(ui::Range::InvalidRange()),
       obscured_(false),
       fade_head_(false),
       fade_tail_(false),
       background_is_transparent_(false),
       clip_to_display_rect_(true),
       cached_bounds_and_offset_valid_(false) {
 }
 
-const Point& RenderText::GetUpdatedDisplayOffset() {
+const Vector2d& RenderText::GetUpdatedDisplayOffset() {
   UpdateCachedBoundsAndOffset();
   return display_offset_;
 }
 
 SelectionModel RenderText::GetAdjacentSelectionModel(
     const SelectionModel& current,
     BreakType break_type,
     VisualCursorDirection direction) {
   EnsureLayout();
 
@@ skipping 53 matching lines @@
     StyleRange replacement_mode_style(default_style_);
     replacement_mode_style.foreground = selection_color_;
     size_t cursor = cursor_position();
     replacement_mode_style.range.set_start(cursor);
     replacement_mode_style.range.set_end(
         IndexOfAdjacentGrapheme(cursor, CURSOR_FORWARD));
     ApplyStyleRangeImpl(style_ranges, replacement_mode_style);
   }
 }
 
-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) {
   if (!fade_head() && !fade_tail())
     return;
 
   const int text_width = GetStringSize().width();
   const int display_width = display_rect().width();
 
   // If the text fits as-is, no need to fade.
@@ skipping 107 matching lines @@
     delta_offset = display_rect_.x() - cursor_bounds_.x();
   } else if (display_offset_.x() != 0) {
     // Reduce the pan offset to show additional overflow text when the display
     // width increases.
     const int negate_rtl = horizontal_alignment_ == ALIGN_RIGHT ? -1 : 1;
     const int offset = negate_rtl * display_offset_.x();
     if (display_width > (content_width + offset))
       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);
 }
 
 void RenderText::DrawSelection(Canvas* canvas) {
   const SkColor color = focused() ? selection_background_focused_color_ :
                                     selection_background_unfocused_color_;
   const std::vector<Rect> sel = GetSubstringBounds(selection());
   for (std::vector<Rect>::const_iterator i = sel.begin(); i < sel.end(); ++i)
     canvas->FillRect(*i, color);
 }
 
 void RenderText::DrawCursor(Canvas* canvas) {
   // Paint cursor. Replace cursor is drawn as rectangle for now.
   // TODO(msw): Draw a better cursor with a better indication of association.
   if (cursor_enabled() && cursor_visible() && focused()) {
     const Rect& bounds = GetUpdatedCursorBounds();
     if (bounds.width() != 0)
       canvas->FillRect(bounds, cursor_color_);
     else
       canvas->DrawRect(bounds, cursor_color_);
   }
 }
 
 }  // namespace gfx