[Media>UI] Migrate media notification tests to JUnit tests

[Media>UI] Migrate media notification tests to JUnit tests

BUG=705919
Review-Url: https://codereview.chromium.org/2831223002
Cr-Commit-Position: refs/heads/master@{#466588}
Committed: https://chromium.googlesource.com/chromium/src/+/61d006d1ee4c7f53b6bf9cad73a59e761dc37e6a

[Zhiqiang Zhang (Slow), 2017-04-20 21:12:29]

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

[Zhiqiang Zhang (Slow), 2017-04-20 21:12:30]

PTAL.

The notification throttling CL should depend on this one as it breaks the tests.

[whywhat, 2017-04-20 22:37:40]

lgtm assuming the test coverage is the same and the tests still pass?

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

https://codereview.chromium.org/2831223002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:303:
} catch (URISyntaxException | UnsatisfiedLinkError /* For testing */ e) {
nit: that looks bad, can we catch the exception in the test? I'd add a separate
line comment and expand it to explain what's going on.

https://codereview.chromium.org/2831223002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:511:
private MediaSession getMediaSessionFromWebContents(WebContents contents) {
nit: just call it getMediaSession()?

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

https://codereview.chromium.org/2831223002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:26:
if (mediaSession instanceof MediaSessionImpl) {
nit: early return with an explanation why it can be something else?
what if I want to write a test for the observer where the mediaSession is a
mock?

[whywhat, 2017-04-20 22:37:48]

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

[commit-bot: I haz the power, 2017-04-20 22:38:26]

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, 2017-04-20 22:41:21]

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

[commit-bot: I haz the power, 2017-04-20 22:41:22]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Zhiqiang Zhang (Slow), 2017-04-21 14:21:18]

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

[Zhiqiang Zhang (Slow), 2017-04-21 14:21:18]

+boliu: content/public/android/.../MediaSessionObserver.java

[Zhiqiang Zhang (Slow), 2017-04-21 15:52:03]

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

https://codereview.chromium.org/2831223002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:303:
} catch (URISyntaxException | UnsatisfiedLinkError /* For testing */ e) {
On 2017/04/20 at 22:37:40, whywhat wrote:
> nit: that looks bad, can we catch the exception in the test? I'd add a
separate line comment and expand it to explain what's going on.

It's not easy to catch in the test, as it is a static method call, and also the
call is inside a method.
Elaborated the comments.

https://codereview.chromium.org/2831223002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:511:
private MediaSession getMediaSessionFromWebContents(WebContents contents) {
On 2017/04/20 at 22:37:40, whywhat wrote:
> nit: just call it getMediaSession()?

Done.

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

https://codereview.chromium.org/2831223002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:26:
if (mediaSession instanceof MediaSessionImpl) {
On 2017/04/20 at 22:37:40, whywhat wrote:
> nit: early return with an explanation why it can be something else?
> what if I want to write a test for the observer where the mediaSession is a
mock?

Done writing an early return.

Actually we should probably move the addObserver/removeObserver to the
MediaSession interface so we don't need this type check. Added a TODO.

[Zhiqiang Zhang (Slow), 2017-04-21 15:53:32]

On 2017/04/20 at 22:37:40, avayvod wrote:
> lgtm assuming the test coverage is the same and the tests still pass?

There's one test that was changed, which is "testSessionStateNewlyPaused". I
would assume the test was out-of-date and I'm double-checking the behavior.

[whywhat, 2017-04-21 18:10:43]

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

https://codereview.chromium.org/2831223002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:26:
if (mediaSession instanceof MediaSessionImpl) {
On 2017/04/21 at 15:52:02, Zhiqiang Zhang wrote:
> On 2017/04/20 at 22:37:40, whywhat wrote:
> > nit: early return with an explanation why it can be something else?
> > what if I want to write a test for the observer where the mediaSession is a
mock?
> 
> Done writing an early return.
> 
> Actually we should probably move the addObserver/removeObserver to the
MediaSession interface so we don't need this type check. Added a TODO.

I thought this was discouraged at the time observers were added in the picture
by content/public/ owners?
Or do you suggest having the methods added but separate implementations by
subclasses and think this would be okay?

[boliu, 2017-04-21 18:34:34]

On 2017/04/21 14:21:18, Zhiqiang Zhang wrote:
> +boliu: content/public/android/.../MediaSessionObserver.java

lgtm

[Zhiqiang Zhang (Slow), 2017-04-22 10:38:18]

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

[commit-bot: I haz the power, 2017-04-22 10:38:27]

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), 2017-04-22 10:45:12]

On 2017/04/21 at 18:10:43, avayvod wrote:
>
https://codereview.chromium.org/2831223002/diff/1/content/public/android/java...
> File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java
(right):
> 
>
https://codereview.chromium.org/2831223002/diff/1/content/public/android/java...
>
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:26:
if (mediaSession instanceof MediaSessionImpl) {
> On 2017/04/21 at 15:52:02, Zhiqiang Zhang wrote:
> > On 2017/04/20 at 22:37:40, whywhat wrote:
> > > nit: early return with an explanation why it can be something else?
> > > what if I want to write a test for the observer where the mediaSession is
a mock?
> > 
> > Done writing an early return.
> > 
> > Actually we should probably move the addObserver/removeObserver to the
MediaSession interface so we don't need this type check. Added a TODO.
> 
> I thought this was discouraged at the time observers were added in the picture
by content/public/ owners?
> Or do you suggest having the methods added but separate implementations by
subclasses and think this would be okay?

Yep, I also remember that it was discouraged. IIRC the reason is that no one
explicitly calls it. Maybe we could make addObserver/removeObserver protected to
avoid exposing them.
I checked WebContents/WebContentsObserver and they are actually using the same
pattern as we currently do. If the type cast is not successful, it just don't
call AddObserver().

[commit-bot: I haz the power, 2017-04-22 11:58:28]

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

[commit-bot: I haz the power, 2017-04-22 11:58:29]

Dry run: This issue passed the CQ dry run.

[Zhiqiang Zhang (Slow), 2017-04-24 04:01:12]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2017-04-24 04:01:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2831223002/#ps40001
(title: "fixed tests")

[commit-bot: I haz the power, 2017-04-24 04:01:30]

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-04-24 05:30:24]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493006472849460,
"parent_rev": "eaa6185759c3d8f7230851633c724caa73f544e9", "commit_rev":
"61d006d1ee4c7f53b6bf9cad73a59e761dc37e6a"}

[commit-bot: I haz the power, 2017-04-24 05:31:21]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493006472849460,
"parent_rev": "eaa6185759c3d8f7230851633c724caa73f544e9", "commit_rev":
"61d006d1ee4c7f53b6bf9cad73a59e761dc37e6a"}

[commit-bot: I haz the power, 2017-04-24 05:31:59]

Description was changed from

==========
[Media>UI] Migrate media notification tests to JUnit tests

BUG=705919
==========

to

==========
[Media>UI] Migrate media notification tests to JUnit tests

BUG=705919

Review-Url: https://codereview.chromium.org/2831223002
Cr-Commit-Position: refs/heads/master@{#466588}
Committed:
https://chromium.googlesource.com/chromium/src/+/61d006d1ee4c7f53b6bf9cad73a5...
==========

[commit-bot: I haz the power, 2017-04-24 05:32:00]

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