Added connection timeout functionality to CastSocket.

Added connection timeout functionality to CastSocket.
Added a default timeout interval (8s) to Cast API. Will expose to callers in a followup CL. 8s seconds was selected (rather arbitrarily) to match cloudview.sender.ChannelService.CHANNEL_CREATION_TIMEOUT_ from the extension.
 
Miscellaneous minor doc fixes, and lint-friendly cleanups.

BUG=335221
R=mfoltz@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285455

[haibinlu, 2014-07-15 22:10:45]

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_channel_api.cc:25: const int64
kDefaultConnectTimeoutMillis = 10000;  // 10 seconds.
5 seconds?  we have been using this value.

[mark a. foltz, 2014-07-15 22:18:16]

Mostly minor comments on naming, although I ask you to carefully look at the
code paths in cast_socket.cc and make sure the following are guaranteed each
time connection is attempted:

(1) The connect callback is called with the correct error code.
(2) The cancelable timer fires or is canceled
(3) If an error occurs any allocated socket(s) are closed

Thanks

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_channel_api.cc:24: //
TODO(kmarshall): add Javascript plumbing so clients can set the timeout.
Can this be done as part of this change?  It should be straightforward to add a
field to the connectInfo object with a default.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right):

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.cc:196:
CloseWithError(CHANNEL_ERROR_CONNECT_ERROR);
I would be in favor of adding an error enum CHANNEL_ERROR_CONNECT_TIMEOUT.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.cc:197: timed_out_ =
true;
Is timed_out_ used anywhere?

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.cc:207: VLOG(1) <<
"Connection timeout; DoConnectLoop aborted.";
Is this the only possible way error_state_ can be set?
Seems safer to log the error_state_ than assume it's one value.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/cast_channel/cast_socket.h (right):

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.h:73: int64 timeout_ms);
connect_timeout_ms

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.h:198: void Timeout();
CancelConnect()?

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.h:324: int64
timeout_interval_ms_;
connect_timeout_ms_

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.h:326:
base::CancelableClosure timeout_callback_;
connect_timeout_callback_

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.h:327: bool timed_out_;
connect_timed_out_

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc (right):

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:159: static
scoped_ptr<TestCastSocket> CreateDoomedToTimeout(
Instead of fudging timeouts the cleaner way to test time-dependent behavior is
to use a mock clock.  Maybe another reviewer can point to a code sample on how
to do that.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:577: //
against timing-related flakiness).
A mock clock would definitely be better here.

[Kevin M, 2014-07-16 22:59:42]

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_channel_api.cc:24: //
TODO(kmarshall): add Javascript plumbing so clients can set the timeout.
On 2014/07/15 22:18:15, mark a. foltz wrote:
> Can this be done as part of this change?  It should be straightforward to add
a
> field to the connectInfo object with a default.

Done.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_channel_api.cc:25: const int64
kDefaultConnectTimeoutMillis = 10000;  // 10 seconds.
On 2014/07/15 22:10:45, haibinlu wrote:
> 5 seconds?  we have been using this value.

Done.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right):

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.cc:196:
CloseWithError(CHANNEL_ERROR_CONNECT_ERROR);
On 2014/07/15 22:18:15, mark a. foltz wrote:
> I would be in favor of adding an error enum CHANNEL_ERROR_CONNECT_TIMEOUT.

Done.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.cc:197: timed_out_ =
true;
On 2014/07/15 22:18:15, mark a. foltz wrote:
> Is timed_out_ used anywhere?

No, I ended up using the error state instead. Removed.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.cc:207: VLOG(1) <<
"Connection timeout; DoConnectLoop aborted.";
On 2014/07/15 22:18:15, mark a. foltz wrote:
> Is this the only possible way error_state_ can be set?
> Seems safer to log the error_state_ than assume it's one value.

Good idea, done.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/cast_channel/cast_socket.h (right):

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.h:73: int64 timeout_ms);
On 2014/07/15 22:18:15, mark a. foltz wrote:
> connect_timeout_ms

Done.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.h:198: void Timeout();
On 2014/07/15 22:18:15, mark a. foltz wrote:
> CancelConnect()?

Done.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.h:324: int64
timeout_interval_ms_;
On 2014/07/15 22:18:15, mark a. foltz wrote:
> connect_timeout_ms_

Done.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.h:326:
base::CancelableClosure timeout_callback_;
On 2014/07/15 22:18:15, mark a. foltz wrote:
> connect_timeout_callback_

Done.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket.h:327: bool timed_out_;
On 2014/07/15 22:18:15, mark a. foltz wrote:
> connect_timed_out_

Done. Removed this variable - code checks the error state instead of this bool.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc (right):

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:159: static
scoped_ptr<TestCastSocket> CreateDoomedToTimeout(
On 2014/07/15 22:18:16, mark a. foltz wrote:
> Instead of fudging timeouts the cleaner way to test time-dependent behavior is
> to use a mock clock.  Maybe another reviewer can point to a code sample on how
> to do that.

Done. New approach uses Timer.

https://codereview.chromium.org/393023003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:577: //
against timing-related flakiness).
On 2014/07/15 22:18:16, mark a. foltz wrote:
> A mock clock would definitely be better here.

Done.

[mark a. foltz, 2014-07-17 23:56:50]

lgtm

Nice patch.

I'm not a committer and I don't think Haibin is either, so you'll want to get
one of Wez/Alpha/Yuri to look at it too.  (It's on my TODO list)

https://codereview.chromium.org/393023003/diff/10001/chrome/browser/extension...
File chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc (right):

https://codereview.chromium.org/393023003/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:174:
connect_timeout_timer_ = connect_timeout_timer_owned.get();
You are creating a shared ownership of the timer between the test fixture and
the socket.  Can you add a GetTimerForTesting() to the socket instead?

https://codereview.chromium.org/393023003/diff/10001/chrome/common/extensions...
File chrome/common/extensions/api/cast_channel.idl (right):

https://codereview.chromium.org/393023003/diff/10001/chrome/common/extensions...
chrome/common/extensions/api/cast_channel.idl:48: connect_timeout
Can we leave "unknown" as the last enum value?

https://codereview.chromium.org/393023003/diff/10001/chrome/common/extensions...
chrome/common/extensions/api/cast_channel.idl:72: long? timeout;
Maybe just 5000 ms.

[Kevin M, 2014-07-22 18:51:50]

https://codereview.chromium.org/393023003/diff/10001/chrome/browser/extension...
File chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc (right):

https://codereview.chromium.org/393023003/diff/10001/chrome/browser/extension...
chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:174:
connect_timeout_timer_ = connect_timeout_timer_owned.get();
On 2014/07/17 23:56:49, mark a. foltz wrote:
> You are creating a shared ownership of the timer between the test fixture and
> the socket.  Can you add a GetTimerForTesting() to the socket instead?

Done.

https://codereview.chromium.org/393023003/diff/10001/chrome/common/extensions...
File chrome/common/extensions/api/cast_channel.idl (right):

https://codereview.chromium.org/393023003/diff/10001/chrome/common/extensions...
chrome/common/extensions/api/cast_channel.idl:48: connect_timeout
On 2014/07/17 23:56:50, mark a. foltz wrote:
> Can we leave "unknown" as the last enum value?

Done. This a forward compatible change, right? Javascript understands enum
values by their identifiers, not ordinal values, right?

https://codereview.chromium.org/393023003/diff/10001/chrome/common/extensions...
chrome/common/extensions/api/cast_channel.idl:72: long? timeout;
On 2014/07/17 23:56:50, mark a. foltz wrote:
> Maybe just 5000 ms.

OK. 8000ms to match cloudview.sender.ChannelService.CHANNEL_CREATION_TIMEOUT_ .

[mark a. foltz, 2014-07-22 18:57:53]

lgtm

https://codereview.chromium.org/393023003/diff/10001/chrome/common/extensions...
File chrome/common/extensions/api/cast_channel.idl (right):

https://codereview.chromium.org/393023003/diff/10001/chrome/common/extensions...
chrome/common/extensions/api/cast_channel.idl:48: connect_timeout
On 2014/07/22 18:51:49, kmarshall wrote:
> On 2014/07/17 23:56:50, mark a. foltz wrote:
> > Can we leave "unknown" as the last enum value?
> 
> Done. This a forward compatible change, right? Javascript understands enum
> values by their identifiers, not ordinal values, right?

Enums are passed by string, e.g. 'connection_timeout', to/from the JS.

[miu, 2014-07-22 20:18:54]

Please use base::TimeDelta throughout the code (i.e., everywhere you have a
xxxxx_ms variable/class member).  This is preferred in Chromium code for both
readability and to ensure the compiler is type-checking all time-related
uses/calculations.  It's extra work, but this has personally saved me from some
very non-obvious bugs in the past.

However, note that static initializers are not allowed, so it's fine to keep the
global constants as:

  static const int64 kSomethingTimePeriodMillis = 4242;

...and wrap them in base::TimeDelta when used.

[Kevin M, 2014-07-22 21:02:36]

On 2014/07/22 20:18:54, miu wrote:
> Please use base::TimeDelta throughout the code (i.e., everywhere you have a
> xxxxx_ms variable/class member).  This is preferred in Chromium code for both
> readability and to ensure the compiler is type-checking all time-related
> uses/calculations.  It's extra work, but this has personally saved me from
some
> very non-obvious bugs in the past.
> 
> However, note that static initializers are not allowed, so it's fine to keep
the
> global constants as:
> 
>   static const int64 kSomethingTimePeriodMillis = 4242;
> 
> ...and wrap them in base::TimeDelta when used.

Done.

[miu, 2014-07-23 03:09:41]

lgtm

[Kevin M, 2014-07-23 16:27:26]

Hey Benjamin,

This CL touches the cast_channel.idl. Would you mind taking a look? Thanks!

[not at google - send to devlin, 2014-07-23 16:32:32]

lgtm

[Kevin M, 2014-07-23 23:43:33]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2014-07-23 23:47:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kmarshall@chromium.org/393023003/70001

[commit-bot: I haz the power, 2014-07-24 05:50:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-24 07:31:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-24 07:31:49]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Kevin M, 2014-07-24 22:08:56]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2014-07-24 22:12:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kmarshall@chromium.org/393023003/90001

[commit-bot: I haz the power, 2014-07-25 02:29:03]

Change committed as 285455