Fix two notification update issues on page navigation

Fix two notification update issues on page navigation

This CL fixes two issues that causes the notification to update
improperly on navigation:

* The notification should reset for same-origin but non-same-page
  navigation (in Java).
* When the RenderFrameHost navigates away, the MediaSession members
  stored in MediaSessionServiceImpl should also be reset.

BUG=696716
Review-Url: https://codereview.chromium.org/2723803002
Cr-Commit-Position: refs/heads/master@{#453906}
Committed: https://chromium.googlesource.com/chromium/src/+/efb50702d432d69e593cf0bafb85ab82093d5395

[Zhiqiang Zhang (Slow), 2017-02-28 20:16:51]

Description was changed from

==========
temporary


temporary


Fix a notification update issue when navigating to a different URL in the same
origin

We should reset the notification info for all non-in-page navigation.

BUG=696716

Review-Url: https://codereview.chromium.org/2720073002
Cr-Commit-Position: refs/heads/master@{#453573}
Committed:
https://chromium.googlesource.com/chromium/src/+/7ed877a5634408b1b4c5791f57ad...

patch from issue 2720073002 at patchset 20001
(http://crrev.com/2720073002#ps20001)
==========

to

==========
Rework "Fix a notification update issue when navigating to a different URL in
the same origin"

We should reset the notification info for all non-in-page navigation.

BUG=696716
==========

[Zhiqiang Zhang (Slow), 2017-02-28 21:09:56]

Description was changed from

==========
Rework "Fix a notification update issue when navigating to a different URL in
the same origin"

We should reset the notification info for all non-in-page navigation.

BUG=696716
==========

to

==========
Fix two notification update issues on page navigation

This CL fixes two issues that causes the notification to update
improperly on navigation:

* The notification should reset for same-origin but non-same-page
  navigation (in Java).
* When the RenderFrameHost navigates away, the MediaSession members
  stored in MediaSessionServiceImpl should also be reset.

BUG=696716
==========

[Zhiqiang Zhang (Slow), 2017-02-28 21:10:06]

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

[Zhiqiang Zhang (Slow), 2017-02-28 21:10:11]

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

[Zhiqiang Zhang (Slow), 2017-02-28 21:10:22]

PTAL

[commit-bot: I haz the power, 2017-02-28 21:11:22]

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

[whywhat, 2017-02-28 21:26:57]

lgtm

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

https://codereview.chromium.org/2723803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:303:
if (mOrigin != null && mOrigin.equals(origin)) return;
So what guarantees that the same URL navigation won't reset the MediaSession?
onDidFinishNavigation doesn't get called in this case?

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

https://codereview.chromium.org/2723803002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:272:
private void simulateInPageNavigation(final Tab tab, final String url) {
can't this be shared with the method above?

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

https://codereview.chromium.org/2723803002/diff/20001/content/browser/media/s...
content/browser/media/session/media_session_impl.cc:126:
services_[rfh]->Reset();
nit: add a comment why Reset needs to be called or call Reset() something like
DidFinishNavigation and add a comment therein the service implementation?

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

https://codereview.chromium.org/2723803002/diff/20001/content/browser/media/s...
content/browser/media/session/media_session_service_impl.cc:47: playback_state_
= blink::mojom::MediaSessionPlaybackState::NONE;
nit: I wonder if you should actually call
SetPlaybackState/SetMetadata/DisableAction so the java part is guaranteed to be
in sync. Currently it's only because you have that code in MediaSessionTabHelper
that does the same.

[commit-bot: I haz the power, 2017-02-28 21:29:54]

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

[commit-bot: I haz the power, 2017-02-28 21:29:55]

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

[Zhiqiang Zhang (Slow), 2017-02-28 22:13:09]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2017-02-28 22:13:10]

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

https://codereview.chromium.org/2723803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:303:
if (mOrigin != null && mOrigin.equals(origin)) return;
On 2017/02/28 at 21:26:56, whywhat wrote:
> So what guarantees that the same URL navigation won't reset the MediaSession?
onDidFinishNavigation doesn't get called in this case?

It is called actually. See the last native browser test.

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

https://codereview.chromium.org/2723803002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/NotificationTitleUpdatedTest.java:272:
private void simulateInPageNavigation(final Tab tab, final String url) {
On 2017/02/28 at 21:26:56, whywhat wrote:
> can't this be shared with the method above?

Done.

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

https://codereview.chromium.org/2723803002/diff/20001/content/browser/media/s...
content/browser/media/session/media_session_impl.cc:126:
services_[rfh]->Reset();
On 2017/02/28 at 21:26:56, whywhat wrote:
> nit: add a comment why Reset needs to be called or call Reset() something like
DidFinishNavigation and add a comment therein the service implementation?

Done.

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

https://codereview.chromium.org/2723803002/diff/20001/content/browser/media/s...
content/browser/media/session/media_session_service_impl.cc:47: playback_state_
= blink::mojom::MediaSessionPlaybackState::NONE;
On 2017/02/28 at 21:26:56, whywhat wrote:
> nit: I wonder if you should actually call
SetPlaybackState/SetMetadata/DisableAction so the java part is guaranteed to be
in sync. Currently it's only because you have that code in MediaSessionTabHelper
that does the same.

Done.
Also I think the actions should be cleared in one run, so I added a private
method `ClearActions()`.

[Zhiqiang Zhang (Slow), 2017-02-28 22:13:10]

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/2723803002/#ps40001
(title: "addressed avayvod's comments")

[commit-bot: I haz the power, 2017-02-28 22:14: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, 2017-02-28 22:49:11]

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

[commit-bot: I haz the power, 2017-02-28 22:49:15]

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

[Zhiqiang Zhang (Slow), 2017-03-01 11:00:33]

The CQ bit was checked by zqzhang@chromium.org

[commit-bot: I haz the power, 2017-03-01 11:00:52]

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, 2017-03-01 11:41:15]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488366033545850,
"parent_rev": "a36b3b3314dd80ef22d20cc86f3146767b7e23b1", "commit_rev":
"efb50702d432d69e593cf0bafb85ab82093d5395"}

[commit-bot: I haz the power, 2017-03-01 11:41:50]

Description was changed from

==========
Fix two notification update issues on page navigation

This CL fixes two issues that causes the notification to update
improperly on navigation:

* The notification should reset for same-origin but non-same-page
  navigation (in Java).
* When the RenderFrameHost navigates away, the MediaSession members
  stored in MediaSessionServiceImpl should also be reset.

BUG=696716
==========

to

==========
Fix two notification update issues on page navigation

This CL fixes two issues that causes the notification to update
improperly on navigation:

* The notification should reset for same-origin but non-same-page
  navigation (in Java).
* When the RenderFrameHost navigates away, the MediaSession members
  stored in MediaSessionServiceImpl should also be reset.

BUG=696716

Review-Url: https://codereview.chromium.org/2723803002
Cr-Commit-Position: refs/heads/master@{#453906}
Committed:
https://chromium.googlesource.com/chromium/src/+/efb50702d432d69e593cf0bafb85...
==========

[commit-bot: I haz the power, 2017-03-01 11:41:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/efb50702d432d69e593cf0bafb85...