MD Settings: implement "RESTART" button for hardware acceleration to take effect

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

Index: chrome/browser/ui/webui/settings/system_handler.cc
diff --git a/chrome/browser/ui/webui/settings/system_handler.cc b/chrome/browser/ui/webui/settings/system_handler.cc
index 49835c5dc4da15111ff55db243a816f09cbcd219..bbfedae9735e9a5ef39e3a42fa340f506c777b92 100644
--- a/chrome/browser/ui/webui/settings/system_handler.cc
+++ b/chrome/browser/ui/webui/settings/system_handler.cc
@@ -4,14 +4,24 @@
 
 #include "chrome/browser/ui/webui/settings/system_handler.h"
 
+#if defined(OS_WIN)
+#include <windows.h>
+#endif
+
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/metrics/user_metrics.h"
 #include "base/metrics/user_metrics_action.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/gpu/gpu_mode_manager.h"
+#include "chrome/browser/lifetime/application_lifetime.h"
+#include "chrome/browser/ui/webui/settings_utils.h"
 #include "content/public/browser/web_ui.h"
+#include "content/public/browser/web_ui_data_source.h"
 
-#if !defined(OS_CHROMEOS)
-#include "chrome/browser/ui/webui/settings_utils.h"
+#if defined(OS_WIN)
+#include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
+#include "chrome/common/chrome_constants.h"
 #endif
 
 namespace settings {
@@ -20,19 +30,45 @@ SystemHandler::SystemHandler() {}
 
 SystemHandler::~SystemHandler() {}
 
+// static
+void SystemHandler::AddLoadTimeData(content::WebUIDataSource* data_source) {

[dpapad, 2016/03/21 18:17:38]

This is related to the "TODO" you are adding in md_settings_localized_strings.

In a similar case for ResetSettingsHandler, load time data that is not related
to localized strings (a boolean for example), is added via
ResetSettingsHandler::Create(), see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
instead of calling this method from md_settings_localized_strings. Perhaps you
could simply do the same here (creating SystemHandler::Create()), and remove the
TODO and this call from md_settings_localized_strings.

[Dan Beam, 2016/03/22 02:16:59]

I see 2 differences with the |Create()| method you linked to:
1) that it's a factory function
2) calling it has a side effect on a shared, global-ish object in a possibly
unexpected way

I don't see how that particular pattern is helpful to this handler.

I don't have a preference between a static Create() or passing dependencies
directly into a constructor.  Most design books will point mildly in favor of a
factory function because it makes construction more flexible, but almost all of
our handlers are already constructed directly (i.e. new Handler(deps...)) and
are really only separate classes to keep our code manageably small/separated
(i.e. they're mainly only used from 1 site).  I would also say one of the main
uses of a |Create()| method is access control to constructors (i.e. a test-only
constructor), which doesn't apply to ResetSettingsHandler nor SystemHandler
currently.

More grandly: I'd prefer to add loadTimeData in one pass and "freeze" the object
(functionally) by removing mutable access to it (i.e. remove or const-ify
WebUIDataSource* pointers).  Creating a static AddLoadTimeData() in this
handler's case disallows access to |this| (good) but still allows access to
file-level globals / shards implementation details per-handler (good).

[dpapad, 2016/03/22 02:40:04]

The main reason ResetSettingsHandler::Create() exists is because the boolean is
not only added to the dictionary but it is also used in the logic (which is why
it is also passed to the constructor on-top of adding to the dictionary). So
that was a way to calculate the value of that boolean (calculation is fairly
involved), only once. Additionally passing the boolean from the constructor
allows the test easily to test both cases. In any case, SystemHandler does not
match this case, so your current way is fine, I was just suggesting in case you
wanted to establish a pattern.

[Dan Beam, 2016/03/22 03:06:36]

i don't really want to use the pattern here, no, but it may make more sense
somewhere else in the future (but like I said: I'd also like to freeze
loadTimeData if possible)

+  data_source->AddBoolean("hardwareAccelerationEnabledAtStartup",
+      g_browser_process->gpu_mode_manager()->initial_gpu_mode_pref());
+}
+
 void SystemHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback("changeProxySettings",
       base::Bind(&SystemHandler::HandleChangeProxySettings,
                  base::Unretained(this)));
+  web_ui()->RegisterMessageCallback("restartBrowser",
+      base::Bind(&SystemHandler::HandleRestartBrowser,
+                 base::Unretained(this)));
 }
 
 void SystemHandler::HandleChangeProxySettings(const base::ListValue* /*args*/) {
   base::RecordAction(base::UserMetricsAction("Options_ShowProxySettings"));
-#if defined(OS_CHROMEOS)
-  NOTREACHED();
-#else
   settings_utils::ShowNetworkProxySettings(web_ui()->GetWebContents());
+}
+
+void SystemHandler::HandleRestartBrowser(const base::ListValue* /*args*/) {
+#if defined(OS_WIN)
+  // On Windows Breakpad will upload crash reports if the breakpad pipe name
+  // environment variable is defined. So we undefine this environment variable
+  // before restarting, as the restarted processes will inherit their
+  // environment variables from ours, thus suppressing crash uploads.
+  if (!ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled()) {
+    HMODULE exe_module = GetModuleHandle(chrome::kBrowserProcessExecutableName);
+    if (exe_module) {
+      typedef void (__cdecl *ClearBreakpadPipeEnvVar)();
+      ClearBreakpadPipeEnvVar clear = reinterpret_cast<ClearBreakpadPipeEnvVar>(
+          GetProcAddress(exe_module, "ClearBreakpadPipeEnvironmentVariable"));
+      if (clear)
+        clear();
+    }
+  }
 #endif
+
+  chrome::AttemptRestart();
 }
 
 }  // namespace settings