Closed Bug 1322114 Opened 3 years ago Closed 3 years ago

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

Categories

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

Unspecified
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
fennec + ---
firefox52 --- affected
firefox53 --- verified

People

(Reporter: itiel_yn8, Assigned: walkingice)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Tim, 
I think Alastor may not have time on media control. Can someone from your team help on this?
Flags: needinfo?(timdream)
Priority: -- → P2
tracking-fennec: --- → ?
Flags: needinfo?(timdream)
per discussion on Dec14 triage, fennec team considered this as tracking+.
tracking-fennec: ? → +
Assignee: nobody → walkingice0204
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
Attachment #8825288 - Flags: review?(alwu) → review+
Attachment #8825288 - Attachment is obsolete: true
Comment on attachment 8827034 [details]
Bug 1322114 - Part 1: Change UI updating flow

https://reviewboard.mozilla.org/r/104864/#review106522
Attachment #8827034 - Flags: review?(alwu) → review+
Comment on attachment 8827035 [details]
Bug 1322114 - Part 2: handle more media state change

https://reviewboard.mozilla.org/r/104866/#review106524
Attachment #8827035 - Flags: review?(alwu) → review+
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".
Attachment #8827036 - Flags: review?(s.kaspari) → review+
Got one 'checkstyle testfailed' on tree herder, but it is not relevant to these patches.
Keywords: checkin-needed
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.
my bad. I misread the comments
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Seems to be fixed in latest Nightly.
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.