[MD settings] adding c++ side of font settings

chrome/browser/ui/webui/settings/settings_font_handler.cc

Index: chrome/browser/ui/webui/settings/settings_font_handler.cc
diff --git a/chrome/browser/ui/webui/settings/settings_font_handler.cc b/chrome/browser/ui/webui/settings/settings_font_handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3f2ffbfd5640285185d39a73977091b73b3c8982
--- /dev/null
+++ b/chrome/browser/ui/webui/settings/settings_font_handler.cc
@@ -0,0 +1,129 @@
+// 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 "chrome/browser/ui/webui/settings/settings_font_handler.h"
+
+#include "base/basictypes.h"
+#include "base/bind.h"
+#include "base/i18n/rtl.h"
+#include "base/prefs/pref_service.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/character_encoding.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/webui/options/font_settings_utils.h"
+#include "chrome/common/pref_names.h"
+#include "content/public/browser/font_list_async.h"
+#include "content/public/browser/web_ui.h"
+
+#if defined(OS_WIN)
+#include "ui/gfx/font.h"
+#include "ui/gfx/platform_font_win.h"
+#endif
+
+namespace {
+
+// Returns the localized name of a font so that settings can find it within the
+// list of system fonts. On Windows, the list of system fonts has names only
+// for the system locale, but the pref value may be in the English name.
+std::string MaybeGetLocalizedFontName(const std::string& font_name) {

[dschuyler, 2015/10/12 23:24:51]

I'm taking it on face value that this is still needed.  The older options code
performed this step, and I'm mimicking it here.

[Dan Beam, 2015/10/15 02:34:41]

probably is

[dschuyler, 2015/10/15 21:54:20]

Acknowledged.

+#if defined(OS_WIN)
+  gfx::Font font(font_name, 12);  // dummy font size
+  return static_cast<gfx::PlatformFontWin*>(
+      font.platform_font())->GetLocalizedFontName();
+#else
+  return font_name;
+#endif
+}
+
+}  // namespace
+
+namespace settings {
+
+SettingsFontHandler::SettingsFontHandler(content::WebUI* webui) {
+  // Perform validation for saved fonts.
+  PrefService* pref_service = Profile::FromWebUI(webui)->GetPrefs();
+  options::FontSettingsUtilities::ValidateSavedFonts(pref_service);
+
+  // Register for preferences that we need to observe manually.
+  font_encoding_.Init(prefs::kDefaultCharset, pref_service);
+
+  standard_font_.Init(prefs::kWebKitStandardFontFamily, pref_service);
+  serif_font_.Init(prefs::kWebKitSerifFontFamily, pref_service);
+  sans_serif_font_.Init(prefs::kWebKitSansSerifFontFamily, pref_service);
+  fixed_font_.Init(prefs::kWebKitFixedFontFamily, pref_service);

[dschuyler, 2015/10/12 23:24:51]

We could pull out these values and strictly use the entries from the prefs_util
code to determine the current selection.  My concern is that simplifying this
code would complicate JavaScript code.

[Dan Beam, 2015/10/15 02:34:41]

i think it'd be pretty simple to bind prefs={{prefs}} and listen for changes to:

  prefs.webkit.webprefs.fonts.*

probably as simple if not simpler than this C++.

the real question is whether we'd like to port the logic (i.e. getting the
localized font, determining if there are strong RTL chars).

fwiw: there seems to be a fontSettings extension API -- would it be worth adding
to that?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

[dschuyler, 2015/10/15 21:54:20]

These have been removed with the change to use the prefs_util entries.
The font_settings_api sounds interesting.

Done.

+}
+
+SettingsFontHandler::~SettingsFontHandler() {}
+
+void SettingsFontHandler::RegisterMessages() {
+  web_ui()->RegisterMessageCallback(
+      "fetchFontsData", base::Bind(&SettingsFontHandler::HandleFetchFontsData,
+                                   base::Unretained(this)));
+}
+
+void SettingsFontHandler::HandleFetchFontsData(const base::ListValue* args) {
+  content::GetFontListAsync(base::Bind(&SettingsFontHandler::FontListHasLoaded,
+                                       base::Unretained(this)));

[Dan Beam, 2015/10/15 02:34:41]

this class should use a WeakPtrFactory<> and instead of base::Unretained(this)
we should be doing weak_ptr_factory_.GetWeakPtr();

i have no idea how this code isn't causing crashes like this

annnnnnnnnnd I was right and here's the fix:
https://codereview.chromium.org/1397903004/

[dschuyler, 2015/10/15 21:54:19]

Done.

+}
+
+void SettingsFontHandler::FontListHasLoaded(scoped_ptr<base::ListValue> list) {
+  // Font list.  Selects the directionality for the fonts in the given list.
+  for (size_t i = 0; i < list->GetSize(); i++) {
+    base::ListValue* font;
+    bool has_font = list->GetList(i, &font);
+    DCHECK(has_font);

[Dan Beam, 2015/10/15 02:34:41]

\n

[dschuyler, 2015/10/15 21:54:19]

Done.

+    base::string16 value;
+    bool has_value = font->GetString(1, &value);
+    DCHECK(has_value);

[Dan Beam, 2015/10/15 02:34:41]

\n

[dschuyler, 2015/10/15 21:54:20]

Done.

+    bool has_rtl_chars = base::i18n::StringContainsStrongRTLChars(value);
+    font->Append(new base::StringValue(has_rtl_chars ? "rtl" : "ltr"));

[dschuyler, 2015/10/12 23:24:51]

Identifying the rtl and ltr fonts is something I'm pulling forward from the
older options code without yet confirming if it's required.

[Dan Beam, 2015/10/15 02:34:41]

this is mildly weird to me.  this is checking for an RTL character in the title
of the font.  but it doesn't check the font's character set.  whatevs

[dschuyler, 2015/10/15 21:54:20]

Acknowledged.

+  }
+
+  // Character encoding list.
+  base::ListValue encoding_list;

[Dan Beam, 2015/10/15 02:34:41]

declare lower

[dschuyler, 2015/10/15 21:54:19]

Done.

+  const std::vector<CharacterEncoding::EncodingInfo>* encodings;
+  PrefService* pref_service = Profile::FromWebUI(web_ui())->GetPrefs();
+  encodings = CharacterEncoding::GetCurrentDisplayEncodings(
+      g_browser_process->GetApplicationLocale(),
+      pref_service->GetString(prefs::kStaticEncodings),
+      pref_service->GetString(prefs::kRecentlySelectedEncoding));
+  DCHECK(encodings);

[Dan Beam, 2015/10/15 02:34:41]

nit: DCHECK(ptr); followed by DCHECK(ptr->DoSomething()); seems silly to me as
ptr->DoSomething() would crash anyways...

[dschuyler, 2015/10/15 21:54:20]

Done.

+  DCHECK(!encodings->empty());

[Dan Beam, 2015/10/15 02:34:41]

\n

[dschuyler, 2015/10/15 21:54:19]

Done.

+  std::vector<CharacterEncoding::EncodingInfo>::const_iterator it;
+  for (it = encodings->begin(); it != encodings->end(); ++it) {

[Dan Beam, 2015/10/15 02:34:41]

you could probably use for ( : ) now

[dschuyler, 2015/10/15 21:54:19]

Done.

+    base::ListValue* option = new base::ListValue();

[Dan Beam, 2015/10/15 02:34:41]

nit: use scoped_ptr to make the ownership more explicit

[dschuyler, 2015/10/15 21:54:19]

Done.

+    if (it->encoding_id) {
+      int cmd_id = it->encoding_id;
+      std::string encoding =
+          CharacterEncoding::GetCanonicalEncodingNameByCommandId(cmd_id);
+      base::string16 name = it->encoding_display_name;
+      bool has_rtl_chars = base::i18n::StringContainsStrongRTLChars(name);
+      option->Append(new base::StringValue(encoding));
+      option->Append(new base::StringValue(name));
+      option->Append(new base::StringValue(has_rtl_chars ? "rtl" : "ltr"));
+    } else {
+      // Add empty name/value to indicate a separator item.

[dschuyler, 2015/10/12 23:24:51]

Using a blank entry is what the old options code did, there may be other ways to
handle adding a separator item, but this seems reasonable, so I didn't change
it.

+      option->Append(new base::StringValue(std::string()));
+      option->Append(new base::StringValue(std::string()));

[Dan Beam, 2015/10/15 02:34:41]

why do we need 2 empty strings?

[dschuyler, 2015/10/15 21:54:19]

It seems to work ok with one.

Done.

+    }
+    encoding_list.Append(option);
+  }
+
+  // Current selections.  The order of items in this tuple are important.

[Dan Beam, 2015/10/15 02:34:41]

would be nice to make this a dictionary instead so the order of items in this
tuple AREN'T important

[dschuyler, 2015/10/15 21:54:20]

Done.

+  base::ListValue selected_values;
+  selected_values.Append(new base::StringValue(
+      MaybeGetLocalizedFontName(standard_font_.GetValue())));
+  selected_values.Append(
+      new base::StringValue(MaybeGetLocalizedFontName(serif_font_.GetValue())));
+  selected_values.Append(new base::StringValue(
+      MaybeGetLocalizedFontName(sans_serif_font_.GetValue())));
+  selected_values.Append(
+      new base::StringValue(MaybeGetLocalizedFontName(fixed_font_.GetValue())));
+  selected_values.Append(new base::StringValue(font_encoding_.GetValue()));
+
+  web_ui()->CallJavascriptFunction("Settings.setFontsData", *list.get(),

[Dan Beam, 2015/10/15 02:34:41]

why is this doing *.get()?  scoped_ptr implements operator*

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

[dschuyler, 2015/10/15 21:54:19]

Done.

+                                   encoding_list, selected_values);
+}
+
+}  // namespace settings