[MD settings] advanced font settings extension link

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

Index: chrome/browser/ui/webui/settings/font_handler.cc
diff --git a/chrome/browser/ui/webui/settings/font_handler.cc b/chrome/browser/ui/webui/settings/font_handler.cc
index 26bbff23ff1525c6c928f64dc0188618900c825c..bd31a0a7210ca1f8d11369da30ecdf91ac882cbf 100644
--- a/chrome/browser/ui/webui/settings/font_handler.cc
+++ b/chrome/browser/ui/webui/settings/font_handler.cc
@@ -13,17 +13,31 @@
 #include "base/i18n/rtl.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/character_encoding.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/webui/options/font_settings_utils.h"
 #include "chrome/common/pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/font_list_async.h"
 #include "content/public/browser/web_ui.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/common/extension_urls.h"
+
+namespace {
+
+const char kAdvancedFontSettingsExtensionId[] =
+    "caclkomlalccbpcdllchkeecicepbmbm";
+
+}  // namespace
 
 namespace settings {
 
 FontHandler::FontHandler(content::WebUI* webui)
-    : profile_(Profile::FromWebUI(webui)),
+    : extension_registry_observer_(this),
+      profile_(Profile::FromWebUI(webui)),
       weak_ptr_factory_(this) {
   // Perform validation for saved fonts.
   options::FontSettingsUtilities::ValidateSavedFonts(profile_->GetPrefs());
@@ -35,18 +49,66 @@ void FontHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
       "fetchFontsData", base::Bind(&FontHandler::HandleFetchFontsData,
                                    base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "openAdvancedFontSettings",
+      base::Bind(&FontHandler::HandleOpenAdvancedFontSettings,
+                 base::Unretained(this)));
 }
 
 void FontHandler::HandleFetchFontsData(const base::ListValue* args) {
   CHECK_EQ(1U, args->GetSize());
   std::string callback_id;
   CHECK(args->GetString(0, &callback_id));
+  extensions::ExtensionRegistry* observer =
+      extensions::ExtensionRegistry::Get(profile_);
+  if (!extension_registry_observer_.IsObserving(observer))
+    extension_registry_observer_.Add(observer);
+  NotifyAdvancedFontSettingsAvailability();

[dpapad, 2016/04/11 21:27:30]

The communication between JS and C++ starts to get a bit complicated with this
addition, if I understand this correctly, what happens now is

1) JS calls fetchFontsData() which returns a Promise
2) Step1 causes an 'advanced-font-settings-installed' WebUI event to fire (and
therefore the JS handler of that event to execute).
3) The promise returned from 1 is resolved, which triggers other JS code.

Can we simplify this? It seems that we are mixing up two unrelated calls.
Maybe adding an explicit getFontSettingsAvailability() JS method that returns a
Promise, and have JS call that method in ready() similar to the fetchFontsData
call? Then the 'advanced-font-settings-installed' event can be fired only when
something truly changes (OnExtensionLoaded and OnExtensionUnloaded).

[dschuyler, 2016/04/11 22:53:35]

Done.

 
   content::GetFontListAsync(base::Bind(&FontHandler::FontListHasLoaded,
                                        weak_ptr_factory_.GetWeakPtr(),
                                        callback_id));
 }
 
+void FontHandler::HandleOpenAdvancedFontSettings(
+    const base::ListValue* /*args*/) {
+  const extensions::Extension* extension = GetAdvancedFontSettingsExtension();
+  if (!extension)
+    return;
+  extensions::ExtensionTabUtil::OpenOptionsPage(
+      extension,
+      chrome::FindBrowserWithWebContents(web_ui()->GetWebContents()));
+}
+
+const extensions::Extension* FontHandler::GetAdvancedFontSettingsExtension() {
+  Profile* profile = Profile::FromWebUI(web_ui());

[dpapad, 2016/04/11 21:27:30]

There is already a profile_ member var.

[dschuyler, 2016/04/11 22:53:35]

Done.

+  ExtensionService* service =
+      extensions::ExtensionSystem::Get(profile)->extension_service();
+  if (!service->IsExtensionEnabled(kAdvancedFontSettingsExtensionId))
+    return nullptr;
+  return service->GetInstalledExtension(kAdvancedFontSettingsExtensionId);
+}
+
+void FontHandler::NotifyAdvancedFontSettingsAvailability() {
+  web_ui()->CallJavascriptFunction(
+      "cr.webUIListenerCallback",
+      base::StringValue("advanced-font-settings-installed"),
+      base::FundamentalValue(GetAdvancedFontSettingsExtension() != nullptr));
+}
+
+void FontHandler::OnExtensionLoaded(
+    content::BrowserContext* /*browser_context*/,

[dpapad, 2016/04/11 21:27:30]

Nit: The "/* foo */" inlined comments here and below don't seem to add any
valuable extra information.

[dschuyler, 2016/04/11 22:53:35]

Done.

+    const extensions::Extension* /*extension*/) {
+  NotifyAdvancedFontSettingsAvailability();
+}
+
+void FontHandler::OnExtensionUnloaded(
+    content::BrowserContext* /*browser_context*/,
+    const extensions::Extension* /*extension*/,
+    extensions::UnloadedExtensionInfo::Reason /*reason*/) {
+  NotifyAdvancedFontSettingsAvailability();
+}
+
 void FontHandler::FontListHasLoaded(std::string callback_id,
                                     std::unique_ptr<base::ListValue> list) {
   // Font list. Selects the directionality for the fonts in the given list.
@@ -91,11 +153,20 @@ void FontHandler::FontListHasLoaded(std::string callback_id,
     encoding_list->Append(std::move(option));
   }
 
+  GURL extension_url(extension_urls::GetWebstoreItemDetailURLPrefix());
+  std::unique_ptr<base::StringValue> advancedExtensionUrl(new base::StringValue(

[dpapad, 2016/04/11 21:27:30]

Is the use of a pointer necessary here? I think it can be done in a simpler way.
See more information at next comment.

[dschuyler, 2016/04/11 22:53:35]

Done.

+      extension_url.Resolve(kAdvancedFontSettingsExtensionId).spec()));
+
   base::DictionaryValue response;
   response.Set("fontList", std::move(list));
   response.Set("encodingList", std::move(encoding_list));
+  response.Set("extensionUrl", std::move(advancedExtensionUrl));

[dpapad, 2016/04/11 21:27:30]

1) How about using SetString() instead?
2) See example at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
where a GURL is passed to JS without the use of a pointer.

[dschuyler, 2016/04/11 22:53:35]

Done.

 
   ResolveJavascriptCallback(base::StringValue(callback_id), response);
 }
 
+void FontHandler::RenderViewReused() {
+  extension_registry_observer_.RemoveAll();
+}
+
 }  // namespace settings