[cast_channel] Make CastSocketService a global leaky singleton

[cast_channel] Make CastSocketService a global leaky singleton

- make CastSocketService a global leaky singleton and remove CastSocketServiceFactory
- make CastChannelAPI and CastMediaSinkService own CastSocket::Observer
- fix unit tests and browser tests

BUG=687377, 725717
Review-Url: https://codereview.chromium.org/2974523002
Cr-Commit-Position: refs/heads/master@{#486886}
Committed: https://chromium.googlesource.com/chromium/src/+/edc090649966c37e060d073addca425382dc4e38

[zhaobin, 2017-07-06 18:56:56]

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

[mark a. foltz, 2017-07-10 20:43:06]

LGTM with one comment

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

https://codereview.chromium.org/2974523002/diff/1/components/cast_channel/cas...
components/cast_channel/cast_socket.h:133: // Unregisters |observer|
Nit: Add period at end.

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

https://codereview.chromium.org/2974523002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:489: void
CastChannelAPI::CastMessageHandler::RegisterExtensionId(
Right now there are only two extension ids that are whitelisted for the
cast.channel API [1], and basically only one can be loaded at a time based on
the --load-media-router-component-extension flag.

Unless anyone knows a case where two different extensions are using the API in
the same profile, I would be okay simplifying this to allow one extension id at
a time.

Derek, WDYT?

[1]
https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur...

[imcheng, 2017-07-11 01:31:15]

lgtm

https://codereview.chromium.org/2974523002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc
(right):

https://codereview.chromium.org/2974523002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:92:
cast_socket_service_ = cast_channel::CastSocketService::GetInstance();
Can this be moved into the initializer block, and then make
|cast_socket_service_| pointer const?

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

https://codereview.chromium.org/2974523002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:489: void
CastChannelAPI::CastMessageHandler::RegisterExtensionId(
On 2017/07/10 20:43:05, mark a. foltz wrote:
> Right now there are only two extension ids that are whitelisted for the
> cast.channel API [1], and basically only one can be loaded at a time based on
> the --load-media-router-component-extension flag.
> 
> Unless anyone knows a case where two different extensions are using the API in
> the same profile, I would be okay simplifying this to allow one extension id
at
> a time.
> 
> Derek, WDYT?
> 
> [1]
>
https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur...
> 
> 

Changing to allow one extension at a time sounds fine.

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

https://codereview.chromium.org/2974523002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.h:75: //     Parameter #0
is a scoped pointer to the event payload.
Update the comments here, but also see mfoltz@'s other comment about extension
ids.

[zhaobin, 2017-07-12 21:58:08]

https://codereview.chromium.org/2974523002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc
(right):

https://codereview.chromium.org/2974523002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc:92:
cast_socket_service_ = cast_channel::CastSocketService::GetInstance();
On 2017/07/11 01:31:15, imcheng wrote:
> Can this be moved into the initializer block, and then make
> |cast_socket_service_| pointer const?

Done.

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

https://codereview.chromium.org/2974523002/diff/1/components/cast_channel/cas...
components/cast_channel/cast_socket.h:133: // Unregisters |observer|
On 2017/07/10 20:43:05, mark a. foltz wrote:
> Nit: Add period at end.

Done.

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

https://codereview.chromium.org/2974523002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:489: void
CastChannelAPI::CastMessageHandler::RegisterExtensionId(
On 2017/07/10 20:43:05, mark a. foltz wrote:
> Right now there are only two extension ids that are whitelisted for the
> cast.channel API [1], and basically only one can be loaded at a time based on
> the --load-media-router-component-extension flag.
> 
> Unless anyone knows a case where two different extensions are using the API in
> the same profile, I would be okay simplifying this to allow one extension id
at
> a time.
> 
> Derek, WDYT?
> 
> [1]
>
https://cs.chromium.org/chromium/src/extensions/common/api/_permission_featur...
> 
> 

Done.

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

https://codereview.chromium.org/2974523002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.h:75: //     Parameter #0
is a scoped pointer to the event payload.
On 2017/07/11 01:31:15, imcheng wrote:
> Update the comments here, but also see mfoltz@'s other comment about extension
> ids.

Done.

[zhaobin, 2017-07-12 21:58:24]

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

[commit-bot: I haz the power, 2017-07-12 21:58: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-07-13 00:34:56]

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

[commit-bot: I haz the power, 2017-07-13 00:34:56]

Dry run: This issue passed the CQ dry run.

[imcheng, 2017-07-13 22:51:59]

lgtm

[zhaobin, 2017-07-13 22:53:56]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-07-13 22:53: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/2974523002/#ps20001
(title: "resolve code review comments from Derek and Mark")

[commit-bot: I haz the power, 2017-07-13 22:54:17]

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-07-14 00:35:45]

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

[commit-bot: I haz the power, 2017-07-14 00:35:47]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zhaobin, 2017-07-14 17:54:59]

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

[commit-bot: I haz the power, 2017-07-14 17:55:13]

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-07-14 18:01:06]

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

[commit-bot: I haz the power, 2017-07-14 18:01:07]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-07-14 19:28:36]

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

[commit-bot: I haz the power, 2017-07-14 19:28:50]

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-07-14 20:49:37]

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

[commit-bot: I haz the power, 2017-07-14 20:49:38]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-07-14 20:59:08]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-07-14 20:59:09]

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/2974523002/#ps40001
(title: "merge with master")

[commit-bot: I haz the power, 2017-07-14 20:59:24]

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-07-14 21:06:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1500065948637680,
"parent_rev": "26a60c2908155fcba28a21b1065d21adbc39a1c6", "commit_rev":
"edc090649966c37e060d073addca425382dc4e38"}

[commit-bot: I haz the power, 2017-07-14 21:06:24]

Description was changed from

==========
[cast_channel] Make CastSocketService a global leaky singleton

- make CastSocketService a global leaky singleton and remove
CastSocketServiceFactory
- make CastChannelAPI and CastMediaSinkService own CastSocket::Observer
- fix unit tests and browser tests

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

to

==========
[cast_channel] Make CastSocketService a global leaky singleton

- make CastSocketService a global leaky singleton and remove
CastSocketServiceFactory
- make CastChannelAPI and CastMediaSinkService own CastSocket::Observer
- fix unit tests and browser tests

BUG=687377

Review-Url: https://codereview.chromium.org/2974523002
Cr-Commit-Position: refs/heads/master@{#486886}
Committed:
https://chromium.googlesource.com/chromium/src/+/edc090649966c37e060d073addca...
==========

[commit-bot: I haz the power, 2017-07-14 21:06:27]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/edc090649966c37e060d073addca...

[zhaobin, 2017-07-17 21:25:01]

Description was changed from

==========
[cast_channel] Make CastSocketService a global leaky singleton

- make CastSocketService a global leaky singleton and remove
CastSocketServiceFactory
- make CastChannelAPI and CastMediaSinkService own CastSocket::Observer
- fix unit tests and browser tests

BUG=687377

Review-Url: https://codereview.chromium.org/2974523002
Cr-Commit-Position: refs/heads/master@{#486886}
Committed:
https://chromium.googlesource.com/chromium/src/+/edc090649966c37e060d073addca...
==========

to

==========
[cast_channel] Make CastSocketService a global leaky singleton

- make CastSocketService a global leaky singleton and remove
CastSocketServiceFactory
- make CastChannelAPI and CastMediaSinkService own CastSocket::Observer
- fix unit tests and browser tests

BUG=687377,725717

Review-Url: https://codereview.chromium.org/2974523002
Cr-Commit-Position: refs/heads/master@{#486886}
Committed:
https://chromium.googlesource.com/chromium/src/+/edc090649966c37e060d073addca...
==========