Add a cache for FillFontFamilyMap.

chrome/browser/font_family_cache.cc

+// Copyright 2014 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 "chrome/browser/font_family_cache.h"
+
+#include <map>
+
+#include "base/prefs/pref_service.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.cc"
+#include "chrome/common/pref_font_webkit_names.h"
+#include "chrome/common/pref_names.h"
+
+namespace chrome {
+
+const char* kFontFamilyCacheKey = "FontFamilyCacheKey";

[Lei Zhang, 2014/08/05 19:26:49]

const char kFontFamilyCacheKey[]

[erikchen, 2014/08/05 20:49:22]

Done.

+
+FontFamilyCache::FontFamilyCache(PrefService* prefs) : prefs_(prefs) {
+  profile_pref_registrar_.Init(prefs);
+}
+
+FontFamilyCache::~FontFamilyCache() {
+}
+
+void FontFamilyCache::FillFontFamilyMap(const char* map_name,
+                                        content::ScriptFontFamilyMap* map) {
+  // TODO(falken): Get rid of the brute-force scan over possible
+  // (font family / script) combinations - see http://crbug.com/308095.
+  for (size_t i = 0; i < prefs::kWebKitScriptsForFontFamilyMapsLength; ++i) {
+    const char* script = prefs::kWebKitScriptsForFontFamilyMaps[i];
+    base::string16 result = FetchAndCacheFont(script, map_name);
+    if (!result.empty())
+      (*map)[script] = result;
+  }
+}
+
+base::string16 FontFamilyCache::FetchFont(const char* script,
+                                          const char* map_name) {
+  std::string pref_name = base::StringPrintf("%s.%s", map_name, script);
+  std::string font = prefs_->GetString(pref_name.c_str());
+  base::string16 font16 = base::UTF8ToUTF16(font);
+
+  // Lazily constructs the map if it doesn't already exist.
+  ScriptFontMap& map = font_family_map_[map_name];
+  map[script] = font16;
+
+  // Register for profile preference changes.
+  profile_pref_registrar_.Add(
+      pref_name.c_str(),
+      base::Bind(&FontFamilyCache::OnPrefsChanged, base::Unretained(this)));
+  return font16;
+}
+
+base::string16 FontFamilyCache::FetchAndCacheFont(const char* script,
+                                                  const char* map_name) {
+  FontFamilyMap::const_iterator it = font_family_map_.find(map_name);
+  if (it != font_family_map_.end()) {
+    ScriptFontMap::const_iterator it2 = it->second.find(script);
+    if (it2 != it->second.end())
+      return it2->second;
+  }
+
+  return FetchFont(script, map_name);
+}
+
+// There are ~1000 entries in the cache. Avoid unnecessary object construction,
+// including std::string.
+void FontFamilyCache::OnPrefsChanged(const std::string& pref_name) {
+  // The delimiter is '.'
+  size_t delimiter_length = 1;

[Lei Zhang, 2014/08/05 19:26:49]

nit: const

[erikchen, 2014/08/05 20:49:23]

Done.

+  for (FontFamilyMap::iterator it = font_family_map_.begin();

[Lei Zhang, 2014/08/05 19:26:49]

Not sure why you are iterating through the key values when you can just do a
find(). How about:

std::vector<std::string> pref_name_split = base::SplitString(pref_name, '.',
&pref_name_split);
if (pref_name_split.size() != 2)
  return;
FontFamilyMap::iterator map_it = font_family_map_.find(pref_name_split[0]);
if (map_it == font_family_map_.end()
  return;
ScriptFontMap& map = it->second;
size_t erased = map.erase(pref_name_split[1]);
if (erased == 0)
  return;
profile_pref_registrar_.Remove(pref_name.c_str());

[erikchen, 2014/08/05 20:49:23]

I tried out your suggestion. For find() to work, a non-default key comparator
needs to be passed as a template argument to the hash_map. We use different
implementations of hash_map on different platforms, so there's no simple way to
do this. It would also introduce insertion/lookup inefficiencies. By relying on
the assumption that the keys are compile time constants, the current hash_map
uses pointer comparison instead of cstring comparison.

std::find works, but requires us to define a custom comparator anyways, at which
point we're not saving on complexity or lines of code.

I added further documentation to the header file.

[Lei Zhang, 2014/08/05 21:15:08]

Ok, but can't you still split the pref_name to help you simplify the code a
little?

[erikchen, 2014/08/05 21:34:16]

Not without making another assumption about delimiters. Separating out
|map_name| from |script| in |pref_name| relies on the assumption that |script|
does not contain any '.' (map_name contains many '.'). This assumption is
currently true, but I didn't see a need to introduce another assumption if its
unneeded.

If you prefer, I can update the code to use that assumption and separate out
|script| and |map_name| from |pref_name|

[Lei Zhang, 2014/08/05 21:48:55]

Ah, n/m, I had a mental model that |pref_name| is just foo.bar.

+       it != font_family_map_.end();
+       ++it) {
+    const char* map_name = it->first;
+    size_t map_name_length = strlen(map_name);
+
+    // If the map name doesn't match, move on.
+    if (pref_name.compare(0, map_name_length, map_name) != 0)
+      continue;
+
+    ScriptFontMap& map = it->second;
+    for (ScriptFontMap::iterator it2 = map.begin(); it2 != map.end(); ++it2) {
+      const char* script = it2->first;
+      size_t script_length = strlen(script);
+
+      // If the length doesn't match, move on.
+      if (pref_name.size() !=
+          map_name_length + script_length + delimiter_length)
+        continue;
+
+      // If the script doesn't match, move on.
+      if (pref_name.compare(
+              map_name_length + delimiter_length, script_length, script) != 0)
+        continue;
+
+      // Clear the cache and the observer.
+      map.erase(it2);
+      profile_pref_registrar_.Remove(pref_name.c_str());
+      break;
+    }
+  }
+}
+
+}  // namespace chrome