Only enable mozFMRadio in B2G and tests

RESOLVED INVALID

Status

()

Core
DOM: Device Interfaces
RESOLVED INVALID
5 years ago
4 years ago

People

(Reporter: reuben, Assigned: reuben)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 751897 [details] [diff] [review]
Only enable mozFMRadio in B2G and tests

Marketplace needs to tell "no permission" and "not implemented here" apart.
Attachment #751897 - Flags: review?(justin.lebar+bug)
Overriding this pref for testing seems like a bad idea.  The mochitest framework should match desktop firefox as closely as possible, imo.  Tests which need this pref enabled should set it themselves using SpecialPowers.pushPrefEnv.

Also, I don't think we should assume that all B2G devices have a built-in FM radio.  If the goal is to allow apps to know whether the current device has a built-in radio, we may need to be more clever...
Attachment #751897 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 2

5 years ago
(In reply to Justin Lebar [:jlebar] from comment #1)
> Overriding this pref for testing seems like a bad idea.  The mochitest
> framework should match desktop firefox as closely as possible, imo.  Tests
> which need this pref enabled should set it themselves using
> SpecialPowers.pushPrefEnv.

The pref is not observed, it's checked once at XPCOM category registration time.

> Also, I don't think we should assume that all B2G devices have a built-in FM
> radio.  If the goal is to allow apps to know whether the current device has
> a built-in radio, we may need to be more clever...

How does the API work if the device does not have built-in FM radio? Should you still be able to call it, but get noop/error results? The objective here is to allow Marketplace to check if an app that uses the API will not work at all, so they can hide it in search results, etc. For example, someone browsing Marketplace on Desktop should never see apps that use the FM radio API.
> The pref is not observed, it's checked once at XPCOM category registration time.

Yeah, you're right.  I don't like it, but what you have is the easiest thing to do, and we shouldn't get hung up on that.

> How does the API work if the device does not have built-in FM radio? Should you still be 
> able to call it, but get noop/error results?

I don't see why the FM radio API should operate any differently on a phone which doesn't have an FM radio as compared to on a desktop machine.  Is there a good reason not to have the same behavior?

> For example, someone browsing Marketplace on Desktop should never see apps that use the 
> FM radio API.

I don't see why desktop vs. mobile should be the distinction rather than "on a device which has an FM radio" vs. "on a device which doesn't"...
(Assignee)

Comment 4

5 years ago
(In reply to Justin Lebar [:jlebar] from comment #3)
> > How does the API work if the device does not have built-in FM radio? Should you still be 
> > able to call it, but get noop/error results?
> 
> I don't see why the FM radio API should operate any differently on a phone
> which doesn't have an FM radio as compared to on a desktop machine.  Is
> there a good reason not to have the same behavior?

Well, for one, it can't actually do anything if there's no FM antenna…
It appears that right now we're not even shipping it on anything that isn't B2G. Gregor, where did you see the permission error for Marketplace? Was it on the device?

> > For example, someone browsing Marketplace on Desktop should never see apps that use the 
> > FM radio API.
> 
> I don't see why desktop vs. mobile should be the distinction rather than "on
> a device which has an FM radio" vs. "on a device which doesn't"...

That was just an example, I meant "someone browsing Marketplace on a device that doesn't have an FM antenna".
Flags: needinfo?(anygregor)
> Well, for one, it can't actually do anything if there's no FM antenna…

Maybe we're not understanding each other?

I'm saying that, if you're on a mobile device which doesn't have an FM radio chip, it should behave exactly the same as a desktop machine which also does not have an FM radio chip, because, as you said, the FM radio API can't do anything interesting in either case.

> I meant "someone browsing Marketplace on a device that doesn't have an FM antenna".

Okay.  So then we agree that this patch isn't what we want to do, because it turns on the FM radio for all Gonk devices?  Or are you saying that all gonk devices presently have an FM radio chip?  (I have no idea if this is true.)
(Assignee)

Comment 6

5 years ago
Comment on attachment 751897 [details] [diff] [review]
Only enable mozFMRadio in B2G and tests

(In reply to Justin Lebar [:jlebar] from comment #5)
> > Well, for one, it can't actually do anything if there's no FM antenna…
> 
> Maybe we're not understanding each other?
> 
> I'm saying that, if you're on a mobile device which doesn't have an FM radio
> chip, it should behave exactly the same as a desktop machine which also does
> not have an FM radio chip, because, as you said, the FM radio API can't do
> anything interesting in either case.

Ah, yes, I agree.

> > I meant "someone browsing Marketplace on a device that doesn't have an FM antenna".
> 
> Okay.  So then we agree that this patch isn't what we want to do, because it
> turns on the FM radio for all Gonk devices?  Or are you saying that all gonk
> devices presently have an FM radio chip?  (I have no idea if this is true.)

This patch does not "turn on FM for all Gonk devices" - in fact, it doesn't change anything for Gonk devices.

Having said that, the objective was to disable the navigator property on Android and Desktop, but it turns out it doesn't change anything there either, since it looks like we're only shipping FM radio on B2G: http://mxr.mozilla.org/mozilla-central/search?string=DOMFMRadio.manifest

I'm waiting on Gregor's response to mark this bug RESOLVED INVALID.
Attachment #751897 - Attachment is obsolete: true
> This patch does not "turn on FM for all Gonk devices" - in fact, it doesn't change 
> anything for Gonk devices.

Okay, fair.  It "turns off FM for non-gonk devices."  But the point is that disabling the navigator property on Android and Desktop isn't quite right -- we want to disable it /anywhere/ that we don't have an FM radio, which might include some Gonk devices.
(In reply to Reuben Morais [:reuben] from comment #4)
> (In reply to Justin Lebar [:jlebar] from comment #3)
> > > How does the API work if the device does not have built-in FM radio? Should you still be 
> > > able to call it, but get noop/error results?
> > 
> > I don't see why the FM radio API should operate any differently on a phone
> > which doesn't have an FM radio as compared to on a desktop machine.  Is
> > there a good reason not to have the same behavior?
> 
> Well, for one, it can't actually do anything if there's no FM antenna…
> It appears that right now we're not even shipping it on anything that isn't
> B2G. Gregor, where did you see the permission error for Marketplace? Was it
> on the device?

This was on the device. Right now the marketplace enumerates all APIs in order to see if they are available. Since the marketplace doesn't have the mozFM permission we print the warning.
We can also wait for the capability API but I don't know how long this will take to design/implement.
Flags: needinfo?(anygregor)
(Assignee)

Comment 9

5 years ago
The warning is harmless, in my opinion. Marking as INVALID, only enabling the API when we have an antenna should be a new bug.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
The behavior that marketplace is expecting is undefined if the feature is not available and null if it doesn't have sufficient privileges.
Right now we don't have a way to say it's not available.
You need to log in before you can comment on or make changes to this bug.