[PresentationAPI, Android] Add ChromeMediaRouterDialogController.java and bind it with the native M…

[PresentationAPI, Android] Add ChromeMediaRouterDialogController.java and bind it with the native MediaRouterDialogControllerAndroid

BUG=517101
Committed: https://crrev.com/52efd93fe1dee30d791a21f844b563fa4d375b50
Cr-Commit-Position: refs/heads/master@{#342399}

[whywhat, 2015-08-06 15:16:20]

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

[whywhat, 2015-08-06 15:16:20]

PTaL

[Bernhard Bauer, 2015-08-06 15:23:01]

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

https://codereview.chromium.org/1270353003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterDialogController.java:17:
public class ChromeMediaRouterDialogController {
What's the purpose of this class, other than, well, being created?

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

https://codereview.chromium.org/1270353003/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:34:
CHECK(env);
Is this CHECK actually necessary? I would only expect this to fail under very
exceptional circumstances, and then you'd presumably crash anyway.

[whywhat, 2015-08-06 15:27:13]

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

https://codereview.chromium.org/1270353003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterDialogController.java:17:
public class ChromeMediaRouterDialogController {
On 2015/08/06 at 15:23:01, Bernhard Bauer wrote:
> What's the purpose of this class, other than, well, being created?

To show/hide media router dialog to select presentation screens. I'm trying to
split up my patches into smaller chunks to avoid long reviews :)

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

https://codereview.chromium.org/1270353003/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:34:
CHECK(env);
On 2015/08/06 at 15:23:01, Bernhard Bauer wrote:
> Is this CHECK actually necessary? I would only expect this to fail under very
exceptional circumstances, and then you'd presumably crash anyway.

I'd think in such exceptional circumstances one should crash earlier than later.
IIRC it's the common thing to do after acquiring |env| in Chromium. Have this
practice changed?

[whywhat, 2015-08-06 15:28:28]

Rebased

[Bernhard Bauer, 2015-08-06 15:40:33]

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

https://codereview.chromium.org/1270353003/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:34:
CHECK(env);
On 2015/08/06 15:27:13, whywhat wrote:
> On 2015/08/06 at 15:23:01, Bernhard Bauer wrote:
> > Is this CHECK actually necessary? I would only expect this to fail under
very
> exceptional circumstances, and then you'd presumably crash anyway.
> 
> I'd think in such exceptional circumstances one should crash earlier than
later.
> IIRC it's the common thing to do after acquiring |env| in Chromium. Have this
> practice changed?

I have never seen it, TBH. If anything, we should check directly in
AttachCurrentThread(), and we usually do DCHECKs for these sorts of things
(CHECKs are mostly used to turn an otherwise unclean memory access into a clean
crash, for security reasons, but dereferencing a null pointer is okay). And
looking at AttachCurrentThread(), there is already a DCHECK for the result of
JavaVM::AttachCurrentThread().

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

https://codereview.chromium.org/1270353003/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:37:
reinterpret_cast<intptr_t>(this),
I think we use long (or jlong) rather than intptr_t.

[whywhat, 2015-08-06 16:25:38]

Removed CHECK, changed intptr_t to jlong

[whywhat, 2015-08-06 16:29:20]

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

https://codereview.chromium.org/1270353003/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:34:
CHECK(env);
On 2015/08/06 at 15:40:33, Bernhard Bauer wrote:
> On 2015/08/06 15:27:13, whywhat wrote:
> > On 2015/08/06 at 15:23:01, Bernhard Bauer wrote:
> > > Is this CHECK actually necessary? I would only expect this to fail under
very
> > exceptional circumstances, and then you'd presumably crash anyway.
> > 
> > I'd think in such exceptional circumstances one should crash earlier than
later.
> > IIRC it's the common thing to do after acquiring |env| in Chromium. Have
this
> > practice changed?
> 
> I have never seen it, TBH. If anything, we should check directly in
AttachCurrentThread(), and we usually do DCHECKs for these sorts of things
(CHECKs are mostly used to turn an otherwise unclean memory access into a clean
crash, for security reasons, but dereferencing a null pointer is okay). And
looking at AttachCurrentThread(), there is already a DCHECK for the result of
JavaVM::AttachCurrentThread().

Ok, CHECK removed.

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

https://codereview.chromium.org/1270353003/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/router/media_router_dialog_controller_android.cc:37:
reinterpret_cast<intptr_t>(this),
On 2015/08/06 at 15:40:33, Bernhard Bauer wrote:
> I think we use long (or jlong) rather than intptr_t.

Done.

[Bernhard Bauer, 2015-08-06 16:31:02]

Thanks! LGTM

[whywhat, 2015-08-06 16:52:38]

rebase

[whywhat, 2015-08-07 15:59:09]

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

[commit-bot: I haz the power, 2015-08-07 15:59:38]

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

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

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

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

Dry run: This issue passed the CQ dry run.

[whywhat, 2015-08-07 19:07:02]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2015-08-07 19:07:02]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1270353003/#ps80001
(title: "Disabled unnecessary DCHECK")

[commit-bot: I haz the power, 2015-08-07 19:07:27]

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

[commit-bot: I haz the power, 2015-08-07 19:13:28]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-07 19:13:57]

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