Implement the remote media-control on Fennec

RESOLVED FIXED in Firefox 52

Status

()

Firefox for Android
General
P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: alwu, Assigned: alwu)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 51
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified)

Details

(URL)

User Story

Fennec lacks a convenient way to directly control the media content, but other Android browsers (chrome, opera) already have that, see comment 1.

Think about following scenario.

When user is playing an media content by Fennec, then switch to other apps to do something. If user want to stop this media content, must have lots steps,
(1) switch back to Fennec
(2) find the audible tab if user have multiple tabs
(3) find the media control in that webpage

It's very cumbersome and inconvenience, I think we should provide an easier way to control media content.

--

When the user is playing the media from arbitrary web contents, then

(1) We should provide the user control interface in the lock-screen and utility tray
(2) We should enable the hardware button control for that media

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments, 1 obsolete attachment)

58 bytes, text/x-review-board-request
ahunt
: review+
Details | Review
58 bytes, text/x-review-board-request
baku
: review+
Details | Review
58 bytes, text/x-review-board-request
baku
: review+
Details | Review
58 bytes, text/x-review-board-request
snorp
: review+
Details | Review
58 bytes, text/x-review-board-request
baku
: review+
Details | Review
58 bytes, text/x-review-board-request
snorp
: review+
Details | Review
58 bytes, text/x-review-board-request
baku
: review+
Details | Review
58 bytes, text/x-review-board-request
baku
: review+
Details | Review
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.
Depends on: 1242874
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
Component: AudioChannel → General
Product: Firefox OS → Firefox for Android
Summary: Implement the media-control for the audio of web contents → Implement the remote media-control on Fennec
Depends on: 1249579
Created attachment 8723999 [details]
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/
User Story: (updated)
Hi, Margaret & Barbara,
I would like to propose this feature to Fennec, could you give me some suggestions?
Very appreciate :)
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(bbermes)
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
Flags: needinfo?(bbermes)
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/

Updated

2 years ago
Flags: needinfo?(margaret.leibovic)

Comment 6

2 years ago
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.
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 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/
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!
Flags: needinfo?(alam)
(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!
Flags: needinfo?(alam) → needinfo?(alwu)
(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).
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!
Flags: needinfo?(alwu) → needinfo?(alam)
(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?
Flags: needinfo?(alam) → needinfo?(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.
Flags: needinfo?(alwu)
Depends on: 1257738
OS: Gonk (Firefox OS) → Android

Comment 15

2 years ago
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 hidden (obsolete)

Updated

2 years ago
Blocks: 1264901

Comment 17

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

Patch moved to bug 1264901.
Attachment #8741691 - Attachment is obsolete: true

Comment 18

2 years ago
You should make sure Chenxia or Sebastian review any Android UI changes here.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(liuche)
The UI part would be moved to bug 1264901, I would remove them from these patches.

Comment 20

2 years ago
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.
Flags: needinfo?(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.
Flags: needinfo?(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/
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
Depends on: 1235612
Removing flags here. I'll comment in bug 1264901.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(liuche)
Blocks: 1268367
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/
Depends on: 1269672
Created attachment 8749600 [details]
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/
Attachment #8723999 - Attachment description: MozReview Request: Bug 1240423 - implement the remote media-control on Fennec. → MozReview Request: Bug 1240423 - part1 : implement the remote media-control on Fennec.
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 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 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/
Priority: -- → P2
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 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/
Created attachment 8751659 [details]
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/
Attachment #8749600 - Attachment description: MozReview Request: Bug 1240423 : part2 : dispatch audio-playback event when pausing from remote control. → MozReview Request: Bug 1240423 - part2 : introduce audible changing reasons.
Created attachment 8751660 [details]
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 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 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/
Created attachment 8751695 [details]
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 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 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 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 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 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/
Attachment #8751659 - Attachment description: MozReview Request: Bug 1240423 - part3 : add audible changing reasion when media element notify audible changing. → MozReview Request: Bug 1240423 - part3 : add reason when media element notify audible changing.
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 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 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 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/
Created attachment 8752090 [details]
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/
Attachment #8751660 - Attachment description: MozReview Request: Bug 1240423 - part4 : modify tests. → MozReview Request: Bug 1240423 - part4 : stop_disposable should reset mSuspendState.
Attachment #8751695 - Attachment description: MozReview Request: Bug 1240423 - part5 : remove useless pref. → MozReview Request: Bug 1240423 - part5 : add attributes for AudioChannelLog.
Created attachment 8752091 [details]
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 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 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 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 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 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/
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a45eeda2877
If the try-result is good, I'll ask for review.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93c7bee85fe3
Created attachment 8752742 [details]
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/
Attachment #8751660 - Attachment description: MozReview Request: Bug 1240423 - part4 : stop_disposable should reset mSuspendState. → MozReview Request: Bug 1240423 - part4 : update audio playing window checking in AndroidBridge.
Attachment #8751695 - Attachment description: MozReview Request: Bug 1240423 - part5 : add attributes for AudioChannelLog. → MozReview Request: Bug 1240423 - part5 : stop_disposable should reset mSuspendState.
Attachment #8752090 - Attachment description: MozReview Request: Bug 1240423 - part6 : remove useless pref. → MozReview Request: Bug 1240423 - part6 : remove useless pref in Android.
Attachment #8752091 - Attachment description: MozReview Request: Bug 1240423 - part7 : modify tests → MozReview Request: Bug 1240423 - part7 : add attributes for AudioChannelLog.
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 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 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 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 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 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 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/
Attachment #8723999 - Flags: review?(margaret.leibovic)
Attachment #8749600 - Flags: review?(amarchesini)
Attachment #8751659 - Flags: review?(amarchesini)
Attachment #8751660 - Flags: review?(snorp)
Attachment #8751695 - Flags: review?(amarchesini)
Attachment #8752090 - Flags: review?(snorp)
Attachment #8752091 - Flags: review?(amarchesini)
Attachment #8752742 - Flags: review?(amarchesini)
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

2 years ago
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.
Attachment #8723999 - Flags: review?(ahunt)
Comment on attachment 8751660 [details]
MozReview Request: Bug 1240423 - part4 : update audio playing window checking in AndroidBridge.

https://reviewboard.mozilla.org/r/52149/#review50024
Attachment #8751660 - Flags: review?(snorp) → review+
Comment on attachment 8752090 [details]
MozReview Request: Bug 1240423 - part6 : remove useless pref in Android.

https://reviewboard.mozilla.org/r/52415/#review50026
Attachment #8752090 - Flags: review?(snorp) → review+

Comment 68

2 years ago
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).
Attachment #8723999 - Flags: review?(ahunt) → review+
(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 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 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 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 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 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 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 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 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/

Updated

2 years ago
Attachment #8723999 - Flags: review?(margaret.leibovic)
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!
Flags: needinfo?(margaret.leibovic)

Comment 79

2 years ago
(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 :)
Flags: needinfo?(margaret.leibovic)
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
QA Contact: sorina.florean
review ping?
Flags: needinfo?(amarchesini)
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)
{
...
Attachment #8749600 - Flags: review?(amarchesini) → review+
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
Attachment #8751659 - Flags: review?(amarchesini) → review+
Attachment #8751695 - Flags: review?(amarchesini) → review+
Comment on attachment 8751695 [details]
MozReview Request: Bug 1240423 - part5 : stop_disposable should reset mSuspendState.

https://reviewboard.mozilla.org/r/52159/#review53074
Comment on attachment 8752091 [details]
MozReview Request: Bug 1240423 - part7 : add attributes for AudioChannelLog.

https://reviewboard.mozilla.org/r/52417/#review53076
Attachment #8752091 - Flags: review?(amarchesini) → review+
Comment on attachment 8752742 [details]
MozReview Request: Bug 1240423 - part8 : modify tests

https://reviewboard.mozilla.org/r/52746/#review53078
Attachment #8752742 - Flags: review?(amarchesini) → review+
Thanks for review :)
Flags: needinfo?(amarchesini)
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/
Attachment #8723999 - Flags: review?(margaret.leibovic)
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 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 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 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 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 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 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/
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fb8a0e4f4cf

Comment 97

2 years ago
Pushed by alwu@mozilla.com:
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

Updated

2 years ago
Attachment #8723999 - Flags: review?(margaret.leibovic)
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
Flags: needinfo?(alwu)

Updated

2 years ago
Depends on: 1277373
Take bug1277373.
Flags: needinfo?(alwu)

Comment 100

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Depends on: 1286818

Updated

a year ago
Depends on: 1288970
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
This feature can only be used on Nightly now (bug1290510), but it would be re-enable soon in bug1290836.
Status: REOPENED → RESOLVED
Last Resolved: 2 years agoa year ago
Resolution: --- → FIXED
Depends on: 1290467
(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?)
Sure, add NI for reminding to change the tracking flags.
Flags: needinfo?(alwu)
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)
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 !!
Flags: needinfo?(rtanglao)
Flags: needinfo?(bbermes)
Agree with Sebastian in bug 1290836 comment 3. We should wait bug 1290467 to be landed and see if there are other problems.
(In reply to Rachelle Yang [:ryang][ryang@mozilla.com] 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
Flags: needinfo?(rtanglao)
https://mozilla.aha.io/features/FENN-452
Flags: needinfo?(bbermes)
Target Milestone: Firefox 49 → Firefox 51
Fixed in FF52, see bug1290836.
status-firefox52: --- → fixed
Flags: needinfo?(alwu)
status-firefox49: fixed → ---
Hello,

Tested on Nexus 9 (Android 7.0) on 52.0a1 (2016-11-10) and no problems were found.
status-firefox52: fixed → verified
You need to log in before you can comment on or make changes to this bug.