Split MediaSessionStateChanged() and MediaSessionMetadataChanged()

Split MediaSessionStateChanged() and MediaSessionMetadataChanged()

This CL splits the metadata out from MediaSessionStateChanged() into
another observer function, in order to avoid sending MediaMetadata
through Java every time, which is very expensive and makes the
MediaSessionTabHelper logic complex.

BUG=621855
Committed: https://crrev.com/dea696202a6c6abfbb156706a722c74b6e4fc89c
Cr-Commit-Position: refs/heads/master@{#424997}

[Zhiqiang Zhang (Slow), 2016-10-11 14:17:00]

Patchset #1 (id:1) has been deleted

[Zhiqiang Zhang (Slow), 2016-10-11 14:17:07]

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

[commit-bot: I haz the power, 2016-10-11 14:17:13]

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-10-11 14:17:28]

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

[Zhiqiang Zhang (Slow), 2016-10-11 14:17:29]

+avayvod/mlamouri for initial round of review

[commit-bot: I haz the power, 2016-10-11 14:34:10]

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

[commit-bot: I haz the power, 2016-10-11 14:34:10]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Zhiqiang Zhang (Slow), 2016-10-11 18:32:29]

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

[commit-bot: I haz the power, 2016-10-11 18:32:50]

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-11 18:45:43]

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

https://codereview.chromium.org/2411723002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:299:
* |mFallbackMetadata| changes.
nit: this comment contradicts the code - you call it when mPageMetadata or tab
title is changed. mFallbackMetadata only changes in ensureFallbackMetadata and
you don't call updateMetadata from there.

I find the code below a bit hard to follow. For example, mFallbackMetadata
stored value is never used - selectMetadata always creates a new instance.

How about having a getMetadata() that will always return the correct metadata
for the current state and then call it whenever tab title updates (keep it
cached) or the page metadata updates (keep it cached as you do).
If that is not equal to the currently shown metadata (cached), update the
notification builder and show the new notification.

void onMediaSessionStateChanged() {
   ...
   mCurrentMetadata = getMetadata();
   ...
      setMetadata(mCurrentMetadata);
   ...
}

void onMediaSessionMetadataChanged(MediaMetadata metadata)
  if (mPageMetadata == null && metadata == null) return;
  if (mPageMetadata != null && mPageMetadata.equals(metadata) return; // note
this doesn't check the artwork...

  mPageMetadata = metadata;
  updateNotificationMetadata();
}

void OnTabTitleChanged() {
  mDefaultTitle = sanitizeMediaTitle(mTab.getTitle());
  updateNotificationMetadata();
}

void updateNotificationMetadata() {
  if (mNotificationInfoBuilder == null) return;

  MediaMetadata newMetadata = getMetadata();
  if (mCurrentMetadata.equals(newMetadata)) return;

  mCurrentMetadata = newMetadata;
  mNotificationInfoBuilder.setMetadata(mCurrentMetadata);
  mNotificationInfoBuilder.show(ContextUtils.getApplicationContext(),
mNotificationInfoBuilder.build());
}

MediaMetadata getMetadata() {
  String title = mDefaultTitle;
  String artist = "";
  String album = "";
  if (mPageMetadata != null) {
    if (!TextUtils.isEmpty(mPageMetadata.getTitle())) return mPageMetadata;

    artist = mPageMetadata.getArtist();
    album = mPageMetadata.getAlbum();
  }

  if (title.equals(mCurrentMetadata.getTitle())
       && artist.equals(mCurrentMetadata.getArtist())
       && album.equals(mCurrentMetadata.getAlbum())) {
    return mCurrentMetadata;
  }

  return new MediaMetadata(title, artist, album);
}

You can remove some checks and mCurrentMetadata perhaps, if we don't care about
not creating a new instance of MediaMetadata in some cases or showing the same
notification.

https://codereview.chromium.org/2411723002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:311:
private MediaMetadata selectMetadata() {
nit: I think this should be called something like getMetadata().
selectMetadata() sounds like a void method rather the one returning something.

https://codereview.chromium.org/2411723002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java
(right):

https://codereview.chromium.org/2411723002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:169:
observers.next().mediaSessionMetadataChanged(metadata);
shouldn't this be a JS call like below? perhaps you could add a TODO with a bug
to change this to JS call when the API is experimental or stable?

[commit-bot: I haz the power, 2016-10-11 19:00:42]

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

[commit-bot: I haz the power, 2016-10-11 19:00:43]

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

[Zhiqiang Zhang (Slow), 2016-10-11 21:48:11]

Patchset #3 (id:60001) has been deleted

[Zhiqiang Zhang (Slow), 2016-10-11 22:13:35]

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

[commit-bot: I haz the power, 2016-10-11 22:14:08]

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-10-11 22:22:11]

PTAL. Addressed Anton's comments.
Thanks for the elaborate code snippet, Anton!

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

https://codereview.chromium.org/2411723002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:299:
* |mFallbackMetadata| changes.
On 2016/10/11 18:45:43, whywhat wrote:
> nit: this comment contradicts the code - you call it when mPageMetadata or tab
> title is changed. mFallbackMetadata only changes in ensureFallbackMetadata and
> you don't call updateMetadata from there.
> 
> I find the code below a bit hard to follow. For example, mFallbackMetadata
> stored value is never used - selectMetadata always creates a new instance.
> 
> How about having a getMetadata() that will always return the correct metadata
> for the current state and then call it whenever tab title updates (keep it
> cached) or the page metadata updates (keep it cached as you do).
> If that is not equal to the currently shown metadata (cached), update the
> notification builder and show the new notification.
> 
> void onMediaSessionStateChanged() {
>    ...
>    mCurrentMetadata = getMetadata();
>    ...
>       setMetadata(mCurrentMetadata);
>    ...
> }
> 
> void onMediaSessionMetadataChanged(MediaMetadata metadata)
>   if (mPageMetadata == null && metadata == null) return;
>   if (mPageMetadata != null && mPageMetadata.equals(metadata) return; // note
> this doesn't check the artwork...
> 
>   mPageMetadata = metadata;
>   updateNotificationMetadata();
> }
> 
> void OnTabTitleChanged() {
>   mDefaultTitle = sanitizeMediaTitle(mTab.getTitle());
>   updateNotificationMetadata();
> }
> 
> void updateNotificationMetadata() {
>   if (mNotificationInfoBuilder == null) return;
> 
>   MediaMetadata newMetadata = getMetadata();
>   if (mCurrentMetadata.equals(newMetadata)) return;
> 
>   mCurrentMetadata = newMetadata;
>   mNotificationInfoBuilder.setMetadata(mCurrentMetadata);
>   mNotificationInfoBuilder.show(ContextUtils.getApplicationContext(),
> mNotificationInfoBuilder.build());
> }
> 
> MediaMetadata getMetadata() {
>   String title = mDefaultTitle;
>   String artist = "";
>   String album = "";
>   if (mPageMetadata != null) {
>     if (!TextUtils.isEmpty(mPageMetadata.getTitle())) return mPageMetadata;
> 
>     artist = mPageMetadata.getArtist();
>     album = mPageMetadata.getAlbum();
>   }
> 
>   if (title.equals(mCurrentMetadata.getTitle())
>        && artist.equals(mCurrentMetadata.getArtist())
>        && album.equals(mCurrentMetadata.getAlbum())) {
>     return mCurrentMetadata;
>   }
> 
>   return new MediaMetadata(title, artist, album);
> }
> 
> You can remove some checks and mCurrentMetadata perhaps, if we don't care
about
> not creating a new instance of MediaMetadata in some cases or showing the same
> notification.

Great thanks, Anton! This is really simpler. I was stuck into mFallbackMetadata
and didn't think of getting rid of it!

https://codereview.chromium.org/2411723002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:311:
private MediaMetadata selectMetadata() {
On 2016/10/11 18:45:43, whywhat wrote:
> nit: I think this should be called something like getMetadata().
> selectMetadata() sounds like a void method rather the one returning something.

Now we have updateNotificationMetadata() and getMetadata().

https://codereview.chromium.org/2411723002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java
(right):

https://codereview.chromium.org/2411723002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:169:
observers.next().mediaSessionMetadataChanged(metadata);
On 2016/10/11 18:45:43, whywhat wrote:
> shouldn't this be a JS call like below? perhaps you could add a TODO with a
bug
> to change this to JS call when the API is experimental or stable?

I think we had some discussion with tedchod@ when these tests were first
written, and we agreed not to involve too much stuffs? See
simulateMediaSessionStateChanged() above for example. I remember we were trying
to directly set the title to tab but that was not safe, so we came up with the
JS solution eventually.

[commit-bot: I haz the power, 2016-10-11 22:30:52]

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

[commit-bot: I haz the power, 2016-10-11 22:30:53]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Zhiqiang Zhang (Slow), 2016-10-11 22:38:36]

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

[commit-bot: I haz the power, 2016-10-11 22:39:18]

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-10-12 00:41:42]

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

[commit-bot: I haz the power, 2016-10-12 00:41:43]

Dry run: This issue passed the CQ dry run.

[whywhat, 2016-10-12 02:34:22]

lgtm % nits

https://codereview.chromium.org/2411723002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java
(right):

https://codereview.chromium.org/2411723002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:169:
observers.next().mediaSessionMetadataChanged(metadata);
On 2016/10/11 at 22:22:10, Zhiqiang Zhang wrote:
> On 2016/10/11 18:45:43, whywhat wrote:
> > shouldn't this be a JS call like below? perhaps you could add a TODO with a
bug
> > to change this to JS call when the API is experimental or stable?
> 
> I think we had some discussion with tedchod@ when these tests were first
written, and we agreed not to involve too much stuffs? See
simulateMediaSessionStateChanged() above for example. I remember we were trying
to directly set the title to tab but that was not safe, so we came up with the
JS solution eventually.

Ok, sounds fair. I liked the idea of executing JS since it would actually test
the whole stack but I guess it does make it flakier because of polling for the
result.

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

https://codereview.chromium.org/2411723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:47:
// The fallback title if |mPageMetadata.getTitle()| is improper to use.
nit: s/improper to use/null or empty/

https://codereview.chromium.org/2411723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:51:
// The currently selected metadata.
nit: s/selected/showing

https://codereview.chromium.org/2411723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:208:
mFallbackTitle = sanitizeMediaTitle(tab.getTitle());
nit: Are we guaranteed that tab.getTitle() is different, esp. after sanitizing?
We could early check if mFallbackTitle changed, but I think
updateNotificationMetadata should take care of it too.

https://codereview.chromium.org/2411723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:302:
* |mCurrentMetadata| is possibly changed.
nit: Probably better to list |mPageMetadata| and |mFallbackTitle| explicitly.

https://codereview.chromium.org/2411723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:306:
if (mCurrentMetadata == getMetadata()) return;
nit: I think you can remove this check as equals() below should cover it.

https://codereview.chromium.org/2411723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:318:
* Returns which metadata should be used.
nit: update the comment as in "@return the up-to-date MediaSession metadata.
Returns the cached object like |mPageMetadata| or |mCurrentMetadata| if it
reflects the current state. Otherwise will return a new {@link MediaMetadata}
object."

[Zhiqiang Zhang (Slow), 2016-10-12 11:17:37]

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

[commit-bot: I haz the power, 2016-10-12 11:17:45]

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-10-12 11:18:38]

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

https://codereview.chromium.org/2411723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:47:
// The fallback title if |mPageMetadata.getTitle()| is improper to use.
On 2016/10/12 02:34:22, whywhat wrote:
> nit: s/improper to use/null or empty/

Done.

https://codereview.chromium.org/2411723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:51:
// The currently selected metadata.
On 2016/10/12 02:34:22, whywhat wrote:
> nit: s/selected/showing

Done.

https://codereview.chromium.org/2411723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:208:
mFallbackTitle = sanitizeMediaTitle(tab.getTitle());
On 2016/10/12 02:34:22, whywhat wrote:
> nit: Are we guaranteed that tab.getTitle() is different, esp. after
sanitizing?
> We could early check if mFallbackTitle changed, but I think
> updateNotificationMetadata should take care of it too.

Done.

https://codereview.chromium.org/2411723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:302:
* |mCurrentMetadata| is possibly changed.
On 2016/10/12 02:34:22, whywhat wrote:
> nit: Probably better to list |mPageMetadata| and |mFallbackTitle| explicitly.

Done.

https://codereview.chromium.org/2411723002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:306:
if (mCurrentMetadata == getMetadata()) return;
On 2016/10/12 02:34:22, whywhat wrote:
> nit: I think you can remove this check as equals() below should cover it.

Done.

[Zhiqiang Zhang (Slow), 2016-10-12 11:21:05]

zqzhang@chromium.org changed reviewers:
+ boliu@chromium.org, clamy@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-12 11:21:05]

+clamy for content/ review
+boliu for content/public/android/ review
+alokp for RS chromecast/browser/

[clamy, 2016-10-12 11:43:51]

Thanks! A few questions below.

https://codereview.chromium.org/2411723002/diff/120001/content/browser/androi...
File content/browser/android/web_contents_observer_proxy.h (left):

https://codereview.chromium.org/2411723002/diff/120001/content/browser/androi...
content/browser/android/web_contents_observer_proxy.h:78: const
base::Optional<MediaMetadata>& metadata) override;
Considering that this is an optional parameter, couldn't you already avoid to
send every time?

https://codereview.chromium.org/2411723002/diff/120001/content/browser/androi...
File content/browser/android/web_contents_observer_proxy.h (right):

https://codereview.chromium.org/2411723002/diff/120001/content/browser/androi...
content/browser/android/web_contents_observer_proxy.h:78: const
base::Optional<MediaMetadata>& metadata) override;
Now that it moves to its own function, should metadata be made non optional?

[Zhiqiang Zhang (Slow), 2016-10-12 12:08:27]

https://codereview.chromium.org/2411723002/diff/120001/content/browser/androi...
File content/browser/android/web_contents_observer_proxy.h (left):

https://codereview.chromium.org/2411723002/diff/120001/content/browser/androi...
content/browser/android/web_contents_observer_proxy.h:78: const
base::Optional<MediaMetadata>& metadata) override;
On 2016/10/12 11:43:50, clamy wrote:
> Considering that this is an optional parameter, couldn't you already avoid to
> send every time?

As I said in the other reply, `nullopt` metadata is used for unsetting. So if
the metadata is not null, it needs to be passed through this function. So this
where we want to improve.

https://codereview.chromium.org/2411723002/diff/120001/content/browser/androi...
File content/browser/android/web_contents_observer_proxy.h (right):

https://codereview.chromium.org/2411723002/diff/120001/content/browser/androi...
content/browser/android/web_contents_observer_proxy.h:78: const
base::Optional<MediaMetadata>& metadata) override;
On 2016/10/12 11:43:50, clamy wrote:
> Now that it moves to its own function, should metadata be made non optional?

Not in our case. `metadata` is passed from Blink, and in Blink we can use null
for unsetting metadata, so we want the "unset" signal be passed to the Java
side.

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

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

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

Dry run: This issue passed the CQ dry run.

[clamy, 2016-10-12 12:25:04]

Thanks! A few comments below.

https://codereview.chromium.org/2411723002/diff/120001/content/browser/androi...
File content/browser/android/web_contents_observer_proxy.h (left):

https://codereview.chromium.org/2411723002/diff/120001/content/browser/androi...
content/browser/android/web_contents_observer_proxy.h:78: const
base::Optional<MediaMetadata>& metadata) override;
On 2016/10/12 12:08:27, Zhiqiang Zhang wrote:
> On 2016/10/12 11:43:50, clamy wrote:
> > Considering that this is an optional parameter, couldn't you already avoid
to
> > send every time?
> 
> As I said in the other reply, `nullopt` metadata is used for unsetting. So if
> the metadata is not null, it needs to be passed through this function. So this
> where we want to improve.

Acknowledged.

https://codereview.chromium.org/2411723002/diff/120001/content/browser/androi...
File content/browser/android/web_contents_observer_proxy.h (right):

https://codereview.chromium.org/2411723002/diff/120001/content/browser/androi...
content/browser/android/web_contents_observer_proxy.h:78: const
base::Optional<MediaMetadata>& metadata) override;
On 2016/10/12 12:08:27, Zhiqiang Zhang wrote:
> On 2016/10/12 11:43:50, clamy wrote:
> > Now that it moves to its own function, should metadata be made non optional?
> 
> Not in our case. `metadata` is passed from Blink, and in Blink we can use null
> for unsetting metadata, so we want the "unset" signal be passed to the Java
> side.

Acknowledged.

https://codereview.chromium.org/2411723002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2411723002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:417: void
OnMediaSessionStateChanged();
It seems that these methods are badly located in the header: in the middle of
WebContents implementation when they appear not to be part of the WebContents
interface. If that's the case, could you move them below GetAllowOtherViews and
add a description for each of them?

https://codereview.chromium.org/2411723002/diff/120001/content/public/browser...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/2411723002/diff/120001/content/public/browser...
content/public/browser/web_contents_observer.h:475: // Invoked when media
session metadata has changed.
Could you add a comment explaining that |metadata| means that a previous
metadata should be unset?

[Zhiqiang Zhang (Slow), 2016-10-12 12:41:16]

PTAL. Addressed clamy's comments.

https://codereview.chromium.org/2411723002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2411723002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:417: void
OnMediaSessionStateChanged();
On 2016/10/12 12:25:04, clamy wrote:
> It seems that these methods are badly located in the header: in the middle of
> WebContents implementation when they appear not to be part of the WebContents
> interface. If that's the case, could you move them below GetAllowOtherViews
and
> add a description for each of them?

Done.

https://codereview.chromium.org/2411723002/diff/120001/content/public/browser...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/2411723002/diff/120001/content/public/browser...
content/public/browser/web_contents_observer.h:475: // Invoked when media
session metadata has changed.
On 2016/10/12 12:25:04, clamy wrote:
> Could you add a comment explaining that |metadata| means that a previous
> metadata should be unset?

Done.

[boliu, 2016-10-12 16:27:25]

lgtm

[clamy, 2016-10-12 16:34:59]

Thanks! Lgtm.

[Zhiqiang Zhang (Slow), 2016-10-12 18:09:54]

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

[Zhiqiang Zhang (Slow), 2016-10-12 18:09:55]

+alokp for rubber stamping chromecast/

[alokp, 2016-10-12 19:59:27]

chromecast/ lgtm

[Zhiqiang Zhang (Slow), 2016-10-12 20:44:37]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-12 20:44:37]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/2411723002/#ps140001
(title: "addressed clamy's comments")

[commit-bot: I haz the power, 2016-10-12 20:44:59]

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-10-12 20:56:17]

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

[commit-bot: I haz the power, 2016-10-12 20:56:18]

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

[Zhiqiang Zhang (Slow), 2016-10-12 21:13:17]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-12 21:13:18]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
avayvod@chromium.org, boliu@chromium.org, alokp@chromium.org
Link to the patchset: https://codereview.chromium.org/2411723002/#ps160001
(title: "fixed deprecated FOR_EACH_OBSERVER")

[commit-bot: I haz the power, 2016-10-12 21:13:48]

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-10-12 22:14:34]

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

[commit-bot: I haz the power, 2016-10-12 22:14:36]

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

[Zhiqiang Zhang (Slow), 2016-10-13 09:38:12]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-13 09:38:13]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
avayvod@chromium.org, boliu@chromium.org, alokp@chromium.org
Link to the patchset: https://codereview.chromium.org/2411723002/#ps180001
(title: "rebased")

[commit-bot: I haz the power, 2016-10-13 09:38:28]

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-10-13 10:27:17]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-10-13 10:29:05]

Description was changed from

==========
Split MediaSessionStateChanged() and MediaSessionMetadataChanged()

This CL splits the metadata out from MediaSessionStateChanged() into
another observer function, in order to avoid sending MediaMetadata
through Java every time, which is very expensive and makes the
MediaSessionTabHelper logic complex.

BUG=621855
==========

to

==========
Split MediaSessionStateChanged() and MediaSessionMetadataChanged()

This CL splits the metadata out from MediaSessionStateChanged() into
another observer function, in order to avoid sending MediaMetadata
through Java every time, which is very expensive and makes the
MediaSessionTabHelper logic complex.

BUG=621855

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

[commit-bot: I haz the power, 2016-10-13 10:29:07]

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