Media Session API: use MediaMetadata in the browser process.

Media Session API: use MediaMetadata in the browser process.

This is adding a Java counterpart to the content::MediaMetadata added in Java
with a proxy to pass the object from C++ to Java.

The MediaMetadata information are only used to replace the title from the media
notification and set some Android metadata. Other UI changes will require UI
review.

BUG=497735, 581728
Committed: https://crrev.com/2d1bfbd36ef0362b28661a09d25f63a7d06fa9f6
Cr-Commit-Position: refs/heads/master@{#381697}

[mlamouri (slow - plz ping), 2015-12-11 16:13:24]

Description was changed from

==========
[WIP] Media Session API: add support for MediaSession.metadata.

This is a quick CL that was written on top of two WIP CLs. Nothing to
look at unless you work on MediaSession implementation :)

BUG=<TODO>

custom stuff
==========

to

==========
[WIP] Media Session API: use MediaMetadata in the browser process.

This is written on top of the following CLs:
https://codereview.chromium.org/1308983005/
https://codereview.chromium.org/1515623002

The code is WIP and there are some issues because the MediaSession management
in the browser process isn't finished yet.

BUG=497735
==========

[mlamouri (slow - plz ping), 2016-02-26 18:13:36]

Description was changed from

==========
[WIP] Media Session API: use MediaMetadata in the browser process.

This is written on top of the following CLs:
https://codereview.chromium.org/1308983005/
https://codereview.chromium.org/1515623002

The code is WIP and there are some issues because the MediaSession management
in the browser process isn't finished yet.

BUG=497735
==========

to

==========
[WIP] Media Session API: use MediaMetadata in the browser process.

This is written on top of the following CLs:
https://codereview.chromium.org/1308983005/
https://codereview.chromium.org/1515623002

The code is WIP and there are some issues because the MediaSession management
in the browser process isn't finished yet.

BUG=497735,581728
==========

[mlamouri (slow - plz ping), 2016-02-26 18:18:41]

Description was changed from

==========
[WIP] Media Session API: use MediaMetadata in the browser process.

This is written on top of the following CLs:
https://codereview.chromium.org/1308983005/
https://codereview.chromium.org/1515623002

The code is WIP and there are some issues because the MediaSession management
in the browser process isn't finished yet.

BUG=497735,581728
==========

to

==========
Media Session API: use MediaMetadata in the browser process.

This is adding a Java counterpart to the content::MediaMetadata added in Java
with a proxy to pass the object from C++ to Java.

The MediaMetadata information are only use to replace the title from the media
notification and set some Android metadata. Other UI changes will require UI
review.

BUG=497735,581728
==========

[mlamouri (slow - plz ping), 2016-02-26 18:26:34]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org, tedchoc@chromium.org

[mlamouri (slow - plz ping), 2016-02-26 18:26:35]

Could you PTAL at:

jochen@
 - content/browser/web_contents/
 - content/public/browser/

tedchoc@
 - everything else

Thanks! :)

[mlamouri (slow - plz ping), 2016-02-26 18:26:40]

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

[mlamouri (slow - plz ping), 2016-02-26 18:29:08]

+avayvod@, +philipj@, +davve@ FYI

[mlamouri (slow - plz ping), 2016-02-26 18:29:14]

Description was changed from

==========
Media Session API: use MediaMetadata in the browser process.

This is adding a Java counterpart to the content::MediaMetadata added in Java
with a proxy to pass the object from C++ to Java.

The MediaMetadata information are only use to replace the title from the media
notification and set some Android metadata. Other UI changes will require UI
review.

BUG=497735,581728
==========

to

==========
Media Session API: use MediaMetadata in the browser process.

This is adding a Java counterpart to the content::MediaMetadata added in Java
with a proxy to pass the object from C++ to Java.

The MediaMetadata information are only used to replace the title from the media
notification and set some Android metadata. Other UI changes will require UI
review.

BUG=497735,581728
==========

[commit-bot: I haz the power, 2016-02-26 18:29:18]

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

[commit-bot: I haz the power, 2016-02-26 19:01:58]

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

[commit-bot: I haz the power, 2016-02-26 19:01:59]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[jochen (gone - plz use gerrit), 2016-03-01 14:24:11]

lgtm

[Ted C, 2016-03-01 17:18:12]

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java
(right):

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:121:
mNotificationBuilder.setMetadata(new MediaMetadata(mTitle, "", ""));
why the empty string and not null?

Is there a requirement that the fields are non-null?

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:46:
private MediaMetadata mMetadata = null;
null is the default for objects, false for booleans, 0 for numbers.  There is no
need to specify them.

Don't need to change the others, but drop the null on this line at least.

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:563:
// Set ARTIST and ALBUM metadata if they were specified.
this comment seems superfluous...the code isn't that complex to need it (and
it's a bit confusing as it implies both are happening in the following block)

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:564:
if (!mMediaNotificationInfo.metadata.artist.isEmpty()) {
Use

TextUtils.isEmpty(mMediaNotificationInfo.metadata.artist)

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:40:
private MediaMetadata mFallbackMetadata = null;
same comment, no need for the null

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:105:
// The page's title is used as a placeholder if no title is
line length is 100 in java

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:107:
if (metadata.title.isEmpty()) {
TextUtils.isEmpty

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:109:
sanitizeMediaTitle(mTab.getTitle()),
+4 indent on the following lines.

https://codereview.chromium.org/1458703003/diff/80001/content/common/android/...
File content/common/android/media_metadata_proxy.h (right):

https://codereview.chromium.org/1458703003/diff/80001/content/common/android/...
content/common/android/media_metadata_proxy.h:16: class MediaMetadataProxy {
The common naming would be MediaMetadataAndroid.

I also wouldn't object making MediaMetadata a class that has this method on it
defined only for Android.  Having to create this second class is somewhat sad to
me.

https://codereview.chromium.org/1458703003/diff/80001/content/common/android/...
content/common/android/media_metadata_proxy.h:18: static
base::android::ScopedJavaLocalRef<jobject> GetJavaObject(
I would use CreateJavaObject, Get implies reuse to me

https://codereview.chromium.org/1458703003/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java
(right):

https://codereview.chromium.org/1458703003/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:20:
public String title;
Can these be final?

Not a big fan of mutable member variables, so if not please add accessors and
modifiers.

https://codereview.chromium.org/1458703003/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:45:
this.title = title;
from my comment earlier in the change, there is nothing stating that these can
not be null.  If you want to enforce that, I would add comments to artist,
album, etc... that specify it (or just add @NonNull)

Also, I would probably change the constructor to ensure it as well.

this.title = title == null ? "" : title;
...

For example, you check for null in hashCode but not in equals.

Granted if you don't care about these being null, then I would change the
earlier changes to pass null and then say what fields can be null.

https://codereview.chromium.org/1458703003/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:56:
return title.equals(other.title)
Use TextUtils.equals

[mlamouri (slow - plz ping), 2016-03-16 15:46:51]

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

[commit-bot: I haz the power, 2016-03-16 15:47:02]

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

[mlamouri (slow - plz ping), 2016-03-16 15:47:16]

Comments applied. PTAL.

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java
(right):

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:121:
mNotificationBuilder.setMetadata(new MediaMetadata(mTitle, "", ""));
On 2016/03/01 at 17:18:11, Ted C wrote:
> why the empty string and not null?
> 
> Is there a requirement that the fields are non-null?

The C++ class is using empty string as a default value so I would prefer to keep
that in the Java class.

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:46:
private MediaMetadata mMetadata = null;
On 2016/03/01 at 17:18:11, Ted C wrote:
> null is the default for objects, false for booleans, 0 for numbers.  There is
no need to specify them.
> 
> Don't need to change the others, but drop the null on this line at least.

Done. Though, for the record, I don't like implicit but that might be because
C++ doesn't do that :)

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:563:
// Set ARTIST and ALBUM metadata if they were specified.
On 2016/03/01 at 17:18:11, Ted C wrote:
> this comment seems superfluous...the code isn't that complex to need it (and
it's a bit confusing as it implies both are happening in the following block)

Done.

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:564:
if (!mMediaNotificationInfo.metadata.artist.isEmpty()) {
On 2016/03/01 at 17:18:11, Ted C wrote:
> Use
> 
> TextUtils.isEmpty(mMediaNotificationInfo.metadata.artist)

Done.

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:40:
private MediaMetadata mFallbackMetadata = null;
On 2016/03/01 at 17:18:11, Ted C wrote:
> same comment, no need for the null

Done.

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:105:
// The page's title is used as a placeholder if no title is
On 2016/03/01 at 17:18:11, Ted C wrote:
> line length is 100 in java

I'm aware of that :) Some people prefer comments to be wrapped at 80 regardless
of line length. Wrapped that to 100.

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:107:
if (metadata.title.isEmpty()) {
On 2016/03/01 at 17:18:11, Ted C wrote:
> TextUtils.isEmpty

Done.

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:109:
sanitizeMediaTitle(mTab.getTitle()),
On 2016/03/01 at 17:18:11, Ted C wrote:
> +4 indent on the following lines.

Done.

https://codereview.chromium.org/1458703003/diff/80001/content/common/android/...
File content/common/android/media_metadata_proxy.h (right):

https://codereview.chromium.org/1458703003/diff/80001/content/common/android/...
content/common/android/media_metadata_proxy.h:16: class MediaMetadataProxy {
On 2016/03/01 at 17:18:11, Ted C wrote:
> The common naming would be MediaMetadataAndroid.

Isn't that mostly used for android implementations of a class?

> I also wouldn't object making MediaMetadata a class that has this method on it
defined only for Android.  Having to create this second class is somewhat sad to
me.

The issue is that MediaMetadata is a content/public/ class so it would be
exposed to content embedders (ie. chrome/) while this proxy is only exposed to
content/. We could work around that by having MediaMetadata and
MediaMetadataImpl which is content/ only and have the CreateJavaObject() method
but that sounds like more boilerplate :)

https://codereview.chromium.org/1458703003/diff/80001/content/common/android/...
content/common/android/media_metadata_proxy.h:18: static
base::android::ScopedJavaLocalRef<jobject> GetJavaObject(
On 2016/03/01 at 17:18:11, Ted C wrote:
> I would use CreateJavaObject, Get implies reuse to me

Done.

https://codereview.chromium.org/1458703003/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java
(right):

https://codereview.chromium.org/1458703003/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:20:
public String title;
On 2016/03/01 at 17:18:11, Ted C wrote:
> Can these be final?
> 
> Not a big fan of mutable member variables, so if not please add accessors and
modifiers.

Hmm, I guess I tried to use this like a struct (ie. the C++ class is a struct).

Changed to be a proper class.

https://codereview.chromium.org/1458703003/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:45:
this.title = title;
On 2016/03/01 at 17:18:11, Ted C wrote:
> from my comment earlier in the change, there is nothing stating that these can
not be null.  If you want to enforce that, I would add comments to artist,
album, etc... that specify it (or just add @NonNull)
> 
> Also, I would probably change the constructor to ensure it as well.
> 
> this.title = title == null ? "" : title;
> ...
> 
> For example, you check for null in hashCode but not in equals.
> 
> Granted if you don't care about these being null, then I would change the
earlier changes to pass null and then say what fields can be null.

Done. Used @NonNull and checked values coming from JNI (@NonNull doesn't work on
@CalledByNative).

https://codereview.chromium.org/1458703003/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:56:
return title.equals(other.title)
On 2016/03/01 at 17:18:11, Ted C wrote:
> Use TextUtils.equals

Done.

[commit-bot: I haz the power, 2016-03-16 16:01:53]

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

[commit-bot: I haz the power, 2016-03-16 16:01:54]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mlamouri (slow - plz ping), 2016-03-16 16:31:57]

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

[commit-bot: I haz the power, 2016-03-16 16:32:14]

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

[Ted C, 2016-03-16 16:55:58]

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java
(right):

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:121:
mNotificationBuilder.setMetadata(new MediaMetadata(mTitle, "", ""));
On 2016/03/16 15:47:15, Mounir Lamouri wrote:
> On 2016/03/01 at 17:18:11, Ted C wrote:
> > why the empty string and not null?
> > 
> > Is there a requirement that the fields are non-null?
> 
> The C++ class is using empty string as a default value so I would prefer to
keep
> that in the Java class.

Acknowledged.

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://codereview.chromium.org/1458703003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:46:
private MediaMetadata mMetadata = null;
On 2016/03/16 15:47:15, Mounir Lamouri wrote:
> On 2016/03/01 at 17:18:11, Ted C wrote:
> > null is the default for objects, false for booleans, 0 for numbers.  There
is
> no need to specify them.
> > 
> > Don't need to change the others, but drop the null on this line at least.
> 
> Done. Though, for the record, I don't like implicit but that might be because
> C++ doesn't do that :)

I never accept the behavior of C++ as a reason to do that anywhere else ;-)

https://codereview.chromium.org/1458703003/diff/80001/content/common/android/...
File content/common/android/media_metadata_proxy.h (right):

https://codereview.chromium.org/1458703003/diff/80001/content/common/android/...
content/common/android/media_metadata_proxy.h:16: class MediaMetadataProxy {
On 2016/03/16 15:47:15, Mounir Lamouri wrote:
> On 2016/03/01 at 17:18:11, Ted C wrote:
> > The common naming would be MediaMetadataAndroid.
> 
> Isn't that mostly used for android implementations of a class?

Nope, web_contents_android, profile_android are prime examples where the android
class is just there to expose functionality to java.

_android usage:
https://code.google.com/p/chromium/codesearch#search/&q=file:.*_android%5C.h&...

_proxy usage:
https://code.google.com/p/chromium/codesearch#search/&q=file:.*_proxy.h%20fil...

While the _android.h is certainly muddled and can definitely mean the android
version of some class.  I still think it makes sense here more than proxy.

This isn't a proxy, it is actually just a class that creates java equivalent
versions.  MediaMetadataFactoryAndroid might be more accurate but more verbose
as well, so I still stand by that MediaMetadataAndroid is the best option for
this.

> 
> > I also wouldn't object making MediaMetadata a class that has this method on
it
> defined only for Android.  Having to create this second class is somewhat sad
to
> me.
> 
> The issue is that MediaMetadata is a content/public/ class so it would be
> exposed to content embedders (ie. chrome/) while this proxy is only exposed to
> content/. We could work around that by having MediaMetadata and
> MediaMetadataImpl which is content/ only and have the CreateJavaObject()
method
> but that sounds like more boilerplate :)

https://codereview.chromium.org/1458703003/diff/140001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java
(right):

https://codereview.chromium.org/1458703003/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:30:
* Returns the title associated with the media session.
for all of these, I'm happy if you remove either the top sentence or the @
values.  They are redundant and we (Chrome) are moving towards a world where we
happily drop one.  Typically the @return

https://codereview.chromium.org/1458703003/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:84:
artist == null ? "" : artist,
should be indented 8 from the start of the line.

If all of these three fit on a single line, then I would move them all down on
the second line indented by 8.

[commit-bot: I haz the power, 2016-03-16 17:40:32]

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

[commit-bot: I haz the power, 2016-03-16 17:40:33]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-03-16 20:31:55]

All comments applied. PTAL.

https://codereview.chromium.org/1458703003/diff/80001/content/common/android/...
File content/common/android/media_metadata_proxy.h (right):

https://codereview.chromium.org/1458703003/diff/80001/content/common/android/...
content/common/android/media_metadata_proxy.h:16: class MediaMetadataProxy {
On 2016/03/16 at 16:55:58, Ted C wrote:
> On 2016/03/16 15:47:15, Mounir Lamouri wrote:
> > On 2016/03/01 at 17:18:11, Ted C wrote:
> > > The common naming would be MediaMetadataAndroid.
> > 
> > Isn't that mostly used for android implementations of a class?
> 
> Nope, web_contents_android, profile_android are prime examples where the
android class is just there to expose functionality to java.
> 
> _android usage:
>
https://code.google.com/p/chromium/codesearch#search/&q=file:.*_android%5C.h&...
> 
> _proxy usage:
>
https://code.google.com/p/chromium/codesearch#search/&q=file:.*_proxy.h%20fil...
> 
> While the _android.h is certainly muddled and can definitely mean the android
version of some class.  I still think it makes sense here more than proxy.
> 
> This isn't a proxy, it is actually just a class that creates java equivalent
versions.  MediaMetadataFactoryAndroid might be more accurate but more verbose
as well, so I still stand by that MediaMetadataAndroid is the best option for
this.

Ok. Done :)

https://codereview.chromium.org/1458703003/diff/140001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java
(right):

https://codereview.chromium.org/1458703003/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:30:
* Returns the title associated with the media session.
On 2016/03/16 at 16:55:58, Ted C wrote:
> for all of these, I'm happy if you remove either the top sentence or the @
values.  They are redundant and we (Chrome) are moving towards a world where we
happily drop one.  Typically the @return

Done.

https://codereview.chromium.org/1458703003/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:84:
artist == null ? "" : artist,
On 2016/03/16 at 16:55:58, Ted C wrote:
> should be indented 8 from the start of the line.

Done.

> If all of these three fit on a single line, then I would move them all down on
the second line indented by 8.

Unfortunately not.

[mlamouri (slow - plz ping), 2016-03-16 20:33:55]

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

[commit-bot: I haz the power, 2016-03-16 20:34:45]

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

[commit-bot: I haz the power, 2016-03-16 21:51:30]

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

[commit-bot: I haz the power, 2016-03-16 21:51:32]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-03-16 23:13:14]

lgtm w/ a couple last nits

https://codereview.chromium.org/1458703003/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java
(right):

https://codereview.chromium.org/1458703003/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:74:
/*
add another * to make it a javadoc

https://codereview.chromium.org/1458703003/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:75:
* Creates a new MediaMetadat from the C++ code. This is exactly like the
Missing an [a] after MediaMetadat

[mlamouri (slow - plz ping), 2016-03-17 11:47:53]

https://codereview.chromium.org/1458703003/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java
(right):

https://codereview.chromium.org/1458703003/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:74:
/*
On 2016/03/16 at 23:13:14, Ted C wrote:
> add another * to make it a javadoc

Done.

https://codereview.chromium.org/1458703003/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:75:
* Creates a new MediaMetadat from the C++ code. This is exactly like the
On 2016/03/16 at 23:13:14, Ted C wrote:
> Missing an [a] after MediaMetadat

Done.

[mlamouri (slow - plz ping), 2016-03-17 11:48:14]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-03-17 11:48:15]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1458703003/#ps200001
(title: "nits")

[commit-bot: I haz the power, 2016-03-17 11:48:30]

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

[commit-bot: I haz the power, 2016-03-17 13:15:03]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-03-17 13:16:08]

Description was changed from

==========
Media Session API: use MediaMetadata in the browser process.

This is adding a Java counterpart to the content::MediaMetadata added in Java
with a proxy to pass the object from C++ to Java.

The MediaMetadata information are only used to replace the title from the media
notification and set some Android metadata. Other UI changes will require UI
review.

BUG=497735,581728
==========

to

==========
Media Session API: use MediaMetadata in the browser process.

This is adding a Java counterpart to the content::MediaMetadata added in Java
with a proxy to pass the object from C++ to Java.

The MediaMetadata information are only used to replace the title from the media
notification and set some Android metadata. Other UI changes will require UI
review.

BUG=497735,581728

Committed: https://crrev.com/2d1bfbd36ef0362b28661a09d25f63a7d06fa9f6
Cr-Commit-Position: refs/heads/master@{#381697}
==========

[commit-bot: I haz the power, 2016-03-17 13:16:10]

Patchset 11 (id:??) landed as
https://crrev.com/2d1bfbd36ef0362b28661a09d25f63a7d06fa9f6
Cr-Commit-Position: refs/heads/master@{#381697}