Upstream Chrome for Android Cast.

Upstream Chrome for Android Cast.

This upstreams all of Chrome for Android's cast code with the exception of:
1. The YouTube support code.
2. The id of the Chrome cast receiver.

The YouTube support code and the id of Chrome cast receiver have to remain
proprietory due to licencing restrictions. The tests will be upstreamed
seperately, but need further work to make them independent of other
downstream code.

BUG=315088
Committed: https://crrev.com/76097722a9e8577767a1d21a4a6b98ca322bfa7b
Cr-Commit-Position: refs/heads/master@{#322569}

[aberent, 2015-02-19 16:04:02]

For information. Not yet ready for full review. Known issues:

- OWNER files needed for new directories
- LICENSE and README files needed for third_party/android_media
- Some illegal dependencies between components.
- Casting from Chrome Shell currently crashes.

[aberent, 2015-02-19 16:05:45]

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

[whywhat, 2015-02-25 16:31:32]

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/res/draw...
File chrome/android/java/res/drawable-v21/notification_icon_bg.xml (right):

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/res/draw...
chrome/android/java/res/drawable-v21/notification_icon_bg.xml:11: <solid
android:color="@color/light_active_color" />
you seem to have changed this from active_control_color to light_active_color?
is this intentional?

https://codereview.chromium.org/928643003/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/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:173:
/**
nit: Lost an empty line before the comment?

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:213:
if (mAvailableRouteListeners.isEmpty()) {
nit: also an empty line between the ifs?

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

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ChromeMediaRouteDialogFactory.java:21:
/**
nit: I believe you had two blank lines between imports and the class?

https://codereview.chromium.org/928643003/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/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:25:
import org.chromium.chrome.R;
nit: need to move it down to the org.chromium.* imports

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

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ExpandedControllerActivity.java:25:
import org.chromium.chrome.R;
nit: ditto

https://codereview.chromium.org/928643003/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/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteController.java:11:
import org.chromium.third_party.android.media.ChromeMediaRouteControllerDialog;
nit: not sorted in alphabetical order (this import should be the last)

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

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/NotificationTransportControl.java:22:
import org.chromium.chrome.R;
nit: join this import with the other org.chromium.* imports below?

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/NotificationTransportControl.java:450:
;
nit: semicolon belongs to the previous line

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

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:22:
import org.chromium.chrome.R;
nit: join with other org.chromium.* imports?

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
File chrome/browser/media/android/remote/record_cast_action.h (right):

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/record_cast_action.h:5: #ifndef
CLANK_NATIVE_FRAMEWORK_CHROME_RECORD_CAST_ACTION_
nit: update the include guards to match the new file location

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:6: #include
"chrome/browser/media/android/remote/remote_media_player_manager.h"
blank line after the class header include

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:22: namespace
{
why didn't we use the anonymous namespace downstream?

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:174: ;
remove the line

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:217: 
nit: remove the empty line

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
File chrome/browser/media/android/remote/remote_media_player_bridge.h (right):

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_bridge.h:5: #ifndef
CLANK_NATIVE_FRAMEWORK_CHROME_REMOTE_MEDIA_PLAYER_BRIDGE_H_
update the include guards

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_bridge.h:8: #include
"media/base/android/media_player_bridge.h"
nit: sort the headers

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_bridge.h:153: #endif  //
CLANK_NATIVE_FRAMEWORK_CHROME_REMOTE_MEDIA_PLAYER_BRIDGE_H_
nit: I'd put a blank line before the #endif

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

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_manager.cc:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
Do we want to update the year in the added files, btw?

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_manager.cc:5: #include
"chrome/browser/media/android/remote/remote_media_player_manager.h"
insert a blank line after the class header file

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_manager.cc:304: } //
namespace remote_media
blank line before the namespace closing?

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

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_manager.h:5: #ifndef
CLANK_NATIVE_FRAMEWORK_CHROME_REMOTE_MEDIA_PLAYER_MANAGER_H_
Update the include guards

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_manager.h:111: } //
namespace remote_media
blank line before the namespace closing bracket

[aberent, 2015-03-10 18:46:47]

Replies to comments that are not simply nits. No code changes uploaded yet.

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/res/draw...
File chrome/android/java/res/drawable-v21/notification_icon_bg.xml (right):

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/res/draw...
chrome/android/java/res/drawable-v21/notification_icon_bg.xml:11: <solid
android:color="@color/light_active_color" />
On 2015/02/25 16:31:31, whywhat wrote:
> you seem to have changed this from active_control_color to light_active_color?
> is this intentional?

Yes, I didn't want to move active_control_color into Chromium
- They are the same color
- Clank uses a mixture of both, and I can find no pattern as to which is used
where.
- In other uses Chromium uses light_active_color where Clank might use
active_control_color (or might use light_active_color)

If you understand the intended difference in use, I would consider changing
this.

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:22: namespace
{
On 2015/02/25 at 16:31:31, whywhat wrote:
> why didn't we use the anonymous namespace downstream?

Stupidity? (I wrote this originally). I added this when adding the other
namespace, which is needed to avoid name clashes while with the Clank version,
and hence to avoid an enormous two sided patch.

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

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_manager.cc:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
On 2015/02/25 at 16:31:32, whywhat wrote:
> Do we want to update the year in the added files, btw?

I don't think so. Copyright normally is about the content of the files, which
hasn't changed significantly, not the name or location of the files.

[aberent, 2015-03-11 18:29:58]

https://codereview.chromium.org/928643003/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/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:173:
/**
On 2015/02/25 16:31:31, whywhat wrote:
> nit: Lost an empty line before the comment?

Done.

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:213:
if (mAvailableRouteListeners.isEmpty()) {
On 2015/02/25 16:31:31, whywhat wrote:
> nit: also an empty line between the ifs?

Done.

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

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ChromeMediaRouteDialogFactory.java:21:
/**
On 2015/02/25 16:31:31, whywhat wrote:
> nit: I believe you had two blank lines between imports and the class?

Done.

https://codereview.chromium.org/928643003/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/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:25:
import org.chromium.chrome.R;
On 2015/02/25 16:31:31, whywhat wrote:
> nit: need to move it down to the org.chromium.* imports

Done.

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

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ExpandedControllerActivity.java:25:
import org.chromium.chrome.R;
On 2015/02/25 16:31:31, whywhat wrote:
> nit: ditto

Done.

https://codereview.chromium.org/928643003/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/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/MediaRouteController.java:11:
import org.chromium.third_party.android.media.ChromeMediaRouteControllerDialog;
On 2015/02/25 16:31:31, whywhat wrote:
> nit: not sorted in alphabetical order (this import should be the last)

Done.

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

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/NotificationTransportControl.java:22:
import org.chromium.chrome.R;
On 2015/02/25 16:31:31, whywhat wrote:
> nit: join this import with the other org.chromium.* imports below?

Done.

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/NotificationTransportControl.java:450:
;
On 2015/02/25 16:31:31, whywhat wrote:
> nit: semicolon belongs to the previous line

Done.

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

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:22:
import org.chromium.chrome.R;
On 2015/02/25 16:31:31, whywhat wrote:
> nit: join with other org.chromium.* imports?

Done.

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
File chrome/browser/media/android/remote/record_cast_action.h (right):

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/record_cast_action.h:5: #ifndef
CLANK_NATIVE_FRAMEWORK_CHROME_RECORD_CAST_ACTION_
On 2015/02/25 16:31:31, whywhat wrote:
> nit: update the include guards to match the new file location

Done.

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:6: #include
"chrome/browser/media/android/remote/remote_media_player_manager.h"
On 2015/02/25 16:31:31, whywhat wrote:
> blank line after the class header include

Done.

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:174: ;
On 2015/02/25 16:31:31, whywhat wrote:
> remove the line

Done.

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:217: 
On 2015/02/25 16:31:31, whywhat wrote:
> nit: remove the empty line

Done.

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
File chrome/browser/media/android/remote/remote_media_player_bridge.h (right):

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_bridge.h:5: #ifndef
CLANK_NATIVE_FRAMEWORK_CHROME_REMOTE_MEDIA_PLAYER_BRIDGE_H_
On 2015/02/25 16:31:31, whywhat wrote:
> update the include guards

Done.

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_bridge.h:8: #include
"media/base/android/media_player_bridge.h"
On 2015/02/25 16:31:31, whywhat wrote:
> nit: sort the headers

Done.

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_bridge.h:153: #endif  //
CLANK_NATIVE_FRAMEWORK_CHROME_REMOTE_MEDIA_PLAYER_BRIDGE_H_
On 2015/02/25 16:31:31, whywhat wrote:
> nit: I'd put a blank line before the #endif

Done.

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

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_manager.cc:5: #include
"chrome/browser/media/android/remote/remote_media_player_manager.h"
On 2015/02/25 16:31:32, whywhat wrote:
> insert a blank line after the class header file

Done.

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_manager.cc:304: } //
namespace remote_media
On 2015/02/25 16:31:32, whywhat wrote:
> blank line before the namespace closing?

Done.

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

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_manager.h:5: #ifndef
CLANK_NATIVE_FRAMEWORK_CHROME_REMOTE_MEDIA_PLAYER_MANAGER_H_
On 2015/02/25 16:31:32, whywhat wrote:
> Update the include guards

Done.

https://codereview.chromium.org/928643003/diff/1/chrome/browser/media/android...
chrome/browser/media/android/remote/remote_media_player_manager.h:111: } //
namespace remote_media
On 2015/02/25 16:31:32, whywhat wrote:
> blank line before the namespace closing bracket

Done.

[whywhat, 2015-03-11 18:33:48]

lgtm % the color question that I defer to Aurimas.

+Aurimas

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/res/draw...
File chrome/android/java/res/drawable-v21/notification_icon_bg.xml (right):

https://codereview.chromium.org/928643003/diff/1/chrome/android/java/res/draw...
chrome/android/java/res/drawable-v21/notification_icon_bg.xml:11: <solid
android:color="@color/light_active_color" />
On 2015/03/10 18:46:46, aberent wrote:
> On 2015/02/25 16:31:31, whywhat wrote:
> > you seem to have changed this from active_control_color to
light_active_color?
> > is this intentional?
> 
> Yes, I didn't want to move active_control_color into Chromium
> - They are the same color
> - Clank uses a mixture of both, and I can find no pattern as to which is used
> where.
> - In other uses Chromium uses light_active_color where Clank might use
> active_control_color (or might use light_active_color)
> 
> If you understand the intended difference in use, I would consider changing
> this.
> 

Might be Aurimas knows? +aurimas@chromium.org

[aurimas (slooooooooow), 2015-03-11 18:36:23]

On 2015/03/11 at 18:33:48, avayvod wrote:
> lgtm % the color question that I defer to Aurimas.
> 
> +Aurimas
> 
>
https://codereview.chromium.org/928643003/diff/1/chrome/android/java/res/draw...
> File chrome/android/java/res/drawable-v21/notification_icon_bg.xml (right):
> 
>
https://codereview.chromium.org/928643003/diff/1/chrome/android/java/res/draw...
> chrome/android/java/res/drawable-v21/notification_icon_bg.xml:11: <solid
android:color="@color/light_active_color" />
> On 2015/03/10 18:46:46, aberent wrote:
> > On 2015/02/25 16:31:31, whywhat wrote:
> > > you seem to have changed this from active_control_color to
light_active_color?
> > > is this intentional?
> > 
> > Yes, I didn't want to move active_control_color into Chromium
> > - They are the same color
> > - Clank uses a mixture of both, and I can find no pattern as to which is
used
> > where.
> > - In other uses Chromium uses light_active_color where Clank might use
> > active_control_color (or might use light_active_color)
> > 
> > If you understand the intended difference in use, I would consider changing
> > this.
> > 
> 
> Might be Aurimas knows? +aurimas@chromium.org

You should use light_active_color. I keep meaning to rename it downstream, but I
keep getting distracted.

Aurimas

[aberent, 2015-03-12 15:43:06]

Patchset #4 (id:60001) has been deleted

[aberent, 2015-03-12 18:14:48]

aberent@chromium.org changed reviewers:
+ aurimas@chromium.org

[aberent, 2015-03-12 18:14:49]

Now ready for full review:

avayvod@ - please (continue to) review against downstream.
qinmin@ - please review for relationship to existing media code.
arimas@ - OWNER review of resources please (almost all directly copied from
downstream).

avayvod@'s utility for comparing with downstream may be highly useful here.

I will add other owners in a separate email.

[aberent, 2015-03-12 18:22:40]

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

[aberent, 2015-03-12 18:22:41]

bauerb@chromium.org: Please review changes in chrome/android/java as OWNER

[aberent, 2015-03-12 18:46:10]

aberent@chromium.org changed reviewers:
+ cpu@chromium.org, scherkus@chromium.org, yfriedman@chromium.org

[aberent, 2015-03-12 18:46:10]

aberent@chromium.org changed required reviewers:
+ aurimas@chromium.org, avayvod@chromium.org, bauerb@chromium.org,
cpu@chromium.org, qinmin@chromium.org, scherkus@chromium.org,
yfriedman@chromium.org

[aberent, 2015-03-12 18:46:11]

yfriedman@chromium.org: Please review changes, as OWNER, in chrome/app

scherkus@chromium.org: Please review changes, as OWNER, in chrome/browser/media

cpu@chromium.org: Please review changes, as OWNER, in chrome.gyp, and addition
of android_media to third_party.

[Yaron, 2015-03-12 19:02:11]

FYI I think Min is out for a bit

https://codereview.chromium.org/928643003/diff/100001/chrome/app/android/DEPS
File chrome/app/android/DEPS (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/app/android/DEPS...
chrome/app/android/DEPS:2: # Fix layering violation crbug.com/396828
Yeesh. I'd rather not block this massive upstreaming (kudos!) based on this but
this isn't ideal. Can you prioritize fixing this as a follow-up?

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:11: #include
"content/browser/android/content_view_core_impl.h"
You can replace this with content/public/browser/android/content_view_core.h and
(ContentViewCore below) to fix a layering issue.

[scherkus (not reviewing), 2015-03-12 19:15:04]

few nits and questions

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/DEPS (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/DEPS:2: # Fix layering violation
crbug.com/396828
this bug was filed in july 2014 -- do we have a timeline on fixing this?

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/record_cast_action.cc (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/record_cast_action.cc:6: #include <jni.h>
nit: you've got the #include ordering / spacing correct in other files -- might
as well fix it up here too!

#include ".h"

#include <system stuff>

#include "everything else in alphabetical order"

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:24: * Dummy
function to fix callbacks; 0 argument Callback constructor creates
I don't think this comment is being very helpful -- a better comment would
explain *why* we don't need to do anything with the player_id when this callback
is run

(also, it should be using // comments seeing as this is c++ code!)

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:36:
RequestMediaResourcesCB(base::Bind(&DoNothing)),
you shouldn't have to specify the constructor, should you? I would think just
binding would work...

[aberent, 2015-03-12 20:02:08]

Here are my replies to the comments on laying violations. In brief these will be
much easier to fix once the code is upstream (more precisely, once it is no
longer downstream), and I intend to do so as soon as the downstream CL has
landed.

Replies to other comments (and fixes) will follow.

Yaron - do you know how long Min is away? His calendar suggests he is back next
Tuesday but I don't know what is normal in America for paternity leave.

https://codereview.chromium.org/928643003/diff/100001/chrome/app/android/DEPS
File chrome/app/android/DEPS (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/app/android/DEPS...
chrome/app/android/DEPS:2: # Fix layering violation crbug.com/396828
On 2015/03/12 19:02:10, Yaron wrote:
> Yeesh. I'd rather not block this massive upstreaming (kudos!) based on this
but
> this isn't ideal. Can you prioritize fixing this as a follow-up? 

Yes, I intend to. The reason I didn't do so first is that, until I get rid of
the downstream version of the code, the obvious fixes require two sided patches.
I also need to discuss with Min what the best fix is (e.g. should
browser_media_player_manager.h be public?).

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/DEPS (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/DEPS:2: # Fix layering violation
crbug.com/396828
On 2015/03/12 19:15:04, scherkus wrote:
> this bug was filed in july 2014 -- do we have a timeline on fixing this?

I intend to fix this once I have got rid of the downstream version of the code
(soon after this CL lands). The obvious fixes involve either making
browser_media_player_manager.h public or making minor changes to other existing
interfaces, which would need simultaneous changes to the upstream and downstream
repositories. Once the code is all upstream this sort of change will be much
easier.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:11: #include
"content/browser/android/content_view_core_impl.h"
On 2015/03/12 19:02:10, Yaron wrote:
> You can replace this with content/public/browser/android/content_view_core.h
and
> (ContentViewCore below) to fix a layering issue.

Unfortunately not. GetContentViewCore() returns a ContentViewCoreImpl, and
without this header file the compiler doesn't know that there is any
relationship between a ContentViewCore and a ContentViewCoreImpl. Once the
downstream code is gone I will create a CL that changes GetContentViewCore() to
return ContentViewCore (I have checked that this works), but until it has gone
that fix would need a two sided patch to avoid breaking the downstream version.

[Yaron, 2015-03-12 20:30:51]

unfortunately, I don't know more than you about when he'll be back

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:11: #include
"content/browser/android/content_view_core_impl.h"
On 2015/03/12 20:02:08, aberent wrote:
> On 2015/03/12 19:02:10, Yaron wrote:
> > You can replace this with content/public/browser/android/content_view_core.h
> and
> > (ContentViewCore below) to fix a layering issue.
> 
> Unfortunately not. GetContentViewCore() returns a ContentViewCoreImpl, and
> without this header file the compiler doesn't know that there is any
> relationship between a ContentViewCore and a ContentViewCoreImpl. Once the
> downstream code is gone I will create a CL that changes GetContentViewCore()
to
> return ContentViewCore (I have checked that this works), but until it has gone
> that fix would need a two sided patch to avoid breaking the downstream
version.

Dang, ok.

[cpu_(ooo_6.6-7.5), 2015-03-12 20:31:15]

I assume you want my review for third party. If so I strongly suggest you split
that into a separate CL.

[Yaron, 2015-03-12 20:31:50]

chrome/app/android lgtm

[aberent, 2015-03-13 09:29:30]

On 2015/03/12 20:31:15, cpu wrote:
> I assume you want my review for third party. If so I strongly suggest you
split
> that into a separate CL.

Yes please - see https://codereview.chromium.org/1008603003. Please also review,
as OWNER, the changes to chrome/chrome.gyp, which I have left in this CL.

[aberent, 2015-03-13 09:41:27]

aberent@chromium.org changed reviewers:
+ jln@chromium.org

[aberent, 2015-03-13 09:41:27]

aberent@chromium.org changed required reviewers:
+ jln@chromium.org

[aberent, 2015-03-13 09:41:29]

jln@ - Please approve, as OWNER, "new" (actually moved from downstream)
dependency on media_player_messages_android.h

[aberent, 2015-03-13 09:41:34]

jln@ - Please approve, as OWNER, "new" (actually moved from downstream)
dependency on media_player_messages_android.h

[Bernhard Bauer, 2015-03-13 12:18:03]

This is a pretty big CL, so it might be that I missed some things and will only
notice them on a later review; I apologize in advance if that happens.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
File chrome/android/java/res/layout/remote_notification_bar.xml (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
chrome/android/java/res/layout/remote_notification_bar.xml:2: 
Use the standard Chromium copyright header.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
File chrome/android/java/res/values-v17/styles.xml (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
chrome/android/java/res/values-v17/styles.xml:229: <!-- Video Fling -->
Cast?

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
File chrome/android/java/res/values-v21/styles.xml (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
chrome/android/java/res/values-v21/styles.xml:69: <style
name="RemoteNotificationProgressBar"
parent="@android:style/Widget.ProgressBar.Horizontal">
Break before parent to fit into 80 columns.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
chrome/android/java/res/values/dimens.xml:128: <dimen
name="remote_notification_logo_max_width">@android:dimen/notification_large_icon_width</dimen>
Break before the closing tag.

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

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:81:
Log.d(TAG,
Wrap this in braces if it doesn't all fit on one line.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ChromeMediaRouteDialogFactory.java
(right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ChromeMediaRouteDialogFactory.java:73:
if (android.os.Build.VERSION.SDK_INT >= 16) {
You could move this check into the method and early-return if it fails; it would
save some code, and everything relating to the version check would be closer
together.

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

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:627:
* Disconnect from the remote screen without stopping the media playing. use
#release for
Nit: Put #release into {@link} so it will be linked, or just write it as
release().

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:724:
@Override
Indent this four spaces less.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:750:
@Override
Indent four spaces less.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:770:
// It is important to not clear the stream state here to let chrome
Nit: Capitalize Chrome.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:961:
@Override
Indent four spaces less.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:966:
@Override
Here as well.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ExpandedControllerActivity.java
(right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ExpandedControllerActivity.java:44:
// We don't use {@link android.media.MediaController}, but a custom one.
Regular comments are not processed by Javadoc, so you might as well just write
MediaController here.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControl.java
(right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControl.java:38:
// Needed to get round findbugs complaints.
Nit: "around"

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControl.java:104:
// notifications.
Yes, we should. :)

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControlV14.java
(right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControlV14.java:41:
private android.media.RemoteControlClient mRemoteControlClient;
Why can't you import this?

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControlV16.java
(right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControlV16.java:15:
@TargetApi(16)
We have Build.VERSION_CODES.XXX for this, BTW.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/TransportControl.java
(right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/TransportControl.java:73:
public final void setError(/*@Nullable*/ String message) {
Why is @Nullable commented out here?

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:1324: <message
name="IDS_ATHOME_NOTIFICATION_STOPPED" desc="AtHome notification text when
stopped. [CHAR LIMIT=40]">
What is AtHome?

https://codereview.chromium.org/928643003/diff/100001/chrome/app/android/chro...
File chrome/app/android/chrome_main_delegate_android.cc (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/app/android/chro...
chrome/app/android/chrome_main_delegate_android.cc:25: static
content::BrowserMediaPlayerManager* CreateRemoteMediaPlayerManager(
Move this to an anonymous namespace at the top of the file?

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/record_cast_action.cc (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/record_cast_action.cc:32: // TODO (aberent)
Change this once we are upstream, when can change it both here
Super-nit: Use the style "TODO(ldap): [...]", as it makes it easier to grep.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/record_cast_action.cc:85: static void
RecordCastEndedTimeRemaining(JNIEnv*, jclass, jint video_total_time,
In C++, put each parameter on a separate line if they don't fit all on the same
line.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:57: if
(url_.is_valid())
Wrap the block braces if it's more than one line.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:338: return
casting_message_.get() ?
You don't need .get() to use it as a boolean expression (scoped_ptr has a
conversion to bool), and you don't need the .get() call if you dereference the
scoped_ptr.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.h (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.h:93: virtual
void PendingSeekInternal(const base::TimeDelta& time);
Why is this method and the next one virtual?

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_manager.cc (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_manager.cc:298: if
(player_id == RemotePlayerId() && replaced_local_player_.get()) {
.get() is unnecessary.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_manager.cc:300: new
MediaPlayerMsg_MediaMetadataChanged(
This could go on the previous line.

[aberent, 2015-03-13 19:04:36]

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
File chrome/android/java/res/layout/remote_notification_bar.xml (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
chrome/android/java/res/layout/remote_notification_bar.xml:2: 
On 2015/03/13 12:18:01, Bernhard Bauer wrote:
> Use the standard Chromium copyright header.

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
File chrome/android/java/res/values-v17/styles.xml (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
chrome/android/java/res/values-v17/styles.xml:229: <!-- Video Fling -->
On 2015/03/13 12:18:01, Bernhard Bauer wrote:
> Cast?

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
File chrome/android/java/res/values-v21/styles.xml (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
chrome/android/java/res/values-v21/styles.xml:69: <style
name="RemoteNotificationProgressBar"
parent="@android:style/Widget.ProgressBar.Horizontal">
On 2015/03/13 12:18:01, Bernhard Bauer wrote:
> Break before parent to fit into 80 columns.

The rest of the file is wider than 80 columns, with no obvious limit, as are the
other XML files near it, so not fixing.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/res...
chrome/android/java/res/values/dimens.xml:128: <dimen
name="remote_notification_logo_max_width">@android:dimen/notification_large_icon_width</dimen>
On 2015/03/13 12:18:01, Bernhard Bauer wrote:
> Break before the closing tag.

No. Current format is consistent with the rest of the file.

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

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:81:
Log.d(TAG,
On 2015/03/13 12:18:01, Bernhard Bauer wrote:
> Wrap this in braces if it doesn't all fit on one line.

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ChromeMediaRouteDialogFactory.java
(right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ChromeMediaRouteDialogFactory.java:73:
if (android.os.Build.VERSION.SDK_INT >= 16) {
On 2015/03/13 12:18:01, Bernhard Bauer wrote:
> You could move this check into the method and early-return if it fails; it
would
> save some code, and everything relating to the version check would be closer
> together.

Done.

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

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:627:
* Disconnect from the remote screen without stopping the media playing. use
#release for
On 2015/03/13 12:18:01, Bernhard Bauer wrote:
> Nit: Put #release into {@link} so it will be linked, or just write it as
> release().

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:724:
@Override
On 2015/03/13 12:18:01, Bernhard Bauer wrote:
> Indent this four spaces less.

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:750:
@Override
On 2015/03/13 12:18:01, Bernhard Bauer wrote:
> Indent four spaces less.

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:770:
// It is important to not clear the stream state here to let chrome
On 2015/03/13 12:18:01, Bernhard Bauer wrote:
> Nit: Capitalize Chrome.

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:961:
@Override
On 2015/03/13 12:18:01, Bernhard Bauer wrote:
> Indent four spaces less.

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:966:
@Override
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> Here as well.

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ExpandedControllerActivity.java
(right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ExpandedControllerActivity.java:44:
// We don't use {@link android.media.MediaController}, but a custom one.
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> Regular comments are not processed by Javadoc, so you might as well just write
> MediaController here.

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControl.java
(right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControl.java:38:
// Needed to get round findbugs complaints.
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> Nit: "around"

Done. Actually both are correct English, and "get round" is possibly more
correct in British English (if you are interested see, for example
http://separatedbyacommonlanguage.blogspot.co.uk/2007/05/round-and-around.html),
but changed since I think we use "get around" more often in Google.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControl.java:104:
// notifications.
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> Yes, we should. :)

I have updated the comment to explain why we can't; at least without further
investigation.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControlV14.java
(right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControlV14.java:41:
private android.media.RemoteControlClient mRemoteControlClient;
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> Why can't you import this?

Because one cannot put @SuppressWarnings("deprecation") on imports, so if we
import this we get a deprecation warning that we can't silence.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControlV16.java
(right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControlV16.java:15:
@TargetApi(16)
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> We have Build.VERSION_CODES.XXX for this, BTW.

True, but the Android API specs use numbers, not names, so it is probably
clearer to match what the specs do.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/TransportControl.java
(right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/TransportControl.java:73:
public final void setError(/*@Nullable*/ String message) {
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> Why is @Nullable commented out here?
No idea, it has been there, and commented out, since the first version in the
Clank repository. Removed since we don't consistently use @Nullable elsewhere.

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:1324: <message
name="IDS_ATHOME_NOTIFICATION_STOPPED" desc="AtHome notification text when
stopped. [CHAR LIMIT=40]">
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> What is AtHome?

Changed to CAST

https://codereview.chromium.org/928643003/diff/100001/chrome/app/android/chro...
File chrome/app/android/chrome_main_delegate_android.cc (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/app/android/chro...
chrome/app/android/chrome_main_delegate_android.cc:25: static
content::BrowserMediaPlayerManager* CreateRemoteMediaPlayerManager(
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> Move this to an anonymous namespace at the top of the file?

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/record_cast_action.cc (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/record_cast_action.cc:6: #include <jni.h>
On 2015/03/12 19:15:04, scherkus wrote:
> nit: you've got the #include ordering / spacing correct in other files --
might
> as well fix it up here too!
> 
> #include ".h"
> 
> #include <system stuff>
> 
> #include "everything else in alphabetical order"

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/record_cast_action.cc:32: // TODO (aberent)
Change this once we are upstream, when can change it both here
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> Super-nit: Use the style "TODO(ldap): [...]", as it makes it easier to grep.

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/record_cast_action.cc:85: static void
RecordCastEndedTimeRemaining(JNIEnv*, jclass, jint video_total_time,
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> In C++, put each parameter on a separate line if they don't fit all on the
same
> line.

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:24: * Dummy
function to fix callbacks; 0 argument Callback constructor creates
On 2015/03/12 19:15:04, scherkus wrote:
> I don't think this comment is being very helpful -- a better comment would
> explain *why* we don't need to do anything with the player_id when this
callback
> is run
> 
> (also, it should be using // comments seeing as this is c++ code!)

Comment updated. The callback is never called by MediaPlayerAndroid, or by
RemoteMediaPlayerBridge, but I need a function to satisfy the compiler. 

Longer term we should probably remove this from the MediaPlayerAndroid
constructor, but I would like to discuss this with Min before I do so.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:36:
RequestMediaResourcesCB(base::Bind(&DoNothing)),
On 2015/03/12 19:15:04, scherkus wrote:
> you shouldn't have to specify the constructor, should you? I would think just
> binding would work...

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:57: if
(url_.is_valid())
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> Wrap the block braces if it's more than one line.

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:338: return
casting_message_.get() ?
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> You don't need .get() to use it as a boolean expression (scoped_ptr has a
> conversion to bool), and you don't need the .get() call if you dereference the
> scoped_ptr.

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.h (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.h:93: virtual
void PendingSeekInternal(const base::TimeDelta& time);
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> Why is this method and the next one virtual?

Done. Due to history, they were originally in a different class. They should be,
and now are, private non-virtual.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_manager.cc (right):

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_manager.cc:298: if
(player_id == RemotePlayerId() && replaced_local_player_.get()) {
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> .get() is unnecessary.

Done.

https://codereview.chromium.org/928643003/diff/100001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_manager.cc:300: new
MediaPlayerMsg_MediaMetadataChanged(
On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> This could go on the previous line.

Done.

[aberent, 2015-03-13 19:13:00]

aberent@chromium.org changed required reviewers:
- bauerb@chromium.org

[aberent, 2015-03-13 19:13:02]

Yaron - Bernhard has gone on vacation for the next week. Are you able to do an
OWNER review of chrome/android/java?

[whywhat, 2015-03-13 19:25:48]

Upstream vs downstream LGTM

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/res...
File chrome/android/java/res/values-v21/styles.xml (right):

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/res...
chrome/android/java/res/values-v21/styles.xml:62: <!-- Note: the values below
are copied from PlayMovies/YouTube's style. -->
Probably remove? Is it still true? I thought we updated the notification style
to match Chromium's.

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/res...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/res...
chrome/android/java/res/values/dimens.xml:127: <!-- Video Fling related
constants -->
s/Video Fling/Cast?

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/res...
File chrome/android/java/res/values/values.xml (right):

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/res...
chrome/android/java/res/values/values.xml:42: <item type="id"
name="remote_notification" />
An item specific comment connecting this notification to Cast support?

https://codereview.chromium.org/928643003/diff/140001/chrome/android/shell/ja...
File chrome/android/shell/java/AndroidManifest.xml.jinja2 (right):

https://codereview.chromium.org/928643003/diff/140001/chrome/android/shell/ja...
chrome/android/shell/java/AndroidManifest.xml.jinja2:68: 
nit: Some unintended ws change?

https://codereview.chromium.org/928643003/diff/140001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/928643003/diff/140001/chrome/chrome.gyp#newco...
chrome/chrome.gyp:627:
'../third_party/android_media/android_media.gyp:android_media_java',
Have you forgotten to update .gn build files?

[Bernhard Bauer, 2015-03-13 20:55:46]

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControlV16.java
(right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControlV16.java:15:
@TargetApi(16)
On 2015/03/13 19:04:35, aberent wrote:
> On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> > We have Build.VERSION_CODES.XXX for this, BTW.
> 
> True, but the Android API specs use numbers, not names, so it is probably
> clearer to match what the specs do.

Yeah, but Build.VERSION_CODES is used throughout the Chromium code base. All
other things being equal, I would prefer version codes over numeric values,
because it makes it easier to find all occurrences of a certain version code,
e.g. if we want to remove code refering to a version that is not supported
anymore.

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ChromeMediaRouteDialogFactory.java
(right):

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ChromeMediaRouteDialogFactory.java:96:
if (android.os.Build.VERSION.SDK_INT >= 16) {
And now you can remove these as well.

https://codereview.chromium.org/928643003/diff/140001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/928643003/diff/140001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:342:
*casting_message_.get() : std::string();
Sorry, the second part of my comment was refering to this: You can dereference a
scoped_ptr just like a regular pointer (i.e. you don't need the .get() here
either).

[cpu_(ooo_6.6-7.5), 2015-03-16 20:23:37]

cpu@chromium.org changed reviewers:
- cpu@chromium.org

[cpu_(ooo_6.6-7.5), 2015-03-16 20:23:37]

cpu@chromium.org changed required reviewers:
- cpu@chromium.org

[aberent, 2015-03-17 21:33:52]

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControlV16.java
(right):

https://codereview.chromium.org/928643003/diff/100001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/LockScreenTransportControlV16.java:15:
@TargetApi(16)
On 2015/03/13 20:55:45, Bernhard (OOO until Mar 23) wrote:
> On 2015/03/13 19:04:35, aberent wrote:
> > On 2015/03/13 12:18:02, Bernhard Bauer wrote:
> > > We have Build.VERSION_CODES.XXX for this, BTW.
> > 
> > True, but the Android API specs use numbers, not names, so it is probably
> > clearer to match what the specs do.
> 
> Yeah, but Build.VERSION_CODES is used throughout the Chromium code base. All
> other things being equal, I would prefer version codes over numeric values,
> because it makes it easier to find all occurrences of a certain version code,
> e.g. if we want to remove code refering to a version that is not supported
> anymore.

Good point. Done throughout the CL.

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/res...
File chrome/android/java/res/values-v21/styles.xml (right):

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/res...
chrome/android/java/res/values-v21/styles.xml:62: <!-- Note: the values below
are copied from PlayMovies/YouTube's style. -->
On 2015/03/13 19:25:48, whywhat wrote:
> Probably remove? Is it still true? I thought we updated the notification style
> to match Chromium's.

Done.

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/res...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/res...
chrome/android/java/res/values/dimens.xml:127: <!-- Video Fling related
constants -->
On 2015/03/13 19:25:48, whywhat wrote:
> s/Video Fling/Cast?

Done.

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/res...
File chrome/android/java/res/values/values.xml (right):

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/res...
chrome/android/java/res/values/values.xml:42: <item type="id"
name="remote_notification" />
On 2015/03/13 19:25:48, whywhat wrote:
> An item specific comment connecting this notification to Cast support?

Done.

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ChromeMediaRouteDialogFactory.java
(right):

https://codereview.chromium.org/928643003/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/ChromeMediaRouteDialogFactory.java:96:
if (android.os.Build.VERSION.SDK_INT >= 16) {
On 2015/03/13 20:55:46, Bernhard (OOO until Mar 23) wrote:
> And now you can remove these as well.

Done.

https://codereview.chromium.org/928643003/diff/140001/chrome/android/shell/ja...
File chrome/android/shell/java/AndroidManifest.xml.jinja2 (right):

https://codereview.chromium.org/928643003/diff/140001/chrome/android/shell/ja...
chrome/android/shell/java/AndroidManifest.xml.jinja2:68: 
On 2015/03/13 19:25:48, whywhat wrote:
> nit: Some unintended ws change?

Done.

https://codereview.chromium.org/928643003/diff/140001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/928643003/diff/140001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:342:
*casting_message_.get() : std::string();
On 2015/03/13 20:55:46, Bernhard (OOO until Mar 23) wrote:
> Sorry, the second part of my comment was refering to this: You can dereference
a
> scoped_ptr just like a regular pointer (i.e. you don't need the .get() here
> either).

Done.

https://codereview.chromium.org/928643003/diff/140001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/928643003/diff/140001/chrome/chrome.gyp#newco...
chrome/chrome.gyp:627:
'../third_party/android_media/android_media.gyp:android_media_java',
On 2015/03/13 19:25:48, whywhat wrote:
> Have you forgotten to update .gn build files?

Done.

[aberent, 2015-03-17 21:41:40]

aberent@chromium.org changed reviewers:
+ thestig@chromium.org

[aberent, 2015-03-17 21:41:41]

thestig@ - Please review, as OWNER, my changes to chrome/chrome.gyp.

[aberent, 2015-03-17 21:42:09]

aberent@chromium.org changed required reviewers:
+ thestig@chromium.org

[Lei Zhang, 2015-03-17 22:17:17]

On 2015/03/17 21:41:41, aberent wrote:
> thestig@ - Please review, as OWNER, my changes to chrome/chrome.gyp.

The GYP chrome_java target has new deps for android_media_java and
android_support_v7_mediarouter_javalib, but the GN version has those plus
//media/base/android:media_java in addition. Can you fix the discrepency or
explain why they are different?

[aberent, 2015-03-18 11:45:31]

On 2015/03/17 22:17:17, Lei Zhang wrote:
> On 2015/03/17 21:41:41, aberent wrote:
> > thestig@ - Please review, as OWNER, my changes to chrome/chrome.gyp.
> 
> The GYP chrome_java target has new deps for android_media_java and
> android_support_v7_mediarouter_javalib, but the GN version has those plus
> //media/base/android:media_java in addition. Can you fix the discrepency or
> explain why they are different?

Fixed. Added dependency to chrome.gyp. It seems that the Java builder we invoke
from GYP builds (but not GN builds) picks up indirect dependencies when
compiling, so this was picking up the dependency on media_java via the
dependency on content_java, hence meaning that the build worked without this.

[Lei Zhang, 2015-03-18 17:27:41]

gyp/gn files lgtm

https://codereview.chromium.org/928643003/diff/180001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/928643003/diff/180001/chrome/chrome.gyp#newco...
chrome/chrome.gyp:632: '../media/media.gyp:media_java',
alphabetical order please

[jln (very slow on Chromium), 2015-03-18 18:09:15]

On 2015/03/13 09:41:34, aberent wrote:
> jln@ - Please approve, as OWNER, "new" (actually moved from downstream)
> dependency on media_player_messages_android.h

Did I understand correctly that you need my approval for
chrome/browser/media/android/remote/DEPS?

Since it's not a security concern or true "IPC" review, I will rubberstamp if a
content/common/media/ owner approves.

[aberent, 2015-03-20 12:06:04]

Ping!

Still looking for review comments and/or lgtm from at least qinmin@, aurimus@
and scherkus@

[aurimas (slooooooooow), 2015-03-20 16:34:06]

chrome/android/java/res/* LGTM % nits

https://chromiumcodereview.appspot.com/928643003/diff/180001/chrome/android/j...
File chrome/android/java/res/drawable/ic_cast_dark.xml (right):

https://chromiumcodereview.appspot.com/928643003/diff/180001/chrome/android/j...
chrome/android/java/res/drawable/ic_cast_dark.xml:2: <!-- Copyright (c) 2013 The
Chromium Authors. All rights reserved.
Remove (c)

https://chromiumcodereview.appspot.com/928643003/diff/180001/chrome/android/j...
File chrome/android/java/res/layout/remote_notification_bar.xml (right):

https://chromiumcodereview.appspot.com/928643003/diff/180001/chrome/android/j...
chrome/android/java/res/layout/remote_notification_bar.xml:8: author:
bclayton@google.com (Ben Clayton)
these author tags are non standard in Chromium. Please remove them.

[aurimas (slooooooooow), 2015-03-20 16:34:19]

aurimas@chromium.org changed required reviewers:
- aurimas@chromium.org, avayvod@chromium.org

[aurimas (slooooooooow), 2015-03-20 16:34:35]

aurimas@chromium.org changed required reviewers:
- scherkus@chromium.org, thestig@chromium.org, yfriedman@chromium.org

[qinmin, 2015-03-20 19:35:47]

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

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:61:
if (mDebug) Log.d(TAG, "Added route " + route.getName() + " " + route.getId());
there are lots of if(mDebug) Log.d() statements, what about simply introduce a
debug(String message) function;

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

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:220:
public void setRemoteVolume(int delta) {
what is delta refer to? db? or percentage?
I think we need better Java docs for all the classes here. The java docs are not
very consistent, some functions have it, while others are not

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:1023:
if (mDebug) Log.d(TAG, "setDataSource called, uri = " + uri);
ditto

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/FullscreenMediaRouteButton.java
(right):

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/FullscreenMediaRouteButton.java:46:
if (wasEnabled == enabled) return;
can this line be moved on top of the super.setEnabled(enabled)?

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfo.java
(right):

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfo.java:66:
if (obj == this) {
nit: can remove the {} and move the return statement up here and beolow

https://codereview.chromium.org/928643003/diff/180001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/928643003/diff/180001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:507: cookies_
= cookies;
Do we need to retrieve auth credentials for basic authentications? MPB does so
here

https://codereview.chromium.org/928643003/diff/180001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.h (right):

https://codereview.chromium.org/928643003/diff/180001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.h:24: class
RemoteMediaPlayerBridge : public media::MediaPlayerAndroid {
There are a lot of similarities between this class and MediaPlayerBridge.
A lot of the variables have the same name and some functions are identical.
Probably add a TODO here to reuse some of the logics in MPB

https://codereview.chromium.org/928643003/diff/180001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_manager.cc (right):

https://codereview.chromium.org/928643003/diff/180001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_manager.cc:136: if
(replaced_local_player_.get()) {
single line if else statement, no need for {} here

[aberent, 2015-03-23 17:35:31]

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/res...
File chrome/android/java/res/drawable/ic_cast_dark.xml (right):

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/res...
chrome/android/java/res/drawable/ic_cast_dark.xml:2: <!-- Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2015/03/20 16:34:06, aurimas wrote:
> Remove (c)

Done.

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/res...
File chrome/android/java/res/layout/remote_notification_bar.xml (right):

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/res...
chrome/android/java/res/layout/remote_notification_bar.xml:8: author:
bclayton@google.com (Ben Clayton)
On 2015/03/20 16:34:06, aurimas wrote:
> these author tags are non standard in Chromium. Please remove them.

Done.

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

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:61:
if (mDebug) Log.d(TAG, "Added route " + route.getName() + " " + route.getId());
On 2015/03/20 19:35:46, qinmin wrote:
> there are lots of if(mDebug) Log.d() statements, what about simply introduce a
> debug(String message) function;

I would like to do this as a separate CL, since, until the upstreaming CLs land,
I would like to keep the upstream and downstream code in step as far as
possible. I have created http://crbug/469649

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

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:220:
public void setRemoteVolume(int delta) {
On 2015/03/20 19:35:46, qinmin wrote:
> what is delta refer to? db? or percentage?
> I think we need better Java docs for all the classes here. The java docs are
not
> very consistent, some functions have it, while others are not
The Javadoc comments for functions that override other functions are with the
overridden functions. This is, I think, the Chrome convention, so I don't think
this, or any other overriding function, should have Javadoc comments. Having
said this, the comments on MediaRouteController.setRemoteVolume needed some
improvement.

I don't know what the delta refers to. It is "Android volume units" which appear
to be undocumented in the Android APIs.

All public members, except overrides and trivial getters and setters, of all
Cast classes now have Javadocs.

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/DefaultMediaRouteController.java:1023:
if (mDebug) Log.d(TAG, "setDataSource called, uri = " + uri);
On 2015/03/20 19:35:46, qinmin wrote:
> ditto

Separate CL.

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/FullscreenMediaRouteButton.java
(right):

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/FullscreenMediaRouteButton.java:46:
if (wasEnabled == enabled) return;
On 2015/03/20 19:35:46, qinmin wrote:
> can this line be moved on top of the super.setEnabled(enabled)?

No idea. There seems to have been a history of subtle changes to this code
downstream (e.g. moving where the super.setEnabled is done) without adequate
documentation. As such, I would, for now, prefer to leave this alone, and do
some experiments later, leading possibly to an additional CL.

Added a TODO.

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfo.java
(right):

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfo.java:66:
if (obj == this) {
On 2015/03/20 19:35:46, qinmin wrote:
> nit: can remove the {} and move the return statement up here and beolow

Done. Also the next 'if'.

https://codereview.chromium.org/928643003/diff/180001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/928643003/diff/180001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:507: cookies_
= cookies;
On 2015/03/20 19:35:46, qinmin wrote:
> Do we need to retrieve auth credentials for basic authentications? MPB does so
> here
Adding a TODO to investigate.

https://codereview.chromium.org/928643003/diff/180001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.h (right):

https://codereview.chromium.org/928643003/diff/180001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.h:24: class
RemoteMediaPlayerBridge : public media::MediaPlayerAndroid {
On 2015/03/20 19:35:46, qinmin wrote:
> There are a lot of similarities between this class and MediaPlayerBridge.
> A lot of the variables have the same name and some functions are identical.
> Probably add a TODO here to reuse some of the logics in MPB

Actually this originally derived from MediaBridgePlayer, and I came to the
conclusion that the there wasn't enough logic in common to be useful, so I split
them. I think most of the common logic remaining is due to this history, rather
than really justified, and I have been gradually removing much of the logic in
here, since a lot of it doesn't actually make sense for the remote case.

https://codereview.chromium.org/928643003/diff/180001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_manager.cc (right):

https://codereview.chromium.org/928643003/diff/180001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_manager.cc:136: if
(replaced_local_player_.get()) {
On 2015/03/20 19:35:46, qinmin wrote:
> single line if else statement, no need for {} here

Done.

https://codereview.chromium.org/928643003/diff/180001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/928643003/diff/180001/chrome/chrome.gyp#newco...
chrome/chrome.gyp:632: '../media/media.gyp:media_java',
On 2015/03/18 17:27:41, Lei Zhang wrote:
> alphabetical order please

Done.

[whywhat, 2015-03-23 17:46:36]

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/FullscreenMediaRouteButton.java
(right):

https://codereview.chromium.org/928643003/diff/180001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/FullscreenMediaRouteButton.java:46:
if (wasEnabled == enabled) return;
On 2015/03/23 17:35:31, aberent wrote:
> On 2015/03/20 19:35:46, qinmin wrote:
> > can this line be moved on top of the super.setEnabled(enabled)?
> 
> No idea. There seems to have been a history of subtle changes to this code
> downstream (e.g. moving where the super.setEnabled is done) without adequate
> documentation. As such, I would, for now, prefer to leave this alone, and do
> some experiments later, leading possibly to an additional CL.
> 
> Added a TODO.

Actually, setButtonVisibility() below depends on isEnabled() state so it has to
be updated before setButtonVisibility() is called. We could pass enabled there
though (in a follow up CL).

[qinmin, 2015-03-23 18:34:54]

lgtm

[aberent, 2015-03-26 21:06:36]

The CQ bit was checked by aberent@chromium.org

[aberent, 2015-03-26 21:06:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org, avayvod@chromium.org, thestig@chromium.org,
aurimas@chromium.org, qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/928643003/#ps220001
(title: "Fix preserving position when switching to local playback")

[commit-bot: I haz the power, 2015-03-26 21:07:12]

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

[commit-bot: I haz the power, 2015-03-26 21:07:15]

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

[commit-bot: I haz the power, 2015-03-26 21:07:17]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[aurimas (slooooooooow), 2015-03-26 21:10:49]

aurimas@chromium.org changed required reviewers:
- jln@chromium.org, qinmin@chromium.org

[aberent, 2015-03-26 21:19:24]

xhwang@ - Can you approve my new dependency on
content/common/media/media_player_messages_android.h so that I can then get a
rubber stamp from jln@ and land this? I was hoping scherkus@ would do so, but I
see he has a "very slow to review" message.

[xhwang, 2015-03-26 21:29:33]

xhwang@chromium.org changed reviewers:
+ xhwang@chromium.org

[xhwang, 2015-03-26 21:29:36]

deps on content/common/media/media_player_messages_android.h lgtm

https://codereview.chromium.org/928643003/diff/220001/chrome/browser/media/an...
File chrome/browser/media/android/remote/DEPS (right):

https://codereview.chromium.org/928643003/diff/220001/chrome/browser/media/an...
chrome/browser/media/android/remote/DEPS:7: # Fix layering violation
crbug.com/396828
Please add TODO and who will fix this :)

[jln (very slow on Chromium), 2015-03-26 22:07:26]

On 2015/03/26 21:29:36, xhwang wrote:
> deps on content/common/media/media_player_messages_android.h lgtm

lgtm rubberstamp

[aberent, 2015-03-27 09:02:06]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2015-03-27 09:02:19]

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

[commit-bot: I haz the power, 2015-03-27 10:51:02]

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

[commit-bot: I haz the power, 2015-03-27 10:51:04]

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_...)

[aberent, 2015-03-27 11:07:40]

The CQ bit was checked by aberent@chromium.org

[aberent, 2015-03-27 11:07:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, qinmin@chromium.org, aurimas@chromium.org,
yfriedman@chromium.org, jln@chromium.org, avayvod@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/928643003/#ps240001
(title: "Fix missing dependency")

[aberent, 2015-03-27 11:07:49]

The CQ bit was unchecked by aberent@chromium.org

[commit-bot: I haz the power, 2015-03-27 11:07:50]

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

[aberent, 2015-03-27 11:15:53]

The CQ bit was checked by aberent@chromium.org

[aberent, 2015-03-27 11:15:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, qinmin@chromium.org, aurimas@chromium.org,
yfriedman@chromium.org, jln@chromium.org, avayvod@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/928643003/#ps260001
(title: "Fix xhwang@'s nit")

[commit-bot: I haz the power, 2015-03-27 11:16:08]

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

[aberent, 2015-03-27 11:16:43]

https://codereview.chromium.org/928643003/diff/220001/chrome/browser/media/an...
File chrome/browser/media/android/remote/DEPS (right):

https://codereview.chromium.org/928643003/diff/220001/chrome/browser/media/an...
chrome/browser/media/android/remote/DEPS:7: # Fix layering violation
crbug.com/396828
On 2015/03/26 21:29:35, xhwang wrote:
> Please add TODO and who will fix this :)

Done.

[aberent, 2015-03-27 12:08:26]

The CQ bit was checked by aberent@chromium.org

[aberent, 2015-03-27 12:08:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, qinmin@chromium.org, aurimas@chromium.org,
yfriedman@chromium.org, jln@chromium.org, avayvod@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/928643003/#ps280001
(title: "Fix dependencies (second attempt).")

[commit-bot: I haz the power, 2015-03-27 12:08:39]

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

[commit-bot: I haz the power, 2015-03-27 13:37:13]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2015-03-27 13:38:06]

Patchset 14 (id:??) landed as
https://crrev.com/76097722a9e8577767a1d21a4a6b98ca322bfa7b
Cr-Commit-Position: refs/heads/master@{#322569}