Log signal strategy progress to server.

Log signal strategy progress to server.

The previous design provided a callback so that an external function could log the progress,
but since it's not possible to log anything until the signal strategy has connected, that
doesn't make a lot of sense. The new design instead tracks progress internally and sends it
when the connection is established.

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

[Jamie, 2015-01-16 22:17:46]

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

[Jamie, 2015-01-16 22:17:46]

ptal

https://codereview.chromium.org/855003008/diff/1/remoting/webapp/crd/js/log_t...
File remoting/webapp/crd/js/log_to_server.js (right):

https://codereview.chromium.org/855003008/diff/1/remoting/webapp/crd/js/log_t...
remoting/webapp/crd/js/log_to_server.js:61: this.maybeExpireSessionId_();
Fixed naming convention for consistency.

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

https://codereview.chromium.org/855003008/diff/1/remoting/webapp/unittests/fa...
remoting/webapp/unittests/fallback_signal_strategy_unittest.js:198:
remoting.FallbackSignalStrategy.Progress.PRIMARY_FAILED);
If both signaling strategies fail, it's not possible to log anything, so there's
no point asserting that the correct Progress values were returned.

[rmsousa, 2015-01-17 00:23:17]

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

https://codereview.chromium.org/855003008/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:132: PRIMARY_SUCCEEDED:
'primary-succeeded',
This naming is fairly ambiguous - I assume the intention of naming them primary
and secondary rather than hardcoding xmpp and wcs is that it is
theoretically/futurely possible to have a different precedence, or new signal
strategy types. But if that's the case, references to "primary" or "secondary"
in the logs are meaningless, because one can't really tell (authoritatively)
what primary and secondary means without external/time-dependent context. A log
entry should be self contained - otherwise, how can one, for example, calculate
aggregate stats for data that spans across a change in the external context?
(especially if that change may take time to propagate from 0 to 100%)

We should have an additional enum for SignalStrategyName (SignalStrategyType?
SignalStrategyImplementation?), and add primaryStrategyName and
secondaryStrategyName log fields.

[Jamie, 2015-01-21 19:41:08]

ptal

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

https://codereview.chromium.org/855003008/diff/1/remoting/webapp/crd/js/fallb...
remoting/webapp/crd/js/fallback_signal_strategy.js:132: PRIMARY_SUCCEEDED:
'primary-succeeded',
On 2015/01/17 00:23:17, rmsousa wrote:
> This naming is fairly ambiguous - I assume the intention of naming them
primary
> and secondary rather than hardcoding xmpp and wcs is that it is
> theoretically/futurely possible to have a different precedence, or new signal
> strategy types. But if that's the case, references to "primary" or "secondary"
> in the logs are meaningless, because one can't really tell (authoritatively)
> what primary and secondary means without external/time-dependent context. A
log
> entry should be self contained - otherwise, how can one, for example,
calculate
> aggregate stats for data that spans across a change in the external context?
> (especially if that change may take time to propagate from 0 to 100%)
> 
> We should have an additional enum for SignalStrategyName (SignalStrategyType?
> SignalStrategyImplementation?), and add primaryStrategyName and
> secondaryStrategyName log fields.

Done.

https://codereview.chromium.org/855003008/diff/20001/remoting/webapp/crd/js/s...
File remoting/webapp/crd/js/signal_strategy.js (right):

https://codereview.chromium.org/855003008/diff/20001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/signal_strategy.js:91:
remoting.SignalStrategy.prototype.getType = function() {};
I'm not crazy about this design, since it means that the base class effectively
needs to know about all (non-test) subclasses, but I don't see a cleaner
alternative that satisfies your logging suggestions (which I agree with).

[rmsousa, 2015-01-22 01:14:18]

lgtm

[Jamie, 2015-01-22 01:20:06]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2015-01-22 01:21:09]

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

[commit-bot: I haz the power, 2015-01-22 01:26:40]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Jamie, 2015-01-22 01:26:44]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2015-01-22 01:26:50]

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

[commit-bot: I haz the power, 2015-01-22 02:45:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-22 02:47:45]

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