[Media Router] Implement JoinRoute + update CreateRoute API.

[Media Router] Implement JoinRoute + update CreateRoute API.

CreateRoute:
- The CreateRoute API from MediaRouter no longer accepts a presentation
  id. The browser will always generate one for the consumer, which will
  be present in the route created (if the provider chooses not to
  override the generated id)
- In addition, CreateRoute now accepts two new arguments - origin and
  tab_id. These are used for enforcing same-origin and/or same-tab
  joining policies and are optional.
- Added the 3 aforementioned arguments to mojom CreateRoute API.
- For DEFAULT mode CreateRoute (e.g., Presentation API or browser
  initiated Presentation), origin and tab_id are derived from the URL
  of presentation's originating frame and tab.

JoinRoute:
- JoinRoute takes (source, presentation_id, origin, tab_id). origin
  and tab_id are used for same-tab/origin join policy. Optional.
- PresentationServiceDelegateImpl invokes JoinRoute() directly for
  incoming Presentation API joinSession() requests.

BUG=461815
Committed: https://crrev.com/a3ee1d154f303e2cfe443c454bb2bd651f3cf321
Cr-Commit-Position: refs/heads/master@{#334474}

[imcheng (use chromium acct), 2015-06-10 18:30:50]

Patchset #1 (id:1) has been deleted

[imcheng (use chromium acct), 2015-06-10 18:30:56]

Patchset #1 (id:20001) has been deleted

[imcheng (use chromium acct), 2015-06-10 18:35:07]

Patchset #1 (id:40001) has been deleted

[imcheng (use chromium acct), 2015-06-10 18:36:58]

imcheng@google.com changed reviewers:
+ mfoltz@chromium.org, rdevlin.cronin@chromium.org

[imcheng (use chromium acct), 2015-06-10 18:36:59]

mark: PTAL at everything.
Devlin: PTAL at extensions/renderer/resources/media_router_bindings.js

Thanks.

[Devlin, 2015-06-10 19:26:24]

extensions lgtm

[mark a. foltz, 2015-06-10 23:18:51]

Let's check the semantics we actually need for CreateRoute/JoinRoute and figure
out if these parameters are optional or mandatory.

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

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/create_session_request_unittest.cc:59:
TEST_F(CreateSessionRequestTest, Callbacks) {
Would like to see test cases that exercise both the success and error callbacks.

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

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:45: // scope, and are optional.
Since this API is for creating a new route, it's a little confusing to say
"same-origin" or "same-tab" - same with respect to what other origin or tab?

IIUC these values are actually for the provider to enforce these constraints on
JoinRoute() requests.

Can you elaborate the documentation to clarify a bit?

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:47: // |tab_id| if not enforcing
same-origin and/or same-tab scope.
The caller doesn't know if the provider is going to enforce this or not - so
should they be mandatory?

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:60: // are optional. (See CreateRoute
documentation).
Similar comment about mandatory vs optional for origin, tab_id

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

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:88: // may be overridden by a
provider implementation. The presentation ID will
Does any provider actually do this?  Last time I checked the Cast MRP did not
need to override the id for new routes.

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:100: string presentation_id,
This might be better named default_presentation_id or original_presentation_id.

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

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:302: std::string
presentation_id(base::GenerateGUID());
I feel like this should have a prefix like "mr-" to distinguish it from other
GUIDs passed around.

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:317: // TODO(imcheng):
What is the origin if not creating route in DEFAULT mode?
If the user is e.g. initiating tab mirroring it should probably be
chrome://media-router/

[imcheng (use chromium acct), 2015-06-11 22:49:13]

PTAL

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

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/create_session_request_unittest.cc:59:
TEST_F(CreateSessionRequestTest, Callbacks) {
On 2015/06/10 23:18:51, mark a. foltz wrote:
> Would like to see test cases that exercise both the success and error
callbacks.

Done. Added a test case where error callback is invoked.

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

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:45: // scope, and are optional.
On 2015/06/10 23:18:51, mark a. foltz wrote:
> Since this API is for creating a new route, it's a little confusing to say
> "same-origin" or "same-tab" - same with respect to what other origin or tab?
> 
> IIUC these values are actually for the provider to enforce these constraints
on
> JoinRoute() requests.
> 
> Can you elaborate the documentation to clarify a bit?

Ok, I added some comments on the values of |origin| and |tab_id|. Also clarified
the join case with example.

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:47: // |tab_id| if not enforcing
same-origin and/or same-tab scope.
On 2015/06/10 23:18:51, mark a. foltz wrote:
> The caller doesn't know if the provider is going to enforce this or not - so
> should they be mandatory?

Good point. We can make them mandatory. I left the option to pass -1 for tab_id
for the Chrome apps use case.

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.h:60: // are optional. (See CreateRoute
documentation).
On 2015/06/10 23:18:51, mark a. foltz wrote:
> Similar comment about mandatory vs optional for origin, tab_id

Done.

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

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:88: // may be overridden by a
provider implementation. The presentation ID will
On 2015/06/10 23:18:51, mark a. foltz wrote:
> Does any provider actually do this?  Last time I checked the Cast MRP did not
> need to override the id for new routes.

I remember in the Start + Join doc that there was a use case where presentation
ID was overridden by the Cast MRP?

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router.mojom:100: string presentation_id,
On 2015/06/10 23:18:51, mark a. foltz wrote:
> This might be better named default_presentation_id or
original_presentation_id.

Done.

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

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_mojo_impl.cc:302: std::string
presentation_id(base::GenerateGUID());
On 2015/06/10 23:18:51, mark a. foltz wrote:
> I feel like this should have a prefix like "mr-" to distinguish it from other
> GUIDs passed around.

Yes we should have mr_ per the Start + Join doc.

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_ui.cc (right):

https://codereview.chromium.org/1173753003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_ui.cc:317: // TODO(imcheng):
What is the origin if not creating route in DEFAULT mode?
On 2015/06/10 23:18:51, mark a. foltz wrote:
> If the user is e.g. initiating tab mirroring it should probably be
> chrome://media-router/

Done.

[mark a. foltz, 2015-06-12 19:43:22]

lgtm

[imcheng (use chromium acct), 2015-06-15 20:16:25]

The CQ bit was checked by imcheng@google.com

[imcheng (use chromium acct), 2015-06-15 20:16:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/1173753003/#ps190001
(title: "Compile fix again x2")

[commit-bot: I haz the power, 2015-06-15 20:17:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1173753003/190001

[commit-bot: I haz the power, 2015-06-15 22:19:01]

Committed patchset #8 (id:190001)

[commit-bot: I haz the power, 2015-06-15 22:19:52]

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