RenderTextWin: Unroll the loop in LayoutTextRun for clarity

ui/gfx/render_text_win.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_win.h"
 
 #include <algorithm>
 
 #include "base/i18n/break_iterator.h"
 #include "base/i18n/char_iterator.h"
@@ skipping 1043 matching lines @@
     preceding_run_widths += run->width;
   }
   string_width_ = preceding_run_widths;
 }
 
 void RenderTextWin::LayoutTextRun(internal::TextRun* run) {
   const size_t run_length = run->range.length();
   const wchar_t* run_text = &(GetLayoutText()[run->range.start()]);
   Font original_font = run->font;
   LinkedFontsIterator fonts(original_font);
-  bool tried_cached_font = false;
-  bool tried_fallback = false;
+
+  run->logical_clusters.reset(new WORD[run_length]);
+
+  // Try to shape with the first font in the fallback list, which is
+  // |original_font|.
+  Font current_font;
+  fonts.NextFont(&current_font);
+  int missing_count = CountCharsWithMissingGlyphs(run,
+      ShapeTextRunWithFont(run, current_font));
+  if (missing_count == 0)
+    return;
+
   // Keep track of the font that is able to display the greatest number of
   // characters for which ScriptShape() returned S_OK. This font will be used
   // in the case where no font is able to display the entire run.
-  int best_partial_font_missing_char_count = INT_MAX;
-  Font best_partial_font = original_font;
-  Font current_font;
+  int best_partial_font_missing_char_count = missing_count;
+  Font best_partial_font = current_font;
 
-  run->logical_clusters.reset(new WORD[run_length]);
-  while (fonts.NextFont(&current_font)) {
-    HRESULT hr = ShapeTextRunWithFont(run, current_font);
-
-    bool glyphs_missing = false;
-    if (hr == USP_E_SCRIPT_NOT_IN_FONT) {
-      glyphs_missing = true;
-    } else if (hr == S_OK) {
-      // If |hr| is S_OK, there could still be missing glyphs in the output.
-      // http://msdn.microsoft.com/en-us/library/windows/desktop/dd368564.aspx
-      const int missing_count = CountCharsWithMissingGlyphs(run);
-      // Track the font that produced the least missing glyphs.
-      if (missing_count < best_partial_font_missing_char_count) {
-        best_partial_font_missing_char_count = missing_count;
-        best_partial_font = run->font;
-      }
-      glyphs_missing = (missing_count != 0);
-    } else {
-      NOTREACHED() << hr;
-    }
-
-    // Use the font if it had glyphs for all characters.
-    if (!glyphs_missing) {
-      // Save the successful fallback font that was chosen.
-      if (tried_fallback)
-        successful_substitute_fonts_[original_font.GetFontName()] = run->font;
+  // Try to shape with the cached font from previous runs, if any.
+  std::map<std::string, Font>::const_iterator it =
+      successful_substitute_fonts_.find(original_font.GetFontName());
+  if (it != successful_substitute_fonts_.end()) {
+    current_font = it->second;
+    missing_count = CountCharsWithMissingGlyphs(run,
+        ShapeTextRunWithFont(run, current_font));
+    if (missing_count == 0)
       return;
-    }
-
-    // First, try the cached font from previous runs, if any.
-    if (!tried_cached_font) {
-      tried_cached_font = true;
-
-      std::map<std::string, Font>::const_iterator it =
-          successful_substitute_fonts_.find(original_font.GetFontName());
-      if (it != successful_substitute_fonts_.end()) {
-        fonts.SetNextFont(it->second);
-        continue;
-      }
-    }
-
-    // If there are missing glyphs, first try finding a fallback font using a
-    // meta file, if it hasn't yet been attempted for this run.
-    // TODO(msw|asvitkine): Support RenderText's font_list()?
-    if (!tried_fallback) {
-      tried_fallback = true;
-
-      Font fallback_font;
-      if (ChooseFallbackFont(cached_hdc_, run->font, run_text, run_length,
-                             &fallback_font)) {
-        fonts.SetNextFont(fallback_font);
-        continue;
-      }
+    if (missing_count < best_partial_font_missing_char_count) {
+      best_partial_font_missing_char_count = missing_count;
+      best_partial_font = current_font;
     }
   }
 
+  // Try finding a fallback font using a meta file.
+  // TODO(msw|asvitkine): Support RenderText's font_list()?
+  if (ChooseFallbackFont(cached_hdc_, run->font, run_text, run_length,
+                         &current_font)) {
+    missing_count = CountCharsWithMissingGlyphs(run,
+        ShapeTextRunWithFont(run, current_font));

[Alexei Svitkine (slow), 2014/07/15 19:48:20]

If these two functions are always called together now, just move the inner call
inside the outer and rename the outer function.

I also wonder if the other parts of the logic can be added to the function (i.e.
it could be responsible for updating best_partial_font and
best_partial_font_missing_char_count). It could take those vars as extra params
or perhaps you could introduce a State struct and pass that instead? Would make
it even more readable.

Maybe just make a new short function:

size_t RenderTextWin::TryFallbackFont(const Font& font,
                                      FontFallbackState* state) {
  HRESULT shaping_result = ShapeTextRunWithFont(state->run, font);
  size_t missing_count = (shaping_result != S_OK) ? INT_MAX :
      CountCharsWithMissingGlyphs(state->run);
  if (missing_count < state->best_partial_font_missing_char_count) {
    state->best_partial_font_missing_char_count = missing_count;
    state->best_partial_font = current_font;
  }
  return missing_count;
}

+    if (missing_count == 0) {
+      successful_substitute_fonts_[original_font.GetFontName()] = current_font;
+      return;
+    }
+    if (missing_count < best_partial_font_missing_char_count) {
+      best_partial_font_missing_char_count = missing_count;
+      best_partial_font = current_font;
+    }
+  }
+
+  // Try the rest of fonts in the fallback list.
+  while (fonts.NextFont(&current_font)) {

[Alexei Svitkine (slow), 2014/07/15 19:48:20]

Now that you're not calling SetNextFont() on |fonts|, can that method be
removed?

+    missing_count = CountCharsWithMissingGlyphs(run,
+        ShapeTextRunWithFont(run, current_font));
+    if (missing_count == 0) {
+      successful_substitute_fonts_[original_font.GetFontName()] = current_font;
+      return;
+    }
+    if (missing_count < best_partial_font_missing_char_count) {
+      best_partial_font_missing_char_count = missing_count;
+      best_partial_font = current_font;
+    }
+  }
+
   // If a font was able to partially display the run, use that now.
   if (best_partial_font_missing_char_count < static_cast<int>(run_length)) {
     // Re-shape the run only if |best_partial_font| differs from the last font.
     if (best_partial_font.GetNativeFont() != run->font.GetNativeFont())
       ShapeTextRunWithFont(run, best_partial_font);
     return;
   }
 
   // If no font was able to partially display the run, replace all glyphs
   // with |wgDefault| from the original font to ensure to they don't hold
@@ skipping 67 matching lines @@
     hr = ScriptShape(cached_hdc_, &run->script_cache, run_text, run_length,
                      max_glyphs, &run->script_analysis, run->glyphs.get(),
                      run->logical_clusters.get(), run->visible_attributes.get(),
                      &run->glyph_count);
     // Ensure that |kMaxGlyphs| is attempted and the loop terminates afterward.
     max_glyphs = std::max(max_glyphs + 1, std::min(max_glyphs * 2, kMaxGlyphs));
   }
   return hr;
 }
 
-int RenderTextWin::CountCharsWithMissingGlyphs(internal::TextRun* run) const {
+int RenderTextWin::CountCharsWithMissingGlyphs(internal::TextRun* run,
+                                               HRESULT shaping_result) const {
+  if (shaping_result != S_OK) {
+    DCHECK_EQ(shaping_result, USP_E_SCRIPT_NOT_IN_FONT);

[Alexei Svitkine (slow), 2014/07/15 19:48:20]

Nit: The expected value constant should be the first param to DCHECK_EQ.

+    return INT_MAX;

[Alexei Svitkine (slow), 2014/07/15 19:48:20]

What does it mean for INT_MAX to be returned by this? Please document in the
header comment.

+  }
+
+  // If |hr| is S_OK, there could still be missing glyphs in the output.
+  // http://msdn.microsoft.com/en-us/library/windows/desktop/dd368564.aspx
   int chars_not_missing_glyphs = 0;
   SCRIPT_FONTPROPERTIES properties;
   memset(&properties, 0, sizeof(properties));
   properties.cBytes = sizeof(properties);
   ScriptGetFontProperties(cached_hdc_, &run->script_cache, &properties);
 
   const wchar_t* run_text = &(GetLayoutText()[run->range.start()]);
   for (size_t char_index = 0; char_index < run->range.length(); ++char_index) {
     const int glyph_index = run->logical_clusters[char_index];
     DCHECK_GE(glyph_index, 0);
@@ skipping 53 matching lines @@
   size_t position = LayoutIndexToTextIndex(run->range.end());
   position = IndexOfAdjacentGrapheme(position, CURSOR_BACKWARD);
   return SelectionModel(position, CURSOR_FORWARD);
 }
 
 RenderText* RenderText::CreateNativeInstance() {
   return new RenderTextWin;
 }
 
 }  // namespace gfx