[cast_channel] Implement CastSocketService::OpenSocket()

[cast_channel] Implement CastSocketService::OpenSocket()

Extract open channel logic from CastChannelOpenFunction::AsyncWorkStart() so that it can be use by CastMediaSinkService.

OpenChannel(ip_address1, ..., op_cb_1);
OpenChannel(ip_address1, ..., op_cb_2);

Should only create one cast socket and invoke both op_cb_1 and op_cb_2 when socket open finishes.

BUG=687377
Review-Url: https://codereview.chromium.org/2925053005
Cr-Commit-Position: refs/heads/master@{#483039}
Committed: https://chromium.googlesource.com/chromium/src/+/9662cb983c489007e8dc9a58f91d4fbb47f64290

[zhaobin, 2017-06-10 01:07:28]

Patchset #2 (id:20001) has been deleted

[zhaobin, 2017-06-10 01:24:38]

Description was changed from

==========
[cast_channel] (WIP) Implement CastSocketService::OpenChannel()

Extract open channel logic from CastChannelOpenFunction::AsyncWorkStart() so
that it can be use by CastMediaSinkService.

OpenChannel(ip_address1, ..., op_cb_1);
OpenChannel(ip_address1, ..., op_cb_2);

Should only create one cast socket and invoke both op_cb_1 and op_cb_2 when
socket open finishes.

BUG=687377
==========

to

==========
[cast_channel] Implement CastSocketService::OpenSocket()

Extract open channel logic from CastChannelOpenFunction::AsyncWorkStart() so
that it can be use by CastMediaSinkService.

OpenChannel(ip_address1, ..., op_cb_1);
OpenChannel(ip_address1, ..., op_cb_2);

Should only create one cast socket and invoke both op_cb_1 and op_cb_2 when
socket open finishes.

BUG=687377
==========

[zhaobin, 2017-06-10 01:25:39]

zhaobin@chromium.org changed reviewers:
+ imcheng@chromium.org, mfoltz@chromium.org

[mark a. foltz, 2017-06-12 21:14:09]

I didn't look at the tests closely yet.  marshallk@ is familiar with this code
and might want to take a look.

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
File components/cast_channel/cast_socket.h (right):

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket.h:130: // |channel_auth|. |channel_auth|
must not be CHANNEL_AUTH_NONE.
CHANNEL_AUTH_NONE has been removed from the IDL, so this comment is out of date.
 It would also be good to remove the channel_auth parameter from these classes
but that might be simpler to do in a separate change.

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
File components/cast_channel/cast_socket_service.cc (right):

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:119: NOTREACHED();
Why is it not possible for the socket to be CLOSING here?

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:143:
std::unique_ptr<CastTransport::Delegate> delegate(pass_through_delegate);
base::MakeUnique?

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:182:
socket_record->pass_through_message_handler->RegisterDelegate(
Why does the CastSocketRecord have a "pass-through" delegate that just invokes
an inner delegate when assigned later?

It seems like there need to be two delegates:
1. One to tell the client (i.e. CastChannelAPI) when messages/errors happen
2. One for the keep alive timer to function.

The latter it might make more sense for the CastSocket to create and own vs.
being injected here.  I don't remember why it was originally created by the Cast
Channel API - maybe Kevin does.

As it is I'm having a hard time following the code that sets up the delegates.

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:226: for (const auto&
socket_record_it : socket_records_) {
Consider using std::find_if

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:234: void
CastSocketService::OnOpen(int channel_id, ChannelError error_state) {
It might make more sense for OnOpen to be declared on CastSocketRecord, since it
owns all of the client callbacks.

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
File components/cast_channel/cast_socket_service.h (right):

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.h:71: //                 receiver.
|channel_auth| must not be CHANNEL_AUTH_NONE.
See previous comment re: CHANNEL_AUTH_NONE

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.h:79: int OpenSocket(const
net::IPEndPoint& ip_endpoint,
With 7 parameters (not including the deprecated ChannelAuthType), it makes an
argument for introducing a Parameter Object (struct).  However it depends on how
many times these are used together.

https://sourcemaking.com/refactoring/introduce-parameter-object

If this parameter list is repeated several times (e.g., in unit tests)
introducing a struct does improve readability I think.

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.h:153: CastSocketRecord*
GetSocketRecord(int channel_id) const;
Should this return a pointer to const, or is it okay for callers to modify the
returned object?

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.h:157: CastSocketRecord*
GetSocketRecord(const net::IPEndPoint& ip_endpoint) const;
Same comment applies here.

https://codereview.chromium.org/2925053005/diff/40001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/2925053005/diff/40001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_channel_api.cc:325: socket,
api_->GetLogger()));
You might want to pass the logger as an argument to onOpen, unless you can
guarantee that cast_socket_service_ (which owns the callback that calls OnOpen)
is destroyed before |api_|.

IIUC, both are keyed services on the UI thread, so you could also make |api_|
depend on CastSocketService, which should guarantee destruction order.

https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv...

Unfortunately I can't find any authoritative documentation on KeyedService :-(

[zhaobin, 2017-06-20 01:37:43]

A different implementation based on https://codereview.chromium.org/2942993003/.

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
File components/cast_channel/cast_socket.h (right):

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket.h:130: // |channel_auth|. |channel_auth|
must not be CHANNEL_AUTH_NONE.
On 2017/06/12 21:14:08, mark a. foltz wrote:
> CHANNEL_AUTH_NONE has been removed from the IDL, so this comment is out of
date.
>  It would also be good to remove the channel_auth parameter from these classes
> but that might be simpler to do in a separate change.

Created crbug.com/732669

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
File components/cast_channel/cast_socket_service.cc (right):

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:119: NOTREACHED();
On 2017/06/12 21:14:08, mark a. foltz wrote:
> Why is it not possible for the socket to be CLOSING here?

ReadyState::CLOSING is not used anywhere.

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:143:
std::unique_ptr<CastTransport::Delegate> delegate(pass_through_delegate);
On 2017/06/12 21:14:08, mark a. foltz wrote:
> base::MakeUnique?

Code removed.

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:182:
socket_record->pass_through_message_handler->RegisterDelegate(
On 2017/06/12 21:14:08, mark a. foltz wrote:
> Why does the CastSocketRecord have a "pass-through" delegate that just invokes
> an inner delegate when assigned later?
> 
> It seems like there need to be two delegates:
> 1. One to tell the client (i.e. CastChannelAPI) when messages/errors happen
> 2. One for the keep alive timer to function.
> 
> The latter it might make more sense for the CastSocket to create and own vs.
> being injected here.  I don't remember why it was originally created by the
Cast
> Channel API - maybe Kevin does.
> 
> As it is I'm having a hard time following the code that sets up the delegates.

Code removed. Use observers instead of delegates
(https://codereview.chromium.org/2942993003/).

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:226: for (const auto&
socket_record_it : socket_records_) {
On 2017/06/12 21:14:08, mark a. foltz wrote:
> Consider using std::find_if

Done.

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:234: void
CastSocketService::OnOpen(int channel_id, ChannelError error_state) {
On 2017/06/12 21:14:08, mark a. foltz wrote:
> It might make more sense for OnOpen to be declared on CastSocketRecord, since
it
> owns all of the client callbacks.

Code removed. Let CastSocket track OnOpen callbacks.

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
File components/cast_channel/cast_socket_service.h (right):

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.h:71: //                 receiver.
|channel_auth| must not be CHANNEL_AUTH_NONE.
On 2017/06/12 21:14:08, mark a. foltz wrote:
> See previous comment re: CHANNEL_AUTH_NONE

Created crbug.com/732669.

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.h:79: int OpenSocket(const
net::IPEndPoint& ip_endpoint,
On 2017/06/12 21:14:08, mark a. foltz wrote:
> With 7 parameters (not including the deprecated ChannelAuthType), it makes an
> argument for introducing a Parameter Object (struct).  However it depends on
how
> many times these are used together.
> 
> https://sourcemaking.com/refactoring/introduce-parameter-object
> 
> If this parameter list is repeated several times (e.g., in unit tests)
> introducing a struct does improve readability I think.
> 
> 


Created crbug.com/734855

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.h:153: CastSocketRecord*
GetSocketRecord(int channel_id) const;
On 2017/06/12 21:14:08, mark a. foltz wrote:
> Should this return a pointer to const, or is it okay for callers to modify the
> returned object?

Code removed.

https://codereview.chromium.org/2925053005/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.h:157: CastSocketRecord*
GetSocketRecord(const net::IPEndPoint& ip_endpoint) const;
On 2017/06/12 21:14:08, mark a. foltz wrote:
> Same comment applies here.

Code removed.

https://codereview.chromium.org/2925053005/diff/40001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/2925053005/diff/40001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_channel_api.cc:325: socket,
api_->GetLogger()));
On 2017/06/12 21:14:09, mark a. foltz wrote:
> You might want to pass the logger as an argument to onOpen, unless you can
> guarantee that cast_socket_service_ (which owns the callback that calls
OnOpen)
> is destroyed before |api_|.
> 
> IIUC, both are keyed services on the UI thread, so you could also make |api_|
> depend on CastSocketService, which should guarantee destruction order.
> 
>
https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv...
> 
> Unfortunately I can't find any authoritative documentation on KeyedService :-(

Code removed. api_->GetLogger() is moved back to
CastChannelOpenFunction::AsyncWorkStart().

[mark a. foltz, 2017-06-21 17:41:43]

LGTM % minor comments

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
File components/cast_channel/cast_socket.cc (right):

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket.cc:256: case ReadyState::CLOSING:
It sounds like this can be removed as it's unused?
If so, add a TODO next to the definition.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket.cc:263: NOTREACHED();
nit: << "Unknown ReadyState: " << ready_state_;

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
File components/cast_channel/cast_socket.h (right):

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket.h:364: // Callback invoked when the socket
is connected or fails to connect.
nit: Callbacks

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
File components/cast_channel/cast_socket_service.cc (right):

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:22: // received from the receiver
during LIVENESS_INTERVAL, it is considered gone.
Where is LIVENESS_INTERVAL defined?  Or should it be
kConnectLivenessTimeoutSecs?

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:98: socket =
socket_for_test_.release();
Nit: Slightly prefer to call AddSocket(std::move(socket_for_test_)) here as it's
more obvious what is going on.  AddSocket could return CastSocket* that was
added.

Not a big deal either way.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
File components/cast_channel/cast_socket_service.h (right):

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:45: // |ip_endpoint|: IP address
of the remote host.
Nit: an endpoint is both an address and a port

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:46: // |channel_auth|:
Authentication method used for connecting to a Cast
Remove this comment

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:54: // |open_cb|: Invoked when
cast socket opens.
Nit: OnOpenCallback invoked when cast socket is opened.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:63: CastSocket::Observer*
observer);
Please document |observer|.

Is ownership taken?  If so it should be passed by std::unique_ptr.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:70: // |open_cb|: first parameter
is channel id of newly created socket; second
Can the parameter comments here be copied from above (as I assume they have the
same meanings)?

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:72: int OpenSocket(const
net::IPEndPoint& ip_endpoint,
Is this ctor invoked anywhere outside of tests?
Maybe it can be removed?

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:84: void SetSocketForTest(
Please document this method

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:85:
std::unique_ptr<cast_channel::CastSocket> socket_for_test);
Don't need cast_channel:: here and below

[zhaobin, 2017-06-21 21:58:26]

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
File components/cast_channel/cast_socket.cc (right):

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket.cc:256: case ReadyState::CLOSING:
On 2017/06/21 17:41:42, mark a. foltz wrote:
> It sounds like this can be removed as it's unused?
> If so, add a TODO next to the definition.

Done.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket.cc:263: NOTREACHED();
On 2017/06/21 17:41:42, mark a. foltz wrote:
> nit: << "Unknown ReadyState: " << ready_state_;

Done.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
File components/cast_channel/cast_socket.h (right):

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket.h:364: // Callback invoked when the socket
is connected or fails to connect.
On 2017/06/21 17:41:42, mark a. foltz wrote:
> nit: Callbacks

Done.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
File components/cast_channel/cast_socket_service.cc (right):

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:22: // received from the receiver
during LIVENESS_INTERVAL, it is considered gone.
On 2017/06/21 17:41:42, mark a. foltz wrote:
> Where is LIVENESS_INTERVAL defined?  Or should it be
> kConnectLivenessTimeoutSecs?

Done.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:98: socket =
socket_for_test_.release();
On 2017/06/21 17:41:42, mark a. foltz wrote:
> Nit: Slightly prefer to call AddSocket(std::move(socket_for_test_)) here as
it's
> more obvious what is going on.  AddSocket could return CastSocket* that was
> added.
> 
> Not a big deal either way.

Done.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
File components/cast_channel/cast_socket_service.h (right):

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:45: // |ip_endpoint|: IP address
of the remote host.
On 2017/06/21 17:41:42, mark a. foltz wrote:
> Nit: an endpoint is both an address and a port

Done.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:46: // |channel_auth|:
Authentication method used for connecting to a Cast
On 2017/06/21 17:41:42, mark a. foltz wrote:
> Remove this comment

Done.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:54: // |open_cb|: Invoked when
cast socket opens.
On 2017/06/21 17:41:42, mark a. foltz wrote:
> Nit: OnOpenCallback invoked when cast socket is opened.

Done.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:63: CastSocket::Observer*
observer);
On 2017/06/21 17:41:42, mark a. foltz wrote:
> Please document |observer|.
> 
> Is ownership taken?  If so it should be passed by std::unique_ptr.

No.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:63: CastSocket::Observer*
observer);
On 2017/06/21 17:41:42, mark a. foltz wrote:
> Please document |observer|.
> 
> Is ownership taken?  If so it should be passed by std::unique_ptr.

Done.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:70: // |open_cb|: first parameter
is channel id of newly created socket; second
On 2017/06/21 17:41:42, mark a. foltz wrote:
> Can the parameter comments here be copied from above (as I assume they have
the
> same meanings)?

Done.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:72: int OpenSocket(const
net::IPEndPoint& ip_endpoint,
On 2017/06/21 17:41:42, mark a. foltz wrote:
> Is this ctor invoked anywhere outside of tests?
> Maybe it can be removed?

CastMediaSinkService will call this in
https://codereview.chromium.org/2927833002/.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:84: void SetSocketForTest(
On 2017/06/21 17:41:42, mark a. foltz wrote:
> Please document this method

Done.

https://codereview.chromium.org/2925053005/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.h:85:
std::unique_ptr<cast_channel::CastSocket> socket_for_test);
On 2017/06/21 17:41:42, mark a. foltz wrote:
> Don't need cast_channel:: here and below

Done.

[imcheng, 2017-06-24 01:29:37]

lgtm

https://codereview.chromium.org/2925053005/diff/100001/components/cast_channe...
File components/cast_channel/cast_socket.cc (right):

https://codereview.chromium.org/2925053005/diff/100001/components/cast_channe...
components/cast_channel/cast_socket.cc:273: DCHECK_EQ(ReadyState::NONE,
ready_state_);
Move the DCHECKs to the top of this method.

https://codereview.chromium.org/2925053005/diff/100001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/2925053005/diff/100001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_channel_api.cc:132: return logger_;
We now have 2 Loggers - one here and one in CastSocketService. For consistency I
think we should remove this one and only use the one from CastSocketService.

[zhaobin, 2017-06-26 19:03:45]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[zhaobin, 2017-06-26 19:04:10]

https://codereview.chromium.org/2925053005/diff/100001/components/cast_channe...
File components/cast_channel/cast_socket.cc (right):

https://codereview.chromium.org/2925053005/diff/100001/components/cast_channe...
components/cast_channel/cast_socket.cc:273: DCHECK_EQ(ReadyState::NONE,
ready_state_);
On 2017/06/24 01:29:36, imcheng wrote:
> Move the DCHECKs to the top of this method. 

Done.

https://codereview.chromium.org/2925053005/diff/100001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/2925053005/diff/100001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_channel_api.cc:132: return logger_;
On 2017/06/24 01:29:36, imcheng wrote:
> We now have 2 Loggers - one here and one in CastSocketService. For consistency
I
> think we should remove this one and only use the one from CastSocketService.

|logger_| is used in several places here (e.g. in CastMessageHandler,
CastChannelCloseFunction, ...). Remove |logger_| from CastSocketService and make
it a parameter for OpenSocket().

[commit-bot: I haz the power, 2017-06-26 19:04:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[imcheng, 2017-06-26 19:51:58]

On 2017/06/26 19:04:10, zhaobin wrote:
>
https://codereview.chromium.org/2925053005/diff/100001/components/cast_channe...
> File components/cast_channel/cast_socket.cc (right):
> 
>
https://codereview.chromium.org/2925053005/diff/100001/components/cast_channe...
> components/cast_channel/cast_socket.cc:273: DCHECK_EQ(ReadyState::NONE,
> ready_state_);
> On 2017/06/24 01:29:36, imcheng wrote:
> > Move the DCHECKs to the top of this method. 
> 
> Done.
> 
>
https://codereview.chromium.org/2925053005/diff/100001/extensions/browser/api...
> File extensions/browser/api/cast_channel/cast_channel_api.cc (right):
> 
>
https://codereview.chromium.org/2925053005/diff/100001/extensions/browser/api...
> extensions/browser/api/cast_channel/cast_channel_api.cc:132: return logger_;
> On 2017/06/24 01:29:36, imcheng wrote:
> > We now have 2 Loggers - one here and one in CastSocketService. For
consistency
> I
> > think we should remove this one and only use the one from CastSocketService.
> 
> |logger_| is used in several places here (e.g. in CastMessageHandler,
> CastChannelCloseFunction, ...). Remove |logger_| from CastSocketService and
make
> it a parameter for OpenSocket().

How will CastMediaSinkService pass in a logger when it needs to open a
CastSocket?

[commit-bot: I haz the power, 2017-06-26 20:22:05]

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

[commit-bot: I haz the power, 2017-06-26 20:22:06]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-06-27 19:17:00]

On 2017/06/26 19:51:58, imcheng wrote:
> On 2017/06/26 19:04:10, zhaobin wrote:
> >
>
https://codereview.chromium.org/2925053005/diff/100001/components/cast_channe...
> > File components/cast_channel/cast_socket.cc (right):
> > 
> >
>
https://codereview.chromium.org/2925053005/diff/100001/components/cast_channe...
> > components/cast_channel/cast_socket.cc:273: DCHECK_EQ(ReadyState::NONE,
> > ready_state_);
> > On 2017/06/24 01:29:36, imcheng wrote:
> > > Move the DCHECKs to the top of this method. 
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2925053005/diff/100001/extensions/browser/api...
> > File extensions/browser/api/cast_channel/cast_channel_api.cc (right):
> > 
> >
>
https://codereview.chromium.org/2925053005/diff/100001/extensions/browser/api...
> > extensions/browser/api/cast_channel/cast_channel_api.cc:132: return logger_;
> > On 2017/06/24 01:29:36, imcheng wrote:
> > > We now have 2 Loggers - one here and one in CastSocketService. For
> consistency
> > I
> > > think we should remove this one and only use the one from
CastSocketService.
> > 
> > |logger_| is used in several places here (e.g. in CastMessageHandler,
> > CastChannelCloseFunction, ...). Remove |logger_| from CastSocketService and
> make
> > it a parameter for OpenSocket().
> 
> How will CastMediaSinkService pass in a logger when it needs to open a
> CastSocket?

Done.

[zhaobin, 2017-06-27 22:59:24]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 22:59:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 00:09:48]

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

[commit-bot: I haz the power, 2017-06-28 00:09:49]

Dry run: This issue passed the CQ dry run.

[imcheng, 2017-06-28 00:13:49]

lgtm

[zhaobin, 2017-06-28 17:09:56]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-06-28 17:09:57]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/2925053005/#ps140001
(title: "move |logger_| from cast_channel_api to cast_socket_service")

[commit-bot: I haz the power, 2017-06-28 17:10:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 17:13:55]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498669796802620,
"parent_rev": "36f8928df6b35afb86b6839f350b09e12ab998a2", "commit_rev":
"9662cb983c489007e8dc9a58f91d4fbb47f64290"}

[commit-bot: I haz the power, 2017-06-28 17:14:08]

Description was changed from

==========
[cast_channel] Implement CastSocketService::OpenSocket()

Extract open channel logic from CastChannelOpenFunction::AsyncWorkStart() so
that it can be use by CastMediaSinkService.

OpenChannel(ip_address1, ..., op_cb_1);
OpenChannel(ip_address1, ..., op_cb_2);

Should only create one cast socket and invoke both op_cb_1 and op_cb_2 when
socket open finishes.

BUG=687377
==========

to

==========
[cast_channel] Implement CastSocketService::OpenSocket()

Extract open channel logic from CastChannelOpenFunction::AsyncWorkStart() so
that it can be use by CastMediaSinkService.

OpenChannel(ip_address1, ..., op_cb_1);
OpenChannel(ip_address1, ..., op_cb_2);

Should only create one cast socket and invoke both op_cb_1 and op_cb_2 when
socket open finishes.

BUG=687377

Review-Url: https://codereview.chromium.org/2925053005
Cr-Commit-Position: refs/heads/master@{#483039}
Committed:
https://chromium.googlesource.com/chromium/src/+/9662cb983c489007e8dc9a58f91d...
==========

[commit-bot: I haz the power, 2017-06-28 17:14:10]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/9662cb983c489007e8dc9a58f91d...