874263 - Only enable mozFMRadio in B2G and tests

Status: RESOLVED INVALID

(Core :: DOM: Device Interfaces, defect)

Description

[Reuben Morais [:reuben]]

Attachment: Only enable mozFMRadio in B2G and tests

Marketplace needs to tell "no permission" and "not implemented here" apart.

Comment 1

[Justin Lebar (not reading bugmail)]

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...

Comment 2

[Reuben Morais [:reuben]]

(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.

Comment 3

[Justin Lebar (not reading bugmail)]

> 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"...

Comment 4

[Reuben Morais [:reuben]]

(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".

Comment 5

[Justin Lebar (not reading bugmail)]

> 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.)

Comment 6

[Reuben Morais [:reuben]]

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.

Comment 7

[Justin Lebar (not reading bugmail)]

> 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.

Comment 8

[Gregor Wagner [:gwagner]]

(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.

Comment 9

[Reuben Morais [:reuben]]

The warning is harmless, in my opinion. Marking as INVALID, only enabling the API when we have an antenna should be a new bug.

Comment 10

[Gregor Wagner [:gwagner]]

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.