[Media Router] Connection state change listening redesign part 2.

[Media Router] Connection state change listening redesign part 2.

- PresentationDispatcher no longer calls ListenForSessionStateChange
on PresentationService startup.
- When a PresentationConnection is successfully created, PSImpl will
call back to PSDImpl to start listening for state changes for that
connection. This required adding a PresentationSessionInfo (to be
renamed soon) param to the PSD's ListenForConnectionStateChange API.
- PSDImpl no longer uses PresentationSessionStateObserver. Instead it
uses PresentationConnectionStateObserver, which listens on a
per-connection basis and doesn't use a MediaRoutesObserver.

Note that this change is not backwards compatible. It is expected this
will break state change for users with older MR extensions. It will
also break Android since state change isn't implemented there yet.
Given that the PresentationConnection states spec is still in progress,
it's probably not an unreasonable thing to do.

Also did some best-effort renaming. (Renaming patch soon)

BUG=529893
Committed: https://crrev.com/f3e5a01a75bbdd97e5d218c4fb5154de28745a4e
Cr-Commit-Position: refs/heads/master@{#360740}

[imcheng, 2015-11-11 00:47:47]

Description was changed from

==========
[Media Router] Connection state change listening redesign part 2.

- PresentationDispatcher no longer calls ListenForSessionStateChange
on PresentationService startup.
- When a PresentationConnection is successfully created, PSImpl will
call back to PSDImpl to start listening for state changes for that
connection. This required adding a PresentationSessionInfo (to be
renamed soon) param to the PSD's ListenForConnectionStateChange API.
- PSDImpl no longer uses PresentationSessionStateObserver. Instead it
uses PresentationConnectionStateObserver, which listens on a
per-connection basis and doesn't use a MediaRoutesObserver.

Note that this change is not backwards compatible. It is expected this
will break state change for users with older MR extensions. It will
also break Android since state change isn't implemented there yet.
Given that the PresentationConnection states spec is still in progress,
it's probably not an unreasonable thing to do.

BUG=529893
==========

to

==========
[Media Router] Connection state change listening redesign part 2.

- PresentationDispatcher no longer calls ListenForSessionStateChange
on PresentationService startup.
- When a PresentationConnection is successfully created, PSImpl will
call back to PSDImpl to start listening for state changes for that
connection. This required adding a PresentationSessionInfo (to be
renamed soon) param to the PSD's ListenForConnectionStateChange API.
- PSDImpl no longer uses PresentationSessionStateObserver. Instead it
uses PresentationConnectionStateObserver, which listens on a
per-connection basis and doesn't use a MediaRoutesObserver.

Note that this change is not backwards compatible. It is expected this
will break state change for users with older MR extensions. It will
also break Android since state change isn't implemented there yet.
Given that the PresentationConnection states spec is still in progress,
it's probably not an unreasonable thing to do.

Also did some best-effort renaming. (Renaming patch soon)

BUG=529893
==========

[imcheng, 2015-11-11 01:06:34]

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

[imcheng, 2015-11-11 01:06:35]

Mark: PTAL. I will look into adding some integration tests in this patch or in a
separate patch.

[mark a. foltz, 2015-11-12 03:44:37]

Looking now.  Since Android is launching 100% with M48 this may not be the best
time to submit a breaking change for them.

[whywhat, 2015-11-12 03:50:26]

avayvod@chromium.org changed reviewers:
+ avayvod@chromium.org

[whywhat, 2015-11-12 03:50:26]

Does Android need to implement something special for state change? I thought
that state change is done purely by MR based on route updates?

[mark a. foltz, 2015-11-12 04:19:37]

mfoltz@chromium.org changed reviewers:
- avayvod@chromium.org

[mark a. foltz, 2015-11-12 04:19:38]

I don't think this will affect Android.  Presentation state observers are not
implemented in the Android media router.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...

Minor comments IL

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:126: class
PresentationConnectionStateChangeHandler {
How about PresentatonConnectionStateChangeCallback and name the mock method Run?

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_connection_state_observer.cc
(right):

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_connection_state_observer.cc:34:
state_changed_cb_.Run(connection_, state);
All this observer object is doing is wrapping a base::Callback.  What is the
advantage over adding an API to the media router,

router->RegisterPresentationConnectionStateCallback(route_id, connection,
state_changed_cb)

?

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_connection_state_observer.h
(right):

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_connection_state_observer.h:16: //
Observes state changes on a PresentationConnection and returns the new state
Is the caller expected to find out the current state of the
PresentationConnection via some other means - then add the observer?

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_connection_state_observer.h:17: // to
the supplied PresentationConnectionStateChangedCallback.
Clarify that this callback is run multiple times - once per state change.

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:287:
base::Unretained(this)),
It's a little weird to bind callbacks to the test itself.  The pattern I would
assume would be to create a separate mock object representing the client of the
API and bind the callbacks to it.  But I don't feel strongly about it.

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:316:
_, content::PRESENTATION_CONNECTION_STATE_TERMINATED))
Strictly speaking, CLOSED is a terminal state for the PresentationConnection so
you wouldn't see a transition from CLOSED to TERMINATED in real usage.

[imcheng, 2015-11-16 23:52:01]

PTAL. I changed the API to take the callback as you suggested, and refactored
some logic into a MediaRouterBase class. I think the base class will be useful
for other common logic as well.

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:126: class
PresentationConnectionStateChangeHandler {
On 2015/11/12 04:19:38, mark a. foltz wrote:
> How about PresentatonConnectionStateChangeCallback and name the mock method
Run?

Done.

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_connection_state_observer.cc
(right):

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_connection_state_observer.cc:34:
state_changed_cb_.Run(connection_, state);
On 2015/11/12 04:19:38, mark a. foltz wrote:
> All this observer object is doing is wrapping a base::Callback.  What is the
> advantage over adding an API to the media router,
> 
> router->RegisterPresentationConnectionStateCallback(route_id, connection,
> state_changed_cb)
> 
> ?

If we pass in a callback, the implementation will use base::CallbackList and
Subscription objects. It will be different from the rest of the APIs in that
there won't a Unregister method, but it might be cleaner. I will give the
callback based API a try.

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_connection_state_observer.h
(right):

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_connection_state_observer.h:16: //
Observes state changes on a PresentationConnection and returns the new state
On 2015/11/12 04:19:38, mark a. foltz wrote:
> Is the caller expected to find out the current state of the
> PresentationConnection via some other means - then add the observer?

For our current use case, it is not needed. We start listening when the route is
created, and it is assumed to be in the initial state (connected).

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_connection_state_observer.h:17: // to
the supplied PresentationConnectionStateChangedCallback.
On 2015/11/12 04:19:38, mark a. foltz wrote:
> Clarify that this callback is run multiple times - once per state change.

Done in media_router.h.

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:287:
base::Unretained(this)),
On 2015/11/12 04:19:38, mark a. foltz wrote:
> It's a little weird to bind callbacks to the test itself.  The pattern I would
> assume would be to create a separate mock object representing the client of
the
> API and bind the callbacks to it.  But I don't feel strongly about it.

Done. I refactored the mock object into test_helper.{cc,h}.

https://codereview.chromium.org/1430413003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:316:
_, content::PRESENTATION_CONNECTION_STATE_TERMINATED))
On 2015/11/12 04:19:38, mark a. foltz wrote:
> Strictly speaking, CLOSED is a terminal state for the PresentationConnection
so
> you wouldn't see a transition from CLOSED to TERMINATED in real usage.

Ack. That part has been moved to media_router_mojo_impl_unittest.cc.

[mark a. foltz, 2015-11-18 00:39:39]

LGTM % comments

Also, please document ownership of the callbacks in MediaRouterBase (i.e. that
they are expected to be owned by the caller).

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:785:
ProcessEventLoop();
Is there test coverage for the removal callback?

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.cc:194: void
PresentationFrame::ListenForConnectionStateChange(
Can this be called more than once per |connection|?
Should the callback be appended to the existing subscription or replace the
existing callback?

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.cc:205:
connection_state_subscriptions_.push_back(
It seems like we only want to add at most one Subscription per route_id per
frame.
How would that be enforced?

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:302:
content::PresentationConnectionStateChangedCallback callback =
Consider adding a method to MPCSCC to get the bound callback, i.e.
mock_callback.GetBoundCallback();

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:307:
EXPECT_CALL(router_, OnAddPresentationConnectionStateChangedCallbackInvoked(
This is a little confusing, is there a way to set an expectation on
router_.AddPresentationConnectionStateChangedCallback directly?

[imcheng, 2015-11-19 23:39:53]

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/media_router_mojo_impl_unittest.cc (right):

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/media_router_mojo_impl_unittest.cc:785:
ProcessEventLoop();
On 2015/11/18 00:39:38, mark a. foltz wrote:
> Is there test coverage for the removal callback?

Added assertion that the CallbackList entry is removed after Subscription object
is destroyed.

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.cc:194: void
PresentationFrame::ListenForConnectionStateChange(
On 2015/11/18 00:39:38, mark a. foltz wrote:
> Can this be called more than once per |connection|?
> Should the callback be appended to the existing subscription or replace the
> existing callback?

Yes. Although it should only be called once by PresentationServiceImpl. See
below commen tas well.

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.cc:205:
connection_state_subscriptions_.push_back(
On 2015/11/18 00:39:38, mark a. foltz wrote:
> It seems like we only want to add at most one Subscription per route_id per
> frame.
> How would that be enforced?

We could do that, by keeping a map from route id to the Subscription object.

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:302:
content::PresentationConnectionStateChangedCallback callback =
On 2015/11/18 00:39:38, mark a. foltz wrote:
> Consider adding a method to MPCSCC to get the bound callback, i.e.
> mock_callback.GetBoundCallback();

Since it's only used in one place, I would like to keep it as is for now.

https://codereview.chromium.org/1430413003/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:307:
EXPECT_CALL(router_, OnAddPresentationConnectionStateChangedCallbackInvoked(
On 2015/11/18 00:39:38, mark a. foltz wrote:
> This is a little confusing, is there a way to set an expectation on
> router_.AddPresentationConnectionStateChangedCallback directly?

Not possible at the moment, because the function has a move-only return type.

[imcheng, 2015-11-20 01:53:43]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2015-11-20 01:53:44]

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/1430413003/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-11-20 01:56:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1430413003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1430413003/200001

[commit-bot: I haz the power, 2015-11-20 04:08:45]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-11-20 04:09:45]

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