[Webapp Refactor] Reliably cancels a connection.

remoting/webapp/base/js/modal_dialogs.js

Index: remoting/webapp/base/js/modal_dialogs.js
diff --git a/remoting/webapp/base/js/modal_dialogs.js b/remoting/webapp/base/js/modal_dialogs.js
index f8837e51026bddb3ee20238b9cf5aa47e9455b08..19af3cf549472e76b51f5e81efd55614de9103fe 100644
--- a/remoting/webapp/base/js/modal_dialogs.js
+++ b/remoting/webapp/base/js/modal_dialogs.js
@@ -56,17 +56,17 @@ remoting.InputDialog.prototype.show = function() {
 /** @return {HTMLElement} */
 remoting.InputDialog.prototype.inputField = function() {
   return this.inputField_;
-}
+};
 
 /** @private */
 remoting.InputDialog.prototype.onSubmit_ = function() {
   this.deferred_.resolve(this.inputField_.value);
-}
+};
 
 /** @private */
 remoting.InputDialog.prototype.onCancel_ = function() {
   this.deferred_.reject(new remoting.Error(remoting.Error.Tag.CANCELLED));
-}
+};
 
 /**
  * @param {function():void} handler
@@ -136,6 +136,12 @@ remoting.MessageDialog.prototype.show = function() {
   return this.deferred_.promise();
 };
 
+remoting.MessageDialog.prototype.reset = function() {

[Jamie, 2015/04/22 23:11:55]

Why not call this dispose() and use the base.Disposable pattern? Alternatively,
make it private.

[kelvinp, 2015/04/23 01:17:23]

Done.

+  base.dispose(this.eventHooks_);
+  this.eventHooks_ = null;
+  this.deferred_ = null;

[Jamie, 2015/04/22 23:11:55]

If this remains public, you should call deferred_.reject(CANCELLED) if it's not
null, since the caller can't know if there's something pending.

[kelvinp, 2015/04/23 01:17:23]

Done.

+};
+
 /**
  * @param {remoting.MessageDialog.Result} result
  * @return {Function}
@@ -143,9 +149,33 @@ remoting.MessageDialog.prototype.show = function() {
  */
 remoting.MessageDialog.prototype.onClicked_ = function(result) {
   this.deferred_.resolve(result);
-  base.dispose(this.eventHooks_);
-  this.eventHooks_ = null;
-  this.deferred_ = null;
+  this.reset();
+};
+
+/**
+ * @param {Function} cancelCallback The callback to invoke when the user clicks
+ *     on the cancel button.

[Jamie, 2015/04/22 23:11:55]

Why not make show return a Promise?

[kelvinp, 2015/04/23 01:17:23]

The cancel dialog has a different use case than other modal dialogs.
For other dialogs, its dismissal is mainly user driven and we would like to
block until the user clicks on a button. A promise is ideal for that.

For the cancel dialog, its dismissal driven by the connection transiting to the
next state (PIN is prompted) AND user cancelation.  By using a callback, instead
of a then handler, it provides a stronger guarantee that the stop callback won't
be invoked multiple times.

[Jamie, 2015/04/24 17:59:18]

I don't understand. A Promise is guaranteed one-shot, whereas a callback can be
invoked by anything that has a reference to it as many times as it likes.

[kelvinp, 2015/04/24 19:03:52]

While a promise only resolves once, every time when then() is called, another
callback is registered to it and will be invoked if the promise has already
resolved.

Taking me2me as an example, show() is called in multiple places (before and
after PIN prompt, with essentially the same callback.  Using a cancelCallback in
the constructor saves us from duplicating the canceling code.

As discussed, we can revisit this in the future to whether repurpose the PIN
dialog as a 
connecting dialog.

+ * @constructor
+ */
+remoting.ConnectingDialog = function(cancelCallback) {
+  /** @private */
+  this.dialog_ = new remoting.MessageDialog(
+      remoting.AppMode.CLIENT_CONNECTING,
+      document.getElementById('cancel-connect-button'));
+  /** @private */
+  this.onCancel_ = cancelCallback;
+};
+
+remoting.ConnectingDialog.prototype.show = function() {
+  var that = this;
+  this.dialog_.show().then(function() {
+    remoting.setMode(remoting.AppMode.HOME);
+    that.onCancel_();
+  });
+};
+
+remoting.ConnectingDialog.prototype.hide = function() {
+  this.dialog_.reset();
 };
 
 })();