1240423 - Implement the remote media-control on Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Alastor Wu [:alwu]]

In MediaSession API, it mentioned two things

"Displays lock-screen and notification area user interfaces when playback begins"
"Reacts to changes in both hardware and software-based media key buttons when playback begins"

Now only the native gaia apps have this control ability, but it's not enough. We should support this feature for arbitrary web contents.

Comment 1

[Alastor Wu [:alwu]]

Here is an example in Android Chrome, it already has the remote media-control feature for arbitrary web contents.
https://drive.google.com/file/d/0B8z53fPg4O7NRlRpNS1zRGdLOUk/view?usp=sharing

Comment 2

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

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

Comment 3

[Alastor Wu [:alwu]]

Hi, Margaret & Barbara,
I would like to propose this feature to Fennec, could you give me some suggestions?
Very appreciate :)

Comment 4

[Barbara Bermes [:barbara] - NI please!]

Seems ok to me -- as always, I wonder how many times this is even applicable, i.e. x% our media content consumers run into this problem.

I'll create an Aha card for it to discuss priorities: https://mozilla.aha.io/features/FENN-452

Comment 5

[Alastor Wu [:alwu]]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

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

Comment 6

[:Margaret Leibovic]

I think this is a good feature idea. We'll discuss it in our funnel meeting on Monday morning (8am PST). Sounds like something we can probably track for Firefox 48 or 49.

Comment 7

[Alastor Wu [:alwu]]

Thanks Margaret!
FYI, now these patches is almost done and can work well. 
There are only user-level details left which need to discuss with UX.

Comment 8

[Alastor Wu [:alwu]]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

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

Comment 9

[Alastor Wu [:alwu]]

Hi, Anthony,
Sorry to bother you, 
Since I saw you're working on Fennec UX design, is it appropriate to discuss the Fennec media-control with you? If not, could you help me to find the right person?
Very appreciate your help!

---

Here are somethings I want to discuss,
(1) The visual design details
The remote media-control has two user interfaces, one is on the notification bar, another is on the lock-screen. 

Now the demo controls I made are look like that [1]. And I want to know what should they look like when we land this feature on Fennec.

* In notification bar control, 
  * what kind of large image we want to display? 
  * what is text of the tittle and the content?
  * what kind of control buttons we want to provide?

* In lock-screen control,
  * what kind of small image we want to display?  
  * what is text of the main tittle and the content?
  * what kind of control buttons we want to provide?

Here are the control interface of other apps (bandcamp/soundclound/spotify), 
https://goo.gl/KWFvSM

[1] Demo visual design of the control
Notification bar : https://goo.gl/jysn3n
Lockscreen : https://goo.gl/Wvznls

---

(2) The behaviors when we operate with the controller 

Now I have implemented some control behaviors, could you help me to verify them?

* touch play/pause button : directly control the playing state of the media content
* swipe the control interface : stop the media content
* double click the  control interface : open the Fennec app

Thanks!

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Alastor Wu [:alwu] from comment #9)
> Hi, Anthony,
> Sorry to bother you, 
> Since I saw you're working on Fennec UX design, is it appropriate to discuss
> the Fennec media-control with you? If not, could you help me to find the
> right person?
> Very appreciate your help!

No bother at all. Thanks for the NI ping! 

Lets see what we can work out here

> ---
> 
> Here are somethings I want to discuss,
> (1) The visual design details
> The remote media-control has two user interfaces, one is on the notification
> bar, another is on the lock-screen. 

Can we start with just the collapsed notification UI first? I.e. the smaller one in the lockscreen. 

I've seen other apps use this same one even in both the notification tray as well as the lock screen. I think this is the best place to start. We can work on an expanded notification after that.

> Now the demo controls I made are look like that [1]. And I want to know what
> should they look like when we land this feature on Fennec.
> 
> * In notification bar control, 
>   * what kind of large image we want to display? 
>   * what is text of the tittle and the content?
>   * what kind of control buttons we want to provide?
> 
> * In lock-screen control,
>   * what kind of small image we want to display?  

For the small icon, can we use the websites' favicon? I'm not sure how difficult this would be but otherwise, our other notifications currently use the single tone firefox icon - we could just reuse that.

>   * what is text of the main tittle and the content?

We can start with Page title as the Title and, the URL as the detail text.

What information do we have available? do we know the video title? 

>   * what kind of control buttons we want to provide?

I think we will just need the play/pause icons. A basic "play" and "pause" functionality would be all we'll need from this collapsed notification and it's a great start.

We should use our own video control icons though. Bug 1250741 has the latest spec and I can attach the icons you need here too. What size do you need them at?

> 
> Here are the control interface of other apps (bandcamp/soundclound/spotify), 
> https://goo.gl/KWFvSM
> 
> [1] Demo visual design of the control
> Notification bar : https://goo.gl/jysn3n
> Lockscreen : https://goo.gl/Wvznls
>
> ---
> 
> (2) The behaviors when we operate with the controller 
> 
> Now I have implemented some control behaviors, could you help me to verify
> them?
> 
> * touch play/pause button : directly control the playing state of the media
> content

As above, this is great!

> * swipe the control interface : stop the media content

Makes sense to me

> * double click the  control interface : open the Fennec app

Not sure what you mean here by "double click". Do you mean "touch/ single-press" the notification? IF so, then yes - it should open the tab that's playing the content

> 
> Thanks!

thank you!

Comment 11

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Anthony Lam (:antlam) from comment #10)
> (In reply to Alastor Wu [:alwu] from comment #9)
> > * In lock-screen control,
> >   * what kind of small image we want to display?  
> 
> For the small icon, can we use the websites' favicon? I'm not sure how
> difficult this would be but otherwise, our other notifications currently use
> the single tone firefox icon - we could just reuse that.

Actually, I just checked and I think we use the favicon if we can grab it. If not, we just fall back to the single tone Firefox icon (that we currently use in our other notifications).

Comment 12

[Alastor Wu [:alwu]]

Hi, Anthony,
Thanks for your reply :)

I have an another question is that if user starts playing music from multiple tabs, how should we handle this situation?

(1) only allow one audible tab
That means we need to increase the audio competing mechanism to Fennec, when the new music is started, the previous one should be stopped.

(2) allow multiple audible tabs 
If so, when user presses the pause button of the media-control, does that mean we should stop both of them? or just control one of them?

In addition, in this case, what favicon, tittle and content text we should display on the media-control? 

Thanks!

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Alastor Wu [:alwu] from comment #12)
> Hi, Anthony,
> Thanks for your reply :)
> 
> I have an another question is that if user starts playing music from
> multiple tabs, how should we handle this situation?

For now, I just want to focus on one tab at a time. 

So, if one tab is already playing something, and the user then switches to another tab and starts playing something else, the FIRST tab should pause automatically. Then the controls in the notification will be for the latest tab that the user started playing something from.

1) User plays video in tab A
2) User switches to tab B
3) User plays video in tab B
4) tab A video + audio pauses
5) tab B video plays
6) User returns to home screen and sees notification controls for tab B's video (that is playing).

> (1) only allow one audible tab
> That means we need to increase the audio competing mechanism to Fennec, when
> the new music is started, the previous one should be stopped.
> 
> (2) allow multiple audible tabs 
> If so, when user presses the pause button of the media-control, does that
> mean we should stop both of them? or just control one of them?

As above, let's just stick to (1) 

> In addition, in this case, what favicon, tittle and content text we should
> display on the media-control? 

These would be grabbed from the web page I assume.

So, the web page's favicon, page title (title), and URL (detail text).

Does that make sense?

Comment 14

[Alastor Wu [:alwu]]

Thanks Anthony!

I would go to study how to get that information (favicon/tittle/content-text) from website, and go to implement the internal audio competing on Fennec.

Comment 15

[Scott Wu [:scottwu]]

Hi Alastor, as per our offline conversation, I found that upon receiving a notification, a paused video would resume playing automatically.

STR:
1. Open Fennec and play a video
2. Pause it
3. Lock the phone
4. Send an email to self (or any way that triggers a notification)

Expected:
Video should stay paused.

Actual:
Video starts playing in the background with audio.

Comment 16

[Scott Wu [:scottwu]]

Attachment: MozReview Request: Bug 1240423 - implement the remote media-control front-end

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

Comment 17

[Scott Wu [:scottwu]]

Comment on attachment 8741691 [details]
MozReview Request: Bug 1240423 - implement the remote media-control front-end

Patch moved to bug 1264901.

Comment 18

[:Margaret Leibovic]

You should make sure Chenxia or Sebastian review any Android UI changes here.

Comment 19

[Alastor Wu [:alwu]]

The UI part would be moved to bug 1264901, I would remove them from these patches.

Comment 20

[:Margaret Leibovic]

What's the state of the patch here? Does this need to be reviewed? This patch doesn't look like it has the same contents as the patch in bug 1264901.

Are you still targeting landing these for Firefox 48? There are only a few days left of development, so I'm worried that you won't get complete reviews in time.

The UI changes will require strings, and we can't uplift strings, so this feature will slip to 49 if it's not landed by the end of this week. Please let us know if you need help.

Comment 21

[Alastor Wu [:alwu]]

Hi, Margaret,
Sorry about that this issue could not target at FF48 in time because we're still blocked by bug1242874 and it's an important bug which implements all platform audio-control logic, but I'm pretty sure this feature would be landed in FF49.

BTW, this bug implements the basic control functionality and bug 1264901 implements the user-interface.

Comment 22

[Alastor Wu [:alwu]]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

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

Comment 23

[Alastor Wu [:alwu]]

During rebasing, I found other two bugs need to handle.
(1) Media control should not be removed as other common notifications when user press "remove all notification"
(2) Play media, then pause, then press play from website. After that, the media control doesn't be updated to the correct state (need bug1235612)
(3) Tab audio competing needs to handle the audio-playback event order

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Removing flags here. I'll comment in bug 1264901.

Comment 25

[Alastor Wu [:alwu]]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

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

Comment 26

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1240423 - part2 : introduce audible changing reasons.

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

Comment 27

[Alastor Wu [:alwu]]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36811/diff/5-6/

Comment 28

[Alastor Wu [:alwu]]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36811/diff/6-7/

Comment 29

[Alastor Wu [:alwu]]

Comment on attachment 8749600 [details]
MozReview Request: Bug 1240423 - part2 : introduce audible changing reasons.

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

Comment 30

[Alastor Wu [:alwu]]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36811/diff/7-8/

Comment 31

[Alastor Wu [:alwu]]

Comment on attachment 8749600 [details]
MozReview Request: Bug 1240423 - part2 : introduce audible changing reasons.

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

Comment 32

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1240423 - part3 : add reason when media element notify audible changing.

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

Comment 33

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1240423 - part4 : update audio playing window checking in AndroidBridge.

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

Comment 34

[Alastor Wu [:alwu]]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36811/diff/8-9/

Comment 35

[Alastor Wu [:alwu]]

Comment on attachment 8749600 [details]
MozReview Request: Bug 1240423 - part2 : introduce audible changing reasons.

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

Comment 36

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1240423 - part5 : stop_disposable should reset mSuspendState.

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

Comment 37

[Alastor Wu [:alwu]]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36811/diff/9-10/

Comment 38

[Alastor Wu [:alwu]]

Comment on attachment 8749600 [details]
MozReview Request: Bug 1240423 - part2 : introduce audible changing reasons.

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

Comment 39

[Alastor Wu [:alwu]]

Comment on attachment 8751659 [details]
MozReview Request: Bug 1240423 - part3 : add reason when media element notify audible changing.

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

Comment 40

[Alastor Wu [:alwu]]

Comment on attachment 8751660 [details]
MozReview Request: Bug 1240423 - part4 : update audio playing window checking in AndroidBridge.

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

Comment 41

[Alastor Wu [:alwu]]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36811/diff/10-11/

Comment 42

[Alastor Wu [:alwu]]

Comment on attachment 8749600 [details]
MozReview Request: Bug 1240423 - part2 : introduce audible changing reasons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51055/diff/5-6/

Comment 43

[Alastor Wu [:alwu]]

Comment on attachment 8751659 [details]
MozReview Request: Bug 1240423 - part3 : add reason when media element notify audible changing.

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

Comment 44

[Alastor Wu [:alwu]]

Comment on attachment 8751660 [details]
MozReview Request: Bug 1240423 - part4 : update audio playing window checking in AndroidBridge.

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

Comment 45

[Alastor Wu [:alwu]]

Comment on attachment 8751695 [details]
MozReview Request: Bug 1240423 - part5 : stop_disposable should reset mSuspendState.

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

Comment 46

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1240423 - part6 : remove useless pref in Android.

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

Comment 47

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1240423 - part7 : add attributes for AudioChannelLog.

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

Comment 48

[Alastor Wu [:alwu]]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36811/diff/11-12/

Comment 49

[Alastor Wu [:alwu]]

Comment on attachment 8749600 [details]
MozReview Request: Bug 1240423 - part2 : introduce audible changing reasons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51055/diff/6-7/

Comment 50

[Alastor Wu [:alwu]]

Comment on attachment 8751659 [details]
MozReview Request: Bug 1240423 - part3 : add reason when media element notify audible changing.

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

Comment 51

[Alastor Wu [:alwu]]

Comment on attachment 8751660 [details]
MozReview Request: Bug 1240423 - part4 : update audio playing window checking in AndroidBridge.

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

Comment 52

[Alastor Wu [:alwu]]

Comment on attachment 8751695 [details]
MozReview Request: Bug 1240423 - part5 : stop_disposable should reset mSuspendState.

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

Comment 53

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a45eeda2877

Comment 54

[Alastor Wu [:alwu]]

If the try-result is good, I'll ask for review.

Comment 55

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=93c7bee85fe3

Comment 56

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1240423 - part8 : modify tests

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

Comment 57

[Alastor Wu [:alwu]]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36811/diff/12-13/

Comment 58

[Alastor Wu [:alwu]]

Comment on attachment 8749600 [details]
MozReview Request: Bug 1240423 - part2 : introduce audible changing reasons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51055/diff/7-8/

Comment 59

[Alastor Wu [:alwu]]

Comment on attachment 8751659 [details]
MozReview Request: Bug 1240423 - part3 : add reason when media element notify audible changing.

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

Comment 60

[Alastor Wu [:alwu]]

Comment on attachment 8751660 [details]
MozReview Request: Bug 1240423 - part4 : update audio playing window checking in AndroidBridge.

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

Comment 61

[Alastor Wu [:alwu]]

Comment on attachment 8751695 [details]
MozReview Request: Bug 1240423 - part5 : stop_disposable should reset mSuspendState.

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

Comment 62

[Alastor Wu [:alwu]]

Comment on attachment 8752090 [details]
MozReview Request: Bug 1240423 - part6 : remove useless pref in Android.

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

Comment 63

[Alastor Wu [:alwu]]

Comment on attachment 8752091 [details]
MozReview Request: Bug 1240423 - part7 : add attributes for AudioChannelLog.

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

Comment 64

[Alastor Wu [:alwu]]

Hi, Baku, Margret, Snorp,
Could you help me review these patches?
This bug is about the implementation of fennec media control, and the UI part would be done in bug1264901.
Very appreciate your help!

Comment 65

[:Margaret Leibovic]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

I may not be able to get to this week. Maybe ahunt can help take over this review.

Comment 66

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8751660 [details]
MozReview Request: Bug 1240423 - part4 : update audio playing window checking in AndroidBridge.

https://reviewboard.mozilla.org/r/52149/#review50024

Comment 67

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8752090 [details]
MozReview Request: Bug 1240423 - part6 : remove useless pref in Android.

https://reviewboard.mozilla.org/r/52415/#review50026

Comment 68

[Andrzej Hunt :ahunt]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

https://reviewboard.mozilla.org/r/36811/#review50106

Looks good to me: two minor issues below, and I'm pretty sure we should remove the MEDIA_CONTENT_CONTROL permission (see below).

::: mobile/android/base/FennecManifest_permissions.xml.in:90
(Diff revision 13)
>  #endif
>      <uses-permission android:name="android.permission.CAMERA" />
>      <uses-feature android:name="android.hardware.camera" android:required="false"/>
>      <uses-feature android:name="android.hardware.camera.autofocus" android:required="false"/>
>  
> +    <uses-permission android:name="android.permission.MEDIA_CONTENT_CONTROL" />

Do we need this permission? I don't see where we use this? (I'm pretty sure we don't need any permissions to send intents to our own services, this is only for accessing other apps?)

Also, If I'm understanding the docs correctly we (as a third party app) shouldn't be using this permission anyways:
https://developer.android.com/reference/android/Manifest.permission.html#MEDIA_CONTENT_CONTROL


(This permission also needs API 19+ or above, but we support 15+, which would probably also prevent us from having this.)

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:114
(Diff revision 13)
> +                break;
> +        }
> +    }
> +
> +    private void getGeckoPreference() {
> +       String[] prefs = { MEDIA_CONTROL_PREF };

nit: this line seems to have different indentation. We can also make this varibale final (final String[] prefs = ...).

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:140
(Diff revision 13)
> +                        handleIntent(intent);
> +                    }
> +                }
> +            }
> +        };
> +        PrefsHelper.addObserver(prefs, mPrefsObserver);

I think we also need to remove this observer using removeObserver() - that can probably be done in onDestroy (since we add this observer during onCreate).

Comment 69

[Alastor Wu [:alwu]]

(In reply to Andrzej Hunt :ahunt from comment #68)
> 
> Looks good to me: two minor issues below, and I'm pretty sure we should
> remove the MEDIA_CONTENT_CONTROL permission (see below).
> 

Thanks! I'll remove this permission and modify my patches!

Comment 70

[Alastor Wu [:alwu]]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36811/diff/13-14/

Comment 71

[Alastor Wu [:alwu]]

Comment on attachment 8749600 [details]
MozReview Request: Bug 1240423 - part2 : introduce audible changing reasons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51055/diff/8-9/

Comment 72

[Alastor Wu [:alwu]]

Comment on attachment 8751659 [details]
MozReview Request: Bug 1240423 - part3 : add reason when media element notify audible changing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52141/diff/5-6/

Comment 73

[Alastor Wu [:alwu]]

Comment on attachment 8751660 [details]
MozReview Request: Bug 1240423 - part4 : update audio playing window checking in AndroidBridge.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52149/diff/5-6/

Comment 74

[Alastor Wu [:alwu]]

Comment on attachment 8751695 [details]
MozReview Request: Bug 1240423 - part5 : stop_disposable should reset mSuspendState.

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

Comment 75

[Alastor Wu [:alwu]]

Comment on attachment 8752090 [details]
MozReview Request: Bug 1240423 - part6 : remove useless pref in Android.

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

Comment 76

[Alastor Wu [:alwu]]

Comment on attachment 8752091 [details]
MozReview Request: Bug 1240423 - part7 : add attributes for AudioChannelLog.

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

Comment 77

[Alastor Wu [:alwu]]

Comment on attachment 8752742 [details]
MozReview Request: Bug 1240423 - part8 : modify tests

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

Comment 78

[Alastor Wu [:alwu]]

Hi, Margret,
Since I modified one file under the toolkit in my patch1 (it just changes a little bit), could you mind to help me review it? 
Thanks!

Comment 79

[:Margaret Leibovic]

(In reply to Alastor Wu [:alwu][PTO 5/24~5/26] from comment #78)
> Hi, Margret,
> Since I modified one file under the toolkit in my patch1 (it just changes a
> little bit), could you mind to help me review it? 
> Thanks!

Sure, I looked these toolkit changes over, and they look fine to me. It looks like you're the person who originally wrote the code around these changes, so you probably know it better than anyone :)

Comment 80

[Sorina Florean [:sflorean]]

Hi all, I will take this feature as a QA. Here is the Test plan based on which the testing is made: 
https://wiki.mozilla.org/QA/Fennec/Control_media_content_when_switching_between_apps

Comment 81

[Alastor Wu [:alwu]]

review ping?

Comment 82

[Andrea Marchesini [:baku]]

Comment on attachment 8749600 [details]
MozReview Request: Bug 1240423 - part2 : introduce audible changing reasons.

https://reviewboard.mozilla.org/r/51055/#review53068

::: dom/audiochannel/AudioChannelService.h:82
(Diff revision 9)
>      eCapturing = true,
>      eNotCapturing = false
>    };
>  
> +  enum AudibleChangedReasons : uint32_t {
> +    eVolumeChanged = 1,

No value 0 ?

::: dom/audiochannel/AudioChannelService.cpp:122
(Diff revision 9)
>      nsCOMPtr<nsIObserverService> observerService =
>        services::GetObserverService();
>      if (NS_WARN_IF(!observerService)) {
>        return NS_ERROR_FAILURE;
>      }
>  

nsAutoString state;
GetActiveState(state);

::: dom/audiochannel/AudioChannelService.cpp:125
(Diff revision 9)
>        return NS_ERROR_FAILURE;
>      }
>  
>      observerService->NotifyObservers(ToSupports(mWindow),
>                                       "audio-playback",
> -                                     mActive ? MOZ_UTF16("active")
> +                                     GetActiveState().get());

if you don't have get(), use BeginReading()

::: dom/audiochannel/AudioChannelService.cpp:133
(Diff revision 9)
>             ("AudioPlaybackRunnable, active = %d\n", mActive));
>      return NS_OK;
>    }
>  
>  private:
> +  nsString GetActiveState()

I don't like to return a nsString. Do:

void
GetActiveState(nsAString& astate)
{
...

Comment 83

[Andrea Marchesini [:baku]]

Comment on attachment 8751659 [details]
MozReview Request: Bug 1240423 - part3 : add reason when media element notify audible changing.

https://reviewboard.mozilla.org/r/52141/#review53072

Comment 84

[Andrea Marchesini [:baku]]

Comment on attachment 8751695 [details]
MozReview Request: Bug 1240423 - part5 : stop_disposable should reset mSuspendState.

https://reviewboard.mozilla.org/r/52159/#review53074

Comment 85

[Andrea Marchesini [:baku]]

Comment on attachment 8752091 [details]
MozReview Request: Bug 1240423 - part7 : add attributes for AudioChannelLog.

https://reviewboard.mozilla.org/r/52417/#review53076

Comment 86

[Andrea Marchesini [:baku]]

Comment on attachment 8752742 [details]
MozReview Request: Bug 1240423 - part8 : modify tests

https://reviewboard.mozilla.org/r/52746/#review53078

Comment 87

[Alastor Wu [:alwu]]

Thanks for review :)

Comment 88

[Alastor Wu [:alwu]]

Comment on attachment 8723999 [details]
MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36811/diff/14-15/

Comment 89

[Alastor Wu [:alwu]]

Comment on attachment 8749600 [details]
MozReview Request: Bug 1240423 - part2 : introduce audible changing reasons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51055/diff/9-10/

Comment 90

[Alastor Wu [:alwu]]

Comment on attachment 8751659 [details]
MozReview Request: Bug 1240423 - part3 : add reason when media element notify audible changing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52141/diff/6-7/

Comment 91

[Alastor Wu [:alwu]]

Comment on attachment 8751660 [details]
MozReview Request: Bug 1240423 - part4 : update audio playing window checking in AndroidBridge.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52149/diff/6-7/

Comment 92

[Alastor Wu [:alwu]]

Comment on attachment 8751695 [details]
MozReview Request: Bug 1240423 - part5 : stop_disposable should reset mSuspendState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52159/diff/5-6/

Comment 93

[Alastor Wu [:alwu]]

Comment on attachment 8752090 [details]
MozReview Request: Bug 1240423 - part6 : remove useless pref in Android.

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

Comment 94

[Alastor Wu [:alwu]]

Comment on attachment 8752091 [details]
MozReview Request: Bug 1240423 - part7 : add attributes for AudioChannelLog.

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

Comment 95

[Alastor Wu [:alwu]]

Comment on attachment 8752742 [details]
MozReview Request: Bug 1240423 - part8 : modify tests

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

Comment 96

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fb8a0e4f4cf

Comment 97

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae72a7d642b3
part1 : implement the remote media-control on Fennec. r=ahunt
https://hg.mozilla.org/integration/mozilla-inbound/rev/d367637208ee
part2 : introduce audible changing reasons. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/180b625d6536
part3 : add reason when media element notify audible changing. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d337dda7dd1
part4 : update audio playing window checking in AndroidBridge. r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/702f05744977
part5 : stop_disposable should reset mSuspendState. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ccfcd9c5430
part6 : remove useless pref in Android. r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/e40fc1609c5e
part7 : add attributes for AudioChannelLog. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/07bc3e5d3815
part8 : modify tests r=baku

Comment 98

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This caused an android lint[1] and checkstyle[2] failure:

Looking in the lint-results-automationDebug.xml[3] file uploaded, the error is:

<issue id="SuspiciousImport" severity="Error" message="Don't include `android.R` here; use a fully qualified name for each usage instead" category="Correctness" priority="9" summary="'`import android.R`' statement" explanation="Importing `android.R` is usually not intentional; it sometimes happens when you use an IDE and ask it to automatically add imports at a time when your project's R class it not present.  Once the import is there you might get a lot of "confusing" error messages because of course the fields available on `android.R` are not the ones you'd expect from just looking at your own `R` class." errorLine1="import android.R;" errorLine2="~~~~~~~~~~~~~~~~~">
    <location file="/home/worker/workspace/build/src/mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java" line="23" column="1"/>
</issue>

These are tier-2 tests, so I'm not backing these out, but they will need to be fixed.

1. https://treeherder.mozilla.org/logviewer.html#?job_id=29197285&repo=mozilla-inbound
2. https://treeherder.mozilla.org/logviewer.html#?job_id=29197039&repo=mozilla-inbound
3. https://public-artifacts.taskcluster.net/aK7x4PXETo687oF91AzgCA/0/public/android/lint/lint-results-automationDebug.xml

Comment 99

[Alastor Wu [:alwu]]

Take bug1277373.

Comment 100

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae72a7d642b3
https://hg.mozilla.org/mozilla-central/rev/d367637208ee
https://hg.mozilla.org/mozilla-central/rev/180b625d6536
https://hg.mozilla.org/mozilla-central/rev/2d337dda7dd1
https://hg.mozilla.org/mozilla-central/rev/702f05744977
https://hg.mozilla.org/mozilla-central/rev/4ccfcd9c5430
https://hg.mozilla.org/mozilla-central/rev/e40fc1609c5e
https://hg.mozilla.org/mozilla-central/rev/07bc3e5d3815

Comment 101

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

i don't see this feature in firefox for 49 beta 8. I also don't see this feature in firefox for 50 aurora. I do see it in firefox for Android 51 nightly (see https://www.flickr.com/photos/roland/28791363753/ )

what am i missing? If i am missing something please explain the feature, thanks! ....Roland Firefox for iOS and Android support aka SUMO

Comment 102

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

steps to reproduce:

1. in firefox for 49 beta 8, play a video from e.g. a video from aaron train's test page:
http://people.mozilla.org/~atrain/mobile/tests/media.html

2. switch to another android app and then open the android utility tray

expected behaviour:
I see a control to pause the video in firefox for android 49 beta 8 (i see this is in FF51 for android nightly)

actual result:
no control is displayed in firefox for android 49 beta 8 or firefox aurora 50

Comment 103

[Alastor Wu [:alwu]]

This feature can only be used on Nightly now (bug1290510), but it would be re-enable soon in bug1290836.

Comment 104

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

(In reply to Alastor Wu [:alwu] from comment #103)
> This feature can only be used on Nightly now (bug1290510), but it would be
> re-enable soon in bug1290836.

cool thanks for the update :alwu! 
When this feature finally lands can you change this FROM:
"RESOLVED FIXED in Firefox 49" 
to:
"RESOLVED FIXED in Firefox 52" (or whatever release it is?)

Comment 105

[Alastor Wu [:alwu]]

Sure, add NI for reminding to change the tracking flags.

Comment 106

[Barbara Bermes [:barbara] - NI please!]

Can you please confirm which release this is tracking?

Marketing and other teams assume this being shipped in 49 (as per the Aha card), if we can't do that, please let us know as soon as issues like these arise.

(Please NI me)

Comment 107

[Rachelle Yang [:ryang][[e-mail]]]

Hi Barbara and Roland, 

We plan to land and enable all these remote media control -related features on fennec in Firefox51.
We set a flag(pref-off) to control these fixes( in Bug 1290510 ) till it's ready and stable enough. 

At present, only one bug(Bug 1290467)for this media feature is not yet resolved and almost get review done.
After Bug 1290467 is ready, we will enable( pref-on) this media feature in all release channels (in Bug 1290836).

As for the target release, please refer to the comment3 in  Bug 1290836.
We hope we could make it in Firefox 51; But it's not ready, we will target 52.
Thank you very much !!

Comment 108

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

Agree with Sebastian in bug 1290836 comment 3. We should wait bug 1290467 to be landed and see if there are other problems.

Comment 109

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

(In reply to Rachelle Yang [:ryang][[e-mail]] from comment #107)
> Hi Barbara and Roland, 
> 
> We plan to land and enable all these remote media control -related features
> on fennec in Firefox51.
> We set a flag(pref-off) to control these fixes( in Bug 1290510 ) till it's
> ready and stable enough. 
> 
> At present, only one bug(Bug 1290467)for this media feature is not yet
> resolved and almost get review done.
> After Bug 1290467 is ready, we will enable( pref-on) this media feature in
> all release channels (in Bug 1290836).
> 
> As for the target release, please refer to the comment3 in  Bug 1290836.
> We hope we could make it in Firefox 51; But it's not ready, we will target
> 52.
> Thank you very much !!

Cheers Rachelle!

sounds good! fingers crossed for 51 but if not, 52 will work too! Clearing needinfo

...Roland

Comment 110

[Barbara Bermes [:barbara] - NI please!]

https://mozilla.aha.io/features/FENN-452

Comment 111

[Alastor Wu [:alwu]]

Fixed in FF52, see bug1290836.

Comment 112

[Bogdan Surd, QA [:bsurd, NI]]

Hello,

Tested on Nexus 9 (Android 7.0) on 52.0a1 (2016-11-10) and no problems were found.