chrome://plugins Mojo-ification part 2/2, populating the page.

chrome/browser/resources/plugins.js

Index: chrome/browser/resources/plugins.js
diff --git a/chrome/browser/resources/plugins.js b/chrome/browser/resources/plugins.js
index 6b19cee625b295a377a3eda2b5646cb9e122d39b..6701e06e3096941700b1e08a0bcfe7ca68c74e93 100644
--- a/chrome/browser/resources/plugins.js
+++ b/chrome/browser/resources/plugins.js
@@ -5,7 +5,7 @@
 /**
  * Takes the |pluginsData| input argument which represents data about the
  * currently installed/running plugins and populates the html jstemplate with
- * that data. It expects an object structure like the above.
+ * that data.
  * @param {Object} pluginsData Detailed info about installed plugins. Same
  *     expected format as returnPluginsData().
  */
@@ -17,23 +17,16 @@ function renderTemplate(pluginsData) {
 }
 
 /**
- * Asks the C++ PluginsDOMHandler to get details about the installed plugins and
- * return detailed data about the configuration. The PluginsDOMHandler should
- * reply to returnPluginsData() (below).
+ * @param {boolean} showDetails
  */
-function requestPluginsData() {
-  chrome.send('requestPluginsData');
-  chrome.send('getShowDetails');
-}
-
-function loadShowDetailsFromPrefs(show_details) {
-  tmiModeExpanded = show_details;
+function loadShowDetailsFromPrefs(showDetails) {
+  tmiModeExpanded = showDetails;

[Dan Beam, 2016/01/28 04:25:08]

arguably: move the declaration of tmiModeExpanded to before it's first use, i.e.

var tmiModeExpanded = false;

function loadShowDetailsFromPrefs(showDetails) {
  tmiModeExpanded = showDetails;
  ...
}

[dpapad, 2016/01/28 18:32:54]

Done.

   $('collapse').style.display =
-      show_details ? 'inline' : 'none';
+      showDetails ? 'inline' : 'none';
   $('expand').style.display =
-      show_details ? 'none' : 'inline';
+      showDetails ? 'none' : 'inline';
 
-  document.body.className = show_details ? 'show-in-tmi-mode' : 'hide-tmi-mode';
+  document.body.className = showDetails ? 'show-in-tmi-mode' : 'hide-tmi-mode';
 }
 
 /**
@@ -50,63 +43,48 @@ function loadShowDetailsFromPrefs(show_details) {
  *         version: 'version',
  *         update_url: 'http://update/',
  *         critical: true,
- *         enabled: true,
- *         identifier: 'plugin-name',
+ *         enabled_mode: 'enabledByUser',
+ *         id: 'plugin-name',
+ *         always_allowed: false,
  *         plugin_files: [
  *           {
- *             path: '/blahblah/blahblah/MyCrappyPlugin.plugin',
- *             name: 'MyCrappyPlugin',
- *             version: '1.2.3',
- *             description: 'My crappy plugin',
- *             mimeTypes: [
- *               { description: 'Foo Media',
- *                 fileExtensions: ['foo'],
- *                 mimeType: 'application/x-my-foo' },
- *               { description: 'Bar Stuff',
- *                 fileExtensions: ['bar', 'baz'],
- *                 mimeType: 'application/my-bar' }
+ *             path: '/foo/bar/baz/MyPlugin.plugin',
+ *             name: 'MyPlugin',
+ *             version: '1.2,3'
+ *             description: 'My plugin',
+ *             type: 'BROWSER PLUGIN',
+ *             mime_types: [
+ *               {
+ *                 description: 'Foo Media',
+ *                 file_extensions: ['pdf'],
+ *                 mime_type: 'application/x-my-foo'
+ *               },
+ *               {
+ *                 description: 'Bar Stuff',
+ *                 file_extensions: ['bar', 'baz'],
+ *                 mime_type: 'application/my-bar'
+ *               }
  *             ],
- *             enabledMode: 'enabledByUser'
+ *             enabled_mode: 'enabledByUser',
  *           },
  *           {
  *             path: '/tmp/MyFirst.plugin',
  *             name: 'MyFirstPlugin',
  *             version: '3.14r15926',
  *             description: 'My first plugin',
- *             mimeTypes: [
- *               { description: 'New Guy Media',
- *                 fileExtensions: ['mfp'],
- *                 mimeType: 'application/x-my-first' }
- *             ],
- *             enabledMode: 'enabledByPolicy'
- *           },
- *           {
- *             path: '/foobar/baz/YourGreatPlugin.plugin',
- *             name: 'YourGreatPlugin',
- *             version: '4.5',
- *             description: 'Your great plugin',
- *             mimeTypes: [
- *               { description: 'Baz Stuff',
- *                 fileExtensions: ['baz'],
- *                 mimeType: 'application/x-your-baz' }
+ *             type: 'BROWSER PLUGIN',
+ *             mime_types: [
+ *               {
+ *                 description: 'New Guy Media',
+ *                 file_extensions: ['mfp'],
+ *                 mime_type: 'application/x-my-first'
+ *               },
  *             ],
- *             enabledMode: 'disabledByUser'
+ *             enabled_mode: 'disabledByUser',
  *           },
- *           {
- *             path: '/foobiz/bar/HisGreatPlugin.plugin',
- *             name: 'HisGreatPlugin',
- *             version: '1.2',
- *             description: 'His great plugin',
- *             mimeTypes: [
- *               { description: 'More baz Stuff',
- *                 fileExtensions: ['bor'],
- *                 mimeType: 'application/x-his-bor' }
- *             ],
- *             enabledMode: 'disabledByPolicy'
- *           }
- *         ]
- *       }
- *     ]
+ *         ],
+ *       },
+ *     ],
  *   }
  */
 function returnPluginsData(pluginsData) {
@@ -200,9 +178,11 @@ function returnPluginsData(pluginsData) {
  *     rather than a single plugin.
  */
 function handleEnablePlugin(node, enable, isGroup) {
-  // Tell the C++ PluginsDOMHandler to enable/disable the plugin.
-  chrome.send('enablePlugin', [String(node.path), String(enable),
-              String(isGroup)]);
+  whenBrowserProxyReady.then(function(browserProxy) {
+    isGroup ?

[Dan Beam, 2016/01/28 04:25:08]

don't do this

[dpapad, 2016/01/28 18:32:54]

It's not apparent why you are suggesting not to do this. Can you explain? I have
been using this pattern a lot (in other codebases) and find it very convenient.

[Dan Beam, 2016/01/28 19:20:34]

if (isGroup)
  browserProxy.setPluginGroupEnabled(node.path, enable);
else
  browserProxy.setPluginEnabled(node.path, enable)
feel free to do it in other codebases as much as you'd like (or find precedent
for this in chrome)

[dpapad, 2016/01/28 20:24:01]

Changed per suggestion.

Finding precedent for something is not golden criterion though. By induction, if
whenever something with no precedent is turned down, there can never be
precedent. The question of whether to allow or disallow something should not be
based just on precedence but on quite a few factors (is it useful? requires less
visual clutter? is it clear to the reader?). The styleguide serves better as a
mean to restrict language usage to a subset, so I suggest making a case there if
you prefer not to repeat the same conversation with another code reviewee in the
future.

+        browserProxy.setPluginGroupEnabled(node.path, enable) :
+        browserProxy.setPluginEnabled(node.path, enable);
+  });
 }
 
 // Keeps track of whether details have been made visible (expanded) or not.
@@ -222,11 +202,15 @@ function toggleTmiMode() {
   document.body.className =
       tmiModeExpanded ? 'show-tmi-mode' : 'hide-tmi-mode';
 
-  chrome.send('saveShowDetailsToPrefs', [String(tmiModeExpanded)]);
+  whenBrowserProxyReady.then(function(browserProxy) {
+    browserProxy.saveShowDetailsToPrefs(tmiModeExpanded);
+  });
 }
 
 function handleSetPluginAlwaysAllowed(el) {
-  chrome.send('setPluginAlwaysAllowed', [el.identifier, el.checked]);
+  whenBrowserProxyReady.then(function(browserProxy) {
+    browserProxy.setPluginAlwaysAllowed(el.identifier, el.checked);
+  });
 }
 
 /**
@@ -259,18 +243,84 @@ function shouldDisplayPluginDescription(plugin) {
  * @return {boolean} Whether the plugin is enabled.
  */
 function isPluginEnabled(plugin) {
-  return plugin.enabledMode == 'enabledByUser' ||
-         plugin.enabledMode == 'enabledByPolicy';
+  return plugin.enabled_mode == 'enabledByUser' ||
+         plugin.enabled_mode == 'enabledByPolicy';
 }
 
 // Unfortunately, we don't have notifications for plugin (list) status changes
 // (yet), so in the meanwhile just update regularly.
-setInterval(requestPluginsData, 30000);
-
-// Get data and have it displayed upon loading.
-document.addEventListener('DOMContentLoaded', requestPluginsData);
+setInterval(function() {
+  whenBrowserProxyReady.then(function(browserProxy) {
+    return browserProxy.getPluginsData();
+  }).then(returnPluginsData);
+}, 30000);
 
 // Add handlers to static HTML elements.
 $('collapse').onclick = toggleTmiMode;
 $('expand').onclick = toggleTmiMode;
 $('details-link').onclick = toggleTmiMode;
+
+// NOTE: Need to keep a reference to the stub here such that it is not garbage
+// collected, which causes the pipe to close and future calls from C++ to JS to
+// get dropped.
+var pluginsPageStub = null;
+
+/**
+ * A promise to use for getting a reference to the proxy object allowing
+ * communication with the browser side.
+ * @type {!Promise}
+ */
+var whenBrowserProxyReady = new Promise(function(resolve, reject) {
+  define([
+    'mojo/public/js/bindings',
+    'mojo/public/js/core',
+    'mojo/public/js/connection',
+    'chrome/browser/ui/webui/plugins/plugins.mojom',
+    'content/public/renderer/service_provider',
+  ], function(bindings, core, connection, pluginsMojom, serviceProvider) {
+    var browserProxy = connection.bindHandleToProxy(
+        serviceProvider.connectToService(
+            pluginsMojom.PluginsHandlerMojo.name),
+        pluginsMojom.PluginsHandlerMojo);
+
+    // Perform necessary setup for the C++ side to make calls to JS at will.
+
+    // Connect pipe handle to JS code.
+    var pipe = core.createMessagePipe();
+    pluginsPageStub = connection.bindHandleToStub(
+        pipe.handle0, pluginsMojom.PluginsPageMojo);
+
+    /** @constructor */
+    function PluginsPageImpl() {}
+    PluginsPageImpl.prototype = {
+      __proto__: pluginsMojom.PluginsPageMojo.stubClass.prototype,
+      onPluginsUpdated: function(plugins) {
+        returnPluginsData({plugins: plugins});
+      },
+    };
+    bindings.StubBindings(pluginsPageStub).delegate = new PluginsPageImpl();

[Dan Beam, 2016/01/28 04:25:08]

nit: this seems to be simpler (and mildly akin to Java's anonymous classes, if
that's a good thing...)

bindings.StubBindings(pluginsPageStub).delegate = {
  __proto__: pluginsMojom.PluginsPageMojo.stubClass.prototype,
  onPluginsUpdated: function() {
    returnPluginsData({plugins: plugins});
  },
};

are we compiling this code yet?

[dpapad, 2016/01/28 18:32:54]

Done.

This is not compiled yet. See
https://code.google.com/p/chromium/issues/detail?id=574512#c3 for more context.
Wanted to chat about it with you anyway (independently of this CL though).

+
+    // Send pipe handle to C++.
+    browserProxy.setClientPage(pipe.handle1);
+
+    resolve(browserProxy);
+  });
+});
+
+/**
+ * @type {!Promise} A promise firing when DOMContentLoaded event is received.
+ */
+var whenDomContentLoaded = new Promise(function(resolve, reject) {
+  document.addEventListener('DOMContentLoaded', resolve);
+});
+
+Promise.all([
+  whenDomContentLoaded,
+  whenBrowserProxyReady
+]).then(function(results) {
+  var browserProxy = results[1];
+  browserProxy.getPluginsData().then(returnPluginsData);
+  browserProxy.getShowDetails().then(function(response) {
+    loadShowDetailsFromPrefs(response.show_details);
+  });
+});