[Cast,Android,Presentation API] Send messages back to the right clients, listen for messages sooner.

[Cast,Android,Presentation API] Send messages back to the right clients, listen for messages sooner.

CastRouteController now sends messages to the route id associated with the client id, mMediaRouteId was removed.
Also PresentationDispatcher subscribes to the PresentationConnection messages before passing the new connection to the page.

BUG=569601
TEST=manually run all previously passing Cast e2e tests with the newly passing auto-join test.

Committed: https://crrev.com/25472eab8bb06c04bb6cc22d0b259f0baba49699
Cr-Commit-Position: refs/heads/master@{#365635}

[whywhat, 2015-12-15 14:18:25]

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

[whywhat, 2015-12-15 14:18:25]

PTaL

[whywhat, 2015-12-15 14:18:40]

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

[commit-bot: I haz the power, 2015-12-15 14:18:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1522313003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1522313003/1

[commit-bot: I haz the power, 2015-12-15 15:23:42]

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

[commit-bot: I haz the power, 2015-12-15 15:23:42]

Dry run: This issue passed the CQ dry run.

[whywhat, 2015-12-15 20:39:29]

Restored saving the last removed route record

[whywhat, 2015-12-15 21:03:41]

PAGE_SCOPED always returns false.

[whywhat, 2015-12-15 21:14:12]

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

[whywhat, 2015-12-15 21:14:25]

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

[whywhat, 2015-12-15 21:14:26]

PTaL

[commit-bot: I haz the power, 2015-12-15 21:15:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1522313003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1522313003/40001

[commit-bot: I haz the power, 2015-12-15 22:36:00]

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

[commit-bot: I haz the power, 2015-12-15 22:36:01]

Dry run: This issue passed the CQ dry run.

[imcheng, 2015-12-15 23:06:39]

Description was changed from

==========
[Cast,Android,Presentation API] Send messages back to the right clients, listen
for messages sooner.

CastRouteController now sends messages to the route id associated with the
client id, mMediaRouteId was removed.
Also PresentationDispatcher subscribes to the PresentationConnection messages
before passing the new connection to the page.

BUG=569501
TEST=manually run all previously passing Cast e2e tests with the newly passing
auto-join test.
==========

to

==========
[Cast,Android,Presentation API] Send messages back to the right clients, listen
for messages sooner.

CastRouteController now sends messages to the route id associated with the
client id, mMediaRouteId was removed.
Also PresentationDispatcher subscribes to the PresentationConnection messages
before passing the new connection to the page.

BUG=569601
TEST=manually run all previously passing Cast e2e tests with the newly passing
auto-join test.
==========

[imcheng, 2015-12-15 23:42:32]

https://codereview.chromium.org/1522313003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/RouteDelegate.java
(right):

https://codereview.chromium.org/1522313003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/router/RouteDelegate.java:29:
void onSessionClosed();
This change and L43 sounds like something specific to Cast; do you really want
to introduce them to this generic interface? Or maybe you should create a
CastRouteDelegate that extends this interface?

https://codereview.chromium.org/1522313003/diff/40001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/1522313003/diff/40001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:349:
presentation_service_->ListenForSessionMessages(session_info.Clone());
This change is probably OK for desktop. As I mentioned in the bug, we can
consider moving this call into browser to avoid this IPC. But that can be done
separately.

[whywhat, 2015-12-16 12:31:20]

https://codereview.chromium.org/1522313003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/RouteDelegate.java
(right):

https://codereview.chromium.org/1522313003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/router/RouteDelegate.java:29:
void onSessionClosed();
On 2015/12/15 at 23:42:32, imcheng1 wrote:
> This change and L43 sounds like something specific to Cast; do you really want
to introduce them to this generic interface? Or maybe you should create a
CastRouteDelegate that extends this interface?

Yeah, I think extracting these interfaces was unnecessary as there's only one
implementation of each anyway :) E.g. CastMediaRouteController just uses the
CastRouteController directly.
Eliminating the interfaces is out of the scope of this fix though.

https://codereview.chromium.org/1522313003/diff/40001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/1522313003/diff/40001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:349:
presentation_service_->ListenForSessionMessages(session_info.Clone());
On 2015/12/15 at 23:42:32, imcheng1 wrote:
> This change is probably OK for desktop. As I mentioned in the bug, we can
consider moving this call into browser to avoid this IPC. But that can be done
separately.

It seems to pass all the tests at least :)

Do you expect PD to just have a OnPresentationConnectionMessage then, similar to
state change, and the service impl would register itself for connection
messages?

[mlamouri (slow - plz ping), 2015-12-16 14:53:04]

lgtm with comments addressed.

Sorry for the delay, I didn't realise this was urgent.

https://codereview.chromium.org/1522313003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastMediaRouteProvider.java
(right):

https://codereview.chromium.org/1522313003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastMediaRouteProvider.java:320:
mLastRemovedRouteRecord = client;
You might end up listing a `client` that doesn't match the route id? Shouldn't
you do an early bail out above:
```
if (!client.routeId.equals(routeId)) continue;
```

[imcheng, 2015-12-16 19:24:36]

lgtm

https://codereview.chromium.org/1522313003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/RouteDelegate.java
(right):

https://codereview.chromium.org/1522313003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/router/RouteDelegate.java:29:
void onSessionClosed();
On 2015/12/16 12:31:19, whywhat wrote:
> On 2015/12/15 at 23:42:32, imcheng1 wrote:
> > This change and L43 sounds like something specific to Cast; do you really
want
> to introduce them to this generic interface? Or maybe you should create a
> CastRouteDelegate that extends this interface?
> 
> Yeah, I think extracting these interfaces was unnecessary as there's only one
> implementation of each anyway :) E.g. CastMediaRouteController just uses the
> CastRouteController directly.
> Eliminating the interfaces is out of the scope of this fix though.

Acknowledged.

https://codereview.chromium.org/1522313003/diff/40001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/1522313003/diff/40001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:349:
presentation_service_->ListenForSessionMessages(session_info.Clone());
On 2015/12/16 12:31:20, whywhat wrote:
> On 2015/12/15 at 23:42:32, imcheng1 wrote:
> > This change is probably OK for desktop. As I mentioned in the bug, we can
> consider moving this call into browser to avoid this IPC. But that can be done
> separately.
> 
> It seems to pass all the tests at least :)
> 
> Do you expect PD to just have a OnPresentationConnectionMessage then, similar
to
> state change, and the service impl would register itself for connection
> messages?

Yeah, PresentationServiceImpl will call back into PresentationServiceDelegate to
listen for state changes before sending the connection back to
PresentationDispatcher. That will simplify the interaction between this class
and PresentationServiceImpl.

[whywhat, 2015-12-16 20:55:26]

Fixed some null pointer exceptions

[whywhat, 2015-12-16 20:58:57]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-12-16 20:58:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
imcheng@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1522313003/#ps80001
(title: "Fixed some null pointer exceptions")

[commit-bot: I haz the power, 2015-12-16 21:00:16]

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

[commit-bot: I haz the power, 2015-12-16 22:14:39]

Description was changed from

==========
[Cast,Android,Presentation API] Send messages back to the right clients, listen
for messages sooner.

CastRouteController now sends messages to the route id associated with the
client id, mMediaRouteId was removed.
Also PresentationDispatcher subscribes to the PresentationConnection messages
before passing the new connection to the page.

BUG=569601
TEST=manually run all previously passing Cast e2e tests with the newly passing
auto-join test.
==========

to

==========
[Cast,Android,Presentation API] Send messages back to the right clients, listen
for messages sooner.

CastRouteController now sends messages to the route id associated with the
client id, mMediaRouteId was removed.
Also PresentationDispatcher subscribes to the PresentationConnection messages
before passing the new connection to the page.

BUG=569601
TEST=manually run all previously passing Cast e2e tests with the newly passing
auto-join test.
==========

[commit-bot: I haz the power, 2015-12-16 22:14:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-16 22:15:46]

Description was changed from

==========
[Cast,Android,Presentation API] Send messages back to the right clients, listen
for messages sooner.

CastRouteController now sends messages to the route id associated with the
client id, mMediaRouteId was removed.
Also PresentationDispatcher subscribes to the PresentationConnection messages
before passing the new connection to the page.

BUG=569601
TEST=manually run all previously passing Cast e2e tests with the newly passing
auto-join test.
==========

to

==========
[Cast,Android,Presentation API] Send messages back to the right clients, listen
for messages sooner.

CastRouteController now sends messages to the route id associated with the
client id, mMediaRouteId was removed.
Also PresentationDispatcher subscribes to the PresentationConnection messages
before passing the new connection to the page.

BUG=569601
TEST=manually run all previously passing Cast e2e tests with the newly passing
auto-join test.

Committed: https://crrev.com/25472eab8bb06c04bb6cc22d0b259f0baba49699
Cr-Commit-Position: refs/heads/master@{#365635}
==========

[commit-bot: I haz the power, 2015-12-16 22:15:47]

Patchset 5 (id:??) landed as
https://crrev.com/25472eab8bb06c04bb6cc22d0b259f0baba49699
Cr-Commit-Position: refs/heads/master@{#365635}