[cast_channel] Make CastMessageHandler a CastSocket::Observer instead of CastTransport::Delegate

[cast_channel] Make CastMessageHandler a CastSocket::Observer instead of CastTransport::Delegate

We are going to create one CastSocket for one IP endpoint and share it between browser and different extensions. To handle message and errors, it seems more reasonable to have one CastTransport::Delegate and multiple observers per CastSocket.

BUG=687377
Review-Url: https://codereview.chromium.org/2942993003
Cr-Commit-Position: refs/heads/master@{#481334}
Committed: https://chromium.googlesource.com/chromium/src/+/487f46f96fbed1f58c8dc3fb2d940dd5f36ca7b3

[zhaobin, 2017-06-16 18:14:31]

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

[mark a. foltz, 2017-06-16 23:26:15]

Looks good overall - one design question

https://codereview.chromium.org/2942993003/diff/1/components/cast_channel/cas...
File components/cast_channel/cast_socket.h (right):

https://codereview.chromium.org/2942993003/diff/1/components/cast_channel/cas...
components/cast_channel/cast_socket.h:422: std::map<std::string,
std::unique_ptr<Observer>> observer_map_;
Would a base::ObserverList work here?  I.e. can something keep track that
exactly one Observer is added for the browser, and one per extension?

https://codereview.chromium.org/2942993003/diff/1/extensions/browser/api/cast...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/2942993003/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:298:
socket->AddObserver(extension_->id(),
I think this code should only run once per extension_id and |socket|?
Maybe you don't need to register the observer with an extension_id then, since
it will be bound in the callback.

[imcheng, 2017-06-16 23:39:25]

My comments are mainly geared towards making CastSocket shared across profiles
in the future. Feel free to chat in person next week to clarify them. Thanks!

https://codereview.chromium.org/2942993003/diff/1/components/cast_channel/cas...
File components/cast_channel/cast_socket.h (right):

https://codereview.chromium.org/2942993003/diff/1/components/cast_channel/cas...
components/cast_channel/cast_socket.h:128: virtual bool HasObserver(const
std::string& id) = 0;
I'm not sure if we need to associate an id with an observer. This method, for
instance, can just take a Observer*.

https://codereview.chromium.org/2942993003/diff/1/components/cast_channel/cas...
components/cast_channel/cast_socket.h:133: std::unique_ptr<Observer> observer) =
0;
It's a bit unusual to take ownership of observers... There's no way to remove
observers, and CastSocket does not know when the observer is no longer valid,
resulting in wasted work (CastMessageHandler::*) and memory leaks
(CastMessageHandlers created in Profiles that no longer exist won't get deleted
until the CastSocket is deleted)

https://codereview.chromium.org/2942993003/diff/1/extensions/browser/api/cast...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/2942993003/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:297: if
(!socket->HasObserver(extension_->id())) {
Since we are thinking about sharing CastSocket across profiles -- I don't think
this will work across profiles, since their MR extensions have the same
extension id.

We could try the following, but feel free to suggest other ideas:
- We could move CastMessageHandler into CastChannelAPI and have CastChannelAPI
manage CastMessageHandler. This means CastMessageHandler has an extension id and
is also tied to a Profile.
- However, because CastMessageHandler works in the IO thread, we may have to
make it thread-safe.
- Ideally, CastMessageHandler should be able to unregister itself from a
CastSocket when it is about to be destroyed. To do this, we need to guarantee
CastSocket outlives all observers, or we will need to make CastSocket support
WeakPtr.

[zhaobin, 2017-06-20 00:53:10]

Patchset #2 (id:20001) has been deleted

[zhaobin, 2017-06-20 00:54:36]

https://codereview.chromium.org/2942993003/diff/1/components/cast_channel/cas...
File components/cast_channel/cast_socket.h (right):

https://codereview.chromium.org/2942993003/diff/1/components/cast_channel/cas...
components/cast_channel/cast_socket.h:128: virtual bool HasObserver(const
std::string& id) = 0;
On 2017/06/16 23:39:25, imcheng wrote:
> I'm not sure if we need to associate an id with an observer. This method, for
> instance, can just take a Observer*.

Done.

https://codereview.chromium.org/2942993003/diff/1/components/cast_channel/cas...
components/cast_channel/cast_socket.h:133: std::unique_ptr<Observer> observer) =
0;
On 2017/06/16 23:39:25, imcheng wrote:
> It's a bit unusual to take ownership of observers... There's no way to remove
> observers, and CastSocket does not know when the observer is no longer valid,
> resulting in wasted work (CastMessageHandler::*) and memory leaks
> (CastMessageHandlers created in Profiles that no longer exist won't get
deleted
> until the CastSocket is deleted)

Make CastSocketService own observers.

https://codereview.chromium.org/2942993003/diff/1/components/cast_channel/cas...
components/cast_channel/cast_socket.h:422: std::map<std::string,
std::unique_ptr<Observer>> observer_map_;
On 2017/06/16 23:26:14, mark a. foltz wrote:
> Would a base::ObserverList work here?  I.e. can something keep track that
> exactly one Observer is added for the browser, and one per extension?

Done.

https://codereview.chromium.org/2942993003/diff/1/extensions/browser/api/cast...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/2942993003/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:297: if
(!socket->HasObserver(extension_->id())) {
On 2017/06/16 23:39:25, imcheng wrote:
> Since we are thinking about sharing CastSocket across profiles -- I don't
think
> this will work across profiles, since their MR extensions have the same
> extension id.
> 
> We could try the following, but feel free to suggest other ideas:
> - We could move CastMessageHandler into CastChannelAPI and have CastChannelAPI
> manage CastMessageHandler. This means CastMessageHandler has an extension id
and
> is also tied to a Profile.
> - However, because CastMessageHandler works in the IO thread, we may have to
> make it thread-safe.
> - Ideally, CastMessageHandler should be able to unregister itself from a
> CastSocket when it is about to be destroyed. To do this, we need to guarantee
> CastSocket outlives all observers, or we will need to make CastSocket support
> WeakPtr.

Doing it a little differently.

1. Create one observer instance for each extension and pass ownership to
CastSocketService.
2. For each newly created socket, get observer ptr for |extension_id| and
register it with socket (does not pass ownership)

CastSocketService owns CastSockets and Observers. It can make sure CastSockets
are destroyed before Observers.
3.

https://codereview.chromium.org/2942993003/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:298:
socket->AddObserver(extension_->id(),
On 2017/06/16 23:26:14, mark a. foltz wrote:
> I think this code should only run once per extension_id and |socket|?
> Maybe you don't need to register the observer with an extension_id then, since
> it will be bound in the callback.
> 

Yes.

Done.

[mark a. foltz, 2017-06-20 01:16:44]

lgtm

I'm not sure there will ever be more than one extension id creating cast
channels at any one time, so maybe we don't need to support observers from
multiple extensions.  But we would still want to check we're sending events to
the same extension id that created the socket.

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

https://codereview.chromium.org/2942993003/diff/40001/components/cast_channel...
components/cast_channel/cast_socket.cc:617:
CastSocketImpl::CastSocketMessageDelegate::~CastSocketMessageDelegate() {}
Nit: prefer = default, but be consistent with surrounding code

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

https://codereview.chromium.org/2942993003/diff/40001/components/cast_channel...
components/cast_channel/cast_socket.h:85: // Ownership of |delegate| is
transferred to this CastSocket.
This comment no longer applies

https://codereview.chromium.org/2942993003/diff/40001/components/cast_channel...
components/cast_channel/cast_socket.h:128: // Register |observer| with current
socket to receive message receipt

Registers ... receive messages and ...

https://codereview.chromium.org/2942993003/diff/40001/components/cast_channel...
components/cast_channel/cast_socket.h:416: // coded string.
This comment is outdated and can be removed

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

https://codereview.chromium.org/2942993003/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:58: [id](const std::pair<const
std::string,
Can the parameter passed to the lambda be const auto ?

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

https://codereview.chromium.org/2942993003/diff/40001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_channel_api.cc:304:
base::WrapUnique(observer));
Or just pass base::MakeUnique<> here?

[zhaobin, 2017-06-20 17:46:05]

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

[commit-bot: I haz the power, 2017-06-20 17:46:31]

Dry run: CQ is trying da patch.

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

[zhaobin, 2017-06-20 17:50:06]

We use socket observer map in CastMediaSinkService
(https://codereview.chromium.org/2927833002/) as well, so keep it for now and
change later?

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

https://codereview.chromium.org/2942993003/diff/40001/components/cast_channel...
components/cast_channel/cast_socket.cc:617:
CastSocketImpl::CastSocketMessageDelegate::~CastSocketMessageDelegate() {}
On 2017/06/20 01:16:43, mark a. foltz wrote:
> Nit: prefer = default, but be consistent with surrounding code

Keep {} to be consistent with

~FakeCertVerifier() override {}

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

https://codereview.chromium.org/2942993003/diff/40001/components/cast_channel...
components/cast_channel/cast_socket.h:85: // Ownership of |delegate| is
transferred to this CastSocket.
On 2017/06/20 01:16:43, mark a. foltz wrote:
> This comment no longer applies

Done.

https://codereview.chromium.org/2942993003/diff/40001/components/cast_channel...
components/cast_channel/cast_socket.h:128: // Register |observer| with current
socket to receive message receipt
On 2017/06/20 01:16:43, mark a. foltz wrote:
> 
> Registers ... receive messages and ...

Done.

https://codereview.chromium.org/2942993003/diff/40001/components/cast_channel...
components/cast_channel/cast_socket.h:416: // coded string.
On 2017/06/20 01:16:43, mark a. foltz wrote:
> This comment is outdated and can be removed

Done.

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

https://codereview.chromium.org/2942993003/diff/40001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:58: [id](const std::pair<const
std::string,
On 2017/06/20 01:16:43, mark a. foltz wrote:
> Can the parameter passed to the lambda be const auto ?

Seems not.. Got ''auto' not allowed in lambda parameter' compile error.

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

https://codereview.chromium.org/2942993003/diff/40001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_channel_api.cc:304:
base::WrapUnique(observer));
On 2017/06/20 01:16:44, mark a. foltz wrote:
> Or just pass base::MakeUnique<> here?

Done.

[zhaobin, 2017-06-20 18:14:32]

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

[commit-bot: I haz the power, 2017-06-20 18:14:42]

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-20 19:07:13]

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

[commit-bot: I haz the power, 2017-06-20 19:07:14]

Dry run: This issue passed the CQ dry run.

[imcheng, 2017-06-21 01:22:52]

As discussed in person, the relationship between CastSocket, CastSocketService
and Observers will have to be re-architected when we make CastSocketService a
singleton.

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

https://codereview.chromium.org/2942993003/diff/60001/components/cast_channel...
components/cast_channel/cast_socket.h:65: cast_channel::ChannelError
error_state) = 0;
cast_channel:: not needed, here and below

https://codereview.chromium.org/2942993003/diff/60001/components/cast_channel...
components/cast_channel/cast_socket.h:127: // Registers |observer| with current
socket to receive messages and error
s/current/the

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

https://codereview.chromium.org/2942993003/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:56: auto it = std::find_if(
It seems sufficient to just use socket_observer_map_.find() here?

[imcheng, 2017-06-21 01:22:58]

lgtm

[zhaobin, 2017-06-21 17:48:13]

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

https://codereview.chromium.org/2942993003/diff/60001/components/cast_channel...
components/cast_channel/cast_socket.h:65: cast_channel::ChannelError
error_state) = 0;
On 2017/06/21 01:22:52, imcheng wrote:
> cast_channel:: not needed, here and below

Done.

https://codereview.chromium.org/2942993003/diff/60001/components/cast_channel...
components/cast_channel/cast_socket.h:127: // Registers |observer| with current
socket to receive messages and error
On 2017/06/21 01:22:52, imcheng wrote:
> s/current/the

Done.

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

https://codereview.chromium.org/2942993003/diff/60001/components/cast_channel...
components/cast_channel/cast_socket_service.cc:56: auto it = std::find_if(
On 2017/06/21 01:22:52, imcheng wrote:
> It seems sufficient to just use socket_observer_map_.find() here?

Done.

[zhaobin, 2017-06-21 17:48:24]

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

[commit-bot: I haz the power, 2017-06-21 17:48:45]

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-21 18:56:38]

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

[commit-bot: I haz the power, 2017-06-21 18:56:39]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-06-21 22:30:53]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-06-21 22:30:53]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2942993003/#ps80001
(title: "resolve code review comments from Derek")

[commit-bot: I haz the power, 2017-06-21 22:31:15]

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-21 22:35:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498084253057990,
"parent_rev": "5c689d84a0061602123af60a7fe8026bc236a918", "commit_rev":
"487f46f96fbed1f58c8dc3fb2d940dd5f36ca7b3"}

[commit-bot: I haz the power, 2017-06-21 22:35:40]

Description was changed from

==========
[cast_channel] Make CastMessageHandler a CastSocket::Observer instead of
CastTransport::Delegate

We are going to create one CastSocket for one IP endpoint and share it between
browser and different extensions. To handle message and errors, it seems more
reasonable to have one CastTransport::Delegate and multiple observers per
CastSocket.

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

to

==========
[cast_channel] Make CastMessageHandler a CastSocket::Observer instead of
CastTransport::Delegate

We are going to create one CastSocket for one IP endpoint and share it between
browser and different extensions. To handle message and errors, it seems more
reasonable to have one CastTransport::Delegate and multiple observers per
CastSocket.

BUG=687377

Review-Url: https://codereview.chromium.org/2942993003
Cr-Commit-Position: refs/heads/master@{#481334}
Committed:
https://chromium.googlesource.com/chromium/src/+/487f46f96fbed1f58c8dc3fb2d94...
==========

[commit-bot: I haz the power, 2017-06-21 22:35:41]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/487f46f96fbed1f58c8dc3fb2d94...