Improve HRD first run experience

remoting/webapp/crd/js/hangout_consent_dialog.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.
+
+/** @suppress {duplicate} */
+var remoting = remoting || {};
+
+(function() {
+
+'use strict';
+
+var DIALOG_URL = 'dialog_hangout_consent.html';
+var DIALOG_WIDTH = 750;
+var DIALOG_HEIGHT = 480;
+var AUTHENTICATED_PARAM = 'authenticated';
+var AUTHENTICATED_MARGIN = 100;

[Jamie, 2015/02/02 19:07:40]

Can these constants be computed (eg, using getBoundingClientRect) rather than
being hard-coded? Hard-coding is risky, especially for localizable content
(which I assume this is). For window position, I think it's fine if you center
it assuming US English content (the worst that can happen is it's off-center
vertically by a small amount), but for anything more visually important, you
should do size-based calculations in the dialog itself, where you can measure
things precisely.

If you can get rid of them, you could consider getting rid of the strings as
well, since you're only referring to them once, and I'm not sure there's much
point in making them variables.

If you do want to keep them, they should probably be local variables inside
show(), since that's the only place they're used.

[kelvinp, 2015/02/03 01:15:33]

Good point. In the new iteration, I added a measuring pass when the page is
loaded to ensure the height is big enough such that contents won't be clipped.

+
+var instance_ = null;
+
+/**
+ * Shows a dialog to ask for the user's permission to accept remote assistance
+ * from a Hangout participant.
+ *
+ * @constructor
+ * @private
+ */
+remoting.HangoutConsentDialog = function() {
+  /**
+   * @type {base.Deferred}
+   * @private
+   */
+  this.onConsentResponseDeferred_ = null;
+
+  base.Ipc.getInstance().register('remoting.HangoutConsentDialog.confirm',
+                                  this.onConfirmResponse_.bind(this));
+};
+
+/**
+ * @param {boolean} confirm whether the user authorize the it2me connection

[Jamie, 2015/02/02 19:07:40]

s/whether/Whether/ (also, capitalize the first word of the parameter description
elsewhere in this file).

s/authorize/authorized/

Full-stop at the end of the sentence, please.

[kelvinp, 2015/02/03 01:15:32]

Done.

[Jamie, 2015/02/03 18:22:44]

You've only addressed the first comment in this group.

[kelvinp, 2015/02/03 19:31:45]

Done.

+ * @private
+ */
+remoting.HangoutConsentDialog.prototype.onConfirmResponse_ = function(confirm) {
+  if (this.onConsentResponseDeferred_) {

[Jamie, 2015/02/02 19:07:40]

Under what circumstances can this be null? Would an assert be a better option
here?

[kelvinp, 2015/02/03 01:15:32]

I am trying guard against invalid IPC's.  I think an Xsrf token is probably a
better solution.

+    if (confirm) {
+      this.onConsentResponseDeferred_.resolve();
+    } else {
+      this.onConsentResponseDeferred_.reject(
+          new Error(remoting.Error.NOT_AUTHORIZED));
+    }
+    this.onConsentResponseDeferred_ = null;
+  }
+};
+
+/**
+ * @param {boolean} authenticated if set, the consent dialog will hide the

[Jamie, 2015/02/02 19:07:40]

s/set/true/

[kelvinp, 2015/02/03 01:15:33]

Done.

+ *  authorization section.

[Jamie, 2015/02/02 19:07:40]

Authentication and authorization have very specific, and different, meanings, so
documenting an |authenticated| parameter as affecting an "authorization" section
is confusing. Pick whichever is more appropriate and be consistent.

[kelvinp, 2015/02/03 01:15:33]

Done.

+ * @param {Bounds=} opt_parentBounds if set, the consent dialog will be centered
+ *  upon |opt_parentBounds|.

[Jamie, 2015/02/02 19:07:40]

s/upon/within/

[kelvinp, 2015/02/03 01:15:33]

Done.

+ * @return {Promise} A Promise that will resolve when permission is granted or
+ *   reject if the user cancels.
+ */
+remoting.HangoutConsentDialog.prototype.show = function(authenticated,
+                                                        opt_parentBounds) {
+  if (!this.onConsentResponseDeferred_) {

[Jamie, 2015/02/02 19:07:40]

I think this needs to be an assert. Returning the same Promise for two separate
calls will (I think) leave one of them hanging, because the Promise will only
resolve once.

[kelvinp, 2015/02/03 01:15:32]

Adding then() is equivalent to subscribing to the 'resolve/reject' event of a
promise, if the promise has already resolved, the then() function will be invoke
immediately (asynchronously of course).  E.g.

var p = Promise.resolve('world');
p.then(function(value){  // hooking then() to a resolved promise.
  console.log('hello ' + value);
}); 

p.then(function(value){  // hooking then() to a resolved promise.
  console.log('this is a small ' + value);
});

will print out 
'hello world'
'this is a small world'

+    var height =
+        (authenticated) ? DIALOG_HEIGHT - AUTHENTICATED_MARGIN : DIALOG_HEIGHT;
+    var createOptions = {
+      frame: 'none',
+      resizable: false,
+      outerBounds: {width: DIALOG_WIDTH, height: height}

[Jamie, 2015/02/02 19:07:40]

Space after '{' and before '}' (the only time we don't do this is in JSDoc type
annotations).

[kelvinp, 2015/02/03 01:15:33]

Done.

+    };
+
+    var url =
+        DIALOG_URL + '?' + AUTHENTICATED_PARAM + '=' + String(!!authenticated);

[Jamie, 2015/02/02 19:07:40]

I think Boolean(authenticated) would be clearer than String(!!authenticated).

[kelvinp, 2015/02/03 01:15:33]

Done.

+
+    if (opt_parentBounds) {
+      // Center the dialog on the parent bounds.
+      var rect = opt_parentBounds;
+      createOptions.outerBounds.top =
+          Math.round(rect.top + rect.height / 2 - DIALOG_HEIGHT / 2);
+      createOptions.outerBounds.left =
+          Math.round(rect.left + rect.width / 2 - DIALOG_WIDTH / 2);
+    }
+
+    this.onConsentResponseDeferred_ = new base.Deferred();
+    chrome.app.window.create(url, createOptions);
+  }
+  return this.onConsentResponseDeferred_.promise();
+};
+
+/** @return {remoting.HangoutConsentDialog} */
+remoting.HangoutConsentDialog.getInstance = function() {
+  if (!instance_) {
+    instance_ = new remoting.HangoutConsentDialog();
+  }
+  return instance_;
+};
+
+}());
+