[MediaRouter] Renames CloseRoute() to Terminate() and creates DetachRoute()

+isherman for histograms.xml
+jdufault for chrome/browser/ui/ash

Renames CloseRoute() to Terminate() and creates DetachRoute().

This clarifies the role of the two methods invoked on PresentationConnection by plumbing the renaming through the various layers.

PresentationConnection.close() -> CloseConnection (PresentationService) -> DetachRoute (MediaRouter)

PresentationConnection.terminate() -> Terminate -> TerminateRoute

It also removes the mapping for closed and terminated presentations from the PresentationFrame to prevent duplicate invocations of DetachRoute.

BUG=540803
Committed: https://crrev.com/d0c3de1a60bd68b88ca020f2a04f249a945f2cc8
Cr-Commit-Position: refs/heads/master@{#364908}

[mark a. foltz, 2015-12-10 00:49:17]

Description was changed from

==========
Merge CL 1482003002

BUG=540803
==========

to

==========
+isherman for histograms.xml
+jdufault for chrome/browser/ui/ash

Renames CloseRoute() to Terminate() and creates DetachRoute().

This clarifies the role of the two methods invoked on PresentationConnection by
plumbing the renaming through the various layers.

PresentationConnection.close() -> CloseConnection (PresentationService) ->
DetachRoute (MediaRouter)

PresentationConnection.terminate() -> Terminate -> TerminateRoute

It also removes the mapping for closed and terminated presentations from the
PresentationFrame to prevent duplicate invocations of DetachRoute.

BUG=540803
==========

[mark a. foltz, 2015-12-10 00:49:17]

mfoltz@chromium.org changed reviewers:
+ imcheng@chromium.org, isherman@chromium.org, mlamouri@chromium.org

[mark a. foltz, 2015-12-10 00:54:47]

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

[mlamouri (slow - plz ping), 2015-12-10 15:39:51]

lgtm. Thanks for taking over!

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/an...
File chrome/browser/media/android/router/media_router_android.cc (right):

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_android.cc:162:
NOTIMPLEMENTED();
Could you add a TODO for avayvod@ and mlamouri@ linked to bug 540803?

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:146: // Termintes the route
specified by |route_id|.
nit: "Terminates"

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_metrics.h (right):

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_metrics.h:32: DETACH_ROUTE = 5,
open question: would there be a benefit in trying to differentiate detached
route because of close() and detached route because of GC? (note: Blink keeps
track of calls to close() so that might not be a big deal)

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:535: if
(presentation_frame)
Is `presentation_frame == nullptr` even possible? Should that be a DCHECK
instead?

[Ilya Sherman, 2015-12-10 16:23:53]

histograms.xml lgtm

[jdufault, 2015-12-10 18:58:13]

chrome/browser/ui/ash lgtm

[imcheng, 2015-12-10 19:50:46]

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/an...
File chrome/browser/media/android/router/media_router_android.h (right):

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_android.h:54: void
OnPresentationSessionDetached(const MediaRoute::Id& route_id) override;
Remove this function

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:103: // Terminated the media route
specified by |route_id|.
s/Terminated/Terminates

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:106: // Detach the media route
specified by |route_id|. The request might come from
s/Detach/Detaches

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:201: // We no
longer need to observe route messages.
nit: new line above comment

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:209: // We
keep the PresentationConnectionStateChangedCallback registered with MR
nit: new line above comment

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:211:
router_->DetachRoute(route_id);
Looks like you are already calling DetachRoute in PSDImpl::CloseConnection(), so
no need to call it again here?

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:780:
frame_manager_->RemoveConnection(rfh_id, presentation_id, route_id);
So we are calling TerminateRoute, and then DetachRoute in
PresentationFrame::RemoveConnection()? Do we only need to call TerminateRoute?

https://codereview.chromium.org/1507743005/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/1507743005/diff/40001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:504:
MediaRouteProvider.prototype.closeRoute = function(routeId) {
s/closeRoute/terminateRoute

https://codereview.chromium.org/1507743005/diff/40001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:573:
MediaRouteProvider.prototype.onPresentationSessionDetached = function(
s/onPresentationSessionDetached/detachRoute

[mark a. foltz, 2015-12-10 23:46:49]

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/an...
File chrome/browser/media/android/router/media_router_android.cc (right):

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_android.cc:162:
NOTIMPLEMENTED();
On 2015/12/10 at 15:39:51, Mounir Lamouri wrote:
> Could you add a TODO for avayvod@ and mlamouri@ linked to bug 540803?

Done.

I think I got the semantics switched here. CloseRoute actually should become
Terminate and DetachRoute is NOT_IMPLEMENTED based on my reading of the Java
code.

Let me know if I'm still wrong.

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:103: // Terminated the media route
specified by |route_id|.
On 2015/12/10 at 19:50:46, imcheng1 wrote:
> s/Terminated/Terminates

Done

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:106: // Detach the media route
specified by |route_id|. The request might come from
On 2015/12/10 at 19:50:46, imcheng1 wrote:
> s/Detach/Detaches

Done

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router.mojom (right):

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:146: // Termintes the route
specified by |route_id|.
On 2015/12/10 at 15:39:51, Mounir Lamouri wrote:
> nit: "Terminates"

Done

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_metrics.h (right):

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_metrics.h:32: DETACH_ROUTE = 5,
On 2015/12/10 at 15:39:51, Mounir Lamouri wrote:
> open question: would there be a benefit in trying to differentiate detached
route because of close() and detached route because of GC? (note: Blink keeps
track of calls to close() so that might not be a big deal)

I think for now we should be able to get a good estimate based on the difference
between this histogram and the usecounter.  If we want to be precise we could
plumb through a DetachReason from Blink.

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:201: // We no
longer need to observe route messages.
On 2015/12/10 at 19:50:46, imcheng1 wrote:
> nit: new line above comment

Done

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:209: // We
keep the PresentationConnectionStateChangedCallback registered with MR
On 2015/12/10 at 19:50:46, imcheng1 wrote:
> nit: new line above comment

Done

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:211:
router_->DetachRoute(route_id);
On 2015/12/10 at 19:50:46, imcheng1 wrote:
> Looks like you are already calling DetachRoute in PSDImpl::CloseConnection(),
so no need to call it again here?

You're right, good catch.

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:535: if
(presentation_frame)
On 2015/12/10 at 15:39:51, Mounir Lamouri wrote:
> Is `presentation_frame == nullptr` even possible? Should that be a DCHECK
instead?

My current understanding is a frame can call connection.close() or .terminate()
multiple times on the same connection, until the connection state change update
comes from content/ and Blink changes the connection state to disconnected or
terminated.

There's a TODO elsewhere to have Blink make the state change for
connection.close(), and have the Mojo call be fire and forget to content/ to
notify the MRP.   For terminate() I think we want the event to be fired when the
actual side effect takes place (so we'll wait for the callback from the MRP). 
At least, that's my thinking based on a conversation I had with imcheng@
yesterday.

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:780:
frame_manager_->RemoveConnection(rfh_id, presentation_id, route_id);
On 2015/12/10 at 19:50:46, imcheng1 wrote:
> So we are calling TerminateRoute, and then DetachRoute in
PresentationFrame::RemoveConnection()? Do we only need to call TerminateRoute?

Yeah removed call to router_->DetachRoute() from RemoveConnection.

https://codereview.chromium.org/1507743005/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/media_router_bindings.js (right):

https://codereview.chromium.org/1507743005/diff/40001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:504:
MediaRouteProvider.prototype.closeRoute = function(routeId) {
On 2015/12/10 at 19:50:46, imcheng1 wrote:
> s/closeRoute/terminateRoute

Done

https://codereview.chromium.org/1507743005/diff/40001/extensions/renderer/res...
extensions/renderer/resources/media_router_bindings.js:573:
MediaRouteProvider.prototype.onPresentationSessionDetached = function(
On 2015/12/10 at 19:50:46, imcheng1 wrote:
> s/onPresentationSessionDetached/detachRoute

Done

[imcheng, 2015-12-11 00:15:16]

lgtm + 1 comment

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/an...
File chrome/browser/media/android/router/media_router_android.cc (right):

https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_android.cc:162:
NOTIMPLEMENTED();
On 2015/12/10 23:46:48, mark a. foltz wrote:
> On 2015/12/10 at 15:39:51, Mounir Lamouri wrote:
> > Could you add a TODO for avayvod@ and mlamouri@ linked to bug 540803?
> 
> Done.
> 
> I think I got the semantics switched here. CloseRoute actually should become
> Terminate and DetachRoute is NOT_IMPLEMENTED based on my reading of the Java
> code.
> 
> Let me know if I'm still wrong.

I thought it already has detachRoute. The logic in OnPresentationSessionDetached
should be moved to DetachRoute, and then OnPresentationSessionDetached should be
removed from here and .h file.

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

The CQ bit was checked by mfoltz@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
jdufault@chromium.org, isherman@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1507743005/#ps60001
(title: "Respond to imcheng@ and mlamouri@ comments")

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

The CQ bit was unchecked by mfoltz@chromium.org

[mark a. foltz, 2015-12-11 06:14:48]

On 2015/12/11 at 00:15:16, imcheng wrote:
> lgtm + 1 comment
> 
>
https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/an...
> File chrome/browser/media/android/router/media_router_android.cc (right):
> 
>
https://codereview.chromium.org/1507743005/diff/40001/chrome/browser/media/an...
> chrome/browser/media/android/router/media_router_android.cc:162:
NOTIMPLEMENTED();
> On 2015/12/10 23:46:48, mark a. foltz wrote:
> > On 2015/12/10 at 15:39:51, Mounir Lamouri wrote:
> > > Could you add a TODO for avayvod@ and mlamouri@ linked to bug 540803?
> > 
> > Done.
> > 
> > I think I got the semantics switched here. CloseRoute actually should become
> > Terminate and DetachRoute is NOT_IMPLEMENTED based on my reading of the Java
> > code.
> > 
> > Let me know if I'm still wrong.
> 
> I thought it already has detachRoute. The logic in
OnPresentationSessionDetached should be moved to DetachRoute, and then
OnPresentationSessionDetached should be removed from here and .h file.

Done.

[mark a. foltz, 2015-12-11 06:17:50]

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

[mark a. foltz, 2015-12-11 06:17:51]

+stevenjb for chrome/browser/ui/ash/OWNERS approval

[mark a. foltz, 2015-12-11 06:18:01]

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

[commit-bot: I haz the power, 2015-12-11 06:18:17]

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

[commit-bot: I haz the power, 2015-12-11 06:39:46]

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

[commit-bot: I haz the power, 2015-12-11 06:39:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[stevenjb, 2015-12-11 22:46:05]

c/b/ui lgtm

[mark a. foltz, 2015-12-12 00:26:48]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2015-12-12 00:26:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
jdufault@chromium.org, imcheng@chromium.org, isherman@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1507743005/#ps140001
(title: "Fix e2e_browsertest")

[commit-bot: I haz the power, 2015-12-12 00:27:29]

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

[commit-bot: I haz the power, 2015-12-12 02:09:05]

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

[commit-bot: I haz the power, 2015-12-12 02:09:06]

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

[mark a. foltz, 2015-12-12 02:34:28]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2015-12-12 02:34:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
jdufault@chromium.org, imcheng@chromium.org, isherman@chromium.org,
stevenjb@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1507743005/#ps160001
(title: "Fix android build")

[commit-bot: I haz the power, 2015-12-12 02:38:51]

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

[commit-bot: I haz the power, 2015-12-12 04:43:16]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-12-12 04:44:00]

Description was changed from

==========
+isherman for histograms.xml
+jdufault for chrome/browser/ui/ash

Renames CloseRoute() to Terminate() and creates DetachRoute().

This clarifies the role of the two methods invoked on PresentationConnection by
plumbing the renaming through the various layers.

PresentationConnection.close() -> CloseConnection (PresentationService) ->
DetachRoute (MediaRouter)

PresentationConnection.terminate() -> Terminate -> TerminateRoute

It also removes the mapping for closed and terminated presentations from the
PresentationFrame to prevent duplicate invocations of DetachRoute.

BUG=540803
==========

to

==========
+isherman for histograms.xml
+jdufault for chrome/browser/ui/ash

Renames CloseRoute() to Terminate() and creates DetachRoute().

This clarifies the role of the two methods invoked on PresentationConnection by
plumbing the renaming through the various layers.

PresentationConnection.close() -> CloseConnection (PresentationService) ->
DetachRoute (MediaRouter)

PresentationConnection.terminate() -> Terminate -> TerminateRoute

It also removes the mapping for closed and terminated presentations from the
PresentationFrame to prevent duplicate invocations of DetachRoute.

BUG=540803

Committed: https://crrev.com/d0c3de1a60bd68b88ca020f2a04f249a945f2cc8
Cr-Commit-Position: refs/heads/master@{#364908}
==========

[commit-bot: I haz the power, 2015-12-12 04:44:03]

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