Mojo-ifying chrome://version.

components/version_ui/resources/about_version.js

Index: components/version_ui/resources/about_version.js
diff --git a/components/version_ui/resources/about_version.js b/components/version_ui/resources/about_version.js
index 476311bfae9837eed6419c27409ddf6eab078d8a..2fc47e3ac068026e8f998c98d2e63614af6c65d6 100644
--- a/components/version_ui/resources/about_version.js
+++ b/components/version_ui/resources/about_version.js
@@ -2,47 +2,57 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 

[Dan Beam, 2015/12/09 21:45:07]

nit: remove extra line

[dpapad, 2015/12/10 01:08:27]

Done.

-/**
- * Callback from the backend with the list of variations to display.
- * This call will build the variations section of the version page, or hide that
- * section if there are none to display.
- * @param {!Array<string>} variationsList The list of variations.
- */
-function returnVariationInfo(variationsList) {
-  $('variations-section').hidden = !variationsList.length;
-  $('variations-list').appendChild(
-      parseHtmlSubset(variationsList.join('<br>'), ['BR']));
-}
 
-/**
- * Callback from the backend with the executable and profile paths to display.
- * @param {string} execPath The executable path to display.
- * @param {string} profilePath The profile path to display.
- */
-function returnFilePaths(execPath, profilePath) {
-  $('executable_path').textContent = execPath;
-  $('profile_path').textContent = profilePath;
+/** @return {!Promise} */
+function getUiHandler() {

[Dan Beam, 2015/12/09 21:45:07]

same question as below as to why this is a method when only used once?

[dpapad, 2015/12/10 01:08:27]

See answer at the end of this file.

+  return new Promise(function(resolve, reject) {
+    define([
+      'mojo/public/js/connection',
+      'chrome/browser/ui/webui/version.mojom',
+      'content/public/renderer/service_provider',
+    ], function(connection, versionMojom, serviceProvider) {
+      var uiHandler = connection.bindHandleToProxy(
+          serviceProvider.connectToService(
+              versionMojom.VersionHandlerMojo.name),
+          versionMojom.VersionHandlerMojo);
+      resolve(uiHandler);
+    });
+  });
 }
 
-/**
- * Callback from the backend with the Flash version to display.
- * @param {string} flashVersion The Flash version to display.
- */
-function returnFlashVersion(flashVersion) {
-  $('flash_version').textContent = flashVersion;
-}
 
 /**
- * Callback from the backend with the OS version to display.
- * @param {string} osVersion The OS version to display.
+ * @return {!Promise} Fires when DOMContentLoaded event is received.
  */
-function returnOsVersion(osVersion) {
-  $('os_version').textContent = osVersion;
+function whenDomContentLoaded() {
+  return new Promise(function(resolve, reject) {
+    document.addEventListener('DOMContentLoaded', resolve);
+  });
 }

[Dan Beam, 2015/12/09 21:45:07]

why is this a method when it's only called once?  why not

var domContentLoaded = new Promise(function(resolve, reject) {
  document.addEventListener('DOMContentLoaded', resolve);
});

[dpapad, 2015/12/10 01:08:27]

See answer at the end of this file.

 
-/* All the work we do onload. */
-function onLoadWork() {
-  chrome.send('requestVersionInfo');
-}
 
-document.addEventListener('DOMContentLoaded', onLoadWork);
+function main() {
+  Promise.all([
+    whenDomContentLoaded(), getUiHandler()
+  ]).then(function(results) {
+    var uiHandler = results[1];
+    uiHandler.getFilePaths().then(function(response) {
+      $('executable_path').textContent = response.execPath;
+      $('profile_path').textContent = response.profilePath;
+    });
+    uiHandler.getFlashVersion().then(function(response) {

[Dan Beam, 2015/12/09 21:45:07]

you're calling this on Android even though flash isn't there, right?

[dpapad, 2015/12/10 01:08:27]

Fixed.

+      $('flash_version').textContent = response.flashVersion;
+    });
+    uiHandler.getVariations().then(function(response) {
+      $('variations-section').hidden = response.variations.length == 0;
+      $('variations-list').appendChild(
+          parseHtmlSubset(response.variations.join('<br>'), ['BR']));
+    });
+    if (cr.isChromeOS) {

[Dan Beam, 2015/12/09 21:45:07]

this is a snapshot of the platforms that use this right now, with no
programmatic enforcement if this changes in the future.

something this would be better:

  if (uiHandler.getOsVersion)
    uiHandler.getOsVersion().then(...)

and we don't provide getOsVersion unless it's useful, because right now this
seems akin to doing:

  document.addEventListener('thingThatWillNeverHappen', function() {
  });

it's just dead code.  it's not a huge deal in this case, but I'm wondering what
precedent we're setting for things like settings where there's *TONS* of
platform-specific stuff (especially ChromeOS-specific)

[dpapad, 2015/12/10 01:08:27]

I don't think that this should be the precedent for cases with TONS of platform
specific stuff, in which case the multiple interfaces should be preferred. Also
regarding this being dead-code, yes it is, as it was before (see all the code
that is removed from about_version_ios.js for example). At least this makes the
fact that it is dead-code for some cases very explicit.

Also, just for completeness, chrome://version does have a lot of platform
specific stuff, but it is mostly handled on the C++ side while populating
loadTimeData. If we decide to convert all loadTimeData for chrome://version to
use Mojo, then I would be in favor of breaking down to multiple interfaces at
that time.

+      uiHandler.getOsVersion().then(function(response) {
+        $('os_version').textContent = response.osVersion;
+      });
+    }
+  });
+}
+main();

[Dan Beam, 2015/12/09 21:45:07]

what's the point of main()?  are we trying to be pythonic or something (i.e. if
__name__ == '__main__':)?

(function() { ... })() would make more sense... but there are no new variables
introduced (so adding a scope isn't too helpful)

[dpapad, 2015/12/10 01:08:28]

The point of this is just readability. It makes it painfully obvious that main()
is the starting point of this script. This is the same reason why I prefer to
have getUIHandler, whenDomContentLoaded as a method instead of turning them to
Promise variables as suggested. The reader can start reading from the first line
of main() knowing that no code has executed before that, makes it easier to
follow IMO.

Also the question "why is X a function if it is only called once?" is not a
golden criterion for the existence of a function. Sometimes methods/functions
exist because they encapsulate/abstract some operation, which is the case here,
not because they are reused multiple times.

I generally don't find such comments useful. From my point of view these
comments fall into the category "why is this piece of code not exactly as I (the
reviewer) would have written it myself?". Authoring code is like hand-writing
style, no two people have the exact same style.
What matters most is whether you had hard time understanding the code, whether
it violates some styleguide or a profound software engineering good practice and
whether it does not do what it advertises of doing.

Feel free to grab me in person and talk about this further if you like, I don't
think the CL is the best place to have such a discussion.