[Presentation API, MediaRouter] Routing from media sink selection to route creation.

[Presentation API, MediaRouter] Routing from media sink selection to route creation.

Extracted little common chunks of code from MediaRouterUI.

BUG=517101
Committed: https://crrev.com/4ccaf67671ff91282ff72ad1d0199320fae70757
Cr-Commit-Position: refs/heads/master@{#344218}

[whywhat, 2015-08-17 17:42:43]

avayvod@chromium.org changed reviewers:
+ imcheng@chromium.org, mlamouri@chromium.org, wez@chromium.org

[whywhat, 2015-08-17 17:42:44]

Mounir and Derek for code review, Wez for ownership of the media router stuff.
Please, take a look.

[whywhat, 2015-08-17 17:42:47]

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

[commit-bot: I haz the power, 2015-08-17 17:43:42]

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

[commit-bot: I haz the power, 2015-08-17 20:13:16]

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

[commit-bot: I haz the power, 2015-08-17 20:13:17]

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

[mlamouri (slow - plz ping), 2015-08-18 13:44:01]

lgtm with comments applied

https://codereview.chromium.org/1294133002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/MediaSink.java
(right):

https://codereview.chromium.org/1294133002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/MediaSink.java:8:
import android.support.v7.media.MediaRouter.RouteInfo;
nit: why do you need the import?

https://codereview.chromium.org/1294133002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/MediaSink.java:59:
public static MediaSink fromRoute(MediaRouter.RouteInfo route) {
It might be a stupid question but could you have a MediaSink ctor taking a
RouteInfo?

https://codereview.chromium.org/1294133002/diff/1/chrome/browser/media/androi...
File
chrome/browser/media/android/router/media_router_dialog_controller_android.cc
(right):

https://codereview.chromium.org/1294133002/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:70:
content::PresentationError(content::PRESENTATION_ERROR_UNKNOWN, ""));
Can we have a better error type and message?

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

https://codereview.chromium.org/1294133002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/create_presentation_session_request.cc:66:
presentation_request->MaybeInvokeErrorCallback(
nit: it's odd to call this "Maybe..." when you basically DCHECK() that it must
be called.

https://codereview.chromium.org/1294133002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/create_presentation_session_request.cc:67:
content::PresentationError(content::PRESENTATION_ERROR_UNKNOWN, error));
Do we really have no idea why it might have failed?

[whywhat, 2015-08-18 14:23:15]

https://codereview.chromium.org/1294133002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/MediaSink.java
(right):

https://codereview.chromium.org/1294133002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/MediaSink.java:8:
import android.support.v7.media.MediaRouter.RouteInfo;
On 2015/08/18 at 13:44:01, Mounir Lamouri wrote:
> nit: why do you need the import?

As I understand it's either that or the java doc below will look like {@link
android.support.v7.media.MediaRouter.RouteInfo}.

https://codereview.chromium.org/1294133002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/MediaSink.java:59:
public static MediaSink fromRoute(MediaRouter.RouteInfo route) {
On 2015/08/18 at 13:44:01, Mounir Lamouri wrote:
> It might be a stupid question but could you have a MediaSink ctor taking a
RouteInfo?

I was inspired by
https://jlordiales.wordpress.com/2012/12/26/static-factory-methods-vs-traditi...

https://codereview.chromium.org/1294133002/diff/1/chrome/browser/media/androi...
File
chrome/browser/media/android/router/media_router_dialog_controller_android.cc
(right):

https://codereview.chromium.org/1294133002/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:70:
content::PresentationError(content::PRESENTATION_ERROR_UNKNOWN, ""));
On 2015/08/18 at 13:44:01, Mounir Lamouri wrote:
> Can we have a better error type and message?

Yes :)

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

https://codereview.chromium.org/1294133002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/create_presentation_session_request.cc:66:
presentation_request->MaybeInvokeErrorCallback(
On 2015/08/18 at 13:44:01, Mounir Lamouri wrote:
> nit: it's odd to call this "Maybe..." when you basically DCHECK() that it must
be called.

Well, probably it has history :) It's an existing method name that I didn't
intend to fix in this CL.

https://codereview.chromium.org/1294133002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/create_presentation_session_request.cc:67:
content::PresentationError(content::PRESENTATION_ERROR_UNKNOWN, error));
On 2015/08/18 at 13:44:01, Mounir Lamouri wrote:
> Do we really have no idea why it might have failed?

I just moved the existing method, I'm not sure about the desktop implementation
:)
In case of Android (see the follow up CL), we provide some info in the message.

[imcheng, 2015-08-18 17:50:34]

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

https://codereview.chromium.org/1294133002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/create_presentation_session_request.cc:66:
presentation_request->MaybeInvokeErrorCallback(
On 2015/08/18 14:23:15, whywhat wrote:
> On 2015/08/18 at 13:44:01, Mounir Lamouri wrote:
> > nit: it's odd to call this "Maybe..." when you basically DCHECK() that it
must
> be called.
> 
> Well, probably it has history :) It's an existing method name that I didn't
> intend to fix in this CL.

Yeah we should rename those in a separate cl.

https://codereview.chromium.org/1294133002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/create_presentation_session_request.cc:67:
content::PresentationError(content::PRESENTATION_ERROR_UNKNOWN, error));
On 2015/08/18 14:23:15, whywhat wrote:
> On 2015/08/18 at 13:44:01, Mounir Lamouri wrote:
> > Do we really have no idea why it might have failed?
> 
> I just moved the existing method, I'm not sure about the desktop
implementation
> :)
> In case of Android (see the follow up CL), we provide some info in the
message.

Currently the desktop version only provides a human readable error string. We
can probably plumb error codes if needed.

https://codereview.chromium.org/1294133002/diff/20001/chrome/browser/media/an...
File
chrome/browser/media/android/router/media_router_dialog_controller_android.cc
(right):

https://codereview.chromium.org/1294133002/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:40:
request(PassPresentationRequest());
Can |request| be null here?

Is there a notion of browser-initiated presentation in android?

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

https://codereview.chromium.org/1294133002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_dialog_controller.cc:119: return
presentation_request_->media_source().id();
what if presentation_request_ is null?

https://codereview.chromium.org/1294133002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_dialog_controller.h (right):

https://codereview.chromium.org/1294133002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_dialog_controller.h:65: std::string
GetMediaSourceId() const;
Please add documentation

[whywhat, 2015-08-18 19:21:24]

PTaL!

https://codereview.chromium.org/1294133002/diff/20001/chrome/browser/media/an...
File
chrome/browser/media/android/router/media_router_dialog_controller_android.cc
(right):

https://codereview.chromium.org/1294133002/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:40:
request(PassPresentationRequest());
On 2015/08/18 at 17:50:34, imcheng1 wrote:
> Can |request| be null here?
> 
> Is there a notion of browser-initiated presentation in android?

Nope, there's no notion of browser-initiated presentation in Android - there's
no UI for that. If we implement tab mirroring on par with the desktop, we might
add the present button in the toolbar/menu and then browser initiated
presentation will be possible.

I believe that it shouldn't be null otherwise (we close the dialog before
resetting the request).

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

https://codereview.chromium.org/1294133002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_dialog_controller.cc:119: return
presentation_request_->media_source().id();
On 2015/08/18 at 17:50:34, imcheng1 wrote:
> what if presentation_request_ is null?

This is only called from CreateDialog so it should never be null.
Changed the method to more generic GetPresentationRequest and added a DCHECK in
there.

https://codereview.chromium.org/1294133002/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/media_router_dialog_controller.h (right):

https://codereview.chromium.org/1294133002/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_dialog_controller.h:65: std::string
GetMediaSourceId() const;
On 2015/08/18 at 17:50:34, imcheng1 wrote:
> Please add documentation

Done.

[whywhat, 2015-08-18 19:21:29]

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

[commit-bot: I haz the power, 2015-08-18 19:21:41]

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

[imcheng, 2015-08-18 20:13:11]

lgtm

[commit-bot: I haz the power, 2015-08-18 20:36:27]

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

[commit-bot: I haz the power, 2015-08-18 20:36:28]

Dry run: This issue passed the CQ dry run.

[Wez, 2015-08-18 21:50:09]

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

https://codereview.chromium.org/1294133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/MediaSink.java:56:
* @param route the route information provided by Android
nit: This is not very readable - should "the" be capitalized? Or a hyphen placed
between |route| and "the" to separate them properly?

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

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:41:
DCHECK(request);
You're dereferencing |request| immediately after this, so there's no need to
DCHECK here.

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:51:
MediaRouter* router = static_cast<MediaRouterAndroid*>(
Why are you static-casting to MediaRouterAndroid* if you're going to assign it
to a MediaRouter*..?

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:54:
DCHECK(router);
Same here.

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:73:
"The device picker dialog has been dismissed"));
Shouldn't this be a localizable string?

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:89:
DCHECK(g_ChromeMediaRouterDialogController_clazz);
I searched about for some relationship between this and RegisterNativesImpl but
I can't even find references to the _clazz, so I can't answer my own questions:
Why do we DCHECK the _clazz but ignore the return-code of RegisterNativesImpl?
What is it that calling code will do if Register() returns false? Is there a
meaningful fallback, or in practice does it mean that Things Went Horribly
Wrong? If the latter, then why not CHECK here?

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

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.h:29:
// JNI functions.
This comment doesn't really tell the reader anything, beyond that they need to
go and look for something in JNI to understand them. e.g. Are these called by
some JNI, or do they call through to JNI? Given that they are not part of an
interface, who is it who will call them?

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

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/create_presentation_session_request.cc:64:
DCHECK(presentation_request);
nit: No need to DCHECK here, since both paths de-ref it, which will DCHECK if
it's NULL anyway.

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

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_dialog_controller.h:63:
scoped_ptr<CreatePresentationSessionRequest> PassPresentationRequest();
nit: "take" seems to be the more common/logical terminology here, since you're
not passing the MediaRouteDialogController, but taking the
PresentationSessionRequest from it.  Up to you, though.

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_dialog_controller.h:67:
CreatePresentationSessionRequest* GetPresentationRequest();
Should the request be const, then? And similarly should the getter be a const
method?

Since this is a simple getter, its implementation should be in-line, and it
should be called presentation_request(), similarly to initiator().

You only need it out-of-line to allow you to DCHECK; is that necessary, or could
you rely on callers DCHECKing, or dereferencing it and crashing?

[Wez, 2015-08-18 21:59:28]

LGTM once my comments are addressed.

[whywhat, 2015-08-19 13:04:29]

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

https://codereview.chromium.org/1294133002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/MediaSink.java:56:
* @param route the route information provided by Android
On 2015/08/18 at 21:50:08, Wez wrote:
> nit: This is not very readable - should "the" be capitalized? Or a hyphen
placed between |route| and "the" to separate them properly?

I believe we have a couple of common styles, this being the one. However,
capitalized word is also common (perhaps prevails):|
https://code.google.com/p/chromium/codesearch#search/&q=file:%5C.java%20@para...

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

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:41:
DCHECK(request);
On 2015/08/18 at 21:50:08, Wez wrote:
> You're dereferencing |request| immediately after this, so there's no need to
DCHECK here.

Done.

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:54:
DCHECK(router);
On 2015/08/18 at 21:50:08, Wez wrote:
> Same here.

Done.

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:73:
"The device picker dialog has been dismissed"));
On 2015/08/18 at 21:50:08, Wez wrote:
> Shouldn't this be a localizable string?

This is an error message returned to the web developer along with the error code
so I don't think so.

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:89:
DCHECK(g_ChromeMediaRouterDialogController_clazz);
On 2015/08/18 at 21:50:08, Wez wrote:
> I searched about for some relationship between this and RegisterNativesImpl
but I can't even find references to the _clazz, so I can't answer my own
questions: Why do we DCHECK the _clazz but ignore the return-code of
RegisterNativesImpl? What is it that calling code will do if Register() returns
false? Is there a meaningful fallback, or in practice does it mean that Things
Went Horribly Wrong? If the latter, then why not CHECK here?

It seems like noone is doing this check any longer in any other Register
methods.
The RegisterNativesImpl is auto generated from "native" methods in the Java
file. It's defined in the jni/ChromeMediaRouterDialogController_jni.h
If it returns false, it means something did went horribly wrong and loading the
Chrome's native .so library will fail, so the app will crash on startup.

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

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.h:29:
// JNI functions.
On 2015/08/18 at 21:50:09, Wez wrote:
> This comment doesn't really tell the reader anything, beyond that they need to
go and look for something in JNI to understand them. e.g. Are these called by
some JNI, or do they call through to JNI? Given that they are not part of an
interface, who is it who will call them?

Done.

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

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/create_presentation_session_request.cc:64:
DCHECK(presentation_request);
On 2015/08/18 at 21:50:09, Wez wrote:
> nit: No need to DCHECK here, since both paths de-ref it, which will DCHECK if
it's NULL anyway.

Done.

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

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_dialog_controller.h:63:
scoped_ptr<CreatePresentationSessionRequest> PassPresentationRequest();
On 2015/08/18 at 21:50:09, Wez wrote:
> nit: "take" seems to be the more common/logical terminology here, since you're
not passing the MediaRouteDialogController, but taking the
PresentationSessionRequest from it.  Up to you, though.

Ack. Will change in a separate code review perhaps :)

https://codereview.chromium.org/1294133002/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/media_router_dialog_controller.h:67:
CreatePresentationSessionRequest* GetPresentationRequest();
On 2015/08/18 at 21:50:09, Wez wrote:
> Should the request be const, then? And similarly should the getter be a const
method?
> 
> Since this is a simple getter, its implementation should be in-line, and it
should be called presentation_request(), similarly to initiator().
> 
> You only need it out-of-line to allow you to DCHECK; is that necessary, or
could you rely on callers DCHECKing, or dereferencing it and crashing?

Done.

[whywhat, 2015-08-19 13:04:33]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-08-19 13:04:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, imcheng@chromium.org, wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1294133002/#ps60001
(title: "Fixed comments and naming and DCHECKs")

[commit-bot: I haz the power, 2015-08-19 13:04:42]

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

[commit-bot: I haz the power, 2015-08-19 14:05:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-19 14:06:19]

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