[Webapp Refactor] Reliably cancels a connection.

remoting/webapp/base/js/modal_dialogs.js

 // Copyright 2015 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';
@@ skipping 38 matching lines @@
       new base.DomEventHook(this.cancelButton_, 'click', onCancel, false));
   base.debug.assert(this.deferred_ === null);
   this.deferred_ = new base.Deferred();
   remoting.setMode(this.appMode_);
   return this.deferred_.promise();
 };
 
 /** @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
  * @return {Function}
  * @private
  */
 remoting.InputDialog.prototype.createFormEventHandler_ = function(handler) {
   var that = this;
   return function (/** Event */ e) {
     // Prevents form submission from reloading the v1 app.
@@ skipping 10 matching lines @@
   };
 };
 
 /**
  * A helper class for implementing MessageDialog with a primary and
  * and secondary button using remoting.setMode().
  *
  * @param {remoting.AppMode} mode
  * @param {HTMLElement} primaryButton
  * @param {HTMLElement=} opt_secondaryButton
+ *
  * @constructor
+ * @implements {base.Disposable}
  */
 remoting.MessageDialog = function(mode, primaryButton, opt_secondaryButton) {
   /** @private @const */
   this.mode_ = mode;
   /** @private @const */
   this.primaryButton_ = primaryButton;
   /** @private @const */
   this.secondaryButton_ = opt_secondaryButton;
   /** @private {base.Deferred} */
   this.deferred_ = null;
@@ skipping 18 matching lines @@
         this.onClicked_.bind(this, remoting.MessageDialog.Result.SECONDARY),
         false));
   }
 
   base.debug.assert(this.deferred_ === null);
   this.deferred_ = new base.Deferred();
   remoting.setMode(this.mode_);
   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}
  * @private
  */
 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();
 };
 
 })();
 
 /**
  * Define the enum at the end of file as JSCompile doesn't understand enums that
  * are defined within an IIFE (Immediately Invoked Function Expression).
  * @enum {number}
  */
 remoting.MessageDialog.Result = {
   PRIMARY: 0,
   SECONDARY: 1
 };