Use GetDisplayText() instead of text() for rendering text.

ui/gfx/render_text_mac.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_mac.h"
 
 #include <ApplicationServices/ApplicationServices.h>
 
 #include <algorithm>
 #include <cmath>
@@ skipping 76 matching lines @@
   return Range();
 }
 
 std::vector<Rect> RenderTextMac::GetSubstringBounds(const Range& range) {
   // TODO(asvitkine): Implement this. http://crbug.com/131618
   return std::vector<Rect>();
 }
 
 size_t RenderTextMac::TextIndexToDisplayIndex(size_t index) {
   // TODO(asvitkine): Implement this. http://crbug.com/131618
-  return index;
+  return std::min(index, GetDisplayText().size());
 }
 
 size_t RenderTextMac::DisplayIndexToTextIndex(size_t index) {
   // TODO(asvitkine): Implement this. http://crbug.com/131618
   return index;
 }
 
 bool RenderTextMac::IsValidCursorIndex(size_t index) {
   // TODO(asvitkine): Implement this. http://crbug.com/131618
   return IsValidLogicalIndex(index);
 }
 
 void RenderTextMac::OnLayoutTextAttributeChanged(bool text_changed) {
   DCHECK(!multiline()) << "RenderTextMac does not support multi line";
   if (text_changed) {
     if (elide_behavior() != NO_ELIDE &&
         elide_behavior() != FADE_TAIL &&
         !layout_text().empty()) {
       UpdateDisplayText(GetContentWidth());

[tapted, 2015/03/03 08:11:21]

Gosh this is hard to explain :/

But, I think the problem boils down to this line.

GetContentWidth() calls GetStringSizeF() [just above] which calls EnsureLayout()
[just below] which [now] calls GetDisplayText(), but the display text isn't
properly updated until *after* this line.

For example, if the content rect changes from zero to non-zero while the eliding
behavior is eliding (which is true whenever display_text != text, which is
always true when the width is 0), then an incorrect contentwidth will be passed
here.

[Jun Mukai, 2015/03/03 08:34:00]

I believe this is not a problem, it looks like cyclic, but it's not actually.
To work more precisely, I assume |text_elided_| to be false for the first time,
then GetContentWidth() invokes EnsureLayout(), which actually computes the size
based on layout_text() instead of display_text(). Then UpdateDisplayText()
computes the elided display_text() (and set |text_elided_| to true), and then
|line_| is cleared below, thus next EnsureLayout() will compute display_text()
correctly.

That said, I believe |text_elided_| should be set to false on render_text.cc
properly.

[tapted, 2015/03/03 09:30:32]

I think it is cyclic. But feels like it isn't because EnsureLayout() caches its
result :). That it, it cycles every time there is a "change".

As a proof, here add an extra UpdateDisplayText. i.e.

      UpdateDisplayText(GetContentWidth());
      line_.reset();
      UpdateDisplayText(GetContentWidth());

This will make the test in patchset 9 fail:

../../ui/gfx/render_text_unittest.cc:2633: Failure
Expected: (text.size()) > (static_cast<size_t>(glyph_count)), actual: 19 vs 19

[Jun Mukai, 2015/03/04 08:33:23]

You're right! Sorry for my previous comment.
So, GetStringSize() should be computed from layout_text_ while DrawVisualText()
should draw display_text_. this was done in RenderTextHarfBuzz but not correctly
done in RenderTextMac.

Uploaded a fix, which separates the size calculation and EnsureLayout(). PTAL.

[oshima, 2015/03/04 10:17:21]

FYI: This is cyclic by nature because you use the content width to compute the
text that determines the content width. This works because this is called only
after the text has been updated. The above scenario fails because it violates
that assumption.

One way to improve this would be to add separate GetLayoutTextWidth().

[Jun Mukai, 2015/03/05 00:33:52]

The latest patchset does that thing -- separating width for layout_text and
computing line_ for GetDisplayText(), since they can be different when elide
behavior is set.

     } else {
       UpdateDisplayText(0);
     }
   }
   line_.reset();
   attributes_.reset();
   runs_.clear();
   runs_valid_ = false;
 }
 
@@ skipping 14 matching lines @@
   const void* values[] = { ct_font };
   base::ScopedCFTypeRef<CFDictionaryRef> attributes(
       CFDictionaryCreate(NULL,
                          keys,
                          values,
                          arraysize(keys),
                          NULL,
                          &kCFTypeDictionaryValueCallBacks));
 
   base::ScopedCFTypeRef<CFStringRef> cf_text(
-      base::SysUTF16ToCFStringRef(text()));
+      base::SysUTF16ToCFStringRef(GetDisplayText()));
   base::ScopedCFTypeRef<CFAttributedStringRef> attr_text(
       CFAttributedStringCreate(NULL, cf_text, attributes));
   base::ScopedCFTypeRef<CFMutableAttributedStringRef> attr_text_mutable(
       CFAttributedStringCreateMutableCopy(NULL, 0, attr_text));
 
   // TODO(asvitkine|msw): Respect GetTextDirection(), which may not match the
   // natural text direction. See kCTTypesetterOptionForcedEmbeddingLevel, etc.
 
   ApplyStyles(attr_text_mutable, ct_font);
   line_.reset(CTLineCreateWithAttributedString(attr_text_mutable));
@@ skipping 196 matching lines @@
     CTUnderlineStyle value = kCTUnderlineStyleNone;
     if (underline && CFNumberGetValue(underline, kCFNumberSInt32Type, &value))
       run->underline = (value == kCTUnderlineStyleSingle);
 
     run_origin.offset(run_width, 0);
   }
   runs_valid_ = true;
 }
 
 }  // namespace gfx