1322114 - Media control notification's text changes only if "pause" button is tapped

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect, P2)

Description

[Itiel]

Environment:
Samsung Galaxy S5 G900F Android 6.0.1
Firefox Nightly 53.0a1 2016-12-04

STR:
1. Open youtube.com and play a video
2. From the same tab, click one of the video suggestion below and play
3. Go to the android notification bar

Expected result:
Text displayed on media control notification should correspond to the video that currently being played.

Actual result:
Text displayed on media control notification has the title of the previous video. It is being updated only if the pause button is tapped.

Comment 1

[Blake Wu [:bwu][:blakewu]]

Tim, 
I think Alastor may not have time on media control. Can someone from your team help on this?

Comment 2

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

per discussion on Dec14 triage, fennec team considered this as tracking+.

Comment 3

[Julian Chu [:walkingice]]

Attachment: Bug 1322114 - handle more media state change

When the playing media changes, there will be one more event
MEDIA_PLAYING_CHANGE, and the tab is still playing media.

This fix is to handle this situation then update Notification Bar.

Review commit: https://reviewboard.mozilla.org/r/103456/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103456/

Comment 4

[Alastor Wu [:alwu]]

Comment on attachment 8825288 [details]
Bug 1322114 - handle more media state change

https://reviewboard.mozilla.org/r/103456/#review104406

r+ after solving following issue.

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:116
(Diff revision 1)
>                  if (playingTab != tab && tab.isMediaPlaying()) {
>                      mTabReference = new WeakReference<>(tab);
>                      notifyControlInterfaceChanged(ACTION_PAUSE);
> -                } else if (playingTab == tab && !tab.isMediaPlaying()) {
> -                    notifyControlInterfaceChanged(ACTION_STOP);
> -                    mTabReference = new WeakReference<>(null);
> +                } else if (playingTab == tab) {
> +                    // to understand what action happened in this event
> +                    String action = tab.isMediaPlaying() ?  ACTION_RESUME : ACTION_STOP;

When the tab has playing media, action should be ACTION_PAUSE. (showing the pause icon in media control)

nit : s/action/uiAction

Comment 5

[Julian Chu [:walkingice]]

Attachment: Bug 1322114 - Part 1: Change UI updating flow

This patch involve State into MediaControlService.

ACTION is behaviour for an intention, and so far we use it for storing
media state. Now involve enum to distinguish State from Action.

So flow becomes: Action happens -> Change State -> UI reflect state
changing.

Review commit: https://reviewboard.mozilla.org/r/104864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104864/

Comment 6

[Julian Chu [:walkingice]]

Attachment: Bug 1322114 - Part 2: handle more media state change

When the playing-media changes (ie. in Youtube click another
video when it is alreay playing one), there will be one more
event MEDIA_PLAYING_CHANGE, and the tab is still playing media.

This fix is to handle this situation then update state.

Review commit: https://reviewboard.mozilla.org/r/104866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104866/

Comment 7

[Julian Chu [:walkingice]]

Attachment: Bug 1322114 - Add test cases for MediaControlService

Also refactor MediaControlService to be test-friendly.

Review commit: https://reviewboard.mozilla.org/r/104868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104868/

Comment 8

[Alastor Wu [:alwu]]

Comment on attachment 8827034 [details]
Bug 1322114 - Part 1: Change UI updating flow

https://reviewboard.mozilla.org/r/104864/#review106522

Comment 9

[Alastor Wu [:alwu]]

Comment on attachment 8827035 [details]
Bug 1322114 - Part 2: handle more media state change

https://reviewboard.mozilla.org/r/104866/#review106524

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8827036 [details]
Bug 1322114 - Add test cases for MediaControlService

https://reviewboard.mozilla.org/r/104868/#review106652

Nice!

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:375
(Diff revision 1)
> -        boolean isPlayAction = mMediaState.equals(State.PAUSED);
> +        Intent intent = createIntentUponState(mMediaState);
> +        boolean isPlayAction = intent.getAction().equals(ACTION_RESUME);

nit: final?

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:392
(Diff revision 1)
> +    /**
> +     * This method encapsulated UI logic. For PLAYING state, UI should display a PAUSE icon.
> +     * @param state The expected current state of MediaControlService
> +     * @return corresponding Intent to be used for Notification
> +     */
> +    protected Intent createIntentUponState(State state) {

I guess this method is protected so that it can be accessed in tests. Let's add the @VisibleForTesting annotation to make this more obvious.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/media/TestMediaControlService.java:1
(Diff revision 1)
> +package org.mozilla.gecko.media;

nit: License headers. Unlike our other code test code is considered "public domain".

Comment 11

[Julian Chu [:walkingice]]

Comment on attachment 8827036 [details]
Bug 1322114 - Add test cases for MediaControlService

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104868/diff/1-2/

Comment 12

[Julian Chu [:walkingice]]

Got one 'checkstyle testfailed' on tree herder, but it is not relevant to these patches.

Comment 13

[Alastor Wu [:alwu]]

From the log, I saw the following message,

>  INFO -  :app:checkstyle[ant:checkstyle] /home/worker/workspace/build/src/mobile/android/base/java
> /org/mozilla/gecko/media/MediaControlService.java:395:52: '?' is not preceded with whitespace.

Comment 14

[Julian Chu [:walkingice]]

my bad. I misread the comments

Comment 15

[Julian Chu [:walkingice]]

Comment on attachment 8827036 [details]
Bug 1322114 - Add test cases for MediaControlService

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104868/diff/2-3/

Comment 16

[Julian Chu [:walkingice]]

Comment on attachment 8827036 [details]
Bug 1322114 - Add test cases for MediaControlService

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104868/diff/3-4/

Comment 17

[Julian Chu [:walkingice]]

Fixed style
https://treeherder.mozilla.org/#/jobs?repo=try&revision=69276ff42e406353ee566709c2db4964f8eb7288

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63bf33197a07
Part 1: Change UI updating flow r=alwu
https://hg.mozilla.org/integration/autoland/rev/daf0f6eccbb6
Part 2: handle more media state change r=alwu
https://hg.mozilla.org/integration/autoland/rev/c69a94671a4a
Add test cases for MediaControlService r=sebastian

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/63bf33197a07
https://hg.mozilla.org/mozilla-central/rev/daf0f6eccbb6
https://hg.mozilla.org/mozilla-central/rev/c69a94671a4a

Comment 20

[Itiel]

Seems to be fixed in latest Nightly.

Comment 21

[Sorina Florean [:sflorean]]

Verified as fixed in build 54.0a1 (2017-02-15) and build 53.0a2 (2017-02-15);
Device: Asus Zenpad 8 (Android 6.0.1).