Fix RenderText cached bounds and offset logic; update clients.

ui/gfx/render_text.cc

 // Copyright (c) 2011 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 112 matching lines @@
                           CaretPlacement placement) {
   selection_start_ = start;
   selection_end_ = end;
   caret_pos_ = pos;
   caret_placement_ = placement;
 }
 
 RenderText::~RenderText() {
 }
 
-void RenderText::set_display_rect(const Rect& r) {
-  display_rect_ = r;
-}
-
 void RenderText::SetText(const string16& text) {
   size_t old_text_length = text_.length();
   text_ = text;
 
   // Update the style ranges as needed.
   if (text_.empty()) {
     style_ranges_.clear();
   } else if (style_ranges_.empty()) {
     ApplyDefaultStyle();
   } else if (text_.length() > old_text_length) {
@@ skipping 17 matching lines @@
 }
 
 void RenderText::SetSelectionModel(const SelectionModel& sel) {
   size_t start = sel.selection_start();
   size_t end = sel.selection_end();
   selection_model_.set_selection_start(std::min(start, text().length()));
   selection_model_.set_selection_end(std::min(end, text().length()));
   selection_model_.set_caret_pos(std::min(sel.caret_pos(), text().length()));
   selection_model_.set_caret_placement(sel.caret_placement());
 
-  cursor_bounds_valid_ = false;
+  cached_bounds_and_offset_valid_ = false;
+}
+
+void RenderText::SetDisplayRect(const Rect& r) {
+  display_rect_ = r;
+  cached_bounds_and_offset_valid_ = false;

[xji, 2011/08/10 00:36:15]

does it make sense to invalidate this in SetText() as well?

[msw, 2011/08/10 01:27:39]

Done.

 }
 
 size_t RenderText::GetCursorPosition() const {
   return selection_model_.selection_end();
 }
 
 void RenderText::SetCursorPosition(const size_t position) {
   SelectionModel sel(selection_model());
   sel.set_selection_start(position);
   sel.set_selection_end(position);
@@ skipping 136 matching lines @@
 void RenderText::ApplyStyleRange(StyleRange style_range) {
   const ui::Range& new_range = style_range.range;
   if (!new_range.IsValid() || new_range.is_empty())
     return;
   CHECK(!new_range.is_reversed());
   CHECK(ui::Range(0, text_.length()).Contains(new_range));
   ApplyStyleRangeImpl(&style_ranges_, style_range);
 #ifndef NDEBUG
   CheckStyleRanges(style_ranges_, text_.length());
 #endif
-  // TODO(xji): only invalidate cursor_bounds if font or underline change.
-  cursor_bounds_valid_ = false;
+  // TODO(xji): only invalidate if font or underline changes.
+  cached_bounds_and_offset_valid_ = false;
 }
 
 void RenderText::ApplyDefaultStyle() {
   style_ranges_.clear();
   StyleRange style = StyleRange(default_style_);
   style.range.set_end(text_.length());
   style_ranges_.push_back(style);
-  cursor_bounds_valid_ = false;
+  cached_bounds_and_offset_valid_ = false;
 }
 
 base::i18n::TextDirection RenderText::GetTextDirection() const {
   // TODO(msw): Bidi implementation, intended to replace the functionality added
   //  in crrev.com/91881 (discussed in codereview.chromium.org/7324011).
   return base::i18n::LEFT_TO_RIGHT;
 }
 
 int RenderText::GetStringWidth() {
-  return GetSubstringBounds(0, text_.length())[0].width();
+  return default_style_.font.GetStringWidth(text());
 }
 
 void RenderText::Draw(Canvas* canvas) {
   // Clip the canvas to the text display area.
   canvas->ClipRectInt(display_rect_.x(), display_rect_.y(),
                       display_rect_.width(), display_rect_.height());
 
   // Draw the selection.
   std::vector<Rect> selection(GetSubstringBounds(GetSelectionStart(),
                                                  GetCursorPosition()));
   SkColor selection_color =
       focused() ? kFocusedSelectionColor : kUnfocusedSelectionColor;
   for (std::vector<Rect>::const_iterator i = selection.begin();
        i < selection.end(); ++i) {
     Rect r(*i);
-    r.Offset(display_rect_.origin());
-    r.Offset(display_offset_);
-    // Center the rect vertically in |display_rect_|.
-    r.Offset(Point(0, (display_rect_.height() - r.height()) / 2));
     canvas->FillRectInt(selection_color, r.x(), r.y(), r.width(), r.height());
   }
 
   // Create a temporary copy of the style ranges for composition and selection.
   StyleRanges style_ranges(style_ranges_);
   ApplyCompositionAndSelectionStyles(&style_ranges);
 
   // Draw the text.
   Rect bounds(display_rect_);
-  bounds.Offset(display_offset_);
+  bounds.Offset(GetUpdatedDisplayOffset());
   for (StyleRanges::const_iterator i = style_ranges.begin();
        i < style_ranges.end(); ++i) {
     const Font& font = !i->underline ? i->font :
         i->font.DeriveFont(0, i->font.GetStyle() | Font::UNDERLINED);
     string16 text = text_.substr(i->range.start(), i->range.length());
     bounds.set_width(font.GetStringWidth(text));
     canvas->DrawStringInt(text, font, i->foreground, bounds);
 
     // Draw the strikethrough.
     if (i->strike) {
       SkPaint paint;
       paint.setAntiAlias(true);
       paint.setStyle(SkPaint::kFill_Style);
       paint.setColor(i->foreground);
       paint.setStrokeWidth(kStrikeWidth);
       canvas->AsCanvasSkia()->drawLine(SkIntToScalar(bounds.x()),
                                        SkIntToScalar(bounds.bottom()),
                                        SkIntToScalar(bounds.right()),
                                        SkIntToScalar(bounds.y()),
                                        paint);
     }
 
     bounds.set_x(bounds.x() + bounds.width());
   }
 
   // Paint cursor. Replace cursor is drawn as rectangle for now.
-  if (cursor_visible() && focused()) {
-    bounds = CursorBounds();
-    bounds.Offset(display_offset_);
-    if (!bounds.IsEmpty())
-      canvas->DrawRectInt(kCursorColor,
-                          bounds.x(),
-                          bounds.y(),
-                          bounds.width(),
-                          bounds.height());
-  }
+  Rect cursor(GetUpdatedCursorBounds());
+  if (cursor_visible() && focused() && !cursor.IsEmpty())
+    canvas->DrawRectInt(kCursorColor, cursor.x(), cursor.y(),
+                        cursor.width(), cursor.height());
 }
 
 SelectionModel RenderText::FindCursorPosition(const Point& point) {
   const Font& font = default_style_.font;
   int left = 0;
   int left_pos = 0;
   int right = font.GetStringWidth(text());
   int right_pos = text().length();
 
-  int x = point.x();
+  int x = point.x() - (display_rect_.x() + GetUpdatedDisplayOffset().x());
   if (x <= left) return SelectionModel(left_pos);
   if (x >= right) return SelectionModel(right_pos);
   // binary searching the cursor position.
   // TODO(oshima): use the center of character instead of edge.
   // Binary search may not work for language like arabic.
   while (std::abs(static_cast<long>(right_pos - left_pos)) > 1) {
     int pivot_pos = left_pos + (right_pos - left_pos) / 2;
     int pivot = font.GetStringWidth(text().substr(0, pivot_pos));
     if (pivot < x) {
       left = pivot;
       left_pos = pivot_pos;
     } else if (pivot == x) {
       return SelectionModel(pivot_pos);
     } else {
       right = pivot;
       right_pos = pivot_pos;
     }
   }
   return SelectionModel(left_pos);
 }
 
 std::vector<Rect> RenderText::GetSubstringBounds(
-    size_t from, size_t to) const {
+    size_t from, size_t to) {
   size_t start = std::min(from, to);
   size_t end = std::max(from, to);
   const Font& font = default_style_.font;
   int start_x = font.GetStringWidth(text().substr(0, start));
   int end_x = font.GetStringWidth(text().substr(0, end));
-  std::vector<Rect> bounds;
-  bounds.push_back(Rect(start_x, 0, end_x - start_x, font.GetHeight()));
-  return bounds;
+  Rect rect(start_x, 0, end_x - start_x, font.GetHeight());
+  rect.Offset(display_rect_.origin());
+  rect.Offset(GetUpdatedDisplayOffset());
+  // Center the rect vertically in |display_rect_|.
+  rect.Offset(Point(0, (display_rect_.height() - rect.height()) / 2));
+  return std::vector<Rect>(1, rect);
 }
 
 Rect RenderText::GetCursorBounds(const SelectionModel& selection,
                                  bool insert_mode) {
-  size_t cursor_pos = selection.selection_end();
-  const Font& font = default_style_.font;
-  int x = font.GetStringWidth(text_.substr(0U, cursor_pos));
-  DCHECK_GE(x, 0);
-  int h = std::min(display_rect_.height(), font.GetHeight());
-  Rect bounds(x, (display_rect_.height() - h) / 2, 1, h);
-  if (!insert_mode && text_.length() != cursor_pos)
-    bounds.set_width(font.GetStringWidth(text_.substr(0, cursor_pos + 1)) - x);
-  return bounds;
+  size_t from = selection.selection_end();
+  size_t to = insert_mode ? from : std::min(text_.length(), from + 1);
+  return GetSubstringBounds(from, to)[0];
 }
 
-const Rect& RenderText::CursorBounds() {
-  if (cursor_bounds_valid_ == false) {
-    UpdateCursorBoundsAndDisplayOffset();
-    cursor_bounds_valid_ = true;
-  }
+const Rect& RenderText::GetUpdatedCursorBounds() {
+  UpdateCachedBoundsAndOffset();
   return cursor_bounds_;
 }
 
 RenderText::RenderText()
     : text_(),
       selection_model_(),
       cursor_bounds_(),
-      cursor_bounds_valid_(false),
       cursor_visible_(false),
       insert_mode_(true),
       composition_range_(),
       style_ranges_(),
       default_style_(),
       display_rect_(),
-      display_offset_() {
+      display_offset_(),
+      cached_bounds_and_offset_valid_(false) {
+}
+
+const Point& RenderText::GetUpdatedDisplayOffset() {
+  UpdateCachedBoundsAndOffset();
+  return display_offset_;
 }
 
 SelectionModel RenderText::GetLeftSelectionModel(const SelectionModel& current,
                                                  BreakType break_type) {
   if (break_type == LINE_BREAK)
     return SelectionModel(0, 0, SelectionModel::LEADING);
   size_t pos = std::max(static_cast<long>(current.selection_end() - 1),
                         static_cast<long>(0));
   if (break_type == CHARACTER_BREAK)
     return SelectionModel(pos, pos, SelectionModel::LEADING);
@@ skipping 75 matching lines @@
     selection_style.range.set_end(MaxOfSelection());
     ApplyStyleRangeImpl(style_ranges, selection_style);
   }
 }
 
 bool RenderText::IsPositionAtWordSelectionBoundary(size_t pos) {
   return pos == 0 || (u_isalnum(text()[pos - 1]) && !u_isalnum(text()[pos])) ||
       (!u_isalnum(text()[pos - 1]) && u_isalnum(text()[pos]));
 }
 
-void RenderText::UpdateCursorBoundsAndDisplayOffset() {
-  cursor_bounds_ = GetCursorBounds(selection_model_, insert_mode());
+void RenderText::UpdateCachedBoundsAndOffset() {
+  if (cached_bounds_and_offset_valid_)
+    return;
+  cached_bounds_and_offset_valid_ = true;
+  cursor_bounds_ = GetCursorBounds(selection_model_, insert_mode_);
+  cursor_bounds_.set_width(std::max(cursor_bounds_.width(), 1));
   // Update |display_offset_| to ensure the current cursor is visible.
   int display_width = display_rect_.width();
   int string_width = GetStringWidth();
+  int delta_offset = 0;
   if (string_width < display_width) {
     // Show all text whenever the text fits to the size.
-    display_offset_.set_x(0);
-  } else if ((display_offset_.x() + cursor_bounds_.right()) > display_width) {
+    delta_offset = -display_offset_.x();
+  } else if (cursor_bounds_.right() > display_rect_.right()) {
     // Pan to show the cursor when it overflows to the right,
-     display_offset_.set_x(display_width - cursor_bounds_.right());
-  } else if ((display_offset_.x() + cursor_bounds_.x()) < 0) {
+    delta_offset = display_rect_.right() - cursor_bounds_.right();
+  } else if (cursor_bounds_.x() < display_rect_.x()) {
     // Pan to show the cursor when it overflows to the left.
-    display_offset_.set_x(-cursor_bounds_.x());
+    delta_offset = display_rect_.x() - cursor_bounds_.x();
   }
+  display_offset_.Offset(delta_offset, 0);
+  cursor_bounds_.Offset(delta_offset, 0);

[xji, 2011/08/10 00:36:15]

In the case of both cursor_bounds_ and display_offset_ need to be updated, looks
like GetCursorBounds() will compute the cursor_bounds_ using old
display_offset_, and the delta_offset will offset cursor_bounds_ to the correct
value. 

Then, does it mean GetCursorBounds() is not an accurate function to get the
correct cursor_bounds_ (if the value is not just used for comparison) and should
not be exposed as public API? Or it is ok for all the callers to get might be
overflowed cursor_bounds by GetCursorBounds()?

[msw, 2011/08/10 01:27:39]

Your first paragraph is correct, I've added a comment explaining that.

GetCursorBounds and its functional core, GetSubstringBounds, are okay as they
are. GetSubstringBounds calls GetUpdatedDisplayOffset(), which will fix the
current offset (and current cached cursor bounds) if needed, and then calculate
the correct substring bounds. If users call these functions for bounds that fall
outside the display rect, it won't re-adjust the offset to accommodate those
bounds, it'll just return results outside the display rect. It's up to the user
to check and handle that condition if it's relevant.

The only (fairly obvious) caveat is that: if a some client code caches their own
selection or cursor bounds (ex/ GetCursorBounds(new_cursor, insert)) before
calling SetSelectionModel(new_cursor) to update display_offset_, their cached
bounds could be stale. I think common sense, our API offering of
GetUpdatedCursorBounds, and our comment around cached_bounds_and_offset_valid_
cover this scenario. Do you have any additional ideas to safeguard against this?

[xji, 2011/08/10 18:15:54]

I looked at GetCursorBounds() again. And I think you are right since it calles
GetUpdatedDisplayOffset() which will update both cursor_bounds_ and
display_offset_ to the correct values if needed.
But I am confused by the above statement.
I do not have a better way (algorithm) wise to safeguard this on top of my head.

 }
 
 }  // namespace gfx