Uses and returns the fractional width in text eliding

ui/gfx/text_elider_unittest.cc

Index: ui/gfx/text_elider_unittest.cc
diff --git a/ui/gfx/text_elider_unittest.cc b/ui/gfx/text_elider_unittest.cc
index 2a90e5d73cbcca9366f4fa2eeab1a8246fb97284..2d29dc9eb522da80b4e02d7d34bf2ca5b6571c51 100644
--- a/ui/gfx/text_elider_unittest.cc
+++ b/ui/gfx/text_elider_unittest.cc
@@ -13,6 +13,8 @@
 #include "base/strings/utf_string_conversions.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gfx/font.h"
+#include "ui/gfx/font_list.h"
+#include "ui/gfx/text_utils.h"
 #include "url/gurl.h"
 
 namespace gfx {
@@ -40,6 +42,11 @@ struct TestData {
   const int compare_result;
 };
 
+float GetStringWidth(const string16& string, const Font& font) {

[msw, 2013/09/27 21:54:48]

All the callsites for this function should construct FontLists instead of
constructing a Font and then making a FontList out of that. Also, each callsite
can potentially use FontList::GetStringWidth or gfx::GetStringWidth directly.

[jianli, 2013/10/01 00:32:58]

Done.

+  gfx::FontList font_list(font);

[msw, 2013/09/27 21:54:48]

nit: inline this, or make it const.

[jianli, 2013/10/01 00:32:58]

Not needed. Removed.

+  return gfx::GetStringWidth(string, font_list);
+}
+
 void RunUrlTest(Testcase* testcases, size_t num_testcases) {
   static const gfx::Font font;
   for (size_t i = 0; i < num_testcases; ++i) {
@@ -48,29 +55,15 @@ void RunUrlTest(Testcase* testcases, size_t num_testcases) {
     // That's kinda redundant with net_util_unittests.
     EXPECT_EQ(UTF8ToUTF16(testcases[i].output),
               ElideUrl(url, font,
-                       font.GetStringWidth(UTF8ToUTF16(testcases[i].output)),
+                       GetStringWidth(UTF8ToUTF16(testcases[i].output), font),
                        std::string()));
   }
 }
 
-gfx::Font GetTestingFont() {
-  gfx::Font font;
-#if defined(OS_MACOSX)
-  // Use a specific font for certain tests on Mac.
-  // 1) Different Mac machines might be configured with different default font.
-  //    The number of extra pixels needed to make ElideEmail/TestFilenameEliding
-  //    tests work might vary per the default font.
-  // 2) This specific font helps expose the line width exceeding problem as in
-  //    ElideRectangleTextCheckLineWidth.
-  font = gfx::Font("LucidaGrande", 12);
-#endif
-  return font;
-}
-
 }  // namespace
 
-// TODO(ios): Complex eliding is off by one for some of those tests on iOS.
-// See crbug.com/154019
+// TODO(ios): This test fails on iOS because iOS version of gfx::GetStringWidth
+// that calls [NSString sizeWithFont] returns the rounded string width.
 #if defined(OS_IOS)
 #define MAYBE_ElideEmail DISABLED_ElideEmail
 #else
@@ -121,22 +114,13 @@ TEST(TextEliderTest, MAYBE_ElideEmail) {
       {"mmmmm@llllllllll", "m" + kEllipsisStr + "@l" + kEllipsisStr},
   };
 
-  const gfx::Font font = GetTestingFont();
+  const gfx::Font font;
   for (size_t i = 0; i < arraysize(testcases); ++i) {
     const string16 expected_output = UTF8ToUTF16(testcases[i].output);
-    int available_width = font.GetStringWidth(expected_output);
-#if defined(OS_MACOSX)
-    // Give two extra pixels to offset the ceiling width returned by
-    // GetStringWidth on Mac. This workaround will no longer be needed once
-    // the floating point width is adopted (http://crbug.com/288987).
-    // Note that we need one more pixel than TestFilenameEliding because
-    // multiple strings are elided and we need to offset more.
-    available_width += 2;
-#endif
     EXPECT_EQ(expected_output,
               ElideEmail(UTF8ToUTF16(testcases[i].input),
                          font,
-                         available_width));
+                         GetStringWidth(expected_output, font)));
   }
 }
 
@@ -201,15 +185,17 @@ TEST(TextEliderTest, TestTrailingEllipsisSlashEllipsisHack) {
   // Very little space, would cause double ellipsis.
   gfx::Font font;
   GURL url("http://battersbox.com/directory/foo/peter_paul_and_mary.html");
-  int available_width = font.GetStringWidth(
-      UTF8ToUTF16("battersbox.com/" + kEllipsisStr + "/" + kEllipsisStr));
+  float available_width = GetStringWidth(
+      UTF8ToUTF16("battersbox.com/" + kEllipsisStr + "/" + kEllipsisStr),
+      font);
 
   // Create the expected string, after elision. Depending on font size, the
   // directory might become /dir... or /di... or/d... - it never should be
   // shorter than that. (If it is, the font considers d... to be longer
   // than .../... -  that should never happen).
-  ASSERT_GT(font.GetStringWidth(UTF8ToUTF16(kEllipsisStr + "/" + kEllipsisStr)),
-      font.GetStringWidth(UTF8ToUTF16("d" + kEllipsisStr)));
+  ASSERT_GT(
+      GetStringWidth(UTF8ToUTF16(kEllipsisStr + "/" + kEllipsisStr), font),
+      GetStringWidth(UTF8ToUTF16("d" + kEllipsisStr), font));
   GURL long_url("http://battersbox.com/directorynameisreallylongtoforcetrunc");
   string16 expected = ElideUrl(long_url, font, available_width, std::string());
   // Ensure that the expected result still contains part of the directory name.
@@ -288,7 +274,8 @@ TEST(TextEliderTest, TestFileURLEliding) {
   RunUrlTest(testcases, arraysize(testcases));
 }
 
-// TODO(ios): Complex eliding is off by one for some of those tests on iOS.
+// TODO(ios): This test fails on iOS because iOS version of gfx::GetStringWidth
+// that calls [NSString sizeWithFont] returns the rounded string width.
 // See crbug.com/154019
 #if defined(OS_IOS)
 #define MAYBE_TestFilenameEliding DISABLED_TestFilenameEliding
@@ -334,28 +321,23 @@ TEST(TextEliderTest, MAYBE_TestFilenameEliding) {
       "file.name.re" + kEllipsisStr + "emelylongext"}
   };
 
-  static const gfx::Font font = GetTestingFont();
+  static const gfx::Font font;
   for (size_t i = 0; i < arraysize(testcases); ++i) {
     base::FilePath filepath(testcases[i].input);
     string16 expected = UTF8ToUTF16(testcases[i].output);
     expected = base::i18n::GetDisplayStringInLTRDirectionality(expected);
-    int available_width = font.GetStringWidth(UTF8ToUTF16(testcases[i].output));
-#if defined(OS_MACOSX)
-    // Give one extra pixel to offset the ceiling width returned by
-    // GetStringWidth on Mac. This workaround will no longer be needed once
-    // the floating point width is adopted (http://crbug.com/288987).
-    available_width += 1;
-#endif
+    float available_width = GetStringWidth(UTF8ToUTF16(testcases[i].output),
+                                           font);
     EXPECT_EQ(expected, ElideFilename(filepath, font, available_width));
   }
 }
 
 TEST(TextEliderTest, ElideTextTruncate) {
   const gfx::Font font;
-  const int kTestWidth = font.GetStringWidth(ASCIIToUTF16("Test"));
+  const float kTestWidth = GetStringWidth(ASCIIToUTF16("Test"), font);
   struct TestData {
     const char* input;
-    int width;
+    float width;
     const char* output;
   } cases[] = {
     { "", 0, "" },
@@ -375,12 +357,12 @@ TEST(TextEliderTest, ElideTextTruncate) {
 
 TEST(TextEliderTest, ElideTextEllipsis) {
   const gfx::Font font;
-  const int kTestWidth = font.GetStringWidth(ASCIIToUTF16("Test"));
+  const float kTestWidth = GetStringWidth(ASCIIToUTF16("Test"), font);
   const char* kEllipsis = "\xE2\x80\xA6";
-  const int kEllipsisWidth = font.GetStringWidth(UTF8ToUTF16(kEllipsis));
+  const float kEllipsisWidth = GetStringWidth(UTF8ToUTF16(kEllipsis), font);
   struct TestData {
     const char* input;
-    int width;
+    float width;
     const char* output;
   } cases[] = {
     { "", 0, "" },
@@ -424,14 +406,14 @@ TEST(TextEliderTest, ElideTextSurrogatePairs) {
   const std::string kSurrogate = "\xF0\x9D\x84\x9E";
   const string16 kTestString =
       UTF8ToUTF16(kSurrogate + "ab" + kSurrogate + kSurrogate + "cd");
-  const int kTestStringWidth = font.GetStringWidth(kTestString);
+  const float kTestStringWidth = GetStringWidth(kTestString, font);
   const char16 kSurrogateFirstChar = kTestString[0];
   const char16 kSurrogateSecondChar = kTestString[1];
   string16 result;
 
   // Elide |kTextString| to all possible widths and check that no instance of
   // |kSurrogate| was split in two.
-  for (int width = 0; width <= kTestStringWidth; width++) {
+  for (float width = 0; width <= kTestStringWidth; width += 1) {

[msw, 2013/09/27 21:54:48]

nit: ++width or leave this as it was. Why "+=1"?

[jianli, 2013/10/01 00:32:58]

I though this did not work for float. It turned out I was wrong. Corrected.

     result = ElideText(kTestString, font, width, TRUNCATE_AT_END);
     CheckSurrogatePairs(result, kSurrogateFirstChar, kSurrogateSecondChar);
 
@@ -468,13 +450,13 @@ TEST(TextEliderTest, ElideTextLongStrings) {
   };
 
   const gfx::Font font;
-  int ellipsis_width = font.GetStringWidth(kEllipsisStr);
+  float ellipsis_width = gfx::GetStringWidth(kEllipsisStr, FontList(font));
   for (size_t i = 0; i < arraysize(testcases_end); ++i) {
     // Compare sizes rather than actual contents because if the test fails,
     // output is rather long.
     EXPECT_EQ(testcases_end[i].output.size(),
               ElideText(testcases_end[i].input, font,
-                        font.GetStringWidth(testcases_end[i].output),
+                        GetStringWidth(testcases_end[i].output, font),
                         ELIDE_AT_END).size());
     EXPECT_EQ(kEllipsisStr,
               ElideText(testcases_end[i].input, font, ellipsis_width,
@@ -499,7 +481,7 @@ TEST(TextEliderTest, ElideTextLongStrings) {
     // output is rather long.
     EXPECT_EQ(testcases_middle[i].output.size(),
               ElideText(testcases_middle[i].input, font,
-                        font.GetStringWidth(testcases_middle[i].output),
+                        GetStringWidth(testcases_middle[i].output, font),
                         ELIDE_AT_END).size());
     EXPECT_EQ(kEllipsisStr,
               ElideText(testcases_middle[i].input, font, ellipsis_width,
@@ -581,13 +563,13 @@ TEST(TextEliderTest, ElideString) {
 
 TEST(TextEliderTest, ElideRectangleText) {
   const gfx::Font font;
-  const int line_height = font.GetHeight();
-  const int test_width = font.GetStringWidth(ASCIIToUTF16("Test"));
+  const float line_height = font.GetHeight();
+  const float test_width = GetStringWidth(ASCIIToUTF16("Test"), font);
 
   struct TestData {
     const char* input;
-    int available_pixel_width;
-    int available_pixel_height;
+    float available_pixel_width;
+    float available_pixel_height;
     bool truncated_y;
     const char* output;
   } cases[] = {
@@ -638,14 +620,14 @@ TEST(TextEliderTest, ElideRectangleText) {
 
 TEST(TextEliderTest, ElideRectangleTextPunctuation) {
   const gfx::Font font;
-  const int line_height = font.GetHeight();
-  const int test_width = font.GetStringWidth(ASCIIToUTF16("Test"));
-  const int test_t_width = font.GetStringWidth(ASCIIToUTF16("Test T"));
+  const float line_height = font.GetHeight();
+  const float test_width = GetStringWidth(ASCIIToUTF16("Test"), font);
+  const float test_t_width = GetStringWidth(ASCIIToUTF16("Test T"), font);
 
   struct TestData {
     const char* input;
-    int available_pixel_width;
-    int available_pixel_height;
+    float available_pixel_width;
+    float available_pixel_height;
     bool wrap_words;
     bool truncated_x;
     const char* output;
@@ -678,14 +660,14 @@ TEST(TextEliderTest, ElideRectangleTextPunctuation) {
 
 TEST(TextEliderTest, ElideRectangleTextLongWords) {
   const gfx::Font font;
-  const int kAvailableHeight = 1000;
+  const float kAvailableHeight = 1000;
   const string16 kElidedTesting = UTF8ToUTF16(std::string("Tes") + kEllipsis);
-  const int elided_width = font.GetStringWidth(kElidedTesting);
-  const int test_width = font.GetStringWidth(ASCIIToUTF16("Test"));
+  const float elided_width = GetStringWidth(kElidedTesting, font);
+  const float test_width = GetStringWidth(ASCIIToUTF16("Test"), font);
 
   struct TestData {
     const char* input;
-    int available_pixel_width;
+    float available_pixel_width;
     WordWrapBehavior wrap_behavior;
     bool truncated_x;
     const char* output;
@@ -735,24 +717,19 @@ TEST(TextEliderTest, ElideRectangleTextLongWords) {
   }
 }
 
-// TODO(ios): Complex eliding is off by one for some of those tests on iOS.
-// See crbug.com/154019
-#if defined(OS_IOS)
-#define MAYBE_ElideRectangleTextCheckLineWidth \
-    DISABLED_ElideRectangleTextCheckLineWidth
-#else
-#define MAYBE_ElideRectangleTextCheckLineWidth ElideRectangleTextCheckLineWidth
-#endif
-
 // This test is to make sure that the width of each wrapped line does not
 // exceed the available width. On some platform like Mac, this test used to
 // fail because the truncated integer width is returned for the string
 // and the accumulation of the truncated values causes the elide function
 // to wrap incorrectly.
-TEST(TextEliderTest, MAYBE_ElideRectangleTextCheckLineWidth) {
-  gfx::Font font = GetTestingFont();
-  const int kAvailableWidth = 235;
-  const int kAvailableHeight = 1000;
+TEST(TextEliderTest, ElideRectangleTextCheckLineWidth) {
+  gfx::Font font;
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  // Use a specific font to expose the line width exceeding problem.
+  font = gfx::Font("LucidaGrande", 12);
+#endif
+  const float kAvailableWidth = 235;
+  const float kAvailableHeight = 1000;
   const char text[] = "that Russian place we used to go to after fencing";
   std::vector<string16> lines;
   EXPECT_EQ(0, ElideRectangleText(UTF8ToUTF16(text),
@@ -762,8 +739,8 @@ TEST(TextEliderTest, MAYBE_ElideRectangleTextCheckLineWidth) {
                                   WRAP_LONG_WORDS,
                                   &lines));
   ASSERT_EQ(2u, lines.size());
-  EXPECT_LE(font.GetStringWidth(lines[0]), kAvailableWidth);
-  EXPECT_LE(font.GetStringWidth(lines[1]), kAvailableWidth);
+  EXPECT_LE(GetStringWidth(lines[0], font), kAvailableWidth);
+  EXPECT_LE(GetStringWidth(lines[1], font), kAvailableWidth);
 }
 
 TEST(TextEliderTest, ElideRectangleString) {