Only show cast button if video can be cast

Only show cast button if video can be cast

This moves the check of the CORS headers for casting to the
initialization of the RemoteMediaPlayerBridge. As a result we can
suppress the cast button if the video isn't playable on Chromecast.

BUG=539406
Committed: https://crrev.com/16865ed7e365226b34c48503828e4ed255241e53
Cr-Commit-Position: refs/heads/master@{#370412}

[aberent, 2016-01-19 17:34:31]

aberent@chromium.org changed reviewers:
+ avayvod@chromium.org, dgn@chromium.org

[whywhat, 2016-01-19 18:45:59]

lgtm with nits

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java
(right):

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:340:
// This exists for compatability with old downstream code
nit: s/compatability/compatibility

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:342:
protected void cast() {
But this is not used yet by the downstream code, isn't it? I preferred the
startCastingVideo name as simply cast() seems to abstract and could mean many
things for an accidental reader :(

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerBridge.java
(right):

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerBridge.java:376:
mRouteController.getValidityChecker().check(mOriginalSourceUrl,
mOriginalFrameUrl, cookies,
Why can't just route controller have the check method? Seems to be an overkill
to introduce an extra interface here.

[dgn, 2016-01-20 11:48:33]

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java
(right):

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:828:
RecordCastAction.castPlayRequested();
I tried to find what is expected to be recorded here, but couldn't find it.
Should this be called only when we can play the URI?

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:835:
if (mLocalVideoUri == null) return;
Shouldn't this be a pathological case now? check/log when we hit it?

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:873:
public void setUri(Uri uri, boolean playable) {
Unrelated to the current CL, but the get/setUri method names aren't great at
setting expectations for what those methods  do :/

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerBridge.java
(right):

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerBridge.java:383:
mIsPlayable = true;
Should this be set either way? can't a video be playable and then become
unplayable? If a cookie expired or something like that.

[aberent, 2016-01-20 12:37:40]

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java
(right):

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:340:
// This exists for compatability with old downstream code
On 2016/01/19 18:45:58, whywhat wrote:
> nit: s/compatability/compatibility

Done.

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:342:
protected void cast() {
On 2016/01/19 18:45:58, whywhat wrote:
> But this is not used yet by the downstream code, isn't it? 
Ambiguity of protected! This is intended to be implemented, not used, by the
concrete classes. An implementation of it is needed here because it is not yet
implemented by the downstream concrete class, but called by notifyRouteSelected
(above).

The whole toing and froing between the concrete and abstract classes of
notifyRouteSelected is actually a bit of a mess, but I decided not to sort that
out yet.

> I preferred the
> startCastingVideo name as simply cast() seems to abstract and could mean many
> things for an accidental reader :(
Ok, you are probably right. Done.

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java
(right):

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:828:
RecordCastAction.castPlayRequested();
On 2016/01/20 11:48:33, dgn wrote:
> I tried to find what is expected to be recorded here, but couldn't find it.
> Should this be called only when we can play the URI?

See RecordCastAction.java. Actually it should be called much earlier, I have
moved it to onRouteSelectedEvent().

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:835:
if (mLocalVideoUri == null) return;
On 2016/01/20 11:48:33, dgn wrote:
> Shouldn't this be a pathological case now? check/log when we hit it?

Done.

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:873:
public void setUri(Uri uri, boolean playable) {
On 2016/01/20 11:48:33, dgn wrote:
> Unrelated to the current CL, but the get/setUri method names aren't great at
> setting expectations for what those methods  do :/

Done.

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerBridge.java
(right):

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerBridge.java:376:
mRouteController.getValidityChecker().check(mOriginalSourceUrl,
mOriginalFrameUrl, cookies,
On 2016/01/19 18:45:58, whywhat wrote:
> Why can't just route controller have the check method? Seems to be an overkill
> to introduce an extra interface here.

Good point. I was thinking I would need to hold a reference to the checker, but
actually I don't. Done.

https://codereview.chromium.org/1607043002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerBridge.java:383:
mIsPlayable = true;
On 2016/01/20 11:48:33, dgn wrote:
> Should this be set either way? can't a video be playable and then become
> unplayable? If a cookie expired or something like that.

In the current code this can never happen, since this is only called as a
delayed call during initialization of the C++ RemoteMediaPlayer, however fixed
anyway, since I don't think there is any penalty for doing so.

[aberent, 2016-01-20 12:37:44]

The CQ bit was checked by aberent@chromium.org

[aberent, 2016-01-20 12:37:44]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/1607043002/#ps20001
(title: "Fix review comments")

[commit-bot: I haz the power, 2016-01-20 12:38:01]

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

[aberent, 2016-01-20 12:49:43]

The CQ bit was unchecked by aberent@chromium.org

[dgn, 2016-01-20 12:57:27]

lgtm

nit: comment formatting went wild

[aberent, 2016-01-20 13:52:08]

The CQ bit was checked by aberent@chromium.org

[aberent, 2016-01-20 13:52:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/1607043002/#ps40001
(title: "Fix comment formatting")

[commit-bot: I haz the power, 2016-01-20 13:52:25]

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

[commit-bot: I haz the power, 2016-01-20 14:59:23]

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

[commit-bot: I haz the power, 2016-01-20 14:59:24]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[aberent, 2016-01-20 15:51:56]

The CQ bit was checked by aberent@chromium.org

[aberent, 2016-01-20 15:51:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/1607043002/#ps60001
(title: "Fix null pointer error (found by telemetry smoke tests!)")

[commit-bot: I haz the power, 2016-01-20 15:52:40]

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

[commit-bot: I haz the power, 2016-01-20 16:44:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-20 16:45:51]

Description was changed from

==========
Only show cast button if video can be cast

This moves the check of the CORS headers for casting to the
initialization of the RemoteMediaPlayerBridge. As a result we can
suppress the cast button if the video isn't playable on Chromecast.

BUG=539406
==========

to

==========
Only show cast button if video can be cast

This moves the check of the CORS headers for casting to the
initialization of the RemoteMediaPlayerBridge. As a result we can
suppress the cast button if the video isn't playable on Chromecast.

BUG=539406

Committed: https://crrev.com/16865ed7e365226b34c48503828e4ed255241e53
Cr-Commit-Position: refs/heads/master@{#370412}
==========

[commit-bot: I haz the power, 2016-01-20 16:45:51]

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