[Media Router] Add a presentation id to MediaRoute mapping in the MR

[Media Router] Add a presentation id to MediaRoute mapping in the MR

Address comments from https://codereview.chromium.org/2714783002/

"High level sketch:

- The mapping could be kept in InternalMediaRoutesObserver inside
  MediaRouterBase
- Either keep a std::map<> inside, or add presentation ID to MediaRoute and
  search the list kept by the observer
- Watch out for special IDs passed by Cast"

BUG=688233

[zhaobin, 2017-03-03 22:58:38]

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

[mark a. foltz, 2017-03-06 21:02:20]

This works, but creates a duplicate id -> route mapping in
PresentationServiceDelegate and MediaRouterBase.

Have you discussed with Derek the pros and cons of adding an optional
presentation_id to MediaRoute itself?

[imcheng, 2017-03-07 00:58:19]

I am not opposed to adding the presentation ID field onto MediaRoute itself. But
we would still have to maintain a data structure in PresentationFrame to keep
track of MediaRoutes that are only in the associated RFH. Also to support quick
lookups we'd still need a map indexed by Presentation IDs. So I don't think we
will get much savings either way.

[imcheng, 2017-03-08 20:06:35]

Looks good overall. Let's investigate whether we can eliminate the mapping from
PresentationServiceDelegate separately.

https://codereview.chromium.org/2731043002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/2731043002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:192: // Register |route| associated
with |presentation_id| with this MediaRouter.
Registers

https://codereview.chromium.org/2731043002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:196: // Unregister media route
associated with |presentation_id| with this
Unregisters

https://codereview.chromium.org/2731043002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:201: // Returns underlying media
route associated with |presentation_id|. Returns
s/underlying/the
s/media route/MediaRoute

https://codereview.chromium.org/2731043002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router_base.cc (right):

https://codereview.chromium.org/2731043002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_base.cc:49:
presentation_id_route.erase(presentation_id);
If a MediaRoute was removed by the MRP, how is the associated entry removed from
the map?

[mark a. foltz, 2017-03-08 22:01:48]

https://codereview.chromium.org/2731043002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router_base.cc (right):

https://codereview.chromium.org/2731043002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_base.cc:49:
presentation_id_route.erase(presentation_id);
On 2017/03/08 at 20:06:35, imcheng wrote:
> If a MediaRoute was removed by the MRP, how is the associated entry removed
from the map?

One solution would be for each update sent to the internal routes observer would
check if a newly-removed route was in the map, and remove it.

If we went with keeping the id in the route, then you would only need to have
add the Get() method to iterate through the routes in the internal observer and
find the matching route.

[zhaobin, 2017-03-09 00:34:27]

Added an optional presentation_id to MediaRoute.

https://codereview.chromium.org/2731043002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router.h (right):

https://codereview.chromium.org/2731043002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:192: // Register |route| associated
with |presentation_id| with this MediaRouter.
On 2017/03/08 20:06:35, imcheng wrote:
> Registers

Code removed.

https://codereview.chromium.org/2731043002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:196: // Unregister media route
associated with |presentation_id| with this
On 2017/03/08 20:06:35, imcheng wrote:
> Unregisters

Code removed.

https://codereview.chromium.org/2731043002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router.h:201: // Returns underlying media
route associated with |presentation_id|. Returns
On 2017/03/08 20:06:35, imcheng wrote:
> s/underlying/the
> s/media route/MediaRoute

Done.

https://codereview.chromium.org/2731043002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/media_router_base.cc (right):

https://codereview.chromium.org/2731043002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/media_router_base.cc:49:
presentation_id_route.erase(presentation_id);
On 2017/03/08 22:01:47, mark a. foltz wrote:
> On 2017/03/08 at 20:06:35, imcheng wrote:
> > If a MediaRoute was removed by the MRP, how is the associated entry removed
> from the map?
> 
> One solution would be for each update sent to the internal routes observer
would
> check if a newly-removed route was in the map, and remove it.
> 
> If we went with keeping the id in the route, then you would only need to have
> add the Get() method to iterate through the routes in the internal observer
and
> find the matching route.

Done.

[mark a. foltz, 2017-03-10 00:56:28]

LGTM and seems simpler.

https://codereview.chromium.org/2731043002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_base.cc (right):

https://codereview.chromium.org/2731043002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_base.cc:44: if
(route.presentation_id().has_value() &&
I think .has_value() is optional here.

https://codereview.chromium.org/2731043002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_base.cc:45: (*route.presentation_id())
== presentation_id) {
In other code I have used .value() explicitly to retrieve the value (so I don't
have to remember what * does) but I don't feel strongly either way.

[zhaobin, 2017-03-10 01:50:40]

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

[commit-bot: I haz the power, 2017-03-10 01:51:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[zhaobin, 2017-03-10 01:51:11]

https://codereview.chromium.org/2731043002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_base.cc (right):

https://codereview.chromium.org/2731043002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_base.cc:44: if
(route.presentation_id().has_value() &&
On 2017/03/10 00:56:28, mark a. foltz wrote:
> I think .has_value() is optional here.

Done.

https://codereview.chromium.org/2731043002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_base.cc:45: (*route.presentation_id())
== presentation_id) {
On 2017/03/10 00:56:28, mark a. foltz wrote:
> In other code I have used .value() explicitly to retrieve the value (so I
don't
> have to remember what * does) but I don't feel strongly either way.

Done.

[commit-bot: I haz the power, 2017-03-10 02:36:13]

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

[commit-bot: I haz the power, 2017-03-10 02:36:14]

Dry run: This issue passed the CQ dry run.

[imcheng, 2017-03-10 23:53:45]

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

https://codereview.chromium.org/2731043002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/route_request_result.cc:39:
route_->set_presentation_id(presentation_id);
Two comments:

1) It isn't clear to me how this MediaRoute with presentation ID will end up in
MediaRouterBase / InternalMediaRoutesObserver -- it seems they need to be
informed when a route is created.
2) The extension itself does not pass back presentation IDs in MediaRoutes. We
need to ensure that the presentation ID is preserved when OnRoutesUpdated
happens.

[zhaobin, 2017-03-11 03:42:31]

On 2017/03/10 23:53:45, imcheng wrote:
>
https://codereview.chromium.org/2731043002/diff/40001/chrome/browser/media/ro...
> File chrome/browser/media/router/route_request_result.cc (right):
> 
>
https://codereview.chromium.org/2731043002/diff/40001/chrome/browser/media/ro...
> chrome/browser/media/router/route_request_result.cc:39:
> route_->set_presentation_id(presentation_id);
> Two comments:
> 
> 1) It isn't clear to me how this MediaRoute with presentation ID will end up
in
> MediaRouterBase / InternalMediaRoutesObserver -- it seems they need to be
> informed when a route is created.
> 2) The extension itself does not pass back presentation IDs in MediaRoutes. We
> need to ensure that the presentation ID is preserved when OnRoutesUpdated
> happens.

1) Done.
2) Done.

It looks a little complicated now. Shall we revert to std::map or add
presentation_id to route in extension or any other suggestions?

[mark a. foltz, 2017-03-13 17:52:11]

https://codereview.chromium.org/2731043002/diff/60001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_base.cc (right):

https://codereview.chromium.org/2731043002/diff/60001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_base.cc:72: void
SetPresentationId(const MediaRoute::Id& route_id,
Can the presentation ID just be set on the Route in
RouteRequestResult::FromSuccess?

[mark a. foltz, 2017-03-13 18:26:45]

On 2017/03/13 at 17:52:11, mark a. foltz wrote:
>
https://codereview.chromium.org/2731043002/diff/60001/chrome/browser/media/ro...
> File chrome/browser/media/router/media_router_base.cc (right):
> 
>
https://codereview.chromium.org/2731043002/diff/60001/chrome/browser/media/ro...
> chrome/browser/media/router/media_router_base.cc:72: void
SetPresentationId(const MediaRoute::Id& route_id,
> Can the presentation ID just be set on the Route in
RouteRequestResult::FromSuccess?

Hm, that ID may not be passed with CreateRoute() at the time the offscreen tab
mirroring route is created.

[mark a. foltz, 2017-03-13 18:38:50]

On 2017/03/13 at 18:26:45, mark a. foltz wrote:
> On 2017/03/13 at 17:52:11, mark a. foltz wrote:
> >
https://codereview.chromium.org/2731043002/diff/60001/chrome/browser/media/ro...
> > File chrome/browser/media/router/media_router_base.cc (right):
> > 
> >
https://codereview.chromium.org/2731043002/diff/60001/chrome/browser/media/ro...
> > chrome/browser/media/router/media_router_base.cc:72: void
SetPresentationId(const MediaRoute::Id& route_id,
> > Can the presentation ID just be set on the Route in
RouteRequestResult::FromSuccess?
> 
> Hm, that ID may not be passed with CreateRoute() at the time the offscreen tab
mirroring route is created.

Looking at the current patch, it may be that the original patch that kept the
mapping in the OffscreenPresentationManager is better.

Another two options would be:

- Change the implementation to always set offscreen presentation id onto the
offscreen tab mirroring route.  A little confusing because the route source and
ID don't necessarily match, but might be okay.

- As far as I can tell, the only reason that we need to is so that
PresentationServiceDelegateImpl::PresentationFrame::Reset() can call
detachRoute() on the MR when the frame goes away.  If we don't need to do that
for 1-UA mode, then we can possibly avoid this altogether.

[imcheng, 2017-03-15 00:48:57]

Unless the extension also passes back the presentation ID for each route,
maintaining the mapping in MediaRouter will be a tricky but should be
manageable; when a successful route response is received, the MediaRouter impl
can signal to the InternalMediaRoutesObserver to add the [presentation_id,
route] pair.  (Presentation ID is always available as it's generated by MR)

If we are adding presentation_id field to MediaRoute, then we also need to make
sure the presentation ID isn't wiped out by a OnRoutesUpdated. So it needs to
take the existing route list and "transfer" the presentation IDs to the updated
route list. So MediaRouterMojoImpl::OnRoutesUpdated should do something like
this:

1) Get the existing route list from InternalsMediaRouteObserver
2) Backfills presentation IDs into updated list
3) Notifies observer with augmented list (which includes
InternalMediaRoutesObserver)

That said, having gone through this exercise, I also feel adding the mapping in
OPM is probably the simplest. If we do need to support it generally (i.e.
presentation_id field in MediaRoute) then we should look into having the
extension provide it in the MediaRoute struct rather than having the browser
backfill it.

[mark a. foltz, 2017-03-16 23:09:11]

Bin, sorry for taking you on a wild goose chase.  I think we learned something
in the process though :)

On 2017/03/15 at 00:48:57, imcheng wrote:
> Unless the extension also passes back the presentation ID for each route,
maintaining the mapping in MediaRouter will be a tricky but should be
manageable; when a successful route response is received, the MediaRouter impl
can signal to the InternalMediaRoutesObserver to add the [presentation_id,
route] pair.  (Presentation ID is always available as it's generated by MR)
> 
> If we are adding presentation_id field to MediaRoute, then we also need to
make sure the presentation ID isn't wiped out by a OnRoutesUpdated. So it needs
to take the existing route list and "transfer" the presentation IDs to the
updated route list. So MediaRouterMojoImpl::OnRoutesUpdated should do something
like this:
> 
> 1) Get the existing route list from InternalsMediaRouteObserver
> 2) Backfills presentation IDs into updated list
> 3) Notifies observer with augmented list (which includes
InternalMediaRoutesObserver)

Yeah.  I think of the ids as bookkeeping for the Presentation API and not
necessary to expose to lower layers, it was a design mistake to plumb them
through the MR.  Someday we can move reconnect() entirely in-browser for both
1-UA and 2-UA and remove the need to pass presentation IDs into and out of the
MRP.  

> 
> That said, having gone through this exercise, I also feel adding the mapping
in OPM is probably the simplest. If we do need to support it generally (i.e.
presentation_id field in MediaRoute) then we should look into having the
extension provide it in the MediaRoute struct rather than having the browser
backfill it.

Agreed.

The extension should not be generating presentation IDs; they need to come from
a trusted process, as access to the ID allows connection to a presentation.