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

chrome/browser/resources/plugins.js

 // 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.
 
 /**
  * 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().
  */
 function renderTemplate(pluginsData) {
   // This is the javascript code that processes the template:
   var input = new JsEvalContext(pluginsData);
   var output = $('pluginTemplate');
   jstProcess(input, output);
 }
 
 /**
- * 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(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 =
+      showDetails ? 'inline' : 'none';
+  $('expand').style.display =
+      showDetails ? 'none' : 'inline';
 
-function loadShowDetailsFromPrefs(show_details) {
-  tmiModeExpanded = show_details;
-  $('collapse').style.display =
-      show_details ? 'inline' : 'none';
-  $('expand').style.display =
-      show_details ? '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';
 }
 
 /**
  * Called by the web_ui_ to re-populate the page with data representing the
  * current state of installed plugins.
  * @param {Object} pluginsData Detailed info about installed plugins. The
  *     template expects each plugin's format to match the following structure to
  *     correctly populate the page:
  *   {
  *     plugins: [
  *       {
  *         name: 'Group Name',
  *         description: 'description',
  *         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' }
+ *             type: 'BROWSER PLUGIN',
+ *             mime_types: [
+ *               {
+ *                 description: 'New Guy Media',
+ *                 file_extensions: ['mfp'],
+ *                 mime_type: 'application/x-my-first'
+ *               },
  *             ],
- *             enabledMode: 'enabledByPolicy'
+ *             enabled_mode: 'disabledByUser',
  *           },
- *           {
- *             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' }
- *             ],
- *             enabledMode: '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) {
   var bodyContainer = $('body-container');
   var body = document.body;
 
   // Set all page content to be visible so we can measure heights.
   bodyContainer.style.visibility = 'hidden';
   body.className = '';
   var slidables = document.getElementsByClassName('show-in-tmi-mode');
@@ skipping 73 matching lines @@
 }
 
 /**
  * Handles a 'enable' or 'disable' button getting clicked.
  * @param {HTMLElement} node The HTML element for the plugin being changed.
  * @param {boolean} enable Whether to enable or disable the plugin.
  * @param {boolean} isGroup True if we're enabling/disabling a plugin group,
  *     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.
 var tmiModeExpanded = false;
 
 /*
  * Toggles visibility of details.
  */
 function toggleTmiMode() {
   tmiModeExpanded = !tmiModeExpanded;
 
   $('collapse').style.display =
       tmiModeExpanded ? 'inline' : 'none';
   $('expand').style.display =
       tmiModeExpanded ? 'none' : 'inline';
 
   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);
+  });
 }
 
 /**
  * @param {Object} plugin An object containing the information about a plugin.
  *     See returnPluginsData() for the format of this object.
  * @return {boolean} Whether the plugin's version should be displayed.
  */
 function shouldDisplayPluginVersion(plugin) {
   return !!plugin.version && plugin.version != '0';
 }
@@ skipping 12 matching lines @@
          plugin.description != 'Version ' + plugin.version &&
          plugin.description != plugin.name + ' ' + plugin.version;
 }
 
 /**
  * @param {Object} plugin An object containing the information about a plugin.
  *     See returnPluginsData() for the format of this object.
  * @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);
+  });
+});