[Webapp Refactor] Reliably cancels a connection.

[Webapp Refactor] Reliably cancels a connection.

Prior to this CL, clicking cancel on the connecting dialog simply
returns the user to the home screen, without disconnecting the session or
cleaning up any related resources.

This CL provides a cleaner way to cancel a session with by implementing
remoting.ConnectingDialog which calls stop() on the activity.

BUG=477522
Committed: https://crrev.com/e45afaccefbe7b093c7052ac475e9f3168f1c3c2
Cr-Commit-Position: refs/heads/master@{#326926}

[kelvinp, 2015-04-22 01:15:02]

kelvinp@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[kelvinp, 2015-04-22 01:19:01]

PTAL

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/crd/js/crd_...
File remoting/webapp/crd/js/crd_event_handlers.js (left):

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/crd/js/crd_...
remoting/webapp/crd/js/crd_event_handlers.js:14: var goFinishedIT2Me =
function() {
Dead code.

[kelvinp, 2015-04-22 20:55:16]

kelvinp@chromium.org changed reviewers:
+ garykac@chromium.org

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

Can this be split into multiple CLs? It feels like the three points you call out
in the CL description are independent changes and would be much easier to review
as such.

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
File remoting/webapp/base/js/modal_dialogs.js (right):

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
remoting/webapp/base/js/modal_dialogs.js:139:
remoting.MessageDialog.prototype.reset = function() {
Why not call this dispose() and use the base.Disposable pattern? Alternatively,
make it private.

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
remoting/webapp/base/js/modal_dialogs.js:142: this.deferred_ = null;
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.

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
remoting/webapp/base/js/modal_dialogs.js:157: *     on the cancel button.
Why not make show return a Promise?

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/crd/js/clie...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/crd/js/clie...
remoting/webapp/crd/js/client_session.js:271: if (this.isFinished()) {
Does the sendIq call (below) still happen in all cases?

[kelvinp, 2015-04-23 00:58:33]

Patchset #2 (id:20001) has been deleted

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

PTAL

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
File remoting/webapp/base/js/modal_dialogs.js (right):

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
remoting/webapp/base/js/modal_dialogs.js:139:
remoting.MessageDialog.prototype.reset = function() {
On 2015/04/22 23:11:55, Jamie wrote:
> Why not call this dispose() and use the base.Disposable pattern?
Alternatively,
> make it private.

Done.

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
remoting/webapp/base/js/modal_dialogs.js:142: this.deferred_ = null;
On 2015/04/22 23:11:55, Jamie wrote:
> 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.

Done.

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
remoting/webapp/base/js/modal_dialogs.js:157: *     on the cancel button.
On 2015/04/22 23:11:55, Jamie wrote:
> Why not make show return a Promise?

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.

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/crd/js/clie...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/crd/js/clie...
remoting/webapp/crd/js/client_session.js:271: if (this.isFinished()) {
On 2015/04/22 23:11:55, Jamie wrote:
> Does the sendIq call (below) still happen in all cases?

No and it shoudn't happen if the session is already finished.

For most connection state changes, the client session merely responds to
notifications from the plugin.  Disconnect is a special case as the
clientSession is actively sending the IQ termination and faking a state
transition.

In a cleaner design, the clientSession should expose a disconnect() method which
the clientSession can call into and responds to the CLOSED state changes.

[kelvinp, 2015-04-23 01:44:04]

Patchset #3 (id:60001) has been deleted

[kelvinp, 2015-04-23 23:09:56]

On 2015/04/23 01:17:23, kelvinp wrote:
> PTAL
> 
>
https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
> File remoting/webapp/base/js/modal_dialogs.js (right):
> 
>
https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
> remoting/webapp/base/js/modal_dialogs.js:139:
> remoting.MessageDialog.prototype.reset = function() {
> On 2015/04/22 23:11:55, Jamie wrote:
> > Why not call this dispose() and use the base.Disposable pattern?
> Alternatively,
> > make it private.
> 
> Done.
> 
>
https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
> remoting/webapp/base/js/modal_dialogs.js:142: this.deferred_ = null;
> On 2015/04/22 23:11:55, Jamie wrote:
> > 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.
> 
> Done.
> 
>
https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
> remoting/webapp/base/js/modal_dialogs.js:157: *     on the cancel button.
> On 2015/04/22 23:11:55, Jamie wrote:
> > Why not make show return a Promise?
> 
> 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.
> 
>
https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/crd/js/clie...
> File remoting/webapp/crd/js/client_session.js (right):
> 
>
https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/crd/js/clie...
> remoting/webapp/crd/js/client_session.js:271: if (this.isFinished()) {
> On 2015/04/22 23:11:55, Jamie wrote:
> > Does the sendIq call (below) still happen in all cases?
> 
> No and it shoudn't happen if the session is already finished.
> 
> For most connection state changes, the client session merely responds to
> notifications from the plugin.  Disconnect is a special case as the
> clientSession is actively sending the IQ termination and faking a state
> transition.
> 
> In a cleaner design, the clientSession should expose a disconnect() method
which
> the clientSession can call into and responds to the CLOSED state changes.

Ping

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

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
File remoting/webapp/base/js/modal_dialogs.js (right):

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
remoting/webapp/base/js/modal_dialogs.js:157: *     on the cancel button.
On 2015/04/23 01:17:23, kelvinp wrote:
> On 2015/04/22 23:11:55, Jamie wrote:
> > Why not make show return a Promise?
> 
> 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.

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.

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/crd/js/clie...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/crd/js/clie...
remoting/webapp/crd/js/client_session.js:271: if (this.isFinished()) {
On 2015/04/23 01:17:23, kelvinp wrote:
> On 2015/04/22 23:11:55, Jamie wrote:
> > Does the sendIq call (below) still happen in all cases?
> 
> No and it shoudn't happen if the session is already finished.
> 
> For most connection state changes, the client session merely responds to
> notifications from the plugin.  Disconnect is a special case as the
> clientSession is actively sending the IQ termination and faking a state
> transition.
> 
> In a cleaner design, the clientSession should expose a disconnect() method
which
> the clientSession can call into and responds to the CLOSED state changes.

This message is needed so that the host knows that the client has gone away and
can clean up (for example, resetting the desktop size and dismissing the
"connected" dialog). Are you saying that this will no longer happen in some
cases?

https://codereview.chromium.org/1101613003/diff/80001/remoting/webapp/base/js...
File remoting/webapp/base/js/modal_dialogs.js (right):

https://codereview.chromium.org/1101613003/diff/80001/remoting/webapp/base/js...
remoting/webapp/base/js/modal_dialogs.js:157: this.deferred_ = null;
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.

https://codereview.chromium.org/1101613003/diff/80001/remoting/webapp/base/js...
remoting/webapp/base/js/modal_dialogs.js:180:
}).catch(remoting.Error.handler(base.doNothing));
Why is this catch handler needed?

https://codereview.chromium.org/1101613003/diff/80001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/me2me_activity.js (right):

https://codereview.chromium.org/1101613003/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/me2me_activity.js:148:
remoting.hostList.refresh(onHostListRefresh);
this.hostList_?

[kelvinp, 2015-04-24 19:03:29]

Patchset #4 (id:100001) has been deleted

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

PTAL

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
File remoting/webapp/base/js/modal_dialogs.js (right):

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/base/js/mod...
remoting/webapp/base/js/modal_dialogs.js:157: *     on the cancel button.
On 2015/04/24 17:59:18, Jamie wrote:
> On 2015/04/23 01:17:23, kelvinp wrote:
> > On 2015/04/22 23:11:55, Jamie wrote:
> > > Why not make show return a Promise?
> > 
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/crd/js/clie...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/1101613003/diff/1/remoting/webapp/crd/js/clie...
remoting/webapp/crd/js/client_session.js:271: if (this.isFinished()) {
On 2015/04/24 17:59:18, Jamie wrote:
> On 2015/04/23 01:17:23, kelvinp wrote:
> > On 2015/04/22 23:11:55, Jamie wrote:
> > > Does the sendIq call (below) still happen in all cases?
> > 
> > No and it shoudn't happen if the session is already finished.
> > 
> > For most connection state changes, the client session merely responds to
> > notifications from the plugin.  Disconnect is a special case as the
> > clientSession is actively sending the IQ termination and faking a state
> > transition.
> > 
> > In a cleaner design, the clientSession should expose a disconnect() method
> which
> > the clientSession can call into and responds to the CLOSED state changes.
> 
> This message is needed so that the host knows that the client has gone away
and
> can clean up (for example, resetting the desktop size and dismissing the
> "connected" dialog). Are you saying that this will no longer happen in some
> cases?

Discussed offline and comments added.

https://codereview.chromium.org/1101613003/diff/80001/remoting/webapp/base/js...
File remoting/webapp/base/js/modal_dialogs.js (right):

https://codereview.chromium.org/1101613003/diff/80001/remoting/webapp/base/js...
remoting/webapp/base/js/modal_dialogs.js:157: this.deferred_ = null;
On 2015/04/24 17:59:18, Jamie wrote:
> 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.

Done.

https://codereview.chromium.org/1101613003/diff/80001/remoting/webapp/base/js...
remoting/webapp/base/js/modal_dialogs.js:180:
}).catch(remoting.Error.handler(base.doNothing));
On 2015/04/24 17:59:18, Jamie wrote:
> Why is this catch handler needed?

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

https://codereview.chromium.org/1101613003/diff/80001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/me2me_activity.js (right):

https://codereview.chromium.org/1101613003/diff/80001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/me2me_activity.js:148:
remoting.hostList.refresh(onHostListRefresh);
On 2015/04/24 17:59:18, Jamie wrote:
> this.hostList_?

Done.

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

LGTM with one bugfix.

https://codereview.chromium.org/1101613003/diff/80001/remoting/webapp/base/js...
File remoting/webapp/base/js/modal_dialogs.js (right):

https://codereview.chromium.org/1101613003/diff/80001/remoting/webapp/base/js...
remoting/webapp/base/js/modal_dialogs.js:157: this.deferred_ = null;
On 2015/04/24 19:03:52, kelvinp wrote:
> On 2015/04/24 17:59:18, Jamie wrote:
> > 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.
> 
> Done.

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.

[kelvinp, 2015-04-24 22:10:10]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2015-04-24 22:10:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1101613003/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-24 22:12:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1101613003/140001

[commit-bot: I haz the power, 2015-04-24 23:09:18]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-04-24 23:10:06]

Patchset 5 (id:??) landed as
https://crrev.com/e45afaccefbe7b093c7052ac475e9f3168f1c3c2
Cr-Commit-Position: refs/heads/master@{#326926}