Adding new media controls to MediaNotification

Adding media controls to MediaNotification

This CL adds new media controls to MediaNotification. The controls are enabled
from the MediaSession API in blink, and is propagated to the browser side
through mojom MediaSessionService. Then we show a corresponding button in media
notification. When the user clicks the button, the event will be propagate back
to the page through mojom MediaSessionClient.

This CL has the following changes:
* Plumb media control messages from content/ to chrome/ Java.
* Add control buttons to MediaNotification

Slides explaining how media controls work:
https://docs.google.com/a/google.com/presentation/d/1jGNB9aT3mbpPszC3SiQnQ70CjVDmH-haomHCrqSVVAw/edit?usp=sharing

Test URL:
http://xxyzzzq.github.io/sandbox/media-session/full-test.html

BUG=656563
TEST=MANUAL

Committed: https://crrev.com/fb90347c0ad1437e9fa674e7c70db9512b570444
Cr-Commit-Position: refs/heads/master@{#429614}

[Zhiqiang Zhang (Slow), 2016-10-24 10:47:08]

Patchset #1 (id:1) has been deleted

[Zhiqiang Zhang (Slow), 2016-10-24 10:50:53]

zqzhang@chromium.org changed reviewers:
+ avayvod@chromium.org, boliu@chromium.org, jochen@chromium.org,
mlamouri@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-24 10:50:54]

+avayvod/mlamouri for media/session review
+jochen for content/ review
+boliu for JNI review

[boliu, 2016-10-24 17:02:47]

I looked at the whole thing. Pretty straightforward and had some small comments.

Plumbing all of these through WebContents[Observer] is odd, but you have plans
to refactor this already.

The other odd thing is adding APIs that are unused. Not sure how other owners
feel about this, but generally I'd like to not add things and not use them,
because then when you actually write the CL to use them, the APIs you left
unused may need to be changed again.

https://codereview.chromium.org/2442303002/diff/20001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.cc (right):

https://codereview.chromium.org/2442303002/diff/20001/content/browser/android...
content/browser/android/web_contents_observer_proxy.cc:348: 
nit: maybe could remove these empty lines? very much personal preference though

https://codereview.chromium.org/2442303002/diff/20001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/2442303002/diff/20001/content/public/browser/...
content/public/browser/web_contents.h:46: namespace mojom {
put this in the namespace above?

https://codereview.chromium.org/2442303002/diff/20001/content/public/browser/...
content/public/browser/web_contents.h:722: virtual void
DidReceiveMediaSessionAction(
Is this only going to be called from WebContentsAndroid? If so, it doesn't need
to be in the public API.

[Zhiqiang Zhang (Slow), 2016-10-24 17:30:21]

Description was changed from

==========
Plumbing media control messages through WebContents and Java

This CL plumbs media control messages of MediaSessionService through
WebContents and Java. These messages will be used by
MediaSessionTabHelper in follow up CL.

The plumbing should be avoided by decoupling MediaSession and
WebContents, see https://crbug.com/658678

BUG=656563
==========

to

==========
Plumbing media control messages through WebContents and Java

This CL plumbs media control messages of MediaSessionService through
WebContents and Java. These messages will be used by
MediaSessionTabHelper in follow up CL:
https://codereview.chromium.org/2441403002/

The plumbing should be avoided by decoupling MediaSession and
WebContents, see https://crbug.com/658678

BUG=656563
==========

[Zhiqiang Zhang (Slow), 2016-10-24 17:33:10]

On 2016/10/24 17:02:47, boliu wrote:
> The other odd thing is adding APIs that are unused. Not sure how other owners
> feel about this, but generally I'd like to not add things and not use them,
> because then when you actually write the CL to use them, the APIs you left
> unused may need to be changed again.

Yes, I've put the follow-up CL that uses the APIs in the description. I think
there shouldn't be much changes to the API, plus the fact that they are
temporary and will be removed in the refactoring work.

https://codereview.chromium.org/2442303002/diff/20001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.cc (right):

https://codereview.chromium.org/2442303002/diff/20001/content/browser/android...
content/browser/android/web_contents_observer_proxy.cc:348: 
On 2016/10/24 17:02:46, boliu wrote:
> nit: maybe could remove these empty lines? very much personal preference
though

Done.

https://codereview.chromium.org/2442303002/diff/20001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/2442303002/diff/20001/content/public/browser/...
content/public/browser/web_contents.h:46: namespace mojom {
On 2016/10/24 17:02:47, boliu wrote:
> put this in the namespace above?

No need for this since it's not in the public API.

https://codereview.chromium.org/2442303002/diff/20001/content/public/browser/...
content/public/browser/web_contents.h:722: virtual void
DidReceiveMediaSessionAction(
On 2016/10/24 17:02:46, boliu wrote:
> Is this only going to be called from WebContentsAndroid? If so, it doesn't
need
> to be in the public API.

Thanks. Removed.

[boliu, 2016-10-24 17:44:20]

my parts lgtm

Again, I would just pull in the follow up CL into this one and have them be
reviewed together. But you can defer this to the media owners.

I think should probably wait for the follow up CL to be approved before landing
this.

[whywhat, 2016-10-24 21:01:26]

lgtm % nits / questions

https://codereview.chromium.org/2442303002/diff/40001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.h (right):

https://codereview.chromium.org/2442303002/diff/40001/content/browser/android...
content/browser/android/web_contents_observer_proxy.h:81: void
MediaSessionEnabledAction(
hm, what's the plan of getting rid of those?
is it because you're waiting on the Chromecast changes?
I'd say they will have to fix it in their downstream tree when rolling Chromium
so you could remove WebContentsObserver at the same time you add MSO?

https://codereview.chromium.org/2442303002/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session.h (right):

https://codereview.chromium.org/2442303002/diff/40001/content/browser/media/s...
content/browser/media/session/media_session.h:56: class MediaSession : public
WebContentsObserver,
hm, how does this relate to the MediaSession to MediaSessionImpl CL?

https://codereview.chromium.org/2442303002/diff/40001/content/browser/media/s...
content/browser/media/session/media_session.h:121: // Called when an action is
enabled in blink::MediaSession and notify
nit: "Called ... and notify ..." is not a valid syntax structure I believe.
Maybe s/Called/call as a quick fix?

https://codereview.chromium.org/2442303002/diff/40001/content/browser/media/s...
content/browser/media/session/media_session.h:125: // Called when an action is
disabled in blink::MediaSession and notify
ditto

[mlamouri (slow - plz ping), 2016-10-25 10:27:38]

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

[mlamouri (slow - plz ping), 2016-10-25 10:27:38]

Removing self from reviewer list. Please add me back if you need me to review
something in particular.

[Zhiqiang Zhang (Slow), 2016-10-25 10:46:56]

https://codereview.chromium.org/2442303002/diff/40001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.h (right):

https://codereview.chromium.org/2442303002/diff/40001/content/browser/android...
content/browser/android/web_contents_observer_proxy.h:81: void
MediaSessionEnabledAction(
On 2016/10/24 21:01:25, whywhat wrote:
> hm, what's the plan of getting rid of those?
> is it because you're waiting on the Chromecast changes?
> I'd say they will have to fix it in their downstream tree when rolling
Chromium
> so you could remove WebContentsObserver at the same time you add MSO?

Yes, the plan is to get rid of these from both WebContents and
WebContentsObserver.

https://codereview.chromium.org/2442303002/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session.h (right):

https://codereview.chromium.org/2442303002/diff/40001/content/browser/media/s...
content/browser/media/session/media_session.h:56: class MediaSession : public
WebContentsObserver,
On 2016/10/24 21:01:25, whywhat wrote:
> hm, how does this relate to the MediaSession to MediaSessionImpl CL?

It's totally separate. Depending on which one is landed first, the other one
will need to rebase. The rebasing should be simple, just move the MediaSession
observer functions and MediaSession Resume/Suspend/Stop functions to proper
place.

https://codereview.chromium.org/2442303002/diff/40001/content/browser/media/s...
content/browser/media/session/media_session.h:121: // Called when an action is
enabled in blink::MediaSession and notify
On 2016/10/24 21:01:26, whywhat wrote:
> nit: "Called ... and notify ..." is not a valid syntax structure I believe.
> Maybe s/Called/call as a quick fix?

Done.

https://codereview.chromium.org/2442303002/diff/40001/content/browser/media/s...
content/browser/media/session/media_session.h:125: // Called when an action is
disabled in blink::MediaSession and notify
On 2016/10/24 21:01:26, whywhat wrote:
> ditto

Done.

[Zhiqiang Zhang (Slow), 2016-10-25 16:39:29]

zqzhang@chromium.org changed reviewers:
+ jam@chromium.org
- jochen@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-25 16:39:29]

Adding jam@ to review the content/ changes (since jochen's review queue is
crazy, and jam@ is reviewing the MediaSession refactoring CL so he should better
know the context)

[Zhiqiang Zhang (Slow), 2016-10-25 16:53:14]

On 2016/10/25 16:39:29, Zhiqiang Zhang wrote:
> Adding jam@ to review the content/ changes (since jochen's review queue is
> crazy, and jam@ is reviewing the MediaSession refactoring CL so he should
better
> know the context)

BTW, this CL is an example showing why we need to do the MediaSession
refactoring. The plumbing is everywhere in WebContents now.
This CL and the refactoring CL conflicts. If one of them is landed, I'll rebase
the other.

[Zhiqiang Zhang (Slow), 2016-10-25 18:55:00]

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

[commit-bot: I haz the power, 2016-10-25 18:55:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[whywhat, 2016-10-25 19:21:08]

On 2016/10/25 at 16:53:14, zqzhang wrote:
> On 2016/10/25 16:39:29, Zhiqiang Zhang wrote:
> > Adding jam@ to review the content/ changes (since jochen's review queue is
> > crazy, and jam@ is reviewing the MediaSession refactoring CL so he should
better
> > know the context)
> 
> BTW, this CL is an example showing why we need to do the MediaSession
refactoring. The plumbing is everywhere in WebContents now.
> This CL and the refactoring CL conflicts. If one of them is landed, I'll
rebase the other.

You feel a bit rushed to land these :) Why not rebase this on top of the
refactoring one instead of having two parallel CLs?

[commit-bot: I haz the power, 2016-10-25 20:15:00]

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

[commit-bot: I haz the power, 2016-10-25 20:15:01]

Dry run: This issue passed the CQ dry run.

[Zhiqiang Zhang (Slow), 2016-10-26 09:56:31]

On 2016/10/25 19:21:08, whywhat wrote:
> On 2016/10/25 at 16:53:14, zqzhang wrote:
> > On 2016/10/25 16:39:29, Zhiqiang Zhang wrote:
> > > Adding jam@ to review the content/ changes (since jochen's review queue is
> > > crazy, and jam@ is reviewing the MediaSession refactoring CL so he should
> better
> > > know the context)
> > 
> > BTW, this CL is an example showing why we need to do the MediaSession
> refactoring. The plumbing is everywhere in WebContents now.
> > This CL and the refactoring CL conflicts. If one of them is landed, I'll
> rebase the other.
> 
> You feel a bit rushed to land these :) Why not rebase this on top of the
> refactoring one instead of having two parallel CLs?

Yeah, I was to rushing. Let's wait for the refactoring to land :)

[Zhiqiang Zhang (Slow), 2016-10-28 15:06:50]

PTAL

Just rebased this CL onto the "MediaSession refactoring in Java" CL.

The new diffs are pretty much the same so it's easy to review.

[whywhat, 2016-10-28 16:10:23]

lgtm % more nits

https://codereview.chromium.org/2442303002/diff/80001/content/browser/media/s...
File content/browser/media/session/media_session_impl.cc (right):

https://codereview.chromium.org/2442303002/diff/80001/content/browser/media/s...
content/browser/media/session/media_session_impl.cc:111: service_ = service;
nit: I assume this is supposed to be called only once? Otherwise should we do
something with the previous service?
If it's the former, maybe add a DCHECK that service_ is nullptr?

https://codereview.chromium.org/2442303002/diff/80001/content/browser/media/s...
content/browser/media/session/media_session_impl.cc:287: if (service_)
nit: Is it possible to have service_ nullptr?

https://codereview.chromium.org/2442303002/diff/80001/content/browser/media/s...
File content/browser/media/session/media_session_service_impl.cc (right):

https://codereview.chromium.org/2442303002/diff/80001/content/browser/media/s...
content/browser/media/session/media_session_service_impl.cc:23: if
(MediaSessionImpl* session = GetMediaSession())
nit: can this be null here and below?
I'd avoid initialization in the if () condition as it may confuse readers
(although it doesn't seem to be forbidden in the style guide and there're about
250 existing cases I found:
https://cs.chromium.org/search/?q=file:%5C.cc+if%5Cs%5C(%5B%5E)%22%5D*?%5Cs%3...

https://codereview.chromium.org/2442303002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java
(right):

https://codereview.chromium.org/2442303002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java:88:
for (mObserversIterator.rewind(); mObserversIterator.hasNext();) {
nit: add a space between ; and ) to highlight there's no statement there?

https://codereview.chromium.org/2442303002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java:95:
for (mObserversIterator.rewind(); mObserversIterator.hasNext();) {
nit: ditto

[Zhiqiang Zhang (Slow), 2016-11-01 12:34:57]

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

[commit-bot: I haz the power, 2016-11-01 12:35:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 13:20:57]

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

[commit-bot: I haz the power, 2016-11-01 13:20:58]

Dry run: This issue passed the CQ dry run.

[Zhiqiang Zhang (Slow), 2016-11-01 15:03:50]

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

[commit-bot: I haz the power, 2016-11-01 15:04:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhiqiang Zhang (Slow), 2016-11-01 15:13:19]

Description was changed from

==========
Plumbing media control messages through WebContents and Java

This CL plumbs media control messages of MediaSessionService through
WebContents and Java. These messages will be used by
MediaSessionTabHelper in follow up CL:
https://codereview.chromium.org/2441403002/

The plumbing should be avoided by decoupling MediaSession and
WebContents, see https://crbug.com/658678

BUG=656563
==========

to

==========
Adding media control to MediaNotification

This CL adds new media controls to MediaNotification. The controls are enabled
from the MediaSession API in blink, and is propagated to the browser side
through mojom MediaSessionService. Then we show a corresponding button in media
notification. When the user clicks the button, the event will be propagate back
to the page through mojom MediaSessionClient.

This CL has the following changes:
* Plumb media control messages from content/ to chrome/ Java.
* Add control buttons to MediaNotification

BUG=656563
==========

[Zhiqiang Zhang (Slow), 2016-11-01 15:17:41]

Description was changed from

==========
Adding media control to MediaNotification

This CL adds new media controls to MediaNotification. The controls are enabled
from the MediaSession API in blink, and is propagated to the browser side
through mojom MediaSessionService. Then we show a corresponding button in media
notification. When the user clicks the button, the event will be propagate back
to the page through mojom MediaSessionClient.

This CL has the following changes:
* Plumb media control messages from content/ to chrome/ Java.
* Add control buttons to MediaNotification

BUG=656563
==========

to

==========
Adding media controls to MediaNotification

This CL adds new media controls to MediaNotification. The controls are enabled
from the MediaSession API in blink, and is propagated to the browser side
through mojom MediaSessionService. Then we show a corresponding button in media
notification. When the user clicks the button, the event will be propagate back
to the page through mojom MediaSessionClient.

This CL has the following changes:
* Plumb media control messages from content/ to chrome/ Java.
* Add control buttons to MediaNotification

BUG=656563
==========

[jam, 2016-11-01 15:20:56]

On 2016/10/25 16:39:29, Zhiqiang Zhang wrote:
> Adding jam@ to review the content/ changes (since jochen's review queue is
> crazy, and jam@ is reviewing the MediaSession refactoring CL so he should
better
> know the context)

Sorry I just saw this. lgtm for content/public

please get owners review from content/public/android/OWNERS and
content/browser/media/owners

[Zhiqiang Zhang (Slow), 2016-11-01 15:24:15]

https://codereview.chromium.org/2442303002/diff/80001/content/browser/media/s...
File content/browser/media/session/media_session_impl.cc (right):

https://codereview.chromium.org/2442303002/diff/80001/content/browser/media/s...
content/browser/media/session/media_session_impl.cc:111: service_ = service;
On 2016/10/28 16:10:23, whywhat wrote:
> nit: I assume this is supposed to be called only once? Otherwise should we do
> something with the previous service?
> If it's the former, maybe add a DCHECK that service_ is nullptr?

It could be set and unset.
My point is the service is created by RenderFrameHost, so if the RenderFrame
goes away, the service should be destroyed. However I'm not very sure if
WebContents/MediaSession will be destroyed at that moment. So let's be safe.

https://codereview.chromium.org/2442303002/diff/80001/content/browser/media/s...
content/browser/media/session/media_session_impl.cc:287: if (service_)
On 2016/10/28 16:10:23, whywhat wrote:
> nit: Is it possible to have service_ nullptr? 

See my other reply.

https://codereview.chromium.org/2442303002/diff/80001/content/browser/media/s...
File content/browser/media/session/media_session_service_impl.cc (right):

https://codereview.chromium.org/2442303002/diff/80001/content/browser/media/s...
content/browser/media/session/media_session_service_impl.cc:23: if
(MediaSessionImpl* session = GetMediaSession())
On 2016/10/28 16:10:23, whywhat wrote:
> nit: can this be null here and below?
Yes, currently we only bind the top-level frame service with MediaSession, so
GetMediaSession() will return null for child frames.

> I'd avoid initialization in the if () condition as it may confuse readers
> (although it doesn't seem to be forbidden in the style guide and there're
about
> 250 existing cases I found:
>
https://cs.chromium.org/search/?q=file:%5C.cc+if%5Cs%5C(%5B%5E)%22%5D*?%5Cs%3...

Done for the style.

Actually I should also check if |this| == session->service_ here, fixed in the
new patch.

https://codereview.chromium.org/2442303002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java
(right):

https://codereview.chromium.org/2442303002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java:88:
for (mObserversIterator.rewind(); mObserversIterator.hasNext();) {
On 2016/10/28 16:10:23, whywhat wrote:
> nit: add a space between ; and ) to highlight there's no statement there?

Actually `git cl format` don't like the space :/

https://codereview.chromium.org/2442303002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java:95:
for (mObserversIterator.rewind(); mObserversIterator.hasNext();) {
On 2016/10/28 16:10:23, whywhat wrote:
> nit: ditto

See reply above.

[boliu, 2016-11-01 15:27:08]

java lgtm % javadoc

https://codereview.chromium.org/2442303002/diff/140001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java
(right):

https://codereview.chromium.org/2442303002/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:73:
* {@link MediaSessionObserver#getMediaSession} will return null after this
method is called.
what's with this javadoc..? bad conflict resolution?

[Zhiqiang Zhang (Slow), 2016-11-01 15:29:49]

zqzhang@chromium.org changed reviewers:
+ kenrb@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-01 15:29:50]

I merged the chrome/android/ side of the patch, so it's easier to do a security
review.

kenrb@, can you take a look?

We have an earlier CL (https://chromiumcodereview.appspot.com/2426653002) that
needs a security review (due to mojom changes), but that CL wasn't thoroughly
reviewed since we haven't done any user-side change yet. This CL is the
user-side change so should ready to be reviewed now.

[Zhiqiang Zhang (Slow), 2016-11-01 15:39:35]

zqzhang@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-01 15:39:36]

+tedchoc@ for reviewing:
* The new assets (already optimized)
* android_chrome_strings.grd

https://codereview.chromium.org/2442303002/diff/140001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java
(right):

https://codereview.chromium.org/2442303002/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:73:
* {@link MediaSessionObserver#getMediaSession} will return null after this
method is called.
On 2016/11/01 15:27:07, boliu wrote:
> what's with this javadoc..? bad conflict resolution?

Yes... It's fixed now.

[Zhiqiang Zhang (Slow), 2016-11-01 15:42:22]

Description was changed from

==========
Adding media controls to MediaNotification

This CL adds new media controls to MediaNotification. The controls are enabled
from the MediaSession API in blink, and is propagated to the browser side
through mojom MediaSessionService. Then we show a corresponding button in media
notification. When the user clicks the button, the event will be propagate back
to the page through mojom MediaSessionClient.

This CL has the following changes:
* Plumb media control messages from content/ to chrome/ Java.
* Add control buttons to MediaNotification

BUG=656563
==========

to

==========
Adding media controls to MediaNotification

This CL adds new media controls to MediaNotification. The controls are enabled
from the MediaSession API in blink, and is propagated to the browser side
through mojom MediaSessionService. Then we show a corresponding button in media
notification. When the user clicks the button, the event will be propagate back
to the page through mojom MediaSessionClient.

This CL has the following changes:
* Plumb media control messages from content/ to chrome/ Java.
* Add control buttons to MediaNotification

Test URL:
http://xxyzzzq.github.io/sandbox/media-session/full-test.html

BUG=656563
TEST=MANUAL
==========

[Ted C, 2016-11-01 17:34:09]

https://codereview.chromium.org/2442303002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionAction.java
(right):

https://codereview.chromium.org/2442303002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionAction.java:8:
* Java counterpart to native blink::mojom::MediaSessionAction enum.
Aren't these generated for you by default as part of mojoms?

jar tvf
out/Debug/lib.java/third_party/WebKit/public/new_wrapper_types_mojo_bindings_java.jar
| grep MediaSessionAction

org/chromium/blink/mojom/MediaSessionAction

[Zhiqiang Zhang (Slow), 2016-11-01 18:08:31]

https://codereview.chromium.org/2442303002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionAction.java
(right):

https://codereview.chromium.org/2442303002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionAction.java:8:
* Java counterpart to native blink::mojom::MediaSessionAction enum.
On 2016/11/01 17:34:09, Ted C wrote:
> Aren't these generated for you by default as part of mojoms?
> 
> jar tvf
>
out/Debug/lib.java/third_party/WebKit/public/new_wrapper_types_mojo_bindings_java.jar
> | grep MediaSessionAction
> 
> org/chromium/blink/mojom/MediaSessionAction

Ah, yes! This is what we've been looking for. Done.

[Ted C, 2016-11-01 19:51:54]

On 2016/11/01 18:08:31, Zhiqiang Zhang wrote:
>
https://codereview.chromium.org/2442303002/diff/160001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionAction.java
> (right):
> 
>
https://codereview.chromium.org/2442303002/diff/160001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionAction.java:8:
> * Java counterpart to native blink::mojom::MediaSessionAction enum.
> On 2016/11/01 17:34:09, Ted C wrote:
> > Aren't these generated for you by default as part of mojoms?
> > 
> > jar tvf
> >
>
out/Debug/lib.java/third_party/WebKit/public/new_wrapper_types_mojo_bindings_java.jar
> > | grep MediaSessionAction
> > 
> > org/chromium/blink/mojom/MediaSessionAction
> 
> Ah, yes! This is what we've been looking for. Done.

I don't see a new patch set.  Did you by chance forget to upload?

[Ted C, 2016-11-01 19:52:30]

On 2016/11/01 19:51:54, Ted C wrote:
> On 2016/11/01 18:08:31, Zhiqiang Zhang wrote:
> >
>
https://codereview.chromium.org/2442303002/diff/160001/chrome/android/java/sr...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionAction.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2442303002/diff/160001/chrome/android/java/sr...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionAction.java:8:
> > * Java counterpart to native blink::mojom::MediaSessionAction enum.
> > On 2016/11/01 17:34:09, Ted C wrote:
> > > Aren't these generated for you by default as part of mojoms?
> > > 
> > > jar tvf
> > >
> >
>
out/Debug/lib.java/third_party/WebKit/public/new_wrapper_types_mojo_bindings_java.jar
> > > | grep MediaSessionAction
> > > 
> > > org/chromium/blink/mojom/MediaSessionAction
> > 
> > Ah, yes! This is what we've been looking for. Done.
> 
> I don't see a new patch set.  Did you by chance forget to upload?

Regardless, assets and string changes look fine to me.  I assume you'll get the
new version uploaded before submitting.

lgtm

[Zhiqiang Zhang (Slow), 2016-11-01 20:30:53]

On 2016/11/01 19:51:54, Ted C wrote:
> On 2016/11/01 18:08:31, Zhiqiang Zhang wrote:
> >
>
https://codereview.chromium.org/2442303002/diff/160001/chrome/android/java/sr...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionAction.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2442303002/diff/160001/chrome/android/java/sr...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionAction.java:8:
> > * Java counterpart to native blink::mojom::MediaSessionAction enum.
> > On 2016/11/01 17:34:09, Ted C wrote:
> > > Aren't these generated for you by default as part of mojoms?
> > > 
> > > jar tvf
> > >
> >
>
out/Debug/lib.java/third_party/WebKit/public/new_wrapper_types_mojo_bindings_java.jar
> > > | grep MediaSessionAction
> > > 
> > > org/chromium/blink/mojom/MediaSessionAction
> > 
> > Ah, yes! This is what we've been looking for. Done.
> 
> I don't see a new patch set.  Did you by chance forget to upload?

Yes, I forgot it :/
Just uploaded, PTAL (there's dependency addition for chrome_java in BUILD.gn)

[Ted C, 2016-11-01 20:39:26]

On 2016/11/01 20:30:53, Zhiqiang Zhang wrote:
> On 2016/11/01 19:51:54, Ted C wrote:
> > On 2016/11/01 18:08:31, Zhiqiang Zhang wrote:
> > >
> >
>
https://codereview.chromium.org/2442303002/diff/160001/chrome/android/java/sr...
> > > File
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionAction.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2442303002/diff/160001/chrome/android/java/sr...
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionAction.java:8:
> > > * Java counterpart to native blink::mojom::MediaSessionAction enum.
> > > On 2016/11/01 17:34:09, Ted C wrote:
> > > > Aren't these generated for you by default as part of mojoms?
> > > > 
> > > > jar tvf
> > > >
> > >
> >
>
out/Debug/lib.java/third_party/WebKit/public/new_wrapper_types_mojo_bindings_java.jar
> > > > | grep MediaSessionAction
> > > > 
> > > > org/chromium/blink/mojom/MediaSessionAction
> > > 
> > > Ah, yes! This is what we've been looking for. Done.
> > 
> > I don't see a new patch set.  Did you by chance forget to upload?
> 
> Yes, I forgot it :/
> Just uploaded, PTAL (there's dependency addition for chrome_java in BUILD.gn)

BUILD.gn changes lgtm

[Zhiqiang Zhang (Slow), 2016-11-02 13:58:33]

Description was changed from

==========
Adding media controls to MediaNotification

This CL adds new media controls to MediaNotification. The controls are enabled
from the MediaSession API in blink, and is propagated to the browser side
through mojom MediaSessionService. Then we show a corresponding button in media
notification. When the user clicks the button, the event will be propagate back
to the page through mojom MediaSessionClient.

This CL has the following changes:
* Plumb media control messages from content/ to chrome/ Java.
* Add control buttons to MediaNotification

Test URL:
http://xxyzzzq.github.io/sandbox/media-session/full-test.html

BUG=656563
TEST=MANUAL
==========

to

==========
Adding media controls to MediaNotification

This CL adds new media controls to MediaNotification. The controls are enabled
from the MediaSession API in blink, and is propagated to the browser side
through mojom MediaSessionService. Then we show a corresponding button in media
notification. When the user clicks the button, the event will be propagate back
to the page through mojom MediaSessionClient.

This CL has the following changes:
* Plumb media control messages from content/ to chrome/ Java.
* Add control buttons to MediaNotification

Slides explaining how media controls work:
https://docs.google.com/a/google.com/presentation/d/1jGNB9aT3mbpPszC3SiQnQ70C...

Test URL:
http://xxyzzzq.github.io/sandbox/media-session/full-test.html

BUG=656563
TEST=MANUAL
==========

[Zhiqiang Zhang (Slow), 2016-11-02 13:59:12]

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

[commit-bot: I haz the power, 2016-11-02 13:59:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 16:14:25]

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

[commit-bot: I haz the power, 2016-11-02 16:14:27]

Dry run: This issue passed the CQ dry run.

[kenrb, 2016-11-02 16:51:33]

security lgtm

[Zhiqiang Zhang (Slow), 2016-11-03 11:04:18]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-03 11:04:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, jam@chromium.org, boliu@chromium.org, kenrb@chromium.org,
tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2442303002/#ps200001
(title: "rebased")

[commit-bot: I haz the power, 2016-11-03 11:04:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 12:41:42]

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

[commit-bot: I haz the power, 2016-11-03 12:41:43]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Zhiqiang Zhang (Slow), 2016-11-03 13:11:41]

The CQ bit was checked by zqzhang@chromium.org

[commit-bot: I haz the power, 2016-11-03 13:12:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Description was changed from

==========
Adding media controls to MediaNotification

This CL adds new media controls to MediaNotification. The controls are enabled
from the MediaSession API in blink, and is propagated to the browser side
through mojom MediaSessionService. Then we show a corresponding button in media
notification. When the user clicks the button, the event will be propagate back
to the page through mojom MediaSessionClient.

This CL has the following changes:
* Plumb media control messages from content/ to chrome/ Java.
* Add control buttons to MediaNotification

Slides explaining how media controls work:
https://docs.google.com/a/google.com/presentation/d/1jGNB9aT3mbpPszC3SiQnQ70C...

Test URL:
http://xxyzzzq.github.io/sandbox/media-session/full-test.html

BUG=656563
TEST=MANUAL
==========

to

==========
Adding media controls to MediaNotification

This CL adds new media controls to MediaNotification. The controls are enabled
from the MediaSession API in blink, and is propagated to the browser side
through mojom MediaSessionService. Then we show a corresponding button in media
notification. When the user clicks the button, the event will be propagate back
to the page through mojom MediaSessionClient.

This CL has the following changes:
* Plumb media control messages from content/ to chrome/ Java.
* Add control buttons to MediaNotification

Slides explaining how media controls work:
https://docs.google.com/a/google.com/presentation/d/1jGNB9aT3mbpPszC3SiQnQ70C...

Test URL:
http://xxyzzzq.github.io/sandbox/media-session/full-test.html

BUG=656563
TEST=MANUAL
==========

[commit-bot: I haz the power, 2016-11-03 16:34:21]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-11-03 16:43:36]

Description was changed from

==========
Adding media controls to MediaNotification

This CL adds new media controls to MediaNotification. The controls are enabled
from the MediaSession API in blink, and is propagated to the browser side
through mojom MediaSessionService. Then we show a corresponding button in media
notification. When the user clicks the button, the event will be propagate back
to the page through mojom MediaSessionClient.

This CL has the following changes:
* Plumb media control messages from content/ to chrome/ Java.
* Add control buttons to MediaNotification

Slides explaining how media controls work:
https://docs.google.com/a/google.com/presentation/d/1jGNB9aT3mbpPszC3SiQnQ70C...

Test URL:
http://xxyzzzq.github.io/sandbox/media-session/full-test.html

BUG=656563
TEST=MANUAL
==========

to

==========
Adding media controls to MediaNotification

This CL adds new media controls to MediaNotification. The controls are enabled
from the MediaSession API in blink, and is propagated to the browser side
through mojom MediaSessionService. Then we show a corresponding button in media
notification. When the user clicks the button, the event will be propagate back
to the page through mojom MediaSessionClient.

This CL has the following changes:
* Plumb media control messages from content/ to chrome/ Java.
* Add control buttons to MediaNotification

Slides explaining how media controls work:
https://docs.google.com/a/google.com/presentation/d/1jGNB9aT3mbpPszC3SiQnQ70C...

Test URL:
http://xxyzzzq.github.io/sandbox/media-session/full-test.html

BUG=656563
TEST=MANUAL

Committed: https://crrev.com/fb90347c0ad1437e9fa674e7c70db9512b570444
Cr-Commit-Position: refs/heads/master@{#429614}
==========

[commit-bot: I haz the power, 2016-11-03 16:43:38]

Patchset 10 (id:??) landed as
https://crrev.com/fb90347c0ad1437e9fa674e7c70db9512b570444
Cr-Commit-Position: refs/heads/master@{#429614}

[Mathieu, 2016-11-03 21:24:24]

A revert of this CL (patchset #10 id:200001) has been created in
https://codereview.chromium.org/2479603003/ by mathp@chromium.org.

The reason for reverting is: Speculative revert for webview apk test failures:

https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg....