Refactor Clank cast connect logic

Refactor Clank cast connect logic

This is a cleanup of the Clank cast connect logic. It:

1) Removes the assumption in the C++ code that only one video is
being cast. The C++ code doesn't need to know about this restriction,
which could, conceivably, be removed in future.

2) Moves most of the connect logic to Java, reducing the number of
transfers of control between Java and C++. This also simplifies the
connect logic, and removes some unnecessary interactions with the
Chromecast (see https://crbug/443210)

This also fixes pressing cast button on new video while casting
(https://454789).

Pressing the cast button on a new video while casting will now simply
switch to that video. We can't, with the current code structure, switch
cast devices at this point, so there is no point in putting up the chooser
dialog.

The previous behaviour was broken, as described in the bug.

This CL doesn't tidy up the disconnect code, I plan to do that in a separate
CL.

BUG=454798
BUG=443210
Committed: https://crrev.com/28753f3725cd63b67b1bd209592019d2780bc63d
Cr-Commit-Position: refs/heads/master@{#354277}

[aberent, 2015-10-14 11:33:16]

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

[aberent, 2015-10-14 11:33:17]

Mainly done, still some tidying up to do (some comments incomplete etc.) but
probably worth looking at now.

[whywhat, 2015-10-14 13:28:50]

Mostly nits.

https://codereview.chromium.org/1403703005/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/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:350:
// Check that somebody still cares.
nit: this comment seems redundant, maybe say something about the case when the
listener might be null instead?

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:353:
// Check that we actually want to cast the video
nit: redundant comment?

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:360:
* @param route
nit: remove javadoc

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:363:
// Pause the local player
nit: redundant comments?

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:365:
// Swap players
ditto

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:366:
mMediaStateListener.switchToRemotePlayer();
is switchToRemotePlayer always prepended with pauseLocal? should we combine the
two then?

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:367:
setDataSource(Uri.parse(mMediaStateListener.getSourceUrl()),
nit: mMediaStateListener doesn't seem like the best name given that it's not
just state callbacks but also some methods and getters.
mDelegate of type MediaRouteControllerDelegate?

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:369:
prepareAsync(
nit: can't prepareAsync get this from the mMediaStateListener?

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:374:
private boolean shouldCastVideo() {
Seems more like canCastMedia() to me (maybe we should avoid using video since
audio casting works too with the default controls)

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:378:
return true;
nit: how about just

return isRemotePlaybackAvailable() && !routeIsDefaultRoute() &&
currentRouteSupportsRemotePlayback(); ?

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

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteController.java:77:
public void switchToRemotePlayer();
this methods expose some info about the media player implementation. how about
naming them more flexibly, e.g. onBeforeCast(), onAfterCast(), onCastStop() etc?

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteController.java:80:
* Switch between the local and remote players, so that controls
nit: duplicate comment with the method above, they should be different for
different methods probably

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteController.java:86:
* @return the source URL
nit: javadoc seems generated, fix it here and below

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteController.java:119:
public int getSeekLocation();
nit: I don't think visibility modifiers are needed for interfaces? could be
omitted as it's always public, right?

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteController.java:357:
/**
nit: javadoc, the name seems confusing

https://codereview.chromium.org/1403703005/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/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerBridge.java:43:
private String mCookies;
where are these initialized? could most of them be final?

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
File chrome/browser/media/android/remote/remote_media_player_manager.cc (right):

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/remote/remote_media_player_manager.cc:27: if
(remote_player) {
nit: can this actually be null? should this be an assert?

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/remote/remote_media_player_manager.cc:145:
ScopedVector<MediaPlayerAndroid>::iterator it =
nit: you could use auto here and elsewhere.

[dgn, 2015-10-14 16:05:49]

https://codereview.chromium.org/1403703005/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/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:471:
private void setPrepared() {
nit: This has much more than just setting a flag now. Maybe rename it to
prepare() or something that evokes more an action?

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:638:
public boolean videoTakesOverCastDevice(MediaStateListener mediaStateListener) {
nit: The name is a bit strange. I suppose reusing startCastingVideo is not
acceptable? It would have been a nice name.

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
File chrome/browser/media/android/remote/remote_media_player_manager.cc (right):

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/remote/remote_media_player_manager.cc:134: if
(!players_playing_remotely_.count(player_id))
nit: I feel like a [bool isPlayingRemotely(player_id)] method would help with
clarity, since this is done a lot.

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
File chrome/browser/media/android/remote/remote_media_player_manager.h (right):

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/remote/remote_media_player_manager.h:91:
ScopedVector<media::MediaPlayerAndroid>::iterator GetAlternativePlayer(
Return media::MediaPlayerAndroid* instead, and nullptr if there is none? It
doesn't look like usage makes a difference between the entry being erased and
having never existed in the first place.

[aberent, 2015-10-15 10:31:38]

https://codereview.chromium.org/1403703005/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/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:350:
// Check that somebody still cares.
On 2015/10/14 13:28:49, whywhat wrote:
> nit: this comment seems redundant, maybe say something about the case when the
> listener might be null instead?

Done.

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:353:
// Check that we actually want to cast the video
On 2015/10/14 13:28:50, whywhat wrote:
> nit: redundant comment?

Done.

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:360:
* @param route
On 2015/10/14 13:28:50, whywhat wrote:
> nit: remove javadoc

Done.

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:363:
// Pause the local player
On 2015/10/14 13:28:50, whywhat wrote:
> nit: redundant comments?

Done.

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:365:
// Swap players
On 2015/10/14 13:28:49, whywhat wrote:
> ditto

Done.

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:366:
mMediaStateListener.switchToRemotePlayer();
On 2015/10/14 13:28:50, whywhat wrote:
> is switchToRemotePlayer always prepended with pauseLocal? should we combine
the
> two then?

At the moment it is, however I am thinking that Clank should probably pause the
local video as soon as the request to cast is made, which will mean moving
pauseLocal(). Still want to think about it further, so leaving it like this for
now.

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:367:
setDataSource(Uri.parse(mMediaStateListener.getSourceUrl()),
On 2015/10/14 13:28:49, whywhat wrote:
> nit: mMediaStateListener doesn't seem like the best name given that it's not
> just state callbacks but also some methods and getters.
> mDelegate of type MediaRouteControllerDelegate?

Sadly MediaStateListener is referenced in YouTubeMediaRouteController, so
changing this would require a two sided patch; which I want to avoid for this
CL.

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:369:
prepareAsync(
On 2015/10/14 13:28:50, whywhat wrote:
> nit: can't prepareAsync get this from the mMediaStateListener?

It can, and there is more to do in this area; but, in this CL, I don't want to
change the interface of any methods that are used or overridden in the YouTube
code, since doing so will require a two sided patch.

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:374:
private boolean shouldCastVideo() {
On 2015/10/14 13:28:50, whywhat wrote:
> Seems more like canCastMedia() to me (maybe we should avoid using video since
> audio casting works too with the default controls)

Done.

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:378:
return true;
On 2015/10/14 13:28:49, whywhat wrote:
> nit: how about just
> 
> return isRemotePlaybackAvailable() && !routeIsDefaultRoute() &&
> currentRouteSupportsRemotePlayback(); ?

Done.

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:471:
private void setPrepared() {
On 2015/10/14 16:05:49, dgn wrote:
> nit: This has much more than just setting a flag now. Maybe rename it to
> prepare() or something that evokes more an action?

changed to onCasting()

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:638:
public boolean videoTakesOverCastDevice(MediaStateListener mediaStateListener) {
On 2015/10/14 16:05:49, dgn wrote:
> nit: The name is a bit strange. I suppose reusing startCastingVideo is not
> acceptable? It would have been a nice name.

Yes, but it only starts casting the new video if the RouteController is already
in use. This is the reason it is boolean, and the reason for its name (now
changed to playerTakesOverCastDevice.

See how it is used in remote_media_player_manager.cc

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

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteController.java:77:
public void switchToRemotePlayer();
On 2015/10/14 13:28:50, whywhat wrote:
> this methods expose some info about the media player implementation. how about
> naming them more flexibly, e.g. onBeforeCast(), onAfterCast(), onCastStop()
etc?

Done.

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteController.java:80:
* Switch between the local and remote players, so that controls
On 2015/10/14 13:28:50, whywhat wrote:
> nit: duplicate comment with the method above, they should be different for
> different methods probably

Done.

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteController.java:86:
* @return the source URL
On 2015/10/14 13:28:50, whywhat wrote:
> nit: javadoc seems generated, fix it here and below

Done.

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteController.java:119:
public int getSeekLocation();
On 2015/10/14 13:28:50, whywhat wrote:
> nit: I don't think visibility modifiers are needed for interfaces? could be
> omitted as it's always public, right?

Done.

https://codereview.chromium.org/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteController.java:357:
/**
On 2015/10/14 13:28:50, whywhat wrote:
> nit: javadoc, the name seems confusing

See my reply to dgn@ on implementation.

https://codereview.chromium.org/1403703005/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/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerBridge.java:43:
private String mCookies;
On 2015/10/14 13:28:50, whywhat wrote:
> where are these initialized? could most of them be final?

Good catch; looks as if I have deleted the code that sets them. The user  agent
should be, and now is, final. The cookies take time to get so are set later and
are not final.

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
File chrome/browser/media/android/remote/remote_media_player_manager.cc (right):

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/remote/remote_media_player_manager.cc:27: if
(remote_player) {
On 2015/10/14 13:28:50, whywhat wrote:
> nit: can this actually be null? should this be an assert?

Yes, I think it can be. The player_id comes from the renderer, so the message
containing it is asynchronous to the state of the tab in the browser, so, at
least in theory, the local and remote players may have been deleted before the
message reaches the browser, or not yet have been created. I am not exactly sure
under what conditions this can happen, but BrowserMediaPlayerManager::OnStart
has the same check.

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/remote/remote_media_player_manager.cc:134: if
(!players_playing_remotely_.count(player_id))
On 2015/10/14 16:05:49, dgn wrote:
> nit: I feel like a [bool isPlayingRemotely(player_id)] method would help with
> clarity, since this is done a lot.

Done.

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/remote/remote_media_player_manager.cc:145:
ScopedVector<MediaPlayerAndroid>::iterator it =
On 2015/10/14 13:28:50, whywhat wrote:
> nit: you could use auto here and elsewhere.

Done.

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
File chrome/browser/media/android/remote/remote_media_player_manager.h (right):

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/remote/remote_media_player_manager.h:91:
ScopedVector<media::MediaPlayerAndroid>::iterator GetAlternativePlayer(
On 2015/10/14 16:05:49, dgn wrote:
> Return media::MediaPlayerAndroid* instead, and nullptr if there is none? It
> doesn't look like usage makes a difference between the entry being erased and
> having never existed in the first place.

No, I need the iterator in SwapCurrentPlayer, If I return MediaPlayerAndroid*
then I have to duplicate this code in SwapCurrentPlayer.

[aberent, 2015-10-15 10:33:32]

PTAL - now ready to land, as far as I am concerned; so looking for lgtms.

[whywhat, 2015-10-15 10:37:53]

Lgtm

I assume you've run the tests and checked that YT still works.

[dgn, 2015-10-15 11:07:31]

lgtm with nits

https://codereview.chromium.org/1403703005/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/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:360:
* @param route
On 2015/10/15 10:31:37, aberent wrote:
> On 2015/10/14 13:28:50, whywhat wrote:
> > nit: remove javadoc
> 
> Done.

Doesn't seem done. Same for other comment related comments here

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
File chrome/browser/media/android/remote/remote_media_player_manager.cc (right):

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/remote/remote_media_player_manager.cc:134: if
(!players_playing_remotely_.count(player_id))
On 2015/10/15 10:31:38, aberent wrote:
> On 2015/10/14 16:05:49, dgn wrote:
> > nit: I feel like a [bool isPlayingRemotely(player_id)] method would help
with
> > clarity, since this is done a lot.
> 
> Done.

Not done here

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
File chrome/browser/media/android/remote/remote_media_player_manager.h (right):

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/remote/remote_media_player_manager.h:91:
ScopedVector<media::MediaPlayerAndroid>::iterator GetAlternativePlayer(
On 2015/10/15 10:31:38, aberent wrote:
> On 2015/10/14 16:05:49, dgn wrote:
> > Return media::MediaPlayerAndroid* instead, and nullptr if there is none? It
> > doesn't look like usage makes a difference between the entry being erased
and
> > having never existed in the first place.
> 
> No, I need the iterator in SwapCurrentPlayer, If I return MediaPlayerAndroid*
> then I have to duplicate this code in SwapCurrentPlayer.

OK I missed that.
nit: How about having 2 methods then? returning a foo_list::iterator from getFoo
is weird and feels like leaking implementation details IMO. But it's a private
method so that's OK.

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

https://codereview.chromium.org/1403703005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerBridge.java:257:
protected boolean prepareAsync() {
nit: Maybe a comment to explain why it's not doing anything? Say that it's going
away in an upcoming CL?

[aberent, 2015-10-15 12:57:54]

https://codereview.chromium.org/1403703005/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/1403703005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:360:
* @param route
On 2015/10/15 11:07:31, dgn wrote:
> On 2015/10/15 10:31:37, aberent wrote:
> > On 2015/10/14 13:28:50, whywhat wrote:
> > > nit: remove javadoc
> > 
> > Done.
> 
> Doesn't seem done. Same for other comment related comments here

Done.

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
File chrome/browser/media/android/remote/remote_media_player_manager.h (right):

https://codereview.chromium.org/1403703005/diff/1/chrome/browser/media/androi...
chrome/browser/media/android/remote/remote_media_player_manager.h:91:
ScopedVector<media::MediaPlayerAndroid>::iterator GetAlternativePlayer(
On 2015/10/15 11:07:31, dgn wrote:
> On 2015/10/15 10:31:38, aberent wrote:
> > On 2015/10/14 16:05:49, dgn wrote:
> > > Return media::MediaPlayerAndroid* instead, and nullptr if there is none?
It
> > > doesn't look like usage makes a difference between the entry being erased
> and
> > > having never existed in the first place.
> > 
> > No, I need the iterator in SwapCurrentPlayer, If I return
MediaPlayerAndroid*
> > then I have to duplicate this code in SwapCurrentPlayer.
> 
> OK I missed that.
> nit: How about having 2 methods then? returning a foo_list::iterator from
getFoo
> is weird and feels like leaking implementation details IMO. But it's a private
> method so that's OK. 

Seems unnecessary. Not done.

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

https://codereview.chromium.org/1403703005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerBridge.java:257:
protected boolean prepareAsync() {
On 2015/10/15 11:07:31, dgn wrote:
> nit: Maybe a comment to explain why it's not doing anything? Say that it's
going
> away in an upcoming CL?

Actually never called, so it doesn't matter that the super implementation is
wrong, so removed.

[aberent, 2015-10-15 12:58:02]

The CQ bit was checked by aberent@chromium.org

[aberent, 2015-10-15 12:58:02]

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/1403703005/#ps60001
(title: "Fix nits")

[commit-bot: I haz the power, 2015-10-15 12:58:31]

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

[commit-bot: I haz the power, 2015-10-15 14:41:25]

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

[commit-bot: I haz the power, 2015-10-15 14:41:26]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[aberent, 2015-10-15 14:59:11]

The CQ bit was checked by aberent@chromium.org

[aberent, 2015-10-15 14:59:11]

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/1403703005/#ps80001
(title: "Fix trybot detected error.")

[commit-bot: I haz the power, 2015-10-15 15:00:00]

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

[commit-bot: I haz the power, 2015-10-15 15:33:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-15 15:34:24]

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