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

chrome/browser/resources/settings/system_page/system_page_browser_proxy.js

Index: chrome/browser/resources/settings/system_page/system_page_browser_proxy.js
diff --git a/chrome/browser/resources/settings/system_page/system_page_browser_proxy.js b/chrome/browser/resources/settings/system_page/system_page_browser_proxy.js
new file mode 100644
index 0000000000000000000000000000000000000000..bfaf1b25c88df1b58d10a1679288de3ea374c667
--- /dev/null
+++ b/chrome/browser/resources/settings/system_page/system_page_browser_proxy.js
@@ -0,0 +1,54 @@
+// Copyright 2016 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.
+
+/** @fileoverview Handles interprocess communcation for the system page. */
+
+cr.define('settings', function() {
+  /** @interface */
+  function SystemPageBrowserProxy() {}
+
+  SystemPageBrowserProxy.prototype = {
+    /** Allows the user to change native system proxy settings. */
+    changeProxySettings: assertNotReached,

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

Nit: Is assertNotReached() useful here (instead of simply "function() {}"? This
is not a base class, so there is no chance ever for this code to get executed.
@interface is purely a JS compiler concept. I understand that assertNotReached
would make more sense if this was a base class, and you wanted to detect methods
that have not been overridden.

After all, doesn't the compiler tell us if a class claims to implement an
interface but it does not implement all methods?

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

pre-Done (I don't feel strongly enough to slow down review, so I've done this in
good faith).

I'd like to know the reverse question: what do you think the /detriment/ to
using assertNotReached is?  assertNotReached is at least re-using a function
reference (so there's slightly more possibility for the compiler to re-use it
than an anonymous function() {} afaik), and if anybody ever did actually try to
use this they'd fail.  it also makes adds an additional, enforced signal that
this class is abstract.

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

I don't think it is detrimental (which is why I prefixed my comment with "nit"
and I should have also added "optional"). It's just adding more confusion to an
already confusing situation (for the non experienced reader), explaining below.

1) JS interfaces are effectively dead code, that is only useful to the JS
compiler.
2) This dead code lives in the same file as production code (the implementation)
and gets unnecessarily evaluated.
3) Now by using assertNotReached, this dead-code somehow seems (to someone not
familiar with JS compiler) as if it can potentially be also executed under
certain circumstances.

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

yeah, this is what I guessed (that it just makes things look slightly more like
it's real code)

+
+    /** Restarts Chrome so "Use hardware acceleration" can take effect. */
+    restartBrowser: assertNotReached,
+
+    /**
+     * @return {boolean} Whether hardware acceleration was enabled when the user
+     *     started Chrome.
+     */
+    wasHardwareAccelerationEnabledAtStartup: assertNotReached,
+  };
+
+  /**
+   * @constructor
+   * @implements {settings.SystemPageBrowserProxy}
+   */
+  function SystemPageBrowserProxyImpl() {}
+
+  cr.addSingletonGetter(SystemPageBrowserProxyImpl);
+
+  SystemPageBrowserProxyImpl.prototype = {
+    /** @override */
+    changeProxySettings: function() {
+      chrome.send('changeProxySettings');
+    },
+
+    /** @override */
+    restartBrowser: function() {
+      chrome.send('restartBrowser');
+    },
+
+    /** @override */
+    wasHardwareAccelerationEnabledAtStartup: function() {
+      return loadTimeData.getBoolean('hardwareAccelerationEnabledAtStartup');
+    },
+  };
+
+  return {
+    SystemPageBrowserProxy: SystemPageBrowserProxy,
+    SystemPageBrowserProxyImpl: SystemPageBrowserProxyImpl,
+  };
+});