Hangout remote desktop part II - background.html and AppLauncher

remoting/webapp/background/app_launcher.js

+// Copyright 2014 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
+ * AppLauncher is an interface for app launching closing implementations.
+ * It nows the client code to launch and close the app without knowing the
+ * implementation difference between launching a v1 app and a v2 app.

[Jamie, 2014/08/12 02:24:56]

Can you clean up this comment? Specifically, I don't know what "app launching
closing implementations" means, and I think you mean "allows... without needing
to know", not "nows... without knowing".

[kelvinp, 2014/08/12 18:22:44]

Done.

+ */
+
+'use strict';
+
+/** @suppress {duplicate} */
+var remoting = remoting || {};
+
+/** @interface */
+remoting.AppLauncher = function() {};
+
+/**
+ * @param {Object=} opt_launchArgs
+ * @return {Promise}

[Jamie, 2014/08/12 02:24:56]

The methods of this interface need descriptions, or perhaps some pseudo-code in
the fileoverview might be more appropriate, showing how they are typically
called.

[Jamie, 2014/08/12 02:24:56]

Make it clear that when the Promise resolves, it will provide the caller with
the tab id.

[kelvinp, 2014/08/12 18:22:44]

Done.

+ */
+remoting.AppLauncher.prototype.launch = function(opt_launchArgs) { };

[Jamie, 2014/08/12 02:24:56]

Nit: No space between braces.

[kelvinp, 2014/08/12 18:22:44]

Done.

+
+/**
+ * @param {string} id  The id of the tab/window to close.
+ * @return {Promise}
+ */
+remoting.AppLauncher.prototype.close = function(id) {};
+
+/**
+ * @param {string} url
+ * @param {Object.<string>=} opt_params
+ * @return {string}
+ */
+remoting.AppLauncher.urlJoin = function(url, opt_params) {

[Jamie, 2014/08/12 02:24:56]

Interfaces should not have concrete methods, and in any case, this doesn't feel
like it belongs on an AppLauncher class. It should be moved to a separate
utility suite.

[kelvinp, 2014/08/12 18:22:44]

Moved to base.js

+  if (!opt_params) {
+    return url;
+  }
+  var queryParameters = [];
+  for (var key in opt_params) {
+    queryParameters.push(encodeURIComponent(key) + "=" +
+                         encodeURIComponent(opt_params[key]));
+  }
+  return url + '?' + queryParameters.join('&');
+};
+
+/**
+ * @constructor
+ * @implements {remoting.AppLauncher}
+ */
+remoting.V1AppLauncher = function() {};
+
+remoting.V1AppLauncher.prototype.launch = function(opt_launchArgs) {
+  var url = remoting.AppLauncher.urlJoin('main.html', opt_launchArgs);
+
+  /**
+   * @param {function(*=):void} resolve
+   * @param {function(*=):void} reject
+   */
+  return new Promise(function(resolve, reject) {
+      chrome.tabs.create({ url: url, selected: true},

[Jamie, 2014/08/12 02:24:56]

Nit: No space after '{' (or add one after 'true').

[kelvinp, 2014/08/12 18:22:44]

Done.

+        /** @param {chrome.Tab} tab The created tab. */
+        function(tab) {
+          if (!tab) {
+            reject(chrome.runtime.lastError);

[Jamie, 2014/08/12 02:24:56]

Is it better to use new Error to get a stack trace, or does that only work with
the test frameworks?

+          } else {
+            resolve(tab.id);
+          }
+      });
+  });
+};
+
+remoting.V1AppLauncher.prototype.close = function(id) {
+  /**
+   * @param {function(*=):void} resolve
+   * @param {function(*=):void} reject
+   */
+  return new Promise(function(resolve, reject) {
+    /** @param {chrome.Tab} tab The retrieved tab. */
+    chrome.tabs.get(id, function(tab) {
+      if (!tab) {
+        reject(chrome.runtime.lastError.message);

[Jamie, 2014/08/12 02:24:56]

Why .message here but not in launch?

[kelvinp, 2014/08/12 18:22:44]

Good catch. Typo in launch

+      } else {
+        chrome.tabs.remove(tab.id, function(){

[Jamie, 2014/08/12 02:24:56]

Do you need the wrapper function?

[kelvinp, 2014/08/12 18:22:44]

Done.

+          resolve();
+        });
+      }
+    });
+  });
+};
+
+
+/**
+ * @constructor
+ * @implements {remoting.AppLauncher}
+ */
+remoting.V2AppLauncher = function() {};
+
+remoting.V2AppLauncher.nextWindowId_ = 0;

[Jamie, 2014/08/12 02:24:56]

@type and @private annotations, please.

+
+remoting.V2AppLauncher.prototype.launch = function(opt_launchArgs) {
+  var url = remoting.AppLauncher.urlJoin('main.html', opt_launchArgs);
+
+  /**
+   * @param {function(*=):void} resolve
+   * @param {function(*=):void} reject
+   */
+  return new Promise(function(resolve, reject) {
+    chrome.app.window.create(url, {
+        'width': 800,
+        'height': 600,
+        'frame': 'none',
+        'id': "hangout" + remoting.V2AppLauncher.nextWindowId_++

[Jamie, 2014/08/12 02:24:56]

I think the id would be better managed by the caller so that this class can be
completely agnostic about its environment (ie, not specific to Hangouts). Come
to think of it, why is an explicit id needed?

[kelvinp, 2014/08/12 18:22:44]

You are right.  Prefix removed.

+      },
+      /** @param {AppWindow} appWindow */
+      function(appWindow) {
+        if (!appWindow) {
+          reject(chrome.runtime.lastError.message);
+        } else {
+          resolve(appWindow.id);
+        }
+      });
+  });
+};
+
+remoting.V2AppLauncher.prototype.close = function(id) {
+  var appWindow = chrome.app.window.get(id);
+  if (!appWindow) {
+    return Promise.reject(chrome.runtime.lastError.message);
+  }
+  appWindow.close();
+  return Promise.resolve();
+};