Implement FallbackSignalStrategy.

Implement FallbackSignalStrategy.

This allows a preferred signal strategy (currently always XMPP) to be used
speculatively, falling back on a back-up strategy (currently always WCS) if
that fails or times out. There is also a logging hook that will allow us to
log which strategy is in use, as well as corner cases such as the preferred
strategy succeeding after the time-out has expired (which would imply that
the time-out is too short). The actual logging will be a follow-up CL.

BUG=432171
Committed: https://crrev.com/b47eef370e66f6adfddb8b611af6b18672df326a
Cr-Commit-Position: refs/heads/master@{#311567}

[Jamie, 2015-01-13 18:18:20]

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

[Jamie, 2015-01-13 18:18:21]

Sergey: PTAL at the main logic.
Kelvin: Mostly FYI, but PTAL at the unit tests since you've written more of them
than I have.

[kelvinp, 2015-01-13 19:22:48]

General looks good.

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
File remoting/webapp/crd/js/fallback_signal_strategy.js (right):

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:71: this.State = {
Enum should be defined on the 'class' remoting.FallbackSignalStrategy and not
the instance. e.g.
remoting.FallbackSignalStrategy.State = { .. } ;

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:121:
remoting.FallbackSignalStrategy.Result = {
Nit: ProgressResult.  To make it clear that this is used as the return value for
the progressCallback.

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:252:
this.onProgressCallback_(
base.debug.assert(this.state_ == this.State.SECONDARY_PENDING);

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:299:
this.secondary_.connect(this.server_, this.username_, this.authToken_);
Does the primary and the secondary signal strategy shares the same authToken
even they are different services?

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/unittests/fa...
File remoting/webapp/unittests/fallback_signal_strategy_unittest.js (right):

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/unittests/fa...
remoting/webapp/unittests/fallback_signal_strategy_unittest.js:139:
test('primary fails; secondary succeeds; send & receive routed to it',
Maybe worthwhile to to add verify that the onStateChanged Callback gets called
with the correct sequence of state when the fallback happens.

[Sergey Ulanov, 2015-01-13 19:43:51]

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
File remoting/webapp/crd/js/fallback_signal_strategy.js (right):

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:227:
remoting.FallbackSignalStrategy.prototype.onStateChanged_ =
Maybe separate this into two callbacks, one for primary and one for secondary
connection? I think that would simplify state handling.

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:290:
remoting.FallbackSignalStrategy.prototype.reconnect_ = function() {
maybe call it connectSecondary_()?

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:299:
this.secondary_.connect(this.server_, this.username_, this.authToken_);
On 2015/01/13 19:22:48, kelvinp wrote:
> Does the primary and the secondary signal strategy shares the same authToken
> even they are different services?

we use the same token for xmpp and wcs

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:313: * @private
This is called from tests so it isn't really private.

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/signa...
File remoting/webapp/crd/js/signal_strategy.js (right):

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/signa...
remoting/webapp/crd/js/signal_strategy.js:86: function(onStateChangedCallback_)
{
here and below: remove _ suffix from the name

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/signa...
remoting/webapp/crd/js/signal_strategy.js:86: function(onStateChangedCallback_)
{
Maybe move this function out of the argument list, to make this code more
readable.

[Jamie, 2015-01-14 01:36:26]

PTAL

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
File remoting/webapp/crd/js/fallback_signal_strategy.js (right):

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:71: this.State = {
On 2015/01/13 19:22:47, kelvinp wrote:
> Enum should be defined on the 'class' remoting.FallbackSignalStrategy and not
> the instance. e.g.
> remoting.FallbackSignalStrategy.State = { .. } ;

I considered that, but it ends up being incredibly verbose at all the
call-sites, and for an internal private enum, I felt that this was more
idiomatic JavaScript. With the suggested name change for ProgressResult, I've
ended up having to use a "typedef" for shorter lines, so I could do the same
with this type if you feel strongly that it should be defined on the class.

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:121:
remoting.FallbackSignalStrategy.Result = {
On 2015/01/13 19:22:47, kelvinp wrote:
> Nit: ProgressResult.  To make it clear that this is used as the return value
for
> the progressCallback. 

I changed it to Progress, since it's not always a final "result" (the callback
might see primary-failed, then secondary-succeeded, maybe even followed by
primary-succeeded-late).

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:227:
remoting.FallbackSignalStrategy.prototype.onStateChanged_ =
On 2015/01/13 19:43:51, Sergey Ulanov wrote:
> Maybe separate this into two callbacks, one for primary and one for secondary
> connection? I think that would simplify state handling.

Done.

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:252:
this.onProgressCallback_(
On 2015/01/13 19:22:48, kelvinp wrote:
> base.debug.assert(this.state_ == this.State.SECONDARY_PENDING);

I've factored this into two separate functions. LMK if you still think this
assert would make it clearer.

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:290:
remoting.FallbackSignalStrategy.prototype.reconnect_ = function() {
On 2015/01/13 19:43:51, Sergey Ulanov wrote:
> maybe call it connectSecondary_()?

Done.

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:313: * @private
On 2015/01/13 19:43:51, Sergey Ulanov wrote:
> This is called from tests so it isn't really private. 

True, but marking it as private means that the compiler will complain if
production code calls it (we don't run the compiler over our tests). That said,
the same test that calls this accesses PRIMARY_CONNECT_TIMEOUT_MS_ directly, so
I could simply remove these methods. WDYT?

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/signa...
File remoting/webapp/crd/js/signal_strategy.js (right):

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/signa...
remoting/webapp/crd/js/signal_strategy.js:86: function(onStateChangedCallback_)
{
On 2015/01/13 19:43:51, Sergey Ulanov wrote:
> here and below: remove _ suffix from the name

Done.

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/signa...
remoting/webapp/crd/js/signal_strategy.js:86: function(onStateChangedCallback_)
{
On 2015/01/13 19:43:51, Sergey Ulanov wrote:
> here and below: remove _ suffix from the name

Done.

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/unittests/fa...
File remoting/webapp/unittests/fallback_signal_strategy_unittest.js (right):

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/unittests/fa...
remoting/webapp/unittests/fallback_signal_strategy_unittest.js:139:
test('primary fails; secondary succeeds; send & receive routed to it',
On 2015/01/13 19:22:48, kelvinp wrote:
> Maybe worthwhile to to add verify that the onStateChanged Callback gets called
> with the correct sequence of state when the fallback happens.

setStateForTesting already asserts that, based on the expectExternalCallback
parameter. Or have I misunderstood your suggestion?

[Sergey Ulanov, 2015-01-14 19:42:43]

lgtm

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
File remoting/webapp/crd/js/fallback_signal_strategy.js (right):

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:313: * @private
On 2015/01/14 01:36:26, Jamie wrote:
> On 2015/01/13 19:43:51, Sergey Ulanov wrote:
> > This is called from tests so it isn't really private. 
> 
> True, but marking it as private means that the compiler will complain if
> production code calls it (we don't run the compiler over our tests). That
said,
> the same test that calls this accesses PRIMARY_CONNECT_TIMEOUT_MS_ directly,
so
> I could simply remove these methods. WDYT?

sgtm

https://codereview.chromium.org/815943004/diff/20001/remoting/webapp/crd/js/f...
File remoting/webapp/crd/js/fallback_signal_strategy.js (right):

https://codereview.chromium.org/815943004/diff/20001/remoting/webapp/crd/js/f...
remoting/webapp/crd/js/fallback_signal_strategy.js:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2015

[kelvinp, 2015-01-14 19:46:22]

lgtm

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
File remoting/webapp/crd/js/fallback_signal_strategy.js (right):

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:121:
remoting.FallbackSignalStrategy.Result = {
On 2015/01/14 01:36:26, Jamie wrote:
> On 2015/01/13 19:22:47, kelvinp wrote:
> > Nit: ProgressResult.  To make it clear that this is used as the return value
> for
> > the progressCallback. 
> 
> I changed it to Progress, since it's not always a final "result" (the callback
> might see primary-failed, then secondary-succeeded, maybe even followed by
> primary-succeeded-late).

Done.

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:252:
this.onProgressCallback_(
On 2015/01/14 01:36:26, Jamie wrote:
> On 2015/01/13 19:22:48, kelvinp wrote:
> > base.debug.assert(this.state_ == this.State.SECONDARY_PENDING);
> 
> I've factored this into two separate functions. LMK if you still think this
> assert would make it clearer.

I think the code is much clearer with two separate function :)

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/unittests/fa...
File remoting/webapp/unittests/fallback_signal_strategy_unittest.js (right):

https://codereview.chromium.org/815943004/diff/1/remoting/webapp/unittests/fa...
remoting/webapp/unittests/fallback_signal_strategy_unittest.js:139:
test('primary fails; secondary succeeds; send & receive routed to it',
On 2015/01/14 01:36:26, Jamie wrote:
> On 2015/01/13 19:22:48, kelvinp wrote:
> > Maybe worthwhile to to add verify that the onStateChanged Callback gets
called
> > with the correct sequence of state when the fallback happens.
> 
> setStateForTesting already asserts that, based on the expectExternalCallback
> parameter. Or have I misunderstood your suggestion?

You are right.  I must have misread the code earlier.

[Jamie, 2015-01-14 20:53:51]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2015-01-14 20:54:37]

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

[commit-bot: I haz the power, 2015-01-14 22:59:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-14 23:00:56]

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