[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..24e4e9e5da88306e03168c0419f60658fed779ea 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
@@ -97,7 +97,9 @@ remoting.InputDialog.prototype.createFormEventHandler_ = function(handler) {
  * @param {remoting.AppMode} mode
  * @param {HTMLElement} primaryButton
  * @param {HTMLElement=} opt_secondaryButton
+ *
  * @constructor
+ * @implements {base.Disposable}
  */
 remoting.MessageDialog = function(mode, primaryButton, opt_secondaryButton) {
   /** @private @const */
@@ -136,6 +138,15 @@ remoting.MessageDialog.prototype.show = function() {
   return this.deferred_.promise();
 };
 
+remoting.MessageDialog.prototype.dispose = function() {
+  base.dispose(this.eventHooks_);
+  this.eventHooks_ = null;
+  if (this.deferred_) {
+    this.deferred_.reject(new remoting.Error(remoting.Error.Tag.CANCELLED));
+    this.deferred_ = null;
+  }
+};
+
 /**
  * @param {remoting.MessageDialog.Result} result
  * @return {Function}
@@ -143,9 +154,34 @@ 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;

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

Optional: Rejecting a Promise that has already resolved is a no-op, so this line
isn't strictly needed. It's arguably more explicit, so I don't object to you
leaving it, but consider instead a comment in dispose() to the effect that the
reject() call will only have an effect is the Promise is unresolved.

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

Done.

[Jamie, 2015/04/24 20:19:32]

I don't think your new change will work; deferred is null but you're calling
reject on it anyway. I think the previous version was fine, it's just
interesting to note that if any similar call-site forgets to null deferred
before calling dispose, it doesn't actually matter.

+  this.dispose();
+};
+
+/**
+ * @param {Function} cancelCallback The callback to invoke when the user clicks
+ *     on the cancel button.
+ * @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_();
+  }).catch(remoting.Error.handler(base.doNothing));

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

Why is this catch handler needed?

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

The promise rejects when we hide the connection dialog and we don't want to
report that as an error.

+};
+
+remoting.ConnectingDialog.prototype.hide = function() {
+  this.dialog_.dispose();
 };
 
 })();