779500 - WebFM

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Pin Zhang [:pzhang] (inactive)]

Implement the proposed WebFM API: https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.webapi/PraULCQntqA

Comment 1

[Pin Zhang [:pzhang] (inactive)]

Attachment: IDLs

The attribute "speakerEnabled" is not introduced in the proposed API, but the feature to speaker/headset is required in V1.

Comment 2

[Pin Zhang [:pzhang] (inactive)]

Attachment: option MOZ_B2G_FM

Comment 3

[Pin Zhang [:pzhang] (inactive)]

Attachment: DOMFmManager, FmAdaptor and FmAntenna

Comment 4

[Pin Zhang [:pzhang] (inactive)]

Attachment: Fallback service

Comment 5

[Pin Zhang [:pzhang] (inactive)]

Attachment: Add Permissions

Comment 6

[Pin Zhang [:pzhang] (inactive)]

Attachment: ServiceStartup

Comment 7

[Pin Zhang [:pzhang] (inactive)]

 The patch is incomplete, the feature to toggle speaker/headset is not be implemented, it is blocked by bug 749053.

Comment 8

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 647944 [details] [diff] [review]
Fallback service

the logic related to query speaker state and set speakerEnabled is not finished, i marked as "TODO" items, so the reviewer should skip this.

Comment 9

[Justin Lebar (not reading bugmail)]

Can you clarify what is the status of the patches with r? but no reviewer specified?  Would you like me to review those as well?

Comment 10

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #9)
> Can you clarify what is the status of the patches with r? but no reviewer
> specified?  Would you like me to review those as well?

Those patches are ready to be reviewed, i just don't know who i should request, so if you would review those as well, that's great!

Comment 11

[Justin Lebar (not reading bugmail)]

> Those patches are ready to be reviewed, i just don't know who i should request, so if you would 
> review those as well, that's great!

I think I can review them, judging by the diffstats.  (Thanks for including those, by the way!)  I'll have a look soon.

FYI, empty review?'s are often ignored, so it's better to guess at a reviewer and ask if you guessed correctly; if you asked the wrong person, they'll usually re-assign the review to someone better.

Comment 12

[Justin Lebar (not reading bugmail)]

Comment on attachment 647942 [details] [diff] [review]
option MOZ_B2G_FM

I don't feel comfortable reviewing these build changes -- off to a build peer.

Comment 13

[Justin Lebar (not reading bugmail)]

Comment on attachment 647944 [details] [diff] [review]
Fallback service

I don't understand why dom/fm/fallback/FmService.jsm is a fallback service.  Maybe the word "fallback" is getting lost in translation?

> Fallback -- something or someone to turn or return to, especially for help or as 
> an alternative: "His teaching experience would be a fallback if the business 
> failed."

The "fallback" service would be the one which does nothing when the FM radio is unavailable.

Am I missing something here?

Comment 14

[Justin Lebar (not reading bugmail)]

Comment on attachment 647940 [details] [diff] [review]
IDLs

I'm sorry that I have more than minor nits on this IDL, despite the fact that
we went through it on the mailing list.  I hope you won't have to make too many
big changes.

Jonas, I'd appreciate your thoughts on a few comments below.

>diff --git a/dom/fm/Makefile.in b/dom/fm/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/dom/fm/Makefile.in
>@@ -0,0 +1,50 @@
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+# [...]

MPL2, please.

>--- /dev/null
>+++ b/dom/fm/nsIFm.idl
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsISupports.idl"
>+#include "nsIDOMDOMRequest.idl"
>+
>+[scriptable, uuid(1d0443f3-ac30-4f9e-a070-002bb20ce1e6)]
>+interface nsIDOMFmManager : nsISupports {

The filename should match the name of the interface.  Also, "FM", not "Fm", please.

I also think it would be better if we called this interface nsIDOMFMRadio.  For
example, it's nsIDOMScreen, not nsIDOMScreenManager.

>+    attribute boolean speakerEnabled;

Shouldn't this be part of the audio API?

>+    readonly attribute boolean enabled;
>+
>+    // Current frequency, unit: MHz

Nit: "Current frequency, in MHz".

>+    // Signal strength, the range is [0-1]
>+    readonly attribute float signalStrength;

Nit: Why is this a float, while the frequency is a double?  I'd rather use
doubles everywhere, unless there's a reason not to.

Can we add the signal strength in dBm as well, if we have that available?

>+    readonly attribute boolean antennaAvailable;
>+
>+     Returns the band ranges of FM which is a JSON Object like this:
>+     {
>+       FM: {
>+         lower: 87.5,    // unit: MHz
>+         upper: 108
>+       }
>+     }
>+    */
>+    readonly attribute jsval bandRanges;

Nit: Please use javadoc-like formatting for this and all the other comments in the idl files:

 /**
  * Comment comment comment.
  */
 readonly attribute foo;

There's no such thing as a "JSON object" -- that's just an Object in Javascript.  :)

We need to figure out what's going on with these bands.  If the goal is
to support things other than FM radio, then we shouldn't call this
nsIDOMFMManager/nsIDOMFMRadio.

My instictint is to ignore AM, which I think we're unlikely to support due to
physical constraints (the antenna needs to be large).  Then we can spec this
attribute as follows:

"Returns the band ranges supported by this FM radio.  The return
value is a list of objects, each of which indicates the band's upper and lower
limits, in MHz.  For example:

       [ {lower: 87.5, upper: 108},
         {lower: 76,   upper: 90} ]

Also, if we expose all the supported bands on this object, then it doesn't make a lot of sense to me that you can't change the current band using this object.  At the very least, we need to add an attribute which indicates what is the current band.  But IMO, if you can't select a band using nsIDOMFMRadio, we shouldn't tease you with the list of available bands, and should only give you the upper and lower limits of the currently-selected band.

We also need to give the station interval, since that changes with different bands.

Jonas and Ray, what do you think?

>+    attribute nsIDOMEventListener onsignalchange;

onsignalstrength change, perhaps?

Also, could you please add a comment here indicating how we throttle these events?

>+    attribute nsIDOMEventListener onantennachange;

onantennaavailablechange, or onantennastatechange?  Otherwise it sounds like we're changing the antenna itself.

>+    attribute nsIDOMEventListener onenabled;
>+
>+    attribute nsIDOMEventListener ondisabled;

It's weird to have one event which fires when the antenna is plugged/unplugged but then fire a different event 

>+    attribute nsIDOMEventListener onfrequencychange;

We want to support addEventListener() for these too, which means that
nsIDOMFMRadio needs to inherit from nsIDOMEventTarget.

>+    nsIDOMDOMRequest setEnabled(in bool enabled);
>+
>+    // If frequency is out of band ranges, onerror will be fired immediately.
>+    nsIDOMDOMRequest setFrequency(in double frequency);

Nit: "If the given frequency is outside the current band range".

I don't think it's right to say that onerror is fired "immediately".  Surely
you have to wait until we spin the event loop, because otherwise the following
won't work:

  let req = setFrequency(0); // error, fires req.onerror
  req.onerror = function() { dump("An error occurred"); };
  // The dump() is never run, because onerror was already fired.

>+    nsIDOMDOMRequest seekDown();
>+
>+    nsIDOMDOMRequest seekUp();
>+
>+    nsIDOMDOMRequest cancelSeek();

What happens if you call seekDown/seekUp while a seek is running?

>diff --git a/dom/fm/nsIFmAdaptor.idl b/dom/fm/nsIFmAdaptor.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/fm/nsIFmAdaptor.idl
>@@ -0,0 +1,37 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsISupports.idl"
>+#include "nsIDOMEvent.idl"
>+#include "nsIDOMEventTarget.idl"
>+
>+[scriptable, builtinclass, uuid(26288adc-d2c1-4fbc-86b5-ecd8173fbf90)]

It's not wrong to put builtinclass here, but it's a little odd.  khuey says:

  Typically we've added builtinclass to interfaces where if JS implemented it,
  sg:crits (i.e., critical security bugs) would result.  But feel free to
  sprinkle it liberally.

I'd probably leave it off in this case, since nsIFMAdaptor /could/ be implemented in JS with no problems.

Could you please add a comment here indicating what this interface is for?  It took me a while to figure it out.  :)

>+interface nsIFmAdaptor : nsIDOMEventTarget {

nsIFMAdaptor, please, and change the filename to match.  But I'd prefer this matched the name of the DOM interface.  What do you think about calling them nsIDOMFMRadio and nsIFMRadio?

This one is an nsIDOMEventTarget, so maybe you just forgot to put that on the other interface?

>+    readonly attribute boolean enabled;
>+
>+    readonly attribute long frequency;
>+
>+    readonly attribute float signalStrength;

Why does this interface duplicate some (but not all) of nsIDOMFMAdaptor's properties?  I'd rather not duplicate any and just call into nsIDOMFMAdaptor when necessary.

>+    [implicit_jscontext]
>+    void enable(in jsval settings);
>+
>+    void disable();
>+
>+    void seek(in short direction);
>+
>+    void cancelSeek();
>+
>+    [implicit_jscontext]
>+    jsval getSettings();
>+
>+    void setFrequency(in long frequency);
>+
>+    attribute nsIDOMEventListener onseekcomplete;
>+
>+    attribute nsIDOMEventListener onenabled;
>+
>+    attribute nsIDOMEventListener ondisabled;
>+};

>diff --git a/dom/fm/nsIFmAntenna.idl b/dom/fm/nsIFmAntenna.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/fm/nsIFmAntenna.idl
>@@ -0,0 +1,15 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsISupports.idl"
>+#include "nsIDOMEvent.idl"
>+#include "nsIDOMEventTarget.idl"
>+
>+[scriptable, builtinclass, uuid(a68f2143-5665-49ed-a9df-2878785455bf)]

Again, I don't think builtinclass is exactly right here.

Please add a comment indicating that this is implemented by the FM antenna service.

I don't understand why this isn't part of nsIFMAdaptor.  Even if we wanted multiple radios, I don't see how splitting out the interface like this helps us.

>+interface nsIFmAntenna : nsIDOMEventTarget
>+{
>+    readonly attribute boolean isAvailable;
>+    attribute nsIDOMEventListener onstatechange;
>+};

Comment 15

[Justin Lebar (not reading bugmail)]

I'm not going to look at the remaining patches until we get this IDL actually settled, so clearing the r?'s for now.

Comment 16

[Justin Lebar (not reading bugmail)]

> Why does [nsIFMManager] duplicate some (but not all) of nsIDOMFMAdaptor's properties?  I'd rather 
> not duplicate any and just call into nsIDOMFMAdaptor when necessary.

I realize now that I probably don't have enough context to understand this interface, having read only part 1.  Maybe we can figure out the DOM interface and worry about this interface in a later round of reviews.

Comment 17

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #14)
> Comment on attachment 647940 [details] [diff] [review]
> IDLs
> 
> I'm sorry that I have more than minor nits on this IDL, despite the fact that
> we went through it on the mailing list.  I hope you won't have to make too
> many
> big changes.
> 
> Jonas, I'd appreciate your thoughts on a few comments below.
> 
> >diff --git a/dom/fm/Makefile.in b/dom/fm/Makefile.in
> >new file mode 100644
> >--- /dev/null
> >+++ b/dom/fm/Makefile.in
> >@@ -0,0 +1,50 @@
> >+# ***** BEGIN LICENSE BLOCK *****
> >+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
> >+# [...]
> 
> MPL2, please.
OK
> 
> >--- /dev/null
> >+++ b/dom/fm/nsIFm.idl
> >+/* This Source Code Form is subject to the terms of the Mozilla Public
> >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
> >+
> >+#include "nsISupports.idl"
> >+#include "nsIDOMDOMRequest.idl"
> >+
> >+[scriptable, uuid(1d0443f3-ac30-4f9e-a070-002bb20ce1e6)]
> >+interface nsIDOMFmManager : nsISupports {
> 
> The filename should match the name of the interface.  Also, "FM", not "Fm",
> please.
> 
> I also think it would be better if we called this interface nsIDOMFMRadio. 
> For
> example, it's nsIDOMScreen, not nsIDOMScreenManager.
> 
Yep, it's better.
I just take other APIs for references to name it, for example, ContactManager, WifiManager etc.
> >+    attribute boolean speakerEnabled;
> 
> Shouldn't this be part of the audio API?
I was told that there is no such a specific API, maybe a setting will be introduced in the future, but right now, the WebAPI have to implement it by itself.
> 
> >+    readonly attribute boolean enabled;
> >+
> >+    // Current frequency, unit: MHz
> 
> Nit: "Current frequency, in MHz".
> 
> >+    // Signal strength, the range is [0-1]
> >+    readonly attribute float signalStrength;
> 
> Nit: Why is this a float, while the frequency is a double?  I'd rather use
> doubles everywhere, unless there's a reason not to.
> 
It is float in the proposed API.
> Can we add the signal strength in dBm as well, if we have that available?
> 
For the Otoro device, i am not sure if there is such a interface to get signal strength in dBm, i will double check with Steven who is working on the bug 749053.

> >+    readonly attribute boolean antennaAvailable;
> >+
> >+     Returns the band ranges of FM which is a JSON Object like this:
> >+     {
> >+       FM: {
> >+         lower: 87.5,    // unit: MHz
> >+         upper: 108
> >+       }
> >+     }
> >+    */
> >+    readonly attribute jsval bandRanges;
> 
> Nit: Please use javadoc-like formatting for this and all the other comments
> in the idl files:
> 
>  /**
>   * Comment comment comment.
>   */
>  readonly attribute foo;
> 
> There's no such thing as a "JSON object" -- that's just an Object in
> Javascript.  :)
> 
OK
> We need to figure out what's going on with these bands.  If the goal is
> to support things other than FM radio, then we shouldn't call this
> nsIDOMFMManager/nsIDOMFMRadio.
> 
> My instictint is to ignore AM, which I think we're unlikely to support due to
> physical constraints (the antenna needs to be large).  Then we can spec this
> attribute as follows:
> 
> "Returns the band ranges supported by this FM radio.  The return
> value is a list of objects, each of which indicates the band's upper and
> lower
> limits, in MHz.  For example:
> 
>        [ {lower: 87.5, upper: 108},
>          {lower: 76,   upper: 90} ]
> 
> Also, if we expose all the supported bands on this object, then it doesn't
> make a lot of sense to me that you can't change the current band using this
> object.  At the very least, we need to add an attribute which indicates what
> is the current band.  But IMO, if you can't select a band using
> nsIDOMFMRadio, we shouldn't tease you with the list of available bands, and
> should only give you the upper and lower limits of the currently-selected
> band.
> 
> We also need to give the station interval, since that changes with different
> bands.
> 
> Jonas and Ray, what do you think?
> 

I think we have discussed on this in the email, AM might be covered in the future, and that's why the bandRanges object looks like: {"FM": {"upper": 108, "lower": 87.5}}.
And in v1, there is no plan to add an option to change the band in the Settings app,  so "onbandchange" is not introduced to the API in v1, and it is unnecessary to introduce the station interval either.

> >+    attribute nsIDOMEventListener onsignalchange;
> 
> onsignalstrength change, perhaps?
> 
> Also, could you please add a comment here indicating how we throttle these
> events?
> 
OK
> >+    attribute nsIDOMEventListener onantennachange;
> 
> onantennaavailablechange, or onantennastatechange?  Otherwise it sounds like
> we're changing the antenna itself.
> 
> >+    attribute nsIDOMEventListener onenabled;
> >+
> >+    attribute nsIDOMEventListener ondisabled;
> 
> It's weird to have one event which fires when the antenna is
> plugged/unplugged but then fire a different event 
> 

> >+    attribute nsIDOMEventListener onfrequencychange;
> 
> We want to support addEventListener() for these too, which means that
> nsIDOMFMRadio needs to inherit from nsIDOMEventTarget.
> 
> >+    nsIDOMDOMRequest setEnabled(in bool enabled);
> >+
> >+    // If frequency is out of band ranges, onerror will be fired immediately.
> >+    nsIDOMDOMRequest setFrequency(in double frequency);
> 
> Nit: "If the given frequency is outside the current band range".
> 
> I don't think it's right to say that onerror is fired "immediately".  Surely
> you have to wait until we spin the event loop, because otherwise the
> following
> won't work:
> 
>   let req = setFrequency(0); // error, fires req.onerror
>   req.onerror = function() { dump("An error occurred"); };
>   // The dump() is never run, because onerror was already fired.
> 
Yep, the expression is not accurate.
> >+    nsIDOMDOMRequest seekDown();
> >+
> >+    nsIDOMDOMRequest seekUp();
> >+
> >+    nsIDOMDOMRequest cancelSeek();
> 
> What happens if you call seekDown/seekUp while a seek is running?
> 
Fail, request.onerror will be fired.
> >diff --git a/dom/fm/nsIFmAdaptor.idl b/dom/fm/nsIFmAdaptor.idl
> >new file mode 100644
> >--- /dev/null
> >+++ b/dom/fm/nsIFmAdaptor.idl
> >@@ -0,0 +1,37 @@
> >+/* This Source Code Form is subject to the terms of the Mozilla Public
> >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
> >+
> >+#include "nsISupports.idl"
> >+#include "nsIDOMEvent.idl"
> >+#include "nsIDOMEventTarget.idl"
> >+
> >+[scriptable, builtinclass, uuid(26288adc-d2c1-4fbc-86b5-ecd8173fbf90)]
> 
> It's not wrong to put builtinclass here, but it's a little odd.  khuey says:
> 
>   Typically we've added builtinclass to interfaces where if JS implemented
> it,
>   sg:crits (i.e., critical security bugs) would result.  But feel free to
>   sprinkle it liberally.
> 
> I'd probably leave it off in this case, since nsIFMAdaptor /could/ be
> implemented in JS with no problems.
> 
Actually, i don't quite understancd what "builtinclass" is used for, i implemented it without such a modifier,
when i rebased my working branch with latest m-c at one day last month, it failed to compile, then i added the "builtinclass"
just like other idls did.

> Could you please add a comment here indicating what this interface is for? 
> It took me a while to figure it out.  :)
> 
> >+interface nsIFmAdaptor : nsIDOMEventTarget {
> 
> nsIFMAdaptor, please, and change the filename to match.  But I'd prefer this
> matched the name of the DOM interface.  What do you think about calling them
> nsIDOMFMRadio and nsIFMRadio?
> 
Good suggestion~

> This one is an nsIDOMEventTarget, so maybe you just forgot to put that on
> the other interface?
> 
Other IDLs, such as wiki, is just a nsISupports, why? JS Component?

> >+    readonly attribute boolean enabled;
> >+
> >+    readonly attribute long frequency;
> >+
> >+    readonly attribute float signalStrength;
> 
> Why does this interface duplicate some (but not all) of nsIDOMFMAdaptor's
> properties?  I'd rather not duplicate any and just call into nsIDOMFMAdaptor
> when necessary.
> 
The interfaces of FM are implemented in C Plus Plus (you can check the patched in bug 749053 ), and i have to wrapped it into a XPCOM Object which could be called in the JS file (i.e. DOMFmManager.js).
> >+    [implicit_jscontext]
> >+    void enable(in jsval settings);
> >+
> >+    void disable();
> >+
> >+    void seek(in short direction);
> >+
> >+    void cancelSeek();
> >+
> >+    [implicit_jscontext]
> >+    jsval getSettings();
> >+
> >+    void setFrequency(in long frequency);
> >+
> >+    attribute nsIDOMEventListener onseekcomplete;
> >+
> >+    attribute nsIDOMEventListener onenabled;
> >+
> >+    attribute nsIDOMEventListener ondisabled;
> >+};
> 
> >diff --git a/dom/fm/nsIFmAntenna.idl b/dom/fm/nsIFmAntenna.idl
> >new file mode 100644
> >--- /dev/null
> >+++ b/dom/fm/nsIFmAntenna.idl
> >@@ -0,0 +1,15 @@
> >+/* This Source Code Form is subject to the terms of the Mozilla Public
> >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
> >+
> >+#include "nsISupports.idl"
> >+#include "nsIDOMEvent.idl"
> >+#include "nsIDOMEventTarget.idl"
> >+
> >+[scriptable, builtinclass, uuid(a68f2143-5665-49ed-a9df-2878785455bf)]
> 
> Again, I don't think builtinclass is exactly right here.
> 
> Please add a comment indicating that this is implemented by the FM antenna
> service.
> 
> I don't understand why this isn't part of nsIFMAdaptor.  Even if we wanted
> multiple radios, I don't see how splitting out the interface like this helps
> us.
> 
In the very first implementation, it is for the Si4709 chip of SGS2, it's a long story ...
Anyway, i will merge it to nsIFMAdaptor.

> >+interface nsIFmAntenna : nsIDOMEventTarget
> >+{
> >+    readonly attribute boolean isAvailable;
> >+    attribute nsIDOMEventListener onstatechange;
> >+};

Comment 18

[Justin Lebar (not reading bugmail)]

> > >+    attribute boolean speakerEnabled;
> > 
> > Shouldn't this be part of the audio API?
> I was told that there is no such a specific API, maybe a setting will be
> introduced in the future, but right now, the WebAPI have to implement it by
> itself.

If that's the reason, we should make this a separate API.  It's not a lot more
work to make a separate API than it is to include it on the FM radio API.

If FM radio were accessible only to privileged content, then maybe I'd be OK
with a hack like this, but if we're exposing this to the world, then we should
not create bogus APIs because it's slightly easier.

> > >+    // Signal strength, the range is [0-1]
> > >+    readonly attribute float signalStrength;
> > 
> > Nit: Why is this a float, while the frequency is a double?  I'd rather use
> > doubles everywhere, unless there's a reason not to.
> > 
> It is float in the proposed API.

But isn't this the proposed API?  I'm asking, why is the proposed API for
signal strength a float and not a double?

> > We need to figure out what's going on with these bands.  If the goal is
> > to support things other than FM radio, then we shouldn't call this
> > nsIDOMFMManager/nsIDOMFMRadio.
> 
> I think we have discussed on this in the email, AM might be covered in the
> future, and that's why the bandRanges object looks like: {"FM": {"upper":
> 108, "lower": 87.5}}.

AM radio should not be part of the "FM radio" api.  If we really want to be
able to support AM in the future -- and I think that's unlikely to happen,
because AM antennas are quite large -- then we should rename this API to
"TunableRadio" or something.  But I think designing for an unlikely eventuality
is likely a mistake.

If we want to support multiple bands then we should design bandRanges to let
you see multiple FM and multiple AM bands.  At the moment, you can only see one
FM and one AM band.  But if we're designing for future extensibility to AM, we
should also design for future extensibility to multiple FM bands, because the
latter is far more reasonable than the former.

But more to the point: We need some way for the app to know which is the
currently-selected band; apps shouldn't assume that there's only one band in
bandRanges, because that defeats the purpose.

So regardless of whether we're allowing multiple bands now or in the future, we
still need to have

  double upperFreq;
  double lowerFreq
  double channelFreqInterval;

or otherwise

  jsval currentBand;  /* {upper, lower, interval} */

so that you know what the current band is set to.  IMO, if we're not allowing
switching between bands, we should not expose bandRanges, because it's not
useful.

> so "onbandchange" is not introduced to the API in v1, and it
> is unnecessary to introduce the station interval either.

Why doesn't the FM radio app need the interval?  Are you expecting that the app
will have a hardcoded interval?  Or that it will always use seek()?  What if an
app wants to allow the user to manually tune the radio?

> > >+    nsIDOMDOMRequest seekDown();
> > >+
> > >+    nsIDOMDOMRequest seekUp();
> > >+
> > >+    nsIDOMDOMRequest cancelSeek();
> > 
> > What happens if you call seekDown/seekUp while a seek is running?
> > 
> Fail, request.onerror will be fired.

Okay, can you please document that we can never have multiple seeks going at
once, so cancelSeek cancels the one outstanding seek or otherwise fails?

> Actually, i don't quite understancd what "builtinclass" is used for, i
> implemented it without such a modifier,
	> when i rebased my working branch with latest m-c at one day last month, it
	> failed to compile, then i added the "builtinclass"
	> just like other idls did.

What's the compile error?  I'm not totally sure what builtinclass is for
either, but khuey gave me the impression that it did not affect compilation.

> > This one is an nsIDOMEventTarget, so maybe you just forgot to put that on
> > the other interface?
> > 
> Other IDLs, such as wiki, is just a nsISupports, why? JS Component?

Which other IDLs are you talking about?  I'm not sure what you mean by "such as
wiki".

nsIDOMEventTarget is the interface which adds add/removeEventListener.  If you
don't inherit from nsIDOMEventTarget, you can't do

  fmManager.addEventListener('antennachange', function() { ... });

> > >+    readonly attribute boolean enabled;
> > >+
> > >+    readonly attribute long frequency;
> > >+
> > >+    readonly attribute float signalStrength;
> > 
> > Why does this interface duplicate some (but not all) of nsIDOMFMAdaptor's
> > properties?  I'd rather not duplicate any and just call into nsIDOMFMAdaptor
> > when necessary.
> > 
> The interfaces of FM are implemented in C Plus Plus (you can check the
> patched in bug 749053 ), and i have to wrapped it into a XPCOM Object which
> could be called in the JS file (i.e. DOMFmManager.js).

What's confusing is, we have two FM interfaces which do almost but not quite
the same thing.  But I think I understand what's going on, and we can figure
this part out when I look at the other patches.

Comment 19

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #18)
> > > >+    attribute boolean speakerEnabled;
> > > 
> > > Shouldn't this be part of the audio API?
> > I was told that there is no such a specific API, maybe a setting will be
> > introduced in the future, but right now, the WebAPI have to implement it by
> > itself.
> 
> If that's the reason, we should make this a separate API.  It's not a lot
> more
> work to make a separate API than it is to include it on the FM radio API.
> 
> If FM radio were accessible only to privileged content, then maybe I'd be OK
> with a hack like this, but if we're exposing this to the world, then we
> should
> not create bogus APIs because it's slightly easier.
> 
It is for sure that the FM App should be authorized to access the WebFM API, just like other APIs like wifi.
> > > >+    // Signal strength, the range is [0-1]
> > > >+    readonly attribute float signalStrength;
> > > 
> > > Nit: Why is this a float, while the frequency is a double?  I'd rather use
> > > doubles everywhere, unless there's a reason not to.
> > > 
> > It is float in the proposed API.
> 
> But isn't this the proposed API?  I'm asking, why is the proposed API for
> signal strength a float and not a double?
> 
I don't see the difference between a float and a double here, if the reason is to keep the type the same as the frequency, let me change it to double.
> > > We need to figure out what's going on with these bands.  If the goal is
> > > to support things other than FM radio, then we shouldn't call this
> > > nsIDOMFMManager/nsIDOMFMRadio.
> > 
> > I think we have discussed on this in the email, AM might be covered in the
> > future, and that's why the bandRanges object looks like: {"FM": {"upper":
> > 108, "lower": 87.5}}.
> 
> AM radio should not be part of the "FM radio" api.  If we really want to be
> able to support AM in the future -- and I think that's unlikely to happen,
> because AM antennas are quite large -- then we should rename this API to
> "TunableRadio" or something.  But I think designing for an unlikely
> eventuality
> is likely a mistake.
> 
> If we want to support multiple bands then we should design bandRanges to let
> you see multiple FM and multiple AM bands.  At the moment, you can only see
> one
> FM and one AM band.  But if we're designing for future extensibility to AM,
> we
> should also design for future extensibility to multiple FM bands, because the
> latter is far more reasonable than the former.
> 
> But more to the point: We need some way for the app to know which is the
> currently-selected band; apps shouldn't assume that there's only one band in
> bandRanges, because that defeats the purpose.
> 
> So regardless of whether we're allowing multiple bands now or in the future,
> we
> still need to have
> 
>   double upperFreq;
>   double lowerFreq
>   double channelFreqInterval;
> 
> or otherwise
> 
>   jsval currentBand;  /* {upper, lower, interval} */
> 
> so that you know what the current band is set to.  IMO, if we're not allowing
> switching between bands, we should not expose bandRanges, because it's not
> useful.
> 
The FM app is designed to show a dialer, so the app need to know the band ranges, you can check this:
     https://wiki.mozilla.org/images/e/eb/B2g-fm-radio-v07.pdf
> > so "onbandchange" is not introduced to the API in v1, and it
> > is unnecessary to introduce the station interval either.
> 
> Why doesn't the FM radio app need the interval?  Are you expecting that the
> app
> will have a hardcoded interval?  Or that it will always use seek()?  What if
> an
> app wants to allow the user to manually tune the radio?
> 
We can define the interval in a setting, just like we open a setting for the band.
> > > >+    nsIDOMDOMRequest seekDown();
> > > >+
> > > >+    nsIDOMDOMRequest seekUp();
> > > >+
> > > >+    nsIDOMDOMRequest cancelSeek();
> > > 
> > > What happens if you call seekDown/seekUp while a seek is running?
> > > 
> > Fail, request.onerror will be fired.
> 
> Okay, can you please document that we can never have multiple seeks going at
> once, so cancelSeek cancels the one outstanding seek or otherwise fails?
> 
> > Actually, i don't quite understancd what "builtinclass" is used for, i
> > implemented it without such a modifier,
> 	> when i rebased my working branch with latest m-c at one day last month, it
> 	> failed to compile, then i added the "builtinclass"
> 	> just like other idls did.
> 
> What's the compile error?  I'm not totally sure what builtinclass is for
> either, but khuey gave me the impression that it did not affect compilation.
> 
xpidl.IDLError: error: interface 'nsIFmAdaptor' is not builtinclass but derives from builtinclass 'nsIDOMEventTarget', /home/pzhang/work/mozilla-central-fm-otoro-ics/dom/fm/nsIFmAdaptor.idl line 10:0
interface nsIFmAdaptor : nsIDOMEventTarget {
> > > This one is an nsIDOMEventTarget, so maybe you just forgot to put that on
> > > the other interface?
> > > 
> > Other IDLs, such as wiki, is just a nsISupports, why? JS Component?
> 
> Which other IDLs are you talking about?  I'm not sure what you mean by "such
> as
> wiki".
> 
It is "wifi", sorry for the typo.
> nsIDOMEventTarget is the interface which adds add/removeEventListener.  If
> you
> don't inherit from nsIDOMEventTarget, you can't do
> 
>   fmManager.addEventListener('antennachange', function() { ... });
> 
> > > >+    readonly attribute boolean enabled;
> > > >+
> > > >+    readonly attribute long frequency;
> > > >+
> > > >+    readonly attribute float signalStrength;
> > > 
> > > Why does this interface duplicate some (but not all) of nsIDOMFMAdaptor's
> > > properties?  I'd rather not duplicate any and just call into nsIDOMFMAdaptor
> > > when necessary.
> > > 
> > The interfaces of FM are implemented in C Plus Plus (you can check the
> > patched in bug 749053 ), and i have to wrapped it into a XPCOM Object which
> > could be called in the JS file (i.e. DOMFmManager.js).
> 
> What's confusing is, we have two FM interfaces which do almost but not quite
> the same thing.  But I think I understand what's going on, and we can figure
> this part out when I look at the other patches.

Comment 20

[Justin Lebar (not reading bugmail)]

> It is for sure that the FM App should be authorized to access the WebFM API,
> just like other APIs like wifi.

What I meant was, the security discussion indicated that WebFM will be
available implicitly to all apps and (with a prompt) to any webpage.

Since this API will be so widely accessible, I don't think we should add hacks
like "speaker on" to this API.

https://wiki.mozilla.org/WebAPI/Security/FMRadioAPI

> > But isn't this the proposed API?  I'm asking, why is the proposed API for
> > signal strength a float and not a double?
> > 
> I don't see the difference between a float and a double here, if the reason
> is to keep the type the same as the frequency, let me change it to double.

Yeah, it's a consistency argument.  We don't need 52 bits of precision for
either signal strength or FM frequency, so neither of them /needs/ to be a
double.

But double is JavaScript's natural floating-point type (it does not have a
32-bit float, except as a typed array), so in the absence of any good reason to
use a float, I think we should use a double.

> The FM app is designed to show a dialer, so the app need to know the band
> ranges, you can check this:
>      https://wiki.mozilla.org/images/e/eb/B2g-fm-radio-v07.pdf

I think we're not talking about the same thing.  There are two separate things here.

  * "The current band", which indicates the parameters (AM/FM,
    frequency upper and lower bounds, and channel interval) that the radio is
    currently using.  There is only ever one current band.

  * "The band ranges", which indicates all possible values that the current
    band might be set to.

The FM app needs only "the current band" in order to show the dialier.  It does
not need a list of all band ranges that the FM chipset supports.

> > Why doesn't the FM radio app need the interval?  Are you expecting that the
> > app
> > will have a hardcoded interval?  Or that it will always use seek()?  What if
> > an
> > app wants to allow the user to manually tune the radio?
> > 
> We can define the interval in a setting, just like we open a setting for the
> band.

But apps implementing an FM radio cannot necessarily read settings.  The
settings API is not available to unprivileged web content (or even many apps).

If the FM app were able to read settings, then we would not need to expose "the
current band" to the FM app; we could define that information in the settings,
too.  In fact, we would not need the FM API at all; we could define everything
about this API in the settings.

> > What's the compile error?  I'm not totally sure what builtinclass is for
> > either, but khuey gave me the impression that it did not affect compilation.
> > 
> xpidl.IDLError: error: interface 'nsIFmAdaptor' is not builtinclass but
> derives from builtinclass 'nsIDOMEventTarget',
> /home/pzhang/work/mozilla-central-fm-otoro-ics/dom/fm/nsIFmAdaptor.idl line
> 10:0
> interface nsIFmAdaptor : nsIDOMEventTarget {

That settles that, then.  :)

> > > > This one is an nsIDOMEventTarget, so maybe you just forgot to put that on
> > > > the other interface?
> > > > 
> > > Other IDLs, such as wiki, is just a nsISupports, why? JS Component?
> > 
> > Which other IDLs are you talking about?  I'm not sure what you mean by "such
> > as
> > wiki".
> > 
> It is "wifi", sorry for the typo.

Ah, I remember this now.

As you discovered, nsIDOMEventTarget is a builtinclass.  That means that if
nsIDOMFMRadio inherits from nsIDOMEventTarget, you can't implement it in JS.
That's probably why nsIDOMWifiManager doesn't inherit from it.

But if you don't inherit from nsIDOMEventTarget, then you can't do
addEventListener on it.

In other words, you can't implement event targets in JS!

It's bad that nsIDOMWifiManager is not an event target, but I think it would be
worse if the FM radio was not an event target, because we're exposing it to all
of the web.  So I think you're going to have to rewrite the implementation of
nsIDOMFMRadio in C++.  I don't think we want to create a public interface where
you have to use the onX handlers.

Unless Jonas has different thoughts?

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 647942 [details] [diff] [review]
option MOZ_B2G_FM

Review of attachment 647942 [details] [diff] [review]:
-----------------------------------------------------------------

autoconf.mk.in substitution was just rewritten, so this needs to be updated.

Comment 22

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #20)
> > > 
> > It is "wifi", sorry for the typo.
> 
> Ah, I remember this now.
> 
> As you discovered, nsIDOMEventTarget is a builtinclass.  That means that if
> nsIDOMFMRadio inherits from nsIDOMEventTarget, you can't implement it in JS.
> That's probably why nsIDOMWifiManager doesn't inherit from it.
> 
> But if you don't inherit from nsIDOMEventTarget, then you can't do
> addEventListener on it.
> 
> In other words, you can't implement event targets in JS!

We cann't implement the nsIDOMEventTarget in JS, but i think we can add "addEventListener" and "removeEventListener" to nsIFM, and implement the two functions in JS, what do you think?

> 
> It's bad that nsIDOMWifiManager is not an event target, but I think it would
> be
> worse if the FM radio was not an event target, because we're exposing it to
> all
> of the web.  So I think you're going to have to rewrite the implementation of
> nsIDOMFMRadio in C++.  I don't think we want to create a public interface
> where
> you have to use the onX handlers.
> 
> Unless Jonas has different thoughts?

Comment 23

[Justin Lebar (not reading bugmail)]

> We cann't implement the nsIDOMEventTarget in JS, but i think we can add "addEventListener" and 
> "removeEventListener" to nsIFM, and implement the two functions in JS, what do you think?

If that works, that's fine with me.  But I expect that there's some deep problem with trying to do that, otherwise people wouldn't say "you can't implement event targets in JS".  smaug?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Justin Lebar [:jlebar] from comment #23)
> > We cann't implement the nsIDOMEventTarget in JS, but i think we can add "addEventListener" and 
> > "removeEventListener" to nsIFM, and implement the two functions in JS, what do you think?

Horrible horrible hack.  If avoidable, please avoid. EventTargets should be EventTargets

> If that works, that's fine with me.
Sure it could work, but please make sure to implement add/removeEventlistener and dispatchEvent
the way C++ version does.

>  But I expect that there's some deep
> problem with trying to do that, otherwise people wouldn't say "you can't
> implement event targets in JS".  smaug?
You can't implement event targets in JS, because real event handling needs to be sure that
it deals with native objects. Event handling needs to be fast - so, no extra QIing at unsafe times.

In general it isn't clear to me why anyone would like to implement eventtargets in JS.
But looks like the patch does implement FmAdaptor in C++. Implementation should bind the
object to a window though (nsDOMEventTargetHelper::BindToOwner) .

Comment 25

[Justin Lebar (not reading bugmail)]

> In general it isn't clear to me why anyone would like to implement eventtargets in JS.

nsIDOMFMManager wants to be an event target, and is implemented in JS.

Comment 26

[StevenLee[:slee]]

Hi Justin,

Thinker and I proposed a method.
We will implement a class, say JSDOMEventTargetHelper.
JSDOMEventTargetHelper inherits from nsDOMEventTargetHelper and
nsIJSEventTargetHelper.
JSDOMEventTargetHelper is another eventtarget for initializing 
and setting helpers from JS code.

nsIDOMFMRadio provides nsIDOMEventTarget interface.  But, its
QueryInterface returns an JSDOMEventTargetHelper.

Since almost all the functions we need are done in
nsDOMEventTargetHelper, we do not need much effort to implement
JSDOMEventTargetHelper.

So we have 
- JSDOMEventTargetHelper
 - implements nsIJSDOMEventTargetHelper
 - inherit nsDOMEventTargetHelper
- nsIJSDOMEventTargetHelper
 - provide an interface for JS component code for setting helpers

For JS components who want to implement nsIDOMEventTarget, it 
creates an instance of JSDOMEventTargetHelper returned by QueryInterface for nsIDOMEventTarget interface.

Comment 27

[Justin Lebar (not reading bugmail)]

One of the rules of QueryInterface is that you can always get back to your original object:

  nsIFoo *orig = ...;
  nsIBar *bar = QueryInterface(orig);
  nsIFoo *foo = QueryInterface(bar);
  ASSERT(foo == orig);

So to make this work, JSDOMEventTargetHelper's QI function would have to know how to get back to nsIDOMFMRadio.  But I guess you could do that with a custom QI function which calls the fm-radio's QI...

Comment 28

[Justin Lebar (not reading bugmail)]

I'm told that this is a v1 blocker, so I'm noming it.

As I've made clear in a variety of forums, I think it would be a mistake to hold the release for this feature, and I do not think it is a blocker.  In fact, I think it would be prudent to allow this feature to slip, so we have time to focus on other things for v1.  But I understand that is not the decision product has made.

Comment 29

[Justin Lebar (not reading bugmail)]

> Thinker and I proposed a method.

Smaug says this might work, and it sounds fine to me (modulo comment 27).  Go for it!

Comment 30

[Pin Zhang [:pzhang] (inactive)]

Hi Justin, i updated the WebIDL based on your feedback, here is what i did:
  - Set signalStrength as a double
  - rename 'onantennachange' to 'onantennastatechange'
  - rename 'onsignalchange' to 'onsignalstrengthchange'
  - rename 'bandRanges' to 'currentBand', it returns an object like:
        {
           "upper": 87.5,    // in MHz
           "lower": 108,     // in MHz
           "interval":  0.1   // in MHz
        }

and here is the updated WebIDL:

interface FM : EventTarget
{
  /* Indicate if FM is enabled */
  readonly attribute boolean enabled;

  /* Indicate if antenna is available */
  readonly attribute boolean antennaAvailable;

  /**
   * Current frequency in MHZ.
   * The value will be 0 or the last frequency if FM is not enabled. 
   */
  readonly attribute double frequency;

  /* the strength of signal, the range is [0, 1] */
  readonly attribute double signalStrength;

  /**
   * The band info object, it likes:
   *   {
   *     "lower": 87.5,   // in MHz
   *     "upper": 108,    // in MHz
   *     "interval": 0.1  // in MHz
   *   }
   */
  readonly attribute any currentBand;

  /* Fired when the the FM is enabled */
  attribute Function onenabled;

  /* Fired when the the FM is disabled */
  attribute Function ondisabled;

  /* Fired when the state of antenna is changed */
  attribute Function onantennastatechange;

  /* Fired when FM frequency is changed. */
  attribute Function onfrequencychange;

  /* Fired when the strength of signal is changed. */
  attribute Function onsignalstrengthchange;

  /* Power on/off the FM device, onpowerchanged will be fired accordingly */
  DOMRequest setEnabled(in bool enabled);

  /**
   * onsuccess will be triggered if frequency is set successfully.
   * app should listen on the "onfreqchange" to get the right frequency.
   * If the frequency is out of range, then request.onerror will be fired.
   */
  DOMRequest setFrequency(in double frequency);

  /**
   * Tell FM to seek up, if frequency is changed, onfrequencychanged will be triggered.
   * Only one seek is allowed at once, if the FM is seeking, then onerror will be fired.
   */
  DOMRequest seekUp();

  /**
   * Tell FM to seek down, if frequency is changed, onfrequencychanged will be triggered.
   * Only one seek is allowed at once, if the FM is seeking, request.onerror will be fired.
   */
  DOMRequest seekDown();

  /* Cancel the seek action, if the FM is not seeking, request.onerror will be fired */
  DOMRequest cancelSeek();
}

Comment 31

[Justin Lebar (not reading bugmail)]

This looks great.  Just a few small changes, if you don't mind:

> /**
>  * Current frequency in MHZ.
>  * The value will be 0 or the last frequency if FM is not enabled. 
>  */
> readonly attribute double frequency;

Can we make the value here null if FM is not enabled?  That way there's no confusion, nor any contract that this API will remember things.

>  /**
>   * The band info object, it likes:
>   *   {
>   *     "lower": 87.5,   // in MHz
>   *     "upper": 108,    // in MHz
>   *     "interval": 0.1  // in MHz
>   *   }
>   */
>  readonly attribute any currentBand;

If we're only going to expose the current band, I think it would be better to do something simpler:

  readonly attribute double frequencyLowerBound;
  readonly attribute double frequencyUpperBound;
  readonly attribute double channelWidth;

If we have fm.availableBands at some later date, they can be {lowerBound, upperBound, width} objects, which would be fine.  We don't need to expose our current band data as an Object in order to make the future availableBands property make sense.

> /* Power on/off the FM device, onpowerchanged will be fired accordingly */
> DOMRequest setEnabled(in bool enabled);

Perhaps we should have two methods

  DOMRequest disableRadio();
  DOMRequest enableRadio(in double frequency);

Otherwise, you have to enable the radio and then set the frequency.  But after you enable and before you set the frequency, you're in this weird state where the radio is "on" but not tuned.

What do you think?

>  /**
>   * onsuccess will be triggered if frequency is set successfully.
>   * app should listen on the "onfreqchange" to get the right frequency.
>   * If the frequency is out of range, then request.onerror will be fired.
>   */
>  DOMRequest setFrequency(in double frequency);

What happens if I set the frequency to 87.6, when the min freq is 87.5 and the channel width is .2?

I'd guess that can't be an error, because we can't represent all fractions precisely as doubles. Even if I send 87.7, I might actually send 87.70000000000002.  In other words, we have to do some amount of rounding.

So I guess I have a question and a suggestion.  My suggestion is that we say that even if setFrequency(x) succeeds, it is not guaranteed that fm.frequency == x, because we may reduce the precision of the given frequency.

And my question is: Do you know if the radio can tune to halfway between a channel, or must it always tune to precisely one of the channels?  That is, should the user expect their given frequency to be rounded to a channel, or just rounded to something?  Whatever it is is fine, but we should document it.

I have nits on the English in the comments (nothing major).  I was planning to save those for when you post a patch, but I can give them sooner if you'd prefer.

Comment 32

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #31)
> This looks great.  Just a few small changes, if you don't mind:
> 
> > /**
> >  * Current frequency in MHZ.
> >  * The value will be 0 or the last frequency if FM is not enabled. 
> >  */
> > readonly attribute double frequency;
> 
> Can we make the value here null if FM is not enabled?  That way there's no
> confusion, nor any contract that this API will remember things.
> 
Yes
> >  /**
> >   * The band info object, it likes:
> >   *   {
> >   *     "lower": 87.5,   // in MHz
> >   *     "upper": 108,    // in MHz
> >   *     "interval": 0.1  // in MHz
> >   *   }
> >   */
> >  readonly attribute any currentBand;
> 
> If we're only going to expose the current band, I think it would be better
> to do something simpler:
> 
>   readonly attribute double frequencyLowerBound;
>   readonly attribute double frequencyUpperBound;
>   readonly attribute double channelWidth;
> 
> If we have fm.availableBands at some later date, they can be {lowerBound,
> upperBound, width} objects, which would be fine.  We don't need to expose
> our current band data as an Object in order to make the future
> availableBands property make sense.
> 
That's fine to me.
> > /* Power on/off the FM device, onpowerchanged will be fired accordingly */
> > DOMRequest setEnabled(in bool enabled);
> 
> Perhaps we should have two methods
> 
>   DOMRequest disableRadio();
>   DOMRequest enableRadio(in double frequency);
> 
> Otherwise, you have to enable the radio and then set the frequency.  But
> after you enable and before you set the frequency, you're in this weird
> state where the radio is "on" but not tuned.
> 
> What do you think?
enableRadio might fail, the reason may be the FM HW is not be enabled, or the frequency is out of range.
So i prefer keeping it, we can set the frequency with the lower bound after the FM is enabled.
> 
> >  /**
> >   * onsuccess will be triggered if frequency is set successfully.
> >   * app should listen on the "onfreqchange" to get the right frequency.
> >   * If the frequency is out of range, then request.onerror will be fired.
> >   */
> >  DOMRequest setFrequency(in double frequency);
> 
> What happens if I set the frequency to 87.6, when the min freq is 87.5 and
> the channel width is .2?
>
> I'd guess that can't be an error, because we can't represent all fractions
> precisely as doubles. Even if I send 87.7, I might actually send
> 87.70000000000002.  In other words, we have to do some amount of rounding.
> 
I tried this on the Otoro device, yes, it can be set to 87.6, when the min freq is 87.5 and the channel width is .2.

> So I guess I have a question and a suggestion.  My suggestion is that we say
> that even if setFrequency(x) succeeds, it is not guaranteed that
> fm.frequency == x, because we may reduce the precision of the given
> frequency.
> 
Yes, it is not guaranteed that fm.frequency == x.
If the channel width is .2, and we set frequency with 88.05, the FM HW will round the frequency to 88 automatically.

> And my question is: Do you know if the radio can tune to halfway between a
> channel, or must it always tune to precisely one of the channels?  That is,
> should the user expect their given frequency to be rounded to a channel, or
> just rounded to something?  Whatever it is is fine, but we should document
> it.
I will check it on the Otoro device later.

> 
> I have nits on the English in the comments (nothing major).  I was planning
> to save those for when you post a patch, but I can give them sooner if you'd
> prefer.
yes, please

Comment 33

[Pin Zhang [:pzhang] (inactive)]

Hi Justin, about the frequency rounding, i tried on the Otoro device, there are three spacing type: 50KHz, 100KHz and 200KHz, whatever spacing type i set, the result is the same:
  - if set the frequency with 88.05, the frequency will be 88.05
  - if set the frequency with 88.06, the frequency will be rounded to 88.0
So it's for sure the frequency will be rounded by the FM HW, there may be problem with spacing type setting though.

IMO, the frequency should be rounded, depending on the band settings, for example:
   - lower: 87.5, upper: 108, interval: 0.2
     * set with 87.6, the frequency will be 87.5
   - lower: 87.5, upper: 108, interval: 0.1
     * set with 88.05, the frequency will be 88

Comment 34

[Justin Lebar (not reading bugmail)]

> We can set the frequency with the lower bound after the FM is enabled.

But won't that cause the radio to start playing sound?

If the radio doesn't start playing until the event loop flushes, that's probably OK.  (I mean, if you do fm.setEnabled(true); for(var i = 0; i < 10000; i++) {} fm.setFrequency(100);, you shouldn't hear the radio until you set the frequency.)

>   - if set the frequency with 88.05, the frequency will be 88.05
>   - if set the frequency with 88.06, the frequency will be rounded to 88.0

So it's always rounding to 50KHz?  But then why does 88.06 get rounded to 88.0 and not 88.5?  That's pretty weird.

> IMO, the frequency should be rounded, depending on the band settings, for example:
>   - lower: 87.5, upper: 108, interval: 0.2
>     * set with 87.6, the frequency will be 87.5
>   - lower: 87.5, upper: 108, interval: 0.1
>     * set with 88.05, the frequency will be 88

I think that's reasonable for now.  We can always change the comment in the IDL if we don't want to go this route for some reason.

Comment 35

[Justin Lebar (not reading bugmail)]

English nits, fresh off the presses.

>  /* Indicate if FM is enabled */
>  readonly attribute boolean enabled;

s/Indicate/Indicates/
s/FM/the FM radio/

>  /* Indicate if antenna is available */
>  readonly attribute boolean antennaAvailable;

s/Indicate/Indicates/
s/antenna/the antenna/

This is also pretty repetitive with the attribute name.  How about

  Indicates if the antenna is plugged in and available

>  /* the strength of signal, the range is [0, 1] */
>  readonly attribute double signalStrength;

s/^the/The/
s/strength of signal/signal strength/

This is incorrect use of a comma: "the range is [0, 1]" is an independent clause (a complete sentence by itself), so cannot be separated from the clause before it by only a comma.  (It would be a comma splice if "the strength of signal" were itself an independent clause, but it's not.)

How about

   The signal strength.  The value returned is between 0 (no signal) and 1 (full signal), inclusive.

>  /* Fired when the the FM is enabled */
>  attribute Function onenabled;

s/the FM/the FM radio/, or s/the FM/the radio/.

>  /* Fired when the the FM is disabled */
>  attribute Function ondisabled;

ibid.

>  /* Fired when the state of antenna is changed */
>  attribute Function onantennastatechange;

s/of antenna/of the antenna/

Nowhere else do we talk about the "state of the antenna", so I don't think we
should use that in the comment here.  Maybe 

  Fired when the antenna becomes available or unavailable; i.e., fired when the
  antennaAvailable attribute changes.

>  /* Fired when FM frequency is changed. */
>  attribute Function onfrequencychange;

s/FM frequency/the radio's frequency/, or s/FM frequency/the FM radio's frequency/.

>  /* Fired when the strength of signal is changed. */
>  attribute Function onsignalstrengthchange;

s/strength of signal/signal strength/.

We say "signal strength /changes/", instead of "signal strength /is changed/", because the signal strength is beyond anyone's direct control.  By analogy, we'd say "the weather changes", not "the weather is changed" (unless someone has a weather machine!).

Also, you mentioned earlier that this event is throttled -- can you explain how, here?

>  /* Power on/off the FM device, onpowerchanged will be fired accordingly */
>  DOMRequest setEnabled(in bool enabled);

s!Power on/off the FM device!Power the FM radio on/off!

In general, we should refer to events as "powerchange", rather than "onpowerchange".  "powerchange" is the name of the event, and "onpowerchange" is the name of the attribute on the FM radio which is tied to the "powerchange" event.

Also, there's also no onpowerchanged event.

This is a traditional comma splice; the two things separated by a comma are both independent clauses.  You can fix this by either adding a conjunction (e.g. ", and"), using a semicolon instead of the comma, or splitting the sentence in two.  In this case, I think the latter is the best option:

  Power the FM radio on/off.  The powerchange event will be fired if this
  request completes successfully.

>  /**
>   * onsuccess will be triggered if frequency is set successfully.
>   * app should listen on the "onfreqchange" to get the right frequency.
>   * If the frequency is out of range, then request.onerror will be fired.
>   */
>  DOMRequest setFrequency(in double frequency);

I don't think we need to explain when onsuccess and onerror are fired; that's pretty clear from context.

I presume the app can listen to the frequencychange event or alternatively listen to request.onsuccess, right?

Finally, let's add a summary sentence to the beginning, like we have elsewhere.

  Tune the FM radio to the given frequency.  This will fail if the given
  frequency is out of range, or if another setFrequency call is pending.

  Note that the FM radio may not tune to the exact frequency given.  To get the
  frequency the radio is actually tuned to, wait for the request to fire
  onsuccess (or wait for the frequencychange event to fire), and then read the
  frequency attribute.

>  /**
>   * Tell FM to seek up, if frequency is changed, onfrequencychanged will be triggered.
>   * Only one seek is allowed at once, if the FM is seeking, then onerror will be fired.
>   */
>  DOMRequest seekUp();

s/up, if frequency/up to the next channel.  If the frequency/
s/is changed/is successfully changed/
s/onfrequencychanged/the frequencychange event/

s/once, if/once: If/
s/the FM/the FM radio/
s/is seeking/is seeking when seekUp is called/

>  /**
>   * Tell FM to seek down, if frequency is changed, onfrequencychanged will be triggered.
>   * Only one seek is allowed at once, if the FM is seeking, request.onerror will be fired.
>   */
>  DOMRequest seekDown();

ibid

>  /* Cancel the seek action, if the FM is not seeking, request.onerror will be fired */
>  DOMRequest cancelSeek();
>}

s/action, if/action. If/
s/the FM/the FM radio
s/not seeking/not currently seeking up or down

Comment 36

[Boris Zbarsky [:bzbarsky]]

Do we really want to name the interface "FM" as opposed to, say, "FMRadio"?

As far as the implementation plan, why do you need QI weirdness?  It seems like all you really need is a way to create a JS object with the right JSClass and native slot (one which will claim to implement EventTarget and have a native which does so, as discussed) and the right prototype (the FM.prototype or FMRadio.prototype, as desired), no?

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

What native slot? I thought the implementation would be in JS.
Though, comment 30 talks about WebIDL so I'm not sure anymore what kind of EventTarget impl
will be added.

Comment 38

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #34)
> > We can set the frequency with the lower bound after the FM is enabled.
> 
> But won't that cause the radio to start playing sound?
> 
Yes, but i think it is OK, a real FM radio will start playing sound when it is turned on.

> If the radio doesn't start playing until the event loop flushes, that's
> probably OK.  (I mean, if you do fm.setEnabled(true); for(var i = 0; i <
> 10000; i++) {} fm.setFrequency(100);, you shouldn't hear the radio until you
> set the frequency.)
> 
> >   - if set the frequency with 88.05, the frequency will be 88.05
> >   - if set the frequency with 88.06, the frequency will be rounded to 88.0
> 
> So it's always rounding to 50KHz?  But then why does 88.06 get rounded to
> 88.0 and not 88.5?  That's pretty weird.
yep, it's pretty weird.
> 
> > IMO, the frequency should be rounded, depending on the band settings, for example:
> >   - lower: 87.5, upper: 108, interval: 0.2
> >     * set with 87.6, the frequency will be 87.5
> >   - lower: 87.5, upper: 108, interval: 0.1
> >     * set with 88.05, the frequency will be 88
> 
> I think that's reasonable for now.  We can always change the comment in the
> IDL if we don't want to go this route for some reason.

Comment 39

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #35)

> 
> >  /* Fired when the strength of signal is changed. */
> >  attribute Function onsignalstrengthchange;
> 
> s/strength of signal/signal strength/.
> 
> We say "signal strength /changes/", instead of "signal strength /is
> changed/", because the signal strength is beyond anyone's direct control. 
> By analogy, we'd say "the weather changes", not "the weather is changed"
> (unless someone has a weather machine!).
> 
> Also, you mentioned earlier that this event is throttled -- can you explain
> how, here?
>
I set a 2 seconds interval checking in the FmService module.

Comment 40

[Boris Zbarsky [:bzbarsky]]

> What native slot? I thought the implementation would be in JS.

Will this duplicate all the EventTarget stuff instead of inheriting from EventTarget.prototype, then?  Because the methods on EventTarget.prototype will only work if the object is an EventTarget from their point of view.

Maybe I should back up.  What's the goal here?  Are we exposing some chrome object to content JS via COWs or SOWs or whatever the relevant wrapper is and ignoring WebIDL altogether?  Or are we directly creating objects in the content compartment?

Comment 41

[Pin Zhang [:pzhang] (inactive)]

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #36)
> Do we really want to name the interface "FM" as opposed to, say, "FMRadio"?
> 
Justin, what do you think?

Comment 42

[Justin Lebar (not reading bugmail)]

>> But won't that cause the radio to start playing sound?
>> 
> Yes, but i think it is OK, a real FM radio will start playing sound when it is turned on.

Well, we wouldn't want an MP3 interface to start playing a random track as soon as it's initialized because that mirrors the behavior of cassette players.  :)

I don't see any reason to design this annoyance in to our interface when we can easily avoid it.  But, like I said, so long as we wait for a trip through the event loop before we start playing sound, that would be an OK compromise to me.

>> Do we really want to name the interface "FM" as opposed to, say, "FMRadio"?
>> 
> Justin, what do you think?

I agree with Boris.  Frequency modulation is a method of sending data over waves; if you leave out the "radio" part, it doesn't really describe what's going on here.  :)

> Are we exposing some chrome object to content JS via COWs or SOWs or whatever the relevant wrapper 
> is and ignoring WebIDL altogether?  Or are we directly creating objects in the content compartment?

The former, if I understand you correctly.  It's the DOMFMManager in attachment 647943 [details] [diff] [review].

Comment 43

[Boris Zbarsky [:bzbarsky]]

Hmm.  If you're just exposing chrome stuff, then the proposed thing with QI may work, yes, since you're not actually implementing what the rest of the world thinks of as an EventTarget....  It's OK for a short-term hack, though obviously long-term we'll want to do something more like what I described.

By the way, the existing source files there (nsIFm.idl, dom/fm) have the same problem as having an interface called FM, in my opinion.  nsIFmAntenna and nsIFmAdaptor are perhaps fine (though "adaptor" doesn't really say anything useful to me).

Comment 44

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #42)
> >> But won't that cause the radio to start playing sound?
> >> 
> > Yes, but i think it is OK, a real FM radio will start playing sound when it is turned on.
> 
> Well, we wouldn't want an MP3 interface to start playing a random track as
> soon as it's initialized because that mirrors the behavior of cassette
> players.  :)
> 
> I don't see any reason to design this annoyance in to our interface when we
> can easily avoid it.  But, like I said, so long as we wait for a trip
> through the event loop before we start playing sound, that would be an OK
> compromise to me.
> 

I understand you, but your proposed interfaces may still bring the FM to the *weird* state you mentioned: "the radio is 'on' but not tuned", if the frequency is out of the band, say 60MHz: 
   navigator.mozFMRadio.enableRadio(60)

And what an app should do likes this:
   var req = navigator.mozFMRadio.setEnabled(true);
   req.onsuccess = function() {
     navigator.mozFMRadio.setFrequency(91.5);
   };
not what you mentioned: 
   fm.setEnabled(true); for(var i = 0; i < 10000; i++) {} fm.setFrequency(100);

Comment 45

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #18)
> > > >+    attribute boolean speakerEnabled;
> > > 
> > > Shouldn't this be part of the audio API?
> > I was told that there is no such a specific API, maybe a setting will be
> > introduced in the future, but right now, the WebAPI have to implement it by
> > itself.
> 
> If that's the reason, we should make this a separate API.  It's not a lot
> more
> work to make a separate API than it is to include it on the FM radio API.
> 
> If FM radio were accessible only to privileged content, then maybe I'd be OK
> with a hack like this, but if we're exposing this to the world, then we
> should
> not create bogus APIs because it's slightly easier.
> 
Justin, what do you mean by creating bogus APIs?Like this: navigator.mozFMAudio.speakerEnabled? If yes, then what about the security level for such an API, and will it be proposed to W3C?

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #43)
> Hmm.  If you're just exposing chrome stuff, then the proposed thing with QI
> may work, yes, since you're not actually implementing what the rest of the
> world thinks of as an EventTarget....  It's OK for a short-term hack, though
> obviously long-term we'll want to do something more like what I described.
That is my understanding. See also Bug 731746.

Comment 47

[Justin Lebar (not reading bugmail)]

> I understand you, but your proposed interfaces may still bring the FM to the *weird* state you 
> mentioned: "the radio is 'on' but not tuned", if the frequency is out of the band, say 60MHz: 
>    navigator.mozFMRadio.enableRadio(60)

Perhaps we should make this call not turn on the radio at all, if we wanted to avoid this problem.

> And what an app should do likes this:
>    var req = navigator.mozFMRadio.setEnabled(true);
>    req.onsuccess = function() {
>      navigator.mozFMRadio.setFrequency(91.5);
>    };
> not what you mentioned: 
>    fm.setEnabled(true); for(var i = 0; i < 10000; i++) {} fm.setFrequency(100);

I was using the busyloop to illustrate a problem; I was not suggesting that was the right thing to do.  :)

But if I don't want to hear a random channel for a moment before I tune the radio, I shouldn't do

>    navigator.mozFMRadio.setEnabled(true).onsuccess = function() {
>      navigator.mozFMRadio.setFrequency(91.5);
>    };

but should instead do

>    navigator.mozFMRadio.setEnabled(true);
>    navigator.mozFMRadio.setFrequency(91.5);

right?  (Provided, of course, that setEnabled is guaranteed not to take effect until we spin the event loop.)

> Justin, what do you mean by creating bogus APIs? Like this: navigator.mozFMAudio.speakerEnabled? 
> If yes, then what about the security level for such an API, and will it be proposed to W3C?

Yes, I think that API is "bogus" because speaker enabled has nothing to do with specifically FM radio.  (Are you proposing that speakerEnabled be a property on mozFMRadio, or a separate interface called mozFMAudio?)

Since device speakers are used for more than just playing FM radio, the attribute telling you whether or not the speaker is enabled should not be on the FM radio interface.

I think we could discuss details like the security requirements for that separate API and our approach for getting it standardized in a separate bug.

Comment 48

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #47)
> > I understand you, but your proposed interfaces may still bring the FM to the *weird* state you 
> > mentioned: "the radio is 'on' but not tuned", if the frequency is out of the band, say 60MHz: 
> >    navigator.mozFMRadio.enableRadio(60)
> 
> Perhaps we should make this call not turn on the radio at all, if we wanted
> to avoid this problem.
> 
> > And what an app should do likes this:
> >    var req = navigator.mozFMRadio.setEnabled(true);
> >    req.onsuccess = function() {
> >      navigator.mozFMRadio.setFrequency(91.5);
> >    };
> > not what you mentioned: 
> >    fm.setEnabled(true); for(var i = 0; i < 10000; i++) {} fm.setFrequency(100);
> 
> I was using the busyloop to illustrate a problem; I was not suggesting that
> was the right thing to do.  :)
> 
> But if I don't want to hear a random channel for a moment before I tune the
> radio, I shouldn't do
> 
> >    navigator.mozFMRadio.setEnabled(true).onsuccess = function() {
> >      navigator.mozFMRadio.setFrequency(91.5);
> >    };
> 
> but should instead do
> 
> >    navigator.mozFMRadio.setEnabled(true);
> >    navigator.mozFMRadio.setFrequency(91.5);
> 
> right?  (Provided, of course, that setEnabled is guaranteed not to take
> effect until we spin the event loop.)
> 
For Otoro device, it will take about 2s to enable the FM HW, it's doesn't make any sence that we can set the frequency before the HW is turned on, and then the state is also weird: it is enabling and will-be-tuned-to-91.5.
BTW, i don't quite understand what "that setEnabled is guaranteed not to take effect until we spin the event loop" means here, :-(

> > Justin, what do you mean by creating bogus APIs? Like this: navigator.mozFMAudio.speakerEnabled? 
> > If yes, then what about the security level for such an API, and will it be proposed to W3C?
> 
> Yes, I think that API is "bogus" because speaker enabled has nothing to do
> with specifically FM radio.  (Are you proposing that speakerEnabled be a
> property on mozFMRadio, or a separate interface called mozFMAudio?)
> 
I am proposing the speakerEnabled be a property on mozFMRadio, just like the mozTelephony.speakerEnabled.

> Since device speakers are used for more than just playing FM radio, the
> attribute telling you whether or not the speaker is enabled should not be on
> the FM radio interface.
> 
I was told that there will be some settings to control audio routing, but for now, we don't have such settings, and i don't think it is planed in V1.

> I think we could discuss details like the security requirements for that
> separate API and our approach for getting it standardized in a separate bug.

Comment 49

[Justin Lebar (not reading bugmail)]

> For Otoro device, it will take about 2s to enable the FM HW, it's doesn't make any sence that we can 
> set the frequency before the HW is turned on, and then the state is also weird: it is enabling and 
> will-be-tuned-to-91.5.

I don't think "radio is enabling and will be tuned to 91.5" is a particularly strange state, myself.  That seems like exactly the state we want to be in.

But what I'm trying to figure out is how to design our API in such a way as we can avoid playing a random radio station (which may be loud static, piped straight into the user's ears) when the radio is turned on.  I'm not wed to a specific way of accomplishing this, but I think this is an important design consideration.

> BTW, i don't quite understand what "that setEnabled is guaranteed not to take effect until we spin 
> the event loop" means here, :-(

It basically means that if we do

  setTimeout(function() { alert('Radio about to be enabled.'); }, 0);
  fmRadio.setEnabled(true);
  ...

that the alert will show before the radio starts playing.  The setTimeout will fire some time after you've returned from all the code you're currently running.

When whatever JS code you're running finishes, the browser looks at the list of pending tasks (e.g. setTimeouts, onclick handlers, whatever) and runs one.  The code which does this is called the "event loop", and we say the event loop "spins" when it completes once.  If that makes any sense.

> I am proposing the speakerEnabled be a property on mozFMRadio, just like the 
> mozTelephony.speakerEnabled.

Okay.

I think the telephony property is bogus too, but I also think it's not as bad as putting speakerEnabled on the FM interface, because telephony will not be exposed to the vast majority of pages, while webFM will be available to every page on the web.  As a result, we will have much more difficulty removing speakerEnabled from the FM interface than we would removing it from the telephony interface.

> I was told that there will be some settings to control audio routing, but for now, we don't have 
> such settings, and i don't think it is planed in V1.

I don't understand what is the motivation for putting speakerEnabled on the FM interface.  Surely if the FM radio app cares about whether the speaker is enabled, the MP3 player app also cares, right?  But we agree the MP3 player app shouldn't have to look at the FM radio API to determine whether the speaker is on?

Comment 50

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #49)

> > BTW, i don't quite understand what "that setEnabled is guaranteed not to take effect until we spin 
> > the event loop" means here, :-(
> 
> It basically means that if we do
> 
>   setTimeout(function() { alert('Radio about to be enabled.'); }, 0);
>   fmRadio.setEnabled(true);
>   ...
> 
> that the alert will show before the radio starts playing.  The setTimeout
> will fire some time after you've returned from all the code you're currently
> running.
> 
> When whatever JS code you're running finishes, the browser looks at the list
> of pending tasks (e.g. setTimeouts, onclick handlers, whatever) and runs
> one.  The code which does this is called the "event loop", and we say the
> event loop "spins" when it completes once.  If that makes any sense.
> 
"setEnabled" is an async method, when you called "mozFMRadio.setEnabled(true)", the enabling FM action will be executed in one thread or process which is obviously different from the thread/process which will execute the "event loop" (maybe my understanding is totally wrong), then i don't understand how to prevent "setEnabled is guaranteed not to take effect until we spin the event loop". 

> > I am proposing the speakerEnabled be a property on mozFMRadio, just like the 
> > mozTelephony.speakerEnabled.
> 
> Okay.
> 
> I think the telephony property is bogus too, but I also think it's not as
> bad as putting speakerEnabled on the FM interface, because telephony will
> not be exposed to the vast majority of pages, while webFM will be available
> to every page on the web.  As a result, we will have much more difficulty
> removing speakerEnabled from the FM interface than we would removing it from
> the telephony interface.
> 
So there should be an API to control the audio routing which will be exposed to the vast majority of pages, right?
If yes, what i can imagine, the API will not be specific used for FM audio, it will effect not only the FM app but also other apps which play sounds., and obviously it's not planned in V1 and even V2.

> > I was told that there will be some settings to control audio routing, but for now, we don't have 
> > such settings, and i don't think it is planed in V1.
> 
> I don't understand what is the motivation for putting speakerEnabled on the
> FM interface.  Surely if the FM radio app cares about whether the speaker is
> enabled, the MP3 player app also cares, right?  But we agree the MP3 player
> app shouldn't have to look at the FM radio API to determine whether the
> speaker is on?
The B2G is designed to enable the speaker automatically when the headset is unplugged, and change it back when the user plugged the headset, it's OK for the MP3 player or video player, but for the FM, the headset is the antenna, if you unplugged the headset, the sound will become noise, so the FM app need such a interface to enable speaker without unplugging the headset.

Comment 51

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #49)
> > For Otoro device, it will take about 2s to enable the FM HW, it's doesn't make any sence that we can 
> > set the frequency before the HW is turned on, and then the state is also weird: it is enabling and 
> > will-be-tuned-to-91.5.
> 
> I don't think "radio is enabling and will be tuned to 91.5" is a
> particularly strange state, myself.  That seems like exactly the state we
> want to be in.
> 
> But what I'm trying to figure out is how to design our API in such a way as
> we can avoid playing a random radio station (which may be loud static, piped
> straight into the user's ears) when the radio is turned on.  I'm not wed to
> a specific way of accomplishing this, but I think this is an important
> design consideration.
>
I have to say i am totally confused.
If we keep seperating the "setEnable"(not tuned to some frequency automatically) and "setFrequency", the FM will stay quiet after it is enabled and before we tuned it, and the state will be enabled-but-not-tuned which you feel so weird, if we want to kill such a "weird" state, the only thing we can do is combining them into one atom action, i.e. enableRadio(frequency) you proposed, and the app will have to set a frequency which is also a *random* number to the end user when enabling the FM.

Comment 52

[Justin Lebar (not reading bugmail)]

> So there should be an API to control the audio routing which will be exposed to the vast majority of 
> pages, right?

No, because I do not think that a webpage should be able to pipe audio out of the phone's speaker.  That's another good reason not to put speakerEnabled on the FM radio interface.

I think the FM radio should immediately shut itself off when you unplug the headset (which functions as the antenna).  Playing static through the phone's external speaker when the headset is disconnected is not useful in any way.

Then the only use-case for speakerEnabled that Ray and I were able to come up with on IRC is: I want to listen to the FM radio on my phone's speakers, but I have to leave the headphones plugged in because they function as the radio's antenna, and B2G (currently) doesn't have any way for me to direct audio to the external speaker except by unplugging the headphones.

I don't feel like this is a core FM radio use-case.  Even if it's a blocker, I think we should figure it out in a follow-up bug, so as to reduce the scope of this bug and land it sooner.  We can argue about what interface the relevant APIs should live on in that bug.

Comment 53

[Pin Zhang [:pzhang] (inactive)]

Hi Justin, i updated the WebIDL, here is what i did:
  - Break setEnabled into two methods: disable, enable(frequency)
  - Removed bandRanges, and add three new attributes: frequencyUpperBound, frequencyLowerBound and channelWidth
  - Renamed onantennastatechange to onantennaavailablechange which is more intuitively connected to the attribute antennaAvailable
  - Changed the type of frequency to 'any'

and here is the updated webidl:

interface FMRadio : EventTarget
{
  /* Indicates if the FM radio is enabled. */
  readonly attribute boolean enabled;

  /* Indicates if the antenna is plugged and available. */
  readonly attribute boolean antennaAvailable;

  /**
   * Current frequency in MHz.
   * The value will be null if the FM radio is disabled.
   */
  readonly attribute any frequency;

  /**
   * The signal strength.
   * The value returned is between 0 (no signal) and 1 (full signal), inclusive.
   */
  readonly attribute double signalStrength;

  /* The upper bound of frequency in MHz. */
  readonly attribute double frequencyUpperBound;

  /* The lower bound of frequency in MHz. */
  readonly attribute double frequencyLowerBound;

  /**
   * The channel width of the ranges of frequency, in MHz.
   * Usually, the value is one of:
   *  - 0.05 MHz
   *  - 0.1  MHz
   *  - 0.2  MHz
   */
  readonly attribute double channelWidth;

  /* Fired when the FM radio is enabled. */
  attribute Function onenabled;

  /* Fired when the FM radio is disabled. */
  attribute Function ondisabled;

  /**
   * Fired when the antenna becomes available or unavailable, i.e., fired when
   * the antennaAvailable attribute changes.
   */
  attribute Function onantennaavailablechange;

  /* Fired when the FM radio's frequency is changed. */
  attribute Function onfrequencychange;

  /* Fired when the signal strength changes. */
  attribute Function onsignalstrengthchange;

  /**
   * Power the FM radio off.
   * The disabled event will be fired if this request completes successfully.
   */
  DOMRequest disable();

  /**
   * Power the FM radio on, and tuned the radio to the given frequency in MHz.
   * This will fail if the given frequency is out of range.
   * The enabled event and frequencychange event will be fired if this request
   * completes successfully.
   */
  DOMRequest enable(in double frequency);

  /**
   * Tune the FM radio to the given frequency.
   * This will fail if the given frequency is out of range.
   *
   * Note that the FM radio may not tuned to the exact frequency given. To get
   * the frequency the radio is actually tuned to, wait for the request to fire
   * onsucess (or wait for the frequencychange event to fire), and then read the
   * frequency attribute.
   */
  DOMRequest setFrequency(in double frequency);

  /**
   * Tell the FM radio to seek up to the next channel. If the frequency is
   * successfully changed, the frequencychange event will be triggered.
   *
   * Only one seek is allowed at once:
   * If the radio is seeking when the seekUp is called, onerror will be fired.
   */
  DOMRequest seekUp();

  /**
   * Tell the FM radio to seek down to the next channel. If the frequency is
   * successfully changed, the frequencychange event will be triggered.
   *
   * Only one seek is allowed at once:
   * If the radio is seeking when the seekDown is called, onerror will be fired.
   */
  DOMRequest seekDown();

  /**
   * Cancel the seek action.
   * If the radio is not currently seeking up or down, onerror will be fired.
   */
  DOMRequest cancelSeek();
}

Comment 54

[Justin Lebar (not reading bugmail)]

> - Changed the type of frequency to 'any'

This should be a nullable-double, which I think is "double?" in webidl.

> * Power the FM radio on, and tuned the radio to the given frequency in MHz.

Nit: s/tuned/tune

>   /* Fired when the FM radio is enabled. */
>   attribute Function onenabled;
>
>   /* Fired when the FM radio is disabled. */
>   attribute Function ondisabled;

Hm...it's a little weird that we have two events for FM radio enabled/disabled, but only one event for antenna plugged/unplugged.

But antenna unplugged will cause us to disable the radio, so...maybe it's OK.  What do you think?

Aside from the above, this looks good to me.  We should get Jonas's SR (please post a patch), but I don't think you should block on that before working on the rest of the patches.

Comment 55

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #54)
> > - Changed the type of frequency to 'any'
> 
> This should be a nullable-double, which I think is "double?" in webidl.
> 
Yes, you are right.
> > * Power the FM radio on, and tuned the radio to the given frequency in MHz.
> 
> Nit: s/tuned/tune
> 
> >   /* Fired when the FM radio is enabled. */
> >   attribute Function onenabled;
> >
> >   /* Fired when the FM radio is disabled. */
> >   attribute Function ondisabled;
> 
> Hm...it's a little weird that we have two events for FM radio
> enabled/disabled, but only one event for antenna plugged/unplugged.
> 
> But antenna unplugged will cause us to disable the radio, so...maybe it's
> OK.  What do you think?
> 
We have two methods to enable and disable the FM radio, so i think it's OK to keep two events here.

> Aside from the above, this looks good to me.  We should get Jonas's SR
> (please post a patch), but I don't think you should block on that before
> working on the rest of the patches.
OK, i will update the IDL patch for WebFM first.

Comment 56

[Pin Zhang [:pzhang] (inactive)]

Attachment: nsIDOMFMRadio IDL

Comment 57

[Kai-Chih Hu [:khu]]

Let's remove toggle feature out of v1. Just provide a note here.

Comment 58

[Boris Zbarsky [:bzbarsky]]

>  readonly attribute any frequency;

Why does this need to be "any"?  If it's just a number or null, then using a nullable numeric type should work, I'd think.

Comment 59

[Justin Lebar (not reading bugmail)]

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #58)
> Why does this need to be "any"?  If it's just a number or null, then using a
> nullable numeric type should work, I'd think.

Right, see comment 54.

Comment 60

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We're not using WebIDL yet, so that won't work here.

Comment 61

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 653655 [details] [diff] [review]
nsIDOMFMRadio IDL

Review of attachment 653655 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

::: dom/fm/nsIDOMFMRadio.idl
@@ +39,5 @@
> +     *  - 0.05 MHz
> +     *  - 0.1  MHz
> +     *  - 0.2  MHz
> +     */
> +    readonly attribute double channelWidth;

Nit: I think the name here is a bit too technical. Something like channelStep might be easier to understand. I'm totally fine either way though.

Comment 62

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I've provide a WIP on bug 731746 for JSDOMEventTargetHelper mentioned in comment 26.
You can take a look at it to see if this approach matched with WebFM's requirement.

Comment 63

[Pin Zhang [:pzhang] (inactive)]

Attachment: all in one patch, WIP, v1

It depends on the patches of bug731746 and bug749053.

Comment 64

[Justin Lebar (not reading bugmail)]

Bug 731746 has been blocking-basecamp minus'ed.  Please add and implement addEventListener and removeEventListener methods here.

I'm sorry for the churn, guys.

Comment 65

[Justin Lebar (not reading bugmail)]

Could you please obsolete the old patches in this bug?  It's not clear to me what is part of this bug anymore.

Comment 66

[Pin Zhang [:pzhang] (inactive)]

I saw the bug 731746 comment 19, which way should i do, implement the two methods in JS or rewrite all code in c++?
(In reply to Justin Lebar [:jlebar] from comment #64)
> Bug 731746 has been blocking-basecamp minus'ed.  Please add and implement
> addEventListener and removeEventListener methods here.
> 
> I'm sorry for the churn, guys.

Comment 67

[Justin Lebar (not reading bugmail)]

> I saw the bug 731746 comment 19, which way should i do, implement the two methods in JS 
> or rewrite all code in c++?

Re-implementing in C++ would yield the most correct result, but it seems to me that adding add/removeEventListener is the path of least resistance.  Do you think that's right?  A lot of people are itching to get this landed, so we should do whatever is easiest/fastest.

Comment 68

[Pin Zhang [:pzhang] (inactive)]

Hi Justin, i will introduce the two methods, addEventListener and removeEventListener, to the IDL, i should ask for superreview again, right?

Comment 69

[Justin Lebar (not reading bugmail)]

I don't think you need a new SR, since Jonas already approved that change in bug 731746.

Comment 70

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, sr for that change isn't needed.

Comment 71

[Pin Zhang [:pzhang] (inactive)]

Attachment: all-in-one patch, WIP, v2

It depends on the patches of bug749053.

Comment 72

[Justin Lebar (not reading bugmail)]

I'm going to look at bug 749053 first, since changes there may mean you have to make changes here.

Comment 73

[Justin Lebar (not reading bugmail)]

One thing that you could work on here in the meantime is tests.

We should have tests for all the weird edgecase behavior such as

  * what happens if you disable the radio while we're seeking?
  * if you trigger two seeks simultaneously, does the first one always get onsuccess, and does the second one always get onfailure?

etc.

We probably can't write these as mochitests, but most of these could be an automated test that QA could run on the device, as part of one of our test apps in Gaia.

Comment 74

[Michael Wu [:mwu]]

Comment on attachment 656423 [details] [diff] [review]
all-in-one patch, WIP, v2

Review of attachment 656423 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fm/FMRadio.cpp
@@ +16,5 @@
> +#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "FMRadio" , ## args)
> +
> +// The pref which indicates if the device has an internal antenna. 
> +// If the pref is true, the antanna will be always available.
> +#define DOM_FM_ANTENNA_INTERNAL_PREF "dom.fm.antenna.internal"

I think we want the hal backend to tell us this.

::: dom/fm/FMRadioService.jsm
@@ +5,5 @@
> +"use strict"
> +
> +let DEBUG = 0;
> +if (DEBUG)
> +  debug = function(s) { dump("-*- Fallback FMRadioService component: " + s + "\n");  };

Hmm.. This doesn't really sound like a fallback since it's being run when we actually have a fm radio.

@@ +366,5 @@
> +            upperLimit: FM_BANDS[_currentBand].upper,
> +            spaceType:  _currentSpaceType   // 100KHz by default
> +          });
> +
> +          FMRadio.addEventListener("enabled", function on_enabled() {

You should add the event listener before enabling the radio. If enable immediately enables the radio, your listener won't be called. Same thing goes for the disable listener.

Comment 75

[Justin Lebar (not reading bugmail)]

Comment on attachment 656423 [details] [diff] [review]
all-in-one patch, WIP, v2

What's left to make this not a WIP?

Comment 76

[Pin Zhang [:pzhang] (inactive)]

(In reply to Michael Wu [:mwu] from comment #74)
> Comment on attachment 656423 [details] [diff] [review]
> all-in-one patch, WIP, v2
> 
> Review of attachment 656423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/fm/FMRadio.cpp
> @@ +16,5 @@
> > +#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "FMRadio" , ## args)
> > +
> > +// The pref which indicates if the device has an internal antenna. 
> > +// If the pref is true, the antanna will be always available.
> > +#define DOM_FM_ANTENNA_INTERNAL_PREF "dom.fm.antenna.internal"
> 
> I think we want the hal backend to tell us this.
So there should be an interface in the hal part, right?
> 
> ::: dom/fm/FMRadioService.jsm
> @@ +5,5 @@
> > +"use strict"
> > +
> > +let DEBUG = 0;
> > +if (DEBUG)
> > +  debug = function(s) { dump("-*- Fallback FMRadioService component: " + s + "\n");  };
> 
> Hmm.. This doesn't really sound like a fallback since it's being run when we
> actually have a fm radio.
> 
I will remove the *Fallback* keyword.
> @@ +366,5 @@
> > +            upperLimit: FM_BANDS[_currentBand].upper,
> > +            spaceType:  _currentSpaceType   // 100KHz by default
> > +          });
> > +
> > +          FMRadio.addEventListener("enabled", function on_enabled() {
> 
> You should add the event listener before enabling the radio. If enable
> immediately enables the radio, your listener won't be called. Same thing
> goes for the disable listener.
OK

Comment 77

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #75)
> Comment on attachment 656423 [details] [diff] [review]
> all-in-one patch, WIP, v2
> 
> What's left to make this not a WIP?
If the interfaces of the hal part(bug 749053) are settled, i think it is the right version to be reviewed.

Comment 78

[Pin Zhang [:pzhang] (inactive)]

Attachment: all-in-one patch, v1

Comment 79

[Pin Zhang [:pzhang] (inactive)]

Attachment: all-in-one patch, v2

Comment 80

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #73)
> One thing that you could work on here in the meantime is tests.
> 
> We should have tests for all the weird edgecase behavior such as
> 
>   * what happens if you disable the radio while we're seeking?
>   * if you trigger two seeks simultaneously, does the first one always get
> onsuccess, and does the second one always get onfailure?
> 
> etc.
> 
> We probably can't write these as mochitests, but most of these could be an
> automated test that QA could run on the device, as part of one of our test
> apps in Gaia.

Here is the tests: 
  https://github.com/mozilla-b2g/gaia/pull/4298

Comment 81

[Justin Lebar (not reading bugmail)]

> Here is the tests: 
> https://github.com/mozilla-b2g/gaia/pull/4298

Fantastic!

I'm really behind on reviews after the B2G work week, but I'll get to this ASAP (hopefully Wed or Th).  I'm sorry for the long wait.

Comment 82

[Pin Zhang [:pzhang] (inactive)]

Attachment: all-in-one patch, v3

Update the patch to make it work with the latest m-c, see bug788561

Comment 83

[Pin Zhang [:pzhang] (inactive)]

Hi Justin, do you have a plan to review my code?

Comment 84

[Justin Lebar (not reading bugmail)]

Sound the trumpets!

Here's the first part of the review.  It covers everything except the JS bits.  I'm working on the JS code now.

I'm sorry again this took so long.  The next round should go faster.

>+++ b/dom/fm/nsIDOMFMRadio.idl
>+    /**
>+     * This is a temporary hack, will be removed by inheriting
>+     * nsIJSDOMEventTarget, see bug 731746
>+     */

"The addEventListener, removeEventListener, and dispatchEvent methods below are
a temporary hack, which will be removed once nsIDOMFMRadio can inherit from
nsIJSDOMEventTarget.  See bug 731746."

(The main problem here is that we use "this" when we mean "these".)

>+    [optional_argc] void addEventListener(in DOMString type,
>+                                          in nsIDOMEventListener listener,
>+                                          [optional] in boolean useCapture,
>+                                          [optional] in boolean wantsUntrusted);
>+
>+    void  removeEventListener(in DOMString type,
>+                              in nsIDOMEventListener listener,
>+                              [optional] in boolean useCapture);
>+
>+    boolean dispatchEvent(in nsIDOMEvent evt) raises(DOMException);
>+};
>+
>diff --git a/dom/fm/nsIFMRadio.idl b/dom/fm/nsIFMRadio.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/fm/nsIFMRadio.idl
>@@ -0,0 +1,41 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsISupports.idl"
>+#include "nsIDOMEvent.idl"

Does this file not work if you don't include these to files?  It's not clear
why you need them.

>+#include "nsIDOMEventTarget.idl"

Please add a brief comment here explaining what nsIFMRadio is for.

>+[scriptable, builtinclass, uuid(26288adc-d2c1-4fbc-86b5-ecd8173fbf90)]
>+interface nsIFMRadio : nsIDOMEventTarget {
>+    readonly attribute boolean enabled;
>+
>+    readonly attribute long frequency;

Please indicate what the units are here.  (I don't see why this shouldn't be a
double like nsIDOMFMRadio.)

>+    readonly attribute float signalStrength;

Please match nsIDOMFMRadio by making signalStrength a double.

>+    readonly attribute boolean isAntennaAvailable;
>+
>+    [implicit_jscontext]
>+    void enable(in jsval settings);

Please add a comment explaining what settings are expected.

In fact, we really should create a new interface (e.g. nsIFMRadioSettings) and
use that as the argument type here.

>+    void disable();
>+
>+    void seek(in short direction);

Relying on FM_RADIO_SEEK_DIRECTION_{UP,DOWN} is problematic here, because
callers from JS don't have access to those constants.

Instead, you should add constants to this IDL file, like:

      const long SEEK_DIRECTION_UP = 0;
      const long SEEK_DIRECTION_DOWN = 1;

(Notice that these are |long|'s, that is, int32's.  I don't think it makes
sense to use a short here; this is effectively an enum, and we use XPIDL |long|
for that.)

>+    void cancelSeek();
>+
>+    [implicit_jscontext]
>+    jsval getSettings();

Again, switch this to nsIFMRadioSettings, so that if I called this from C++, I
wouldn't have to unpack a mysterious jsval.

>+    void setFrequency(in long frequency);

Again, indicate what the units are on frequency, and consider using a double
here (unless there's a good reason not to?).

>+    [implicit_jscontext] attribute jsval onantennastatechange;
>+    [implicit_jscontext] attribute jsval onseekcomplete;
>+    [implicit_jscontext] attribute jsval onenabled;
>+    [implicit_jscontext] attribute jsval ondisabled;

These should all be nsIRunnable.  That will give you a strongly-typed callback
function, which is what you're going for here, I think.  (In other words, if
you used nsIRunnable here, you wouldn't have to change any of the JS code which
calls this interface.)

>diff --git a/b2g/chrome/content/shell.js b/b2g/chrome/content/shell.js
>--- a/b2g/chrome/content/shell.js
>+++ b/b2g/chrome/content/shell.js
>@@ -8,16 +8,17 @@ const Cc = Components.classes;
> const Ci = Components.interfaces;
> const Cu = Components.utils;
> const Cr = Components.results;
> 
> Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> Cu.import('resource://gre/modules/Services.jsm');
> Cu.import('resource://gre/modules/ContactService.jsm');
> Cu.import('resource://gre/modules/SettingsChangeNotifier.jsm');
>+Cu.import('resource://gre/modules/FMRadioService.jsm');

It looks like FM radio support is guarded by MOZ_B2G_FM, but this JSM is
included on all B2G builds.  Should you add a check here that MOZ_B2G_FM is
set?

>diff --git a/dom/fm/FMRadio.h b/dom/fm/FMRadio.h

>+using namespace mozilla::hal;

We never put |using namespace| inside a header file.  That causes serious
breakage, because it means that every header file which is included below this
one also |uses namespace mozilla::hal|, which effectively puts mozilla::hal
into the global namespace!

>+namespace mozilla {
>+namespace hal {
>+class SwitchEvent;
>+typedef Observer<SwitchEvent> SwitchObserver;
>+} // namespace hal

This is defined in hal/HalTypes.h, which is included above.  Do you need this here?

>+
>+namespace dom {
>+namespace fm {
>+
>+/* Header file */

This comment is not adding anything; please remove it.

>+class FMRadio : public nsDOMEventTargetHelper,
>+                  public nsIFMRadio,
>+                  public FMRadioObserver,
>+                  public mozilla::hal::SwitchObserver

Fix spacing, please.  (Align everything to the first |public| above.)

>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIFMRADIO
>+
>+  NS_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper::)
>+  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(
>+                                                   FMRadio,
>+                                                   nsDOMEventTargetHelper)
>+
>+  FMRadio();
>+
>+private:
>+  ~FMRadio();
>+  bool mHasInternalAntenna;
>+  /**
>+   * Dispatch a trusted non-cancellable and no-bubbling event to itself
>+   */

       Dispatch a trusted, non-cancellable, not-bubbling event to ourself.

>+  nsresult DispatchTrustedEventToSelf(const nsAString& aEventName);
>+
>+protected:
>+  int mStatus;

int32_t, or uint32_t, but almost never raw |int|.

But it looks like this should actually have the type hal::SwitchState.

Please add a comment indicating what this is the status of, and rename the
variable to something more meaningful than |mStatus|.

>+  nsAutoPtr<mozilla::hal::SwitchObserver> mObserver;

Why are these protected and not private?  Does someone inherit from this class?

>+public:
>+  void Notify(const FMRadioOperationInformation& info);
>+  void Notify(const mozilla::hal::SwitchEvent& aEvent);

These are virtual methods, no?  Notify(SwitchEvent&) comes from SwitchObserver,
and Notify(FMRadioOperationInformation&) comes from FMRadioObserver.  Both
SwitchObserver and FMRadioObserver are instances of mozilla::Observer, defined
in xpcom/glue/Observer.h.

In that case, these need to be marked as virtual.  Also, please put them up
with the other public methods.

>diff --git a/dom/fm/FMRadio.cpp b/dom/fm/FMRadio.cpp
>new file mode 100644
>--- /dev/null
>+++ b/dom/fm/FMRadio.cpp

A bunch of things will be changing in this file, per my comments in the IDL.  I
haven't commented on them further here.

>@@ -0,0 +1,260 @@
>+#include <android/log.h>
>+[...]
>+#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "FMRadio" , ## args)

As far as I can tell, these log statements are the only android-only feature of
this file.  Please guard them with an ifdef.  (You can just define LOG to do
nothing if we're not on Android.)

The right solution would be to use prlog, but that's kind of a pain to use in
b2g.  We have a pretty bad logging story. :(

>+#define DOM_FM_ANTENNA_INTERNAL_PREF "dom.fm.antenna.internal"
>+#define RADIO_SEEK_COMPLETE_EVENT_NAME NS_LITERAL_STRING("seekcomplete")
>+#define RADIO_DIABLED_EVENT_NAME NS_LITERAL_STRING("disabled")
>+#define RADIO_ENABLED_EVENT_NAME NS_LITERAL_STRING("enabled")
>+#define ANTENNA_STATE_CHANGED_EVENT_NAME NS_LITERAL_STRING("antennastatechange")

You use these only once, and the macros aren't any shorter than what they're
replacing, so I'd inline the macros, if it was me.  Whatever you prefer is fine.

>+/* Implementation file */

This comment isn't adding anything; please remove.

>+FMRadio::FMRadio(): mStatus(1), mHasInternalAntenna(false)

We format this like:

FMRadio::FMRadio()
  : mStatus(1)
  , mHasInternalAntenna(false)

>+{
>+  LOG("FMRadio is initialized.");
>+
>+  if (!NS_SUCCEEDED(Preferences::GetBool(DOM_FM_ANTENNA_INTERNAL_PREF,
>+            &mHasInternalAntenna)) || !mHasInternalAntenna)

We use NS_FAILED instead of !NS_SUCCEEDED and format this like

    if (NS_FAILED(Preferences::GetBool(DOM_FM_ANTENNA_INTERNAL_PREF,
                                       &mHasInternalAntenna)) ||
        !mHasInternalAntenna) {

But even better, I think, would be to do

    mHasInternalAntenna = Preferences::GetBool("dom.fm.antenna.internal",
                                               /* default = */ false);
    if (mHasInternalAntenna) {
      LOG("We have an internal antenna.");
    } else {
      ...
    }

>+  {
>+    RegisterSwitchObserver(SWITCH_HEADPHONES, this);
>+    mStatus = GetCurrentSwitchState(SWITCH_HEADPHONES);
>+  }
>+  else
>+  {
>+    LOG("We have an internal antenna.");
>+  }
>+
>+  RegisterFMRadioObserver(this);
>+}
>+
>+FMRadio::~FMRadio()
>+{
>+  UnregisterFMRadioObserver(this);
>+  if (!mHasInternalAntenna)
>+  {

Please do

    if (...) {

(We almost never put the brace for an if statement on the next line.)

>+/* readonly attribute boolean isAntennaAvailable; */
>+NS_IMETHODIMP FMRadio::GetIsAntennaAvailable(bool *aIsAvailable)
>+{
>+  if (mHasInternalAntenna)
>+    *aIsAvailable = true;
>+  else
>+    *aIsAvailable = mStatus == 0;

Gecko style is to brace all if/else statements.

Also, when you switch mStatus to the |SwitchState| type, please use
SWITCH_STATE_{ON,OFF}, not "0", here and elsewhere.

>+/* readonly attribute long signalStrength; */
>+NS_IMETHODIMP FMRadio::GetSignalStrength(float *aSignalStrength)
>+{
>+  uint32_t signalStrength;
>+
>+  // The range of signal strengh is [0-100], higher than 100 means signal is strong.

Nit: This is a comma splice.  Replace the comma with a period.

But I also don't understand this comment; the signal strength is [0-100], but
it can be bigger than 100?  Perhaps you could elaborate a bit in the comment.

>+  GetFMRadioSignalStrength(&signalStrength);
>+
>+  *aSignalStrength = (signalStrength > 100 ? 100 : signalStrength) / 100.0;

Nit: how about

    *aSignalStrength = NS_MIN(signalStrength, 100) / 100.0;

>+/* readonly attribute blean enabled; */

blean

>+NS_IMETHODIMP FMRadio::GetEnabled(bool *aEnabled)
>+{
>+  *aEnabled = IsFMRadioOn();
>+  return NS_OK;
>+}
>
>+/* [implicit_jscontext] void enableRadio (in jsval settings); */
>+NS_IMETHODIMP FMRadio::Enable(const JS::Value & settings, JSContext* cx)

Nit: |const JS::Value& settings|.  But this whole thing is going to change
anyway, per the comments on the IDL.

>+void FMRadio::Notify(const SwitchEvent& aEvent)
>+{
>+  int preStatus = mStatus;

|oldStatus| would be more idiomatic.

>+  mStatus = aEvent.status();
>+
>+  if (preStatus != mStatus) {
>+    LOG("Antenna state is changed!");
>+    DispatchTrustedEventToSelf(ANTENNA_STATE_CHANGED_EVENT_NAME);
>+  }
>+}
>+
>+nsresult
>+FMRadio::DispatchTrustedEventToSelf(const nsAString& aEventName)
>+{
>+  nsRefPtr<nsDOMEvent> event = new nsDOMEvent(nullptr, nullptr);
>+  nsresult rv = event->InitEvent(aEventName, false, false);

Nit: Please do

    event->InitEvent(aEventName, /* bubbles = */ false, /* cancelable = */ false);

>diff --git a/dom/fm/Makefile.in b/dom/fm/Makefile.in
>+ifeq ($(MOZ_BUILD_APP),b2g)
>+EXTRA_JS_MODULES =   \
>+  FMRadioService.jsm \

Again, it's not clear to me why this guy is B2G only.

Comment 85

[Justin Lebar (not reading bugmail)]

TBH, it's not clear to me that we're winning anything by using JS here.  We
shouldn't rewrite this whole thing in C++ right now, but I think we should
start on that project once we land this and shake out the first round of bugs.

We'll get increased correctness, which is great, but I think the result may
also be simpler in a lot of ways.  Currently, the control flow is as follows:

  webpage (JS) -> nsIDOMFMRadio (JS) -> FMRadioService (JS) ->
    nsIFMRadio (C++) -> HAL (C++) -> hardware

but if nsIDOMFMRadio were implemented in C++, we could simplify this to

  webpage (JS) -> nsIDOMFMRadio (C++) -> HAL (C++) -> hardware

I think that would be a significant improvement.  What do you think?

>diff --git a/dom/fm/DOMFMRadio.js b/dom/fm/DOMFMRadio.js

I think it would be better to call this file "DOMFMRadioChild.js" and to call
FMRadioService.jsm "DOMFMRadioParent.jsm".  Then we don't have to guess about
what code lives in which process.

>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

I don't use emacs, but shouldn't this be "Mode: JavaScript"?

>+/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */

Please take out autoindent; that's an editing pref, not a formatting pref!

(Where do we get these bizarre modelines from?)

>+XPCOMUtils.defineLazyGetter(Services, "DOMRequest", function() {
>+  return Cc["@mozilla.org/dom/dom-request-service;1"].getService(Ci.nsIDOMRequestService);
>+});
>+
>+XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
>+                                   "@mozilla.org/childprocessmessagemanager;1",
>+                                   "nsISyncMessageSender");
>+
>+function DOMFMRadio() { }
>+
>+DOMFMRadio.prototype = {
>+  __proto__: DOMRequestIpcHelper.prototype,
>+
>+  classID: DOMFMMANAGER_CID,
>+  classInfo: XPCOMUtils.generateCI({
>+               classID: DOMFMMANAGER_CID,
>+               contractID: DOMFMMANAGER_CONTRACTID,
>+               classDescription: "DOMFMRadio",
>+               interfaces: [Ci.nsIDOMFMRadio],
>+               flags: Ci.nsIClassInfo.DOM_OBJECT
>+             }),
>+
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIDOMFMRadio, Ci.nsIDOMGlobalPropertyInitializer]),
>+
>+  // nsIDOMGlobalPropertyInitializer implementation
>+  init: function(aWindow) {
>+    let principal = aWindow.document.nodePrincipal;
>+    let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"].getService(Ci.nsIScriptSecurityManager);
>+
>+    let perm = (principal == secMan.getSystemPrincipal()) ?
>+                 Ci.nsIPermissionManager.ALLOW_ACTION :
>+                 Services.perms.testExactPermission(principal.URI, "fmradio");
>+    this._hasPrivileges = perm == Ci.nsIPermissionManager.ALLOW_ACTION;
>+
>+    const messages = ["DOMFMRadio:enable:Return:OK", "DOMFMRadio:enable:Return:NO",
>+                      "DOMFMRadio:disable:Return:OK", "DOMFMRadio:disable:Return:NO",
>+                      "DOMFMRadio:setFrequency:Return:OK", "DOMFMRadio:setFrequency:Return:NO",
>+                      "DOMFMRadio:seekUp:Return:OK", "DOMFMRadio:seekUp:Return:NO",
>+                      "DOMFMRadio:seekDown:Return:OK", "DOMFMRadio:seekDown:Return:NO",
>+                      "DOMFMRadio:cancelSeek:Return:OK", "DOMFMRadio:cancelSeek:Return:NO",
>+                      "DOMFMRadio:frequencyChange", "DOMFMRadio:powerStateChange",
>+                      "DOMFMRadio:signalStrengthChange", "DOMFMRadio:getAntennaState", "DOMFMRadio:antennaChange"];

Why is getAntennaState in this list?

(I think this whole idiom that DOMRequestIpcHelper has developed is pretty
bogus; you shouldn't list twice each of the events that you want to listen to,
because that's just asking for a mistake.  But that's not your bug to fix
here.)

>+  // Called from DOMRequestIpcHelper
>+  uninit: function() {
>+    this._onFrequencyChange = null;
>+    this._onAntennaChange = null;
>+    this._onDisabled = null;
>+    this._onEnabled = null;
>+    this._onSignalChange = null;
>+  },
>+
>+  _createEvent: function(name) {
>+    return new this._window.Event(name);
>+  },
>+
>+  _sendMessageForRequest: function(name, data, request) {
>+    let id = this.getRequestId(request);
>+    cpmm.sendAsyncMessage(name, {
>+      data: data,
>+      rid: id,
>+      mid: this._id
>+    });
>+  },
>+
>+  _fireFrequencyChangeEvent: function() {
>+    let e = this._createEvent("frequencychange");
>+    if (this._onFrequencyChange) {
>+      this._onFrequencyChange.handleEvent(e);
>+    }
>+    this.dispatchEvent(e);
>+  },
>+
>+  _firePowerStateChangeEvent: function() {
>+    let _enabled = this.enabled;
>+    debug("Current power state: " + _enabled);
>+    if (_enabled) {
>+      let e = this._createEvent("enabled");
>+      if (this._onEnabled) {
>+        this._onEnabled.handleEvent(e);
>+      }
>+      this.dispatchEvent(e);
>+    } else {
>+      let e = this._createEvent("disabled");
>+      if (this._onDisabled) {
>+        this._onDisabled.handleEvent(e);
>+      }
>+      this.dispatchEvent(e);
>+    }
>+  },
>+
>+  _fireSignalChangeEvent: function() {

_fireSignalStrengthChangeEvent, please.

>+    let e = this._createEvent("signalstrengthchange");
>+    if (this._onSignalChange) {
>+      this._onSignalChange.handleEvent(e);
>+    }
>+    this.dispatchEvent(e);
>+  },
>+
>+  _fireAntennaChangeEvent: function() {

_fireAntennaAvailableChangeEvent, please.

>+    let e = this._createEvent("antennaavailablechange");
>+    if (this._onAntennaChange) {
>+      this._onAntennaChange.handleEvent(e);
>+    }
>+    this.dispatchEvent(e);
>+  },
>+
>+  receiveMessage: function(aMessage) {
>+    let msg = aMessage.json;
>+    if (msg.mid && msg.mid != this._id) {
>+      return;
>+    }
>+    let request;
>+    switch (aMessage.name) {
>+      case "DOMFMRadio:enable:Return:OK":
>+        request = this.takeRequest(msg.rid);

What happens if takeRequest fails?

>+        Services.DOMRequest.fireSuccess(request, msg.data);
>+        break;
>+      case "DOMFMRadio:enable:Return:NO":
>+        request = this.takeRequest(msg.rid);
>+        Services.DOMRequest.fireError(request, "Failed to turn on FM");

s/FM/FM radio

>+        break;
>+      case "DOMFMRadio:disable:Return:OK":
>+        request = this.takeRequest(msg.rid);
>+        Services.DOMRequest.fireSuccess(request, msg.data);
>+        break;
>+      case "DOMFMRadio:disable:Return:NO":
>+        request = this.takeRequest(msg.rid);
>+        Services.DOMRequest.fireError(request, "Failed to turn off FM");

s/FM/FM radio

>+        break;
>+      case "DOMFMRadio:setFrequency:Return:OK":
>+        request = this.takeRequest(msg.rid);
>+        Services.DOMRequest.fireSuccess(request, true);
>+        break;
>+      case "DOMFMRadio:setFrequency:Return:NO":
>+        request = this.takeRequest(msg.rid);
>+        Services.DOMRequest.fireError(request, "Failed to set FM frequency");

s/FM/FM radio

>+        break;
>+      case "DOMFMRadio:seekUp:Return:OK":
>+        request = this.takeRequest(msg.rid);
>+        Services.DOMRequest.fireSuccess(request, true);
>+        break;
>+      case "DOMFMRadio:seekUp:Return:NO":
>+        request = this.takeRequest(msg.rid);
>+        Services.DOMRequest.fireError(request, "Failed to seek up frequency");

How about "FM radio seek-up failed".

>+        break;
>+      case "DOMFMRadio:seekDown:Return:OK":
>+        request = this.takeRequest(msg.rid);
>+        Services.DOMRequest.fireSuccess(request, true);
>+        break;
>+      case "DOMFMRadio:seekDown:Return:NO":
>+        request = this.takeRequest(msg.rid);
>+        Services.DOMRequest.fireError(request, "Failed to seek down frequency");

Suggest something like "FM radio seek-down failed."

>+        break;
>+      case "DOMFMRadio:cancelSeek:Return:OK":
>+        request = this.takeRequest(msg.rid);
>+        Services.DOMRequest.fireSuccess(request, true);
>+        break;
>+      case "DOMFMRadio:cancelSeek:Return:NO":
>+        request = this.takeRequest(msg.rid);
>+        Services.DOMRequest.fireError(request, "Failed to cancel seek");
>+        break;
>+      case "DOMFMRadio:powerStateChange":
>+        this._firePowerStateChangeEvent();
>+        break;
>+      case "DOMFMRadio:frequencyChange":
>+        this._fireFrequencyChangeEvent();
>+        break;
>+      case "DOMFMRadio:signalStrengthChange":
>+        this._fireSignalChangeEvent();
>+        break;
>+      case "DOMFMRadio:antennaChange":
>+        this._fireAntennaChangeEvent();
>+        break;
>+    }
>+  },

>+  _call: function(name, arg) {
>+    this._checkPermission();

_call calls _checkPermission(), but everyone who calls _call() also calls
_checkPermission().

>+    var request = this.createRequest();
>+    this._sendMessageForRequest("DOMFMRadio:" + name, arg, request);
>+    return request;
>+  },

>+  // nsIJSDOMEventTarget

Except this is a lie; we're faking it right now.  So please indicate that.

>+  addEventListener: function(type, listener, useCapture, wantsUntrusted) {

You're not using the wantsUntrusted field, so you should omit it.

>+    this._checkPermission();
>+
>+    if (!this._eventTypes) {
>+      this._eventTypes = {};
>+    }

Just set this in the constructor so you don't have to check it everywhere.

Also, this._eventListenersByType would be a better name than _eventTypes.

>diff --git a/dom/fm/FMRadioService.jsm b/dom/fm/FMRadioService.jsm
>+"use strict"
>+
>+let DEBUG = 0;
>+if (DEBUG)
>+  debug = function(s) { dump("-*- FMRadioService component: " + s + "\n");  };
>+else
>+  debug = function(s) {};
>+
>+const Cu = Components.utils;
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+
>+const kMozSettingsChangedObserverTopic = "mozsettings-changed";
>+
>+const BAND_87500_108000_kHz = 1;
>+const BAND_76000_108000_kHz = 2;
>+const BAND_76000_90000_kHz  = 3;
>+
>+const FM_BANDS = { };
>+FM_BANDS[BAND_76000_90000_kHz] = {
>+  lower: 76000,
>+  upper: 90000
>+};
>+
>+FM_BANDS[BAND_87500_108000_kHz] = {
>+  lower: 87500,
>+  upper: 108000
>+};
>+
>+FM_BANDS[BAND_76000_108000_kHz] = {
>+  lower: 76000,
>+  upper: 108000
>+};
>+
>+const BAND_SETTING_KEY = "fm.band";
>+const SPACETYPE_SETTING_KEY = "fm.spaceType";

We're inconsistent about how we name these string constants.  Here they're
capitals, like a C++ macro.  But then we have kMozSettingsChangedObserverTopic.
I don't care what you do, so long as you're consistent.

Also, "spaceType" is not a good name for the setting.  "fm.channelInterval"
would be better.  "fmRadio.cannelInterval" would be even better.  :)

>+// Hal types
>+const RADIO_SEEK_DIRECTION_UP   = 0;
>+const RADIO_SEEK_DIRECTION_DOWN = 1;

We're going to move these onto the nsIFMRadio interface, which will be an
improvement.

>+const RADIO_CHANNEL_SPACE_TYPE_200KHZ = 200;
>+const RADIO_CHANNEL_SPACE_TYPE_100KHZ = 100;
>+const RADIO_CHANNEL_SPACE_TYPE_50KHZ  = 50;

It's not clear to me why we should have constants like this when we could have

const VALID_CHANNEL_INTERVALS = [200, 100, 50]; // KHz
let _currentInterval = ...; // _currentInterval is either 200, 100, or 50.

But in any case, please call these "CHANNEL_INTERVAL" or "INTERVAL", but not
"SPACE_TYPE".

>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource://gre/modules/Services.jsm");
>+Cu.import("resource://gre/modules/ctypes.jsm");

Do you need ctypes here?

>+XPCOMUtils.defineLazyServiceGetter(this, "ppmm",
>+                                   "@mozilla.org/parentprocessmessagemanager;1",
>+                                   "nsIMessageListenerManager");
>+
>+XPCOMUtils.defineLazyGetter(this, "FMRadio", function() {
>+  return Cc["@mozilla.org/fmradio;1"].getService(Ci.nsIFMRadio);
>+});
>+
>+XPCOMUtils.defineLazyServiceGetter(this, "gSettingsService", 
>+                            "@mozilla.org/settingsService;1",
>+                            "nsISettingsService");

Please indent this as


  XPCOMUtils.defineLazyServiceGetter(this, "gSettingsService", 
                                     "@mozilla.org/settingsService;1",
                                     "nsISettingsService");

>+XPCOMUtils.defineLazyGetter(this, "hiddenWindow", function() {
>+  let window = Cc["@mozilla.org/appshell/appShellService;1"]
>+                 .getService(Ci.nsIAppShellService)
>+                 .hiddenDOMWindow;
>+  return window;
>+});
>+
>+let EXPORTED_SYMBOLS = ["FMRadioService"];
>+
>+let _seeking = false;
>+let _seekingCallback = null;
>+
>+function onSeekComplete(forceStop) {
>+  if (_seeking) {
>+    _seeking = false;
>+
>+    if (_seekingCallback) {
>+      forceStop == true ? _seekingCallback(-1) : _seekingCallback();

It looks like you intend to pass the frequency to _seekingCallback, but you
don't here.  I'm not sure, but is the right thing to pass a bool instead of -1?
Seems like you don't miss not having the frequency in the callback.

>+      _seekingCallback = null;
>+    }
>+  }
>+}
>+
>+
>+/**
>+ * Seek the next channel with given direction.
>+ * Only one seek action is allowed at once.
>+ */
>+function seekStation(direction, callback) {
>+  debug("seek station with direction " + direction);
>+
>+  if (_seeking) {
>+    // Pass an negative number to callback which indicates that the seek action fails.

s/an negative/a negative/
s/callback/the callback/
s/fails/failed/

Also, please wrap this line and the other long comments in this patch.

>+    callback(-1);
>+    return;
>+  }
>+
>+  _seekingCallback = callback;
>+  _seeking = true;
>+
>+  FMRadio.seek(direction);
>+  FMRadio.addEventListener("seekcomplete", function FM_onSeekComplete() {
>+    FMRadio.removeEventListener("seekcomplete", FM_onSeekComplete);
>+    onSeekComplete();
>+  });
>+}
>+
>+function cancelSeek() {
>+  if (_seeking) {
>+    FMRadio.cancelSeek();
>+    // No fail-to-seek or similar event will be fired from hal, so execute the seek
>+    // callback here manually.
>+    onSeekComplete(true);
>+    // The FM radio will stop at one frequency without any event, need to update current frequency

s/, need to update current frequency/, so we need to update the current frequency./

>+    FMRadioService._updateFrequency();
>+  }
>+}
>+
>+/**
>+ * Round the frequency to match the range of frequency and the channel width.
>+ * If the given frequency is out of range, return null.
>+ * For example:
>+ *  - lower: 87.5MHz, upper: 108MHz, space: 0.2MHz

We're calling |space| the |interval|, right?

>+ *    87.6 is rounded to 87.7
>+ *    87.58 is runded to 87.5

Please add an example here for what happens when we pass an out-of-range value.

Also, probably better to use values in KHz rather than MHz in your example,
since that's what the function deals with.

>+function roundFrequency(frequencyInKHz) {
>+  if (frequencyInKHz < FM_BANDS[_currentBand].lower ||
>+      frequencyInKHz > FM_BANDS[_currentBand].upper) {
>+    return null;
>+  }
>+
>+  let roundedPart = frequencyInKHz - FM_BANDS[_currentBand].lower;

Don't call this "roundedPart", because it's not rounded yet.  Either shove
|freq - lower| into the line below, or call this something else.

>+  roundedPart = Math.round(roundedPart / _currentSpaceType) * _currentSpaceType;
>+  frequencyInKHz = FM_BANDS[_currentBand].lower + roundedPart;
>+  return frequencyInKHz;
>+}
>+
>+let _inited = false;

|_initialized|

>+// Indicates if the FM radio is enabled currently
>+let _isEnabled = false;
>+// Indicates if the FM radio is being enabled currently
>+let _enabling = false;
>+// Current frequency in KHz
>+let _currentFrequency = 0;
>+// Current band setting
>+let _currentBand = BAND_87500_108000_kHz;
>+let _currentSpaceType = RADIO_CHANNEL_SPACE_TYPE_100KHZ;

_currentInterval.

Why do you initialize _currentBand and _currentSpaceType to actual values,
instead of null?  We shouldn't use these values until we get them from the FM
radio, right?

>+let _currentSignalStrength = 0;
>+// Indicates if the antenna is available currently

s/available currently/currently available

>+let _antennaAvailable = true;

It's confusing that we use some global variables and functions and some class
variables and functions.

Can we make all the variables and functions live on the class, please?

>+let FMRadioService = {
>+  init: function() {
>+    if (_inited === true) {
>+      return;
>+    }
>+    _inited = true;
>+
>+    this._messages = ["DOMFMRadio:enable", "DOMFMRadio:disable",
>+                      "DOMFMRadio:setFrequency", "DOMFMRadio:getCurrentBand", "DOMFMRadio:getPowerState",
>+                      "DOMFMRadio:getFrequency", "DOMFMRadio:getSignalStrength", "DOMFMRadio:getAntennaState",
>+                      "DOMFMRadio:seekUp", "DOMFMRadio:seekDown", "DOMFMRadio:cancelSeek"
>+                     ];
>+    this._messages.forEach(function(msgName) {
>+      ppmm.addMessageListener(msgName, this);
>+    }.bind(this));
>+
>+    Services.obs.addObserver(this, "profile-before-change", false);
>+    Services.obs.addObserver(this, kMozSettingsChangedObserverTopic, false);

We hard-code some observer message names but not others, which is
inconsistent.  I have no problem hard-coding them all.

>+    this._updatePowerState();
>+
>+    // If the FM radio is enabled, update the current frequency immediately,
>+    // and start interval to check the signal strength.
>+    if (_isEnabled) {
>+      this._updateFrequency();
>+      this._startSignalChecking();
>+    }
>+
>+    // Get the band setting
>+    gSettingsService.createLock().get(BAND_SETTING_KEY, this);
>+
>+    // Get the space type setting
>+    gSettingsService.createLock().get(SPACETYPE_SETTING_KEY, this);

It doesn't matter much, but for the sake of cleanliness and not encouraging
others to write bad code, please create one lock and get both settings.

>+    this._updateAntennaState();
>+
>+    let self = this;
>+    FMRadio.onantennastatechange = function onantennachange() {
>+      self._updateAntennaState();
>+    };
>+
>+    debug("Inited");

"Initialized", please.

>+  },
>+
>+  // nsISettingsServiceCallback
>+  handle: function(aName, aResult) {
>+    if (aName == BAND_SETTING_KEY) {
>+      this._updateBand(aResult);
>+    } else if (aName == SPACETYPE_SETTING_KEY) {
>+      this._updateSpaceType(aResult);
>+    }
>+  },
>+
>+  handleError: function(aErrorMessage) {

When does this get called?  And why is this the right reaction to an error?

>+    this._updateBand(BAND_87500_108000_kHz);
>+    this._updateSpaceType(RADIO_CHANNEL_SPACE_TYPE_100KHZ);
>+  },
>+
>+  _updateAntennaState: function() {
>+    let antennaState = FMRadio.isAntennaAvailable;
>+    let antennaChanged = antennaState != _antennaAvailable;
>+    _antennaAvailable = antennaState;
>+
>+    if (antennaChanged) {
>+      ppmm.broadcastAsyncMessage("DOMFMRadio:antennaChange", { });
>+    }
>+  },
>+
>+  _updateBand: function(band) {
>+      switch (parseInt(band)) {
>+        case BAND_87500_108000_kHz:
>+        case BAND_76000_108000_kHz:
>+        case BAND_76000_90000_kHz:
>+          _currentBand = band;
>+          break;
>+      }

We don't need to inform hal that the band has changed?

>+  },
>+
>+  _updateSpaceType: function(spaceType) {
>+    switch (parseInt(spaceType)) {
>+      case RADIO_CHANNEL_SPACE_TYPE_50KHZ:
>+      case RADIO_CHANNEL_SPACE_TYPE_100KHZ:
>+      case RADIO_CHANNEL_SPACE_TYPE_200KHZ:
>+        _currentSpaceType = spaceType;
>+        break;
>+    }

Similarly, we don't need to inform hal here?  I guess hal is also listening to
the settings notifications?

>+  /**
>+   * Update the current signal strength.
>+   * Send strength change message if the signal strength is changed.
>+   */
>+  _updateSignalStrength: function() {
>+    let signalStrength = FMRadio.signalStrength;
>+    let strengthChanged = _currentSignalStrength != signalStrength;
>+    _currentSignalStrength = signalStrength;
>+
>+    if (strengthChanged) {
>+      ppmm.broadcastAsyncMessage("DOMFMRadio:signalStrengthChange", { });
>+    }

Here and elsewhere, you could simplify this by doing

     let signalStrength = FMRadio.signalStrength;
     if (signalStrength != _currentSignalStrength) {
       _currentSignalStrength = signalStrength;
       pmm.broadcastAsyncMessage(...);
     }

I think that more clearly expresses your intent.

>+  /**
>+   * Update and cache the current frequency.
>+   * Send frequency change message if the frequency is changed.
>+   */
>+  _updateFrequency: function(callback) {
>+    let frequency = FMRadio.frequency;
>+    let frequencyChanged = frequency != _currentFrequency;
>+    _currentFrequency = frequency;
>+
>+    if (frequencyChanged) {
>+      ppmm.broadcastAsyncMessage("DOMFMRadio:frequencyChange", { });
>+    }
>+
>+    if (typeof callback == "function") {
>+      callback();
>+    }

Passing a callback function here doesn't make much sense, since you always call
the callback synchronously!

It would make more sense to return true/false, I think.

>+  },
>+
>+  /**
>+   * Update and cache the power state of the FM radio.
>+   * Send message if the power state is changed.
>+   */
>+  _updatePowerState: function() {
>+    let enabled = FMRadio.enabled;
>+    let powerStateChanged = _isEnabled != enabled;
>+    _isEnabled = enabled;
>+
>+    if (powerStateChanged) {
>+      ppmm.broadcastAsyncMessage("DOMFMRadio:powerStateChange", { });
>+    }

Should _updatePowerState call _startSignalChecking/_stopSignalChecking, as
appropriate?  That would eliminate a class of possible bugs.

Also, it sounds like _updatePowerState should call _updateFrequency when we
transition to the |on| state?

>+  /**
>+   * Start an interval to check the signal strength.
>+   */
>+  _startSignalChecking: function() {
>+    this._stopSignalChecking();
>+
>+    var self = this;
>+    this._signalCheckingInterval = hiddenWindow.setInterval(function checkSignal() {
>+      self._updateSignalStrength();
>+    }, 2000);

Please make this interval value (2000) a pref.

>+  },
>+
>+  _stopSignalChecking: function() {
>+    if (this._signalCheckingInterval != null) {

When this._signalCheckingInterval is undefined, it != null, so we'll do this if
statement, even though you don't want us to.

A simple fix would be to initialize this._signalCheckingInterval to null in the
constructor.

>+      hiddenWindow.clearInterval(this._signalCheckingInterval);
>+      this._signalCheckingInterval = null;
>+    }
>+  },

>+  handleMozSettingsChanged: function(settings) {
>+    switch (settings.key) {
>+      case BAND_SETTING_KEY:
>+        this._updateBand(settings.value);
>+        break;
>+      case SPACETYPE_SETTING_KEY:
>+        this._updateSpaceType(settings.value);
>+        break;
>+    }
>+  },
>+
>+  receiveMessage: function(aMessage) {
>+    let msg = aMessage.json || {};
>+    msg.manager = aMessage.target;
>+
>+    let ret = 0;
>+    let self = this;
>+    switch (aMessage.name) {
>+      case "DOMFMRadio:enable": {

I'd prefer that the large bodies in this case statement be broken out into
separate methods.

>+        let frequencyInKHz = roundFrequency(parseInt(msg.data * 1000));

Surely you mean parseInt(msg.data) * 1000!

But if this is working as-is, it sounds like you don't need the parseInt at
all?

>+        // If the FM radio is already enabled or it is being enabled currently or the
>+        // given frequency is out of range, return false.
>+        if (_isEnabled || _enabling || !frequencyInKHz) {
>+          self._sendMessage("DOMFMRadio:enable:Return", false, null, msg);

If you break this out into a separate method, then you can simply return here.  We prefer

  if (...) {
    ...
    return;
  }

  ...

to 

  if (...) {
    ...
  } else {
    ...
  }

in Gecko.

>+        } else {
>+          _enabling = true;
>+
>+          FMRadio.addEventListener("enabled", function on_enabled() {
>+            debug("FM Radio is enabled!");
>+            FMRadio.removeEventListener("enabled", on_enabled);
>+            _enabling = false;
>+
>+            self._updatePowerState();
>+            self._sendMessage("DOMFMRadio:enable:Return", true, true, msg);
>+
>+            // Set frequency with the given frequency after the enable action completes.
>+            FMRadio.setFrequency(frequencyInKHz);
>+            self._updateFrequency();

Doesn't this have the race condition we discussed much earlier, where the radio
is enabled and set to some random frequency, and only later set to the correct
frequency?

>+            // Start to check signal strength after the enable action completes.
>+            self._startSignalChecking();
>+          });
>+
>+          FMRadio.enable({
>+            lowerLimit: FM_BANDS[_currentBand].lower,
>+            upperLimit: FM_BANDS[_currentBand].upper,
>+            spaceType:  _currentSpaceType   // 100KHz by default
>+          });
>+        }
>+        break;
>+      }
>+      case "DOMFMRadio:disable":
>+        // If the FM radio is already disabled, return false.
>+        if (!_isEnabled) {
>+          self._sendMessage("DOMFMRadio:disable:Return", false, null, msg);
>+        } else {
>+          FMRadio.addEventListener("disabled", function on_disabled() {
>+            debug("FM Radio is disabled!");
>+            FMRadio.removeEventListener("disabled", on_disabled);
>+
>+            self._updatePowerState();
>+            self._sendMessage("DOMFMRadio:disable:Return", true, false, msg);
>+
>+            self._stopSignalChecking();
>+            // If the FM Radio is seeking currently, no fail-to-seek or similar

s/seeking currently/currently seeking

>+            // event will be fired, execute the seek callback manually.
>+            onSeekComplete(true);
>+          });
>+          FMRadio.disable();
>+        }
>+        break;
>+      case "DOMFMRadio:setFrequency": {
>+        let frequencyInKHz = roundFrequency(parseInt(msg.data * 1000));

Same parseInt question here.

>+        // If the FM radio is disabled or the given frequency is out of ranges, return false.

s/ranges/range

Also, we're not really doing "return false" so much as "indicate that the
request failed."

>+        if (!_isEnabled || !frequencyInKHz) {
>+          self._sendMessage("DOMFMRadio:setFrequency:Return", false, null, msg);
>+        } else {
>+          FMRadio.setFrequency(frequencyInKHz);
>+          self._sendMessage("DOMFMRadio:setFrequency:Return", true, null, msg);
>+          this._updateFrequency();
>+        }
>+        break;
>+      }
>+      case "DOMFMRadio:getCurrentBand":
>+        // this message is sync
>+        return {
>+          lower: FM_BANDS[_currentBand].lower / 1000,
>+          upper: FM_BANDS[_currentBand].upper / 1000,
>+          channelWidth: _currentSpaceType / 1000
>+        };
>+      case "DOMFMRadio:getPowerState":
>+        // this message is sync
>+        return _isEnabled;
>+      case "DOMFMRadio:getFrequency":
>+        // this message is sync
>+        return _isEnabled ? _currentFrequency / 1000 : null;    // unit MHz
>+      case "DOMFMRadio:getSignalStrength":
>+        // this message is sync
>+        return _currentSignalStrength;
>+      case "DOMFMRadio:getAntennaState":
>+        // this message is sync
>+        return _antennaAvailable;
>+      case "DOMFMRadio:seekDown":
>+        // If the FM radio is disabled, do not execute the seek action.
>+        if (!_isEnabled) {
>+          self._sendMessage("DOMFMRadio:seekDown:Return", false, null, msg);
>+        } else {
>+          seekStation(RADIO_SEEK_DIRECTION_DOWN, function(frequency) {
>+            debug("Seek down completes.");

s/completes/completed

Again, seems like the callback to seekStation should accept a boolean, since it
doesn't care about the exact frequency.

>+            if (!!frequency && frequency < 0) {
>+              self._sendMessage("DOMFMRadio:seekDown:Return", false, null, msg);
>+            } else {
>+              // Make sure the FM app will get the right frequency.
>+              self._updateFrequency(function() {
>+                self._sendMessage("DOMFMRadio:seekDown:Return", true, null, msg);
>+              });

If it so happens that the seek completes and the frequency didn't change
(perhaps because the frequency was manually changed during the seek, maybe for
some other reason), then we'll never fire success or failure on the DOM
request!  That's no good.

>+            }
>+          });
>+        }
>+        break;
>+      case "DOMFMRadio:seekUp":
>+        // If the FM radio is disabled, do not execute the seek action.
>+        if(!_isEnabled) {
>+          self._sendMessage("DOMFMRadio:seekUp:Return", false, null, msg);
>+        } else {
>+          seekStation(RADIO_SEEK_DIRECTION_UP, function(frequency) {
>+            debug("Seek up completes.");
>+            if (!!frequency && frequency < 0) {
>+              self._sendMessage("DOMFMRadio:seekUp:Return", false, null, msg);
>+            } else {
>+              // Make sure the FM app will get the right frequency.
>+              self._updateFrequency(function() {
>+                self._sendMessage("DOMFMRadio:seekUp:Return", true, null, msg);
>+              });
>+            }
>+          });
>+        }
>+        break;

I'd feel better if seekUp and seekDown were implemented by one method.

>+      case "DOMFMRadio:cancelSeek":
>+        // If the FM radio is disabled, do not execute the cancel seek action.
>+        if (!_isEnabled) {
>+          self._sendMessage("DOMFMRadio:cancelSeek:Return", false, null, msg);
>+        } else {
>+          cancelSeek();
>+          // Make sure the FM app will get the right frequency.
>+          self._updateFrequency(function() {
>+             self._sendMessage("DOMFMRadio:cancelSeek:Return", true, true, msg);
>+          })

Missing semicolon after the close paren.

It's not clear to me why you only want to send the DOMFMRadio::cancelSeek
message when updateFrequency changes the frequency (the callback is not invoked
when we don't change the frequency)!

>+        }
>+        break;
>+    }
>+  }
>+};

Most of these changes are cosmetic; this patch is pretty good!  Although one of
the downsides of there being so many layers of indirection between the DOM and
the hardware is that it's hard to tell for sure whether it's correct.  :(

Comment 86

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #84)
> >+/* readonly attribute long signalStrength; */
> >+NS_IMETHODIMP FMRadio::GetSignalStrength(float *aSignalStrength)
> >+{
> >+  uint32_t signalStrength;
> >+
> >+  // The range of signal strengh is [0-100], higher than 100 means signal is strong.
> 
> Nit: This is a comma splice.  Replace the comma with a period.
> 
> But I also don't understand this comment; the signal strength is [0-100], but
> it can be bigger than 100?  Perhaps you could elaborate a bit in the comment.
> 
Steven asked Qualcomm about this, and this is what Qualcomm answered.

Comment 87

[Justin Lebar (not reading bugmail)]

> But I also don't understand this comment; the signal strength is [0-100], but
> it can be bigger than 100?  Perhaps you could elaborate a bit in the comment.
> 
> Steven asked Qualcomm about this, and this is what Qualcomm answered.

It does not make sense to say that a number is "in the range [X, Y]" and also that the number "can be bigger than Y".  Those two statements are contradictory.

Perhaps you mean something like

  The signal strength is interpreted as a percentage (0-100%).  But due to a hardware quirk, s can also be greater than 100, in which case we report the signal strength as 100%.

But actually, hal should be doing this conversion; this is absolutely not a front-end concern.  hal should give back a double in [0,1], or an int in [0,100], or otherwise make /some/ sane guarantee about the value it returns.

Comment 88

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #84)
> >+  GetFMRadioSignalStrength(&signalStrength);
> >+
> >+  *aSignalStrength = (signalStrength > 100 ? 100 : signalStrength) / 100.0;
> 
> Nit: how about
> 
>     *aSignalStrength = NS_MIN(signalStrength, 100) / 100.0;
> 
It seems that NS_MIN can not be used on the uint32_t typed value.

Comment 89

[Justin Lebar (not reading bugmail)]

> It seems that NS_MIN can not be used on the uint32_t typed value.

Um...maybe you need NS_MIN<uint32_t>(signalStrength, 100), because otherwise the compiler doesn't know whether 100 should be signed or unsigned?

http://dxr.mozilla.org/mozilla-central/xpcom/string/public/nsAlgorithm.h.html#l36

Comment 90

[Pin Zhang [:pzhang] (inactive)]

> >diff --git a/dom/fm/Makefile.in b/dom/fm/Makefile.in
> >+ifeq ($(MOZ_BUILD_APP),b2g)
> >+EXTRA_JS_MODULES =   \
> >+  FMRadioService.jsm \
> 
> Again, it's not clear to me why this guy is B2G only.
I remembered i copied the file from somewhere (contacts? maybe) ...
Should i replace
   ifeq ($(MOZ_BUILD_APP),b2g)
with
   ifeq MOZ_B2G_FM
?

Comment 91

[Justin Lebar (not reading bugmail)]

> Should i replace
>   ifeq ($(MOZ_BUILD_APP),b2g)
> with
>    ifeq MOZ_B2G_FM
> ?

I think you should test MOZ_B2G_FM, probably, yes.  Although not with |ifeq MOZ_B2G_FM|, exactly, because that's not valid Make syntax (is it?).

Comment 92

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #85)
> TBH, it's not clear to me that we're winning anything by using JS here.  We
> shouldn't rewrite this whole thing in C++ right now, but I think we should
> start on that project once we land this and shake out the first round of
> bugs.
> 
> We'll get increased correctness, which is great, but I think the result may
> also be simpler in a lot of ways.  Currently, the control flow is as follows:
> 
>   webpage (JS) -> nsIDOMFMRadio (JS) -> FMRadioService (JS) ->
>     nsIFMRadio (C++) -> HAL (C++) -> hardware
> 
> but if nsIDOMFMRadio were implemented in C++, we could simplify this to
> 
>   webpage (JS) -> nsIDOMFMRadio (C++) -> HAL (C++) -> hardware
> 
> I think that would be a significant improvement.  What do you think?
> 
Yes

> >diff --git a/dom/fm/DOMFMRadio.js b/dom/fm/DOMFMRadio.js
> 
> I think it would be better to call this file "DOMFMRadioChild.js" and to call
> FMRadioService.jsm "DOMFMRadioParent.jsm".  Then we don't have to guess about
> what code lives in which process.
> 
> >+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> I don't use emacs, but shouldn't this be "Mode: JavaScript"?
> 
> >+/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> 
> Please take out autoindent; that's an editing pref, not a formatting pref!
> 
> (Where do we get these bizarre modelines from?)
I copied these from DOMWifiManager.js, anyway, i think remove them would be better.

> >+  receiveMessage: function(aMessage) {
> >+    let msg = aMessage.json;
> >+    if (msg.mid && msg.mid != this._id) {
> >+      return;
> >+    }
> >+    let request;
> >+    switch (aMessage.name) {
> >+      case "DOMFMRadio:enable:Return:OK":
> >+        request = this.takeRequest(msg.rid);
> 
> What happens if takeRequest fails?
I think we can just return if takeRequest fails.
DOMWifiManager.js has the same problem.

> 
> >+    this._checkPermission();
> >+
> >+    if (!this._eventTypes) {
> >+      this._eventTypes = {};
> >+    }
> 
> Just set this in the constructor so you don't have to check it everywhere.
Good idea~

> 
> >+  // nsISettingsServiceCallback
> >+  handle: function(aName, aResult) {
> >+    if (aName == BAND_SETTING_KEY) {
> >+      this._updateBand(aResult);
> >+    } else if (aName == SPACETYPE_SETTING_KEY) {
> >+      this._updateSpaceType(aResult);
> >+    }
> >+  },
> >+
> >+  handleError: function(aErrorMessage) {
> 
> When does this get called?  And why is this the right reaction to an error?
> 
I think it is called when an error occurs when reading the setting, and i am trying to set those vars with default values.

> >+  _updateBand: function(band) {
> >+      switch (parseInt(band)) {
> >+        case BAND_87500_108000_kHz:
> >+        case BAND_76000_108000_kHz:
> >+        case BAND_76000_90000_kHz:
> >+          _currentBand = band;
> >+          break;
> >+      }
> 
> We don't need to inform hal that the band has changed?
There is no such a method like "hal::UpdateFMRadioSetting" to change the settings, the only way i can do is power the radio off, and then turn it on by pass the new settings, i don't think it's good.

>
> 
> >+        } else {
> >+          _enabling = true;
> >+
> >+          FMRadio.addEventListener("enabled", function on_enabled() {
> >+            debug("FM Radio is enabled!");
> >+            FMRadio.removeEventListener("enabled", on_enabled);
> >+            _enabling = false;
> >+
> >+            self._updatePowerState();
> >+            self._sendMessage("DOMFMRadio:enable:Return", true, true, msg);
> >+
> >+            // Set frequency with the given frequency after the enable action completes.
> >+            FMRadio.setFrequency(frequencyInKHz);
> >+            self._updateFrequency();
> 
> Doesn't this have the race condition we discussed much earlier, where the
> radio
> is enabled and set to some random frequency, and only later set to the
> correct
> frequency?
> 
Currently, the chip will stay quiet until we set a frequency.

> >+            // Start to check signal strength after the enable action completes.
> >+            if (!!frequency && frequency < 0) {
> >+              self._sendMessage("DOMFMRadio:seekDown:Return", false, null, msg);
> >+            } else {
> >+              // Make sure the FM app will get the right frequency.
> >+              self._updateFrequency(function() {
> >+                self._sendMessage("DOMFMRadio:seekDown:Return", true, null, msg);
> >+              });
> 
> If it so happens that the seek completes and the frequency didn't change
> (perhaps because the frequency was manually changed during the seek, maybe
> for
> some other reason), then we'll never fire success or failure on the DOM
> request!  That's no good.
> 
The callback will be called anyway, the function name or callback name might be kind of misleading, i will change it to make it more clear.

Comment 93

[Justin Lebar (not reading bugmail)]

> I think it is called when an error occurs when reading the setting, and i am trying to set those 
> vars with default values.

Okay, thanks.

> There is no such a method like "hal::UpdateFMRadioSetting" to change the settings, the only way i 
> can do is power the radio off, and then turn it on by pass the new settings, i don't think it's good.

Yeah, agreed.  We're not even attempting to support dynamic FM band changes, so whatever.

> Currently, the chip will stay quiet until we set a frequency.

Oh, fantastic.

But please re-arrange this code so that we don't send a message to the child until you've called hal and set the frequency.  Otherwise the child might be able to notice that, right after calling enable(freq), the frequency isn't actually what it expects!

> The callback will be called anyway, the function name or callback name might be kind of 
> misleading, i will change it to make it more clear.

Oh, I misread the code, sorry.

I don't see why we want a callback there at all, since the callback just gets called when the function completes.  Can we just eliminate that parameter?

Comment 94

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #93)
> 
> I don't see why we want a callback there at all, since the callback just
> gets called when the function completes.  Can we just eliminate that
> parameter?
Yes, i think your suggestion might work, i.e., return true or false which indicates the frequency is changed or not.

Comment 95

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #84)
> 
> >+    [implicit_jscontext] attribute jsval onantennastatechange;
> >+    [implicit_jscontext] attribute jsval onseekcomplete;
> >+    [implicit_jscontext] attribute jsval onenabled;
> >+    [implicit_jscontext] attribute jsval ondisabled;
> 
> These should all be nsIRunnable.  That will give you a strongly-typed
> callback
> function, which is what you're going for here, I think.  (In other words, if
> you used nsIRunnable here, you wouldn't have to change any of the JS code
> which
> calls this interface.)
> 
I don't quite understand what i should do, is there any example codes?

Comment 96

[Boris Zbarsky [:bzbarsky]]

> Um...maybe you need NS_MIN<uint32_t>(signalStrength, 100)

Or NS_MIN(signalStrength, 100u)?

Comment 97

[Justin Lebar (not reading bugmail)]

> I don't quite understand what i should do, is there any example codes?

Actually, I was wrong -- these are real events, not fake ones.  (You call onantennastatechange by firing an event on the FMRadio class, which is the right way to do it.)  You may be able to s/jsval/nsIDOMEventListener without any other changes to your code (see e.g. dom/wifi/nsIWiFi.idl), but if that doesn't work, jsval is fine.

Comment 98

[Boris Zbarsky [:bzbarsky]]

> You may be able to s/jsval/nsIDOMEventListener

That leads to different page-visible behavior.  For example, consider this:

  foo.onenabled = { dispatchEvent: function foo() {} };

This would work if onenabled is an nsIDOMEventListener, but not if it's a jsval.  Other on* attributes are jsval, fwiw.

Comment 99

[Justin Lebar (not reading bugmail)]

> That leads to different page-visible behavior.

fwiw this is only on nsIFMRadio (not to be confused with nsIFMDOMRadio), which isn't exposed to the web.  But it sounds like jsval for the event listeners is fine.  :)

We have some other nsIDOMEventListeners in web-exposed interfaces; I'll file bugs on those.

Comment 100

[Justin Lebar (not reading bugmail)]

> not to be confused with nsIFMDOMRadio

er, nsIDOMFMRadio.

I filed bug 790263 on the other onfoo properties.

Comment 101

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #85)
> >+
> >+XPCOMUtils.defineLazyServiceGetter(this, "gSettingsService", 
> >+                            "@mozilla.org/settingsService;1",
> >+                            "nsISettingsService");
> 
> Please indent this as
> 
> 
>   XPCOMUtils.defineLazyServiceGetter(this, "gSettingsService", 
>                                      "@mozilla.org/settingsService;1",
>                                      "nsISettingsService");
> 
What's the difference?

Comment 102

[Justin Lebar (not reading bugmail)]

In the second one, the second and third lines start in the same column as the t in "this".  This is because the "@mozilla.org" argument is contained within the same parenthesis level as "this".

We break this rule in plenty of places, but in general, we try to indent this way.  It's not always possible, and when it's not, we prefer to indent the second line one indent (that is, two or four spaces, depending on the file).  If you were to do that here, you'd do

>   XPCOMUtils.defineLazyServiceGetter(this, "gSettingsService", 
>       "@mozilla.org/settingsService;1",  "nsISettingsService");

although I don't think that would be the right indentation style here.  On the other hand, if "@mozilla.org/settingsService;1" were instead some very long string, we might do

>   XPCOMUtils.defineLazyServiceGetter(this, "gSettingsService", 
>       "this-string-would-be-too-long-if-we-put-it-under-the-t-in-this",
>       "nsISettingsService");

Or we might just store the long string in a variable.  But in any case, we wouldn't use the style from the first example in comment 101.

Does that make sense?

Comment 103

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #102)
> In the second one, the second and third lines start in the same column as
> the t in "this".  This is because the "@mozilla.org" argument is contained
> within the same parenthesis level as "this".
> 
> We break this rule in plenty of places, but in general, we try to indent
> this way.  It's not always possible, and when it's not, we prefer to indent
> the second line one indent (that is, two or four spaces, depending on the
> file).  If you were to do that here, you'd do
> 
> >   XPCOMUtils.defineLazyServiceGetter(this, "gSettingsService", 
> >       "@mozilla.org/settingsService;1",  "nsISettingsService");
> 
> although I don't think that would be the right indentation style here.  On
> the other hand, if "@mozilla.org/settingsService;1" were instead some very
> long string, we might do
> 
> >   XPCOMUtils.defineLazyServiceGetter(this, "gSettingsService", 
> >       "this-string-would-be-too-long-if-we-put-it-under-the-t-in-this",
> >       "nsISettingsService");
> 
> Or we might just store the long string in a variable.  But in any case, we
> wouldn't use the style from the first example in comment 101.
> 
> Does that make sense?
Yes
BTW, the second and third lines didn't align with 'this' in your comments, that's why i am confused, :-)

Comment 104

[Justin Lebar (not reading bugmail)]

Now I'm confused.  In comment 85, I wrote

  XPCOMUtils.defineLazyServiceGetter(this, "gSettingsService", 
                                     "@mozilla.org/settingsService;1",
                                     "nsISettingsService");

How do you think it should be?

Comment 105

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please keep in mind that if you are reading bugmail through a mail reader, spaces might not come out the right size. Going with a web browser to this bug and looking at the comments here should show the correct indentation though.

Comment 106

[Pin Zhang [:pzhang] (inactive)]

(In reply to Jonas Sicking (:sicking) from comment #105)
> Please keep in mind that if you are reading bugmail through a mail reader,
> spaces might not come out the right size. Going with a web browser to this
> bug and looking at the comments here should show the correct indentation
> though.
I am reading the comments in the browser, so it seems that the "monospace" font is not rendered right in my Firefox ... sorry for that

Comment 107

[Pin Zhang [:pzhang] (inactive)]

Hi Justin, i checked the patches of the bug749053, there is one function to query the FM radio settings by country code. So it seems that the right way to enable the FM radio should be:
  - Get country code
  - Query the settings info from hal::GetFMBandSettings by the country code
  - Call the hal::EnableFMRadio by passing the settings object
I think we can introduce a setting, say "fmRadio.regionCode", to define the country code for the FM radio (maybe there is already one setting pref for general use), but the country code is defined as an Enum type in C, which means, we should build a setting-to-enum map, so if the Enum type is changed, the whole map will be rewritted, i think it's hard to be maintained. 
What do you think?

Comment 108

[Pin Zhang [:pzhang] (inactive)]

Attachment: all-in-one patch, v4

Comment 109

[Justin Lebar (not reading bugmail)]

Can we get a build peer to look at the build changes again?  (You got r- last time.)  I'm not comfortable reviewing that stuff.  (You'll probably get a faster review from khuey if you split the relevant changes out into a separate patch.)

Because I asked you to rename these files, interdiff is now pretty useless comparing v3 --> v4.  :(  I'll fix it up manually and post it here.

Comment 110

[Justin Lebar (not reading bugmail)]

Attachment: Interdiff v3 --> v4

This interdiff was created by manually renaming DOMFMRadio.js --> DOMFMRadioChild.js and FMRadioManager.jsm --> DOMFMRadioParent.jsm in patch v3.

This is missing some uninteresting changes in nsLayoutModule, but is hopefully complete aside from that.  (I've had problems with interdiff mysteriously missing hunks lately, so fingers crossed.)

Comment 111

[Justin Lebar (not reading bugmail)]

Comment on attachment 660466 [details] [diff] [review]
Interdiff v3 --> v4

These are comments on the C++ bits.  I'm working on the JS bits.

Please let me know in what form you'd prefer to get these English nits.  I'm
happy to rewrite sentences, to provide explanations on points of grammar, or
both, whatever you feel is most helpful.  My goal is to help you, not to be
annoying or unhelpfully pedantic, so if I'm not accomplishing my goal, please
let me know.

>diff -u b/dom/fm/nsIDOMFMRadio.idl b/dom/fm/nsIDOMFMRadio.idl
>--- b/dom/fm/nsIDOMFMRadio.idl
>+++ b/dom/fm/nsIDOMFMRadio.idl
>@@ -109,7 +109,7 @@
>     /**
>-     * This is a temporary hack, will be removed by inheriting
>+     * These are temporary hacks, will be removed by inheriting
>      * nsIJSDOMEventTarget, see bug 731746
>      */
>     [optional_argc] void addEventListener(in DOMString type,

I'm sorry to nit, but this still has a few problems.

We need a preposition ("from", in this case) between "inheriting" and
"nsIJSDOMEventTarget".

It's not clear what "these" are.  It would be helpful if you'd explicitly list
the relevant methods.

You also need some kind of connector after the first comma ("hacks, /which/
will be removed", or "hacks; these will be removed").

The second comma creates a comma splice; it should instead begin a new
sentence.

I'd also like a period at the end of the sentence.

>diff -u b/dom/fm/nsIFMRadio.idl b/dom/fm/nsIFMRadio.idl
>+[scriptable, uuid(c142387a-5488-454b-8b5a-91f0dbee833b)]
>+interface nsIFMRadioSettings : nsISupports
>+{
>+    /* Upper limit in KHz */
>+    attribute long upperLimit;
>+    /* Lower limit in KHz */
>+    attribute long lowerLimit;
>+    /* Space type in KHz */
>+    attribute long spaceType;

We decided to call this the channelInterval?

If you want to be fancy, you could put "[infallible]" in front of "attribute".
That would let you do

  nsCOMPtr<nsIFMRadioSettings> settings;
  int32_t lowerLimit = settings->GetLowerLimit();

>+ * This is an interface to expose the FM radio hardware related functions,
>+ * it's kind of the FM radio hardware wrapper interface.

s/,/;/.  Otherwise this is great.

>+ * Because the WebFM API (navigator.mozFMRadio) is implemented in JS component,
>+ * we need such an interface to be accessed in JS component and JS module, do
>+ * confuse it with the WebFM DOM interface(nsIDOMFMRadio).

* s/in JS component/as a JS component/

* "we need such an interface to be accessed in JS component and JS module"

  I'm not sure what you mean, but maybe it should be "it can't access our C++
  hardware interface directly; instead it must go through this interface."

* ", do not confuse it with the WEbFM DOM interface(nsIDOMFMRadio)."

  This is a comma splice, but it's corrected if you take the suggestion above.

  It's not clear what "it" refers to here.  It would be better to say "Do not
  confuse /this interface/".

  Space before the open parenthesis.

>+ * If the WebFM API will be re-written in c++ some day, the interface will be useless.

s/will be/is

(I have no idea what rule explains why we use the present tense for the first
part of this conditional and the future tense for the second part.)

>+    /**
>+     * Current frequency in KHz
>+     */
>     readonly attribute long frequency;

I still don't get why we expose the frequency in KHz here and in MHz in the DOM
interface, but it's not a big deal.

>+    /**
>+     * Indicates if the antenna is plugged and available.
>+     */
>     readonly attribute boolean isAntennaAvailable;

s/plugged/plugged in/

(You can think of "plugged in" as a transitive verb.  Compare "My tub is no
longer leaking; the hole is plugged," and "I plugged in my antenna.")

>+    /**
>+     * Enable the FM radio hardware by giving settings.
>+     */

s/by giving settings/with the given settings/

>     [implicit_jscontext]
>-    void enable(in jsval settings);
>+    void enable(in nsIFMRadioSettings settings);

Do you still need the jscontext?

>+    /**
>+     * Seek the available channel up or down.

Seek to the next available channel (up or down).

>+    /**
>+     * Get current settings.
>+     */

Get the current settings.

>     [implicit_jscontext]
>-    jsval getSettings();
>+    nsIFMRadioSettings getSettings();

Do you still need the jscontext?

>+    /**
>+     * Set frequency in KHz
>+     */
>     void setFrequency(in long frequency);

Set the frequency in KHz

>+    /**
>+     * Fired when the antenna state changed.
>+     */

"state /is/ changed", or "state changes".

Clearer would be "Fired when the antenna goes from plugged in to unplugged (or
vice versa)."

>     [implicit_jscontext] attribute jsval onantennastatechange;
> 
>+    /**
>+     * Fired when the seek action completes.
>+     */

s/the/a

>+++ b/dom/fm/nsFMRadioSettings.h
>@@ -0,0 +1,34 @@
>+#include "nsIFMRadio.h"
>+
>+namespace mozilla {
>+namespace dom {
>+namespace fm {
>+
>+class nsFMRadioSettings : public nsIFMRadioSettings

The "ns" prefix is what we used before we had namespaces (and what we use now
in places where we don't have namespaces, like interfaces).

I'd be OK if this class lived in the top-level namespace.  Otherwise, if you
rename it to FMRadioSettings and leave it in mozilla::dom::fm that works too.

>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIFMRADIOSETTINGS
>+
>+  nsFMRadioSettings(int32_t aUpperLimit, int32_t aLowerLimit, int32_t aSpaceType);

Please rename "spaceType" here and elsewhere.

>diff -u b/dom/fm/FMRadio.h b/dom/fm/FMRadio.h

>+  hal::SwitchState mSwitchState;
>+  nsAutoPtr<hal::SwitchObserver> mObserver;

I think mHeadphoneState and mHeadphoneObserver (or mHeadphoneSwitchState and
mHeadphoneSwitchObserver) would be better names here.  There are lots of
switches and observers you might otherwise have.

>diff -u b/dom/fm/FMRadio.cpp b/dom/fm/FMRadio.cpp
>@@ -12,16 +12,18 @@
> #include "FMRadio.h"
> #include "nsDOMEvent.h"
> #include "nsDOMClassInfo.h"
>+#include "nsFMRadioSettings.h"
>+#include "nsAutoPtr.h"

When you remove the nsAutoPtr below, you can remove this include, too.

> #define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "FMRadio" , ## args)

Looks like you missed my earlier comment about this hunk:

> As far as I can tell, these log statements are the only android-only feature of
> this file.  Please guard them with an ifdef.  (You can just define LOG to do
> nothing if we're not on Android.)
> 
> The right solution would be to use prlog, but that's kind of a pain to use in
> b2g.  We have a pretty bad logging story. :(

>+FMRadio::FMRadio()
>+  : mSwitchState(SWITCH_STATE_OFF)
>+  , mHasInternalAntenna(false)
> {
>   LOG("FMRadio is initialized.");
> 
>-  if (!NS_SUCCEEDED(Preferences::GetBool(DOM_FM_ANTENNA_INTERNAL_PREF,
>-            &mHasInternalAntenna)) || !mHasInternalAntenna)
>-  {
>-    RegisterSwitchObserver(SWITCH_HEADPHONES, this);
>-    mStatus = GetCurrentSwitchState(SWITCH_HEADPHONES);
>-  }
>-  else
>-  {
>+  mHasInternalAntenna = Preferences::GetBool(DOM_FM_ANTENNA_INTERNAL_PREF,
>+                             /* default = */ false);

Please indent "/* default = */ false" so it's the "D" in
DOM_FM_ANTENNA_INTERNAL_PREF.

> /* [implicit_jscontext] void enableRadio (in jsval settings); */
>-NS_IMETHODIMP FMRadio::Enable(const JS::Value & settings, JSContext* cx)
>+NS_IMETHODIMP FMRadio::Enable(nsIFMRadioSettings *settings, JSContext* cx)
> {
>-  if (!settings.isObject()) {
>-    return NS_ERROR_ILLEGAL_VALUE;
>-  }
>-
>-  JSObject *obj = &settings.toObject();
>+  hal::FMRadioSettings info;
>+  int32_t upperLimit, lowerLimit, spaceType;
> 
>-  int32_t upperLimit = GetJsIntValue(cx, obj, "upperLimit");
>-  int32_t lowerLimit = GetJsIntValue(cx, obj, "lowerLimit");
>-  int32_t spaceType  = GetJsIntValue(cx, obj, "spaceType");
>+  settings->GetUpperLimit(&upperLimit);
>+  settings->GetLowerLimit(&lowerLimit);
>+  settings->GetSpaceType(&spaceType);

You could change this to

  info.upperLimit() = settings->GetUpperLimit();

if you made this attribute [infallible].

>-/* [implicit_jscontext] jsval getSettings (); */
>-NS_IMETHODIMP FMRadio::GetSettings(JSContext* cx, JS::Value *_retval)
>+/* [implicit_jscontext] nsIFMRadioSettings getSettings (); */
>+NS_IMETHODIMP FMRadio::GetSettings(JSContext* cx, nsIFMRadioSettings * *_retval)
> {
>-  FMRadioSettings settings;
>+  hal::FMRadioSettings settings;
>   GetFMRadioSettings(&settings);
> 
>-  JSObject* jsVal = JS_NewObject(cx, nullptr, nullptr, nullptr);
>-
>-  jsval upperLimit = JS_NumberValue(settings.upperLimit());
>-  JS_SetProperty(cx, jsVal, "upperLimit", &upperLimit);
>-
>-  jsval lowerLimit = JS_NumberValue(settings.lowerLimit());
>-  JS_SetProperty(cx, jsVal, "lowerLimit", &lowerLimit);
>-
>-  jsval spaceType = JS_NumberValue((uint32_t)settings.spaceType());
>-  JS_SetProperty(cx, jsVal, "spaceType", &spaceType);
>-
>-  *_retval = OBJECT_TO_JSVAL(jsVal);
>+  nsAutoPtr<nsIFMRadioSettings> radioSettings(new nsFMRadioSettings(
>+                                                    settings.upperLimit(),
>+                                                    settings.lowerLimit(),
>+                                                    settings.spaceType()));
>+  *_retval = radioSettings.forget();

nsRefPtr, not nsAutoPtr.

This is actually quite important, and I'm surprised it's not causing crashes.
nsAutoPtr does not call AddRef() on its member.  So that means that *_retval
has a refcount of 0, while the caller of this function expects it to have a
refcount of 1.  Which means we're likely to run into a use-after-free crash
here.

Comment 112

[Justin Lebar (not reading bugmail)]

Comment on attachment 660361 [details] [diff] [review]
all-in-one patch, v4

>diff -u b/dom/fm/DOMFMRadioChild.js b/dom/fm/DOMFMRadioChild.js
>         try {
>           if (typeof listener == "function") {
>             listener.call(this, evt);
>-          } else if (listener && listener.handleEvent && typeof listener.handleEvent == "function") {
>+          } else if (listener && listener.handleEvent
>+                     && typeof listener.handleEvent == "function") {

Nit: Put the && on the line above.  (As a rule, we don't start a line with a
binary operator like && or +.)

>diff -u b/dom/fm/DOMFMRadioParent.jsm b/dom/fm/DOMFMRadioParent.jsm
>--- b/dom/fm/DOMFMRadioParent.jsm
>+++ b/dom/fm/DOMFMRadioParent.jsm

>@@ -308,20 +264,90 @@

>+  _onSeekComplete: function(forceStop) {
>+    if (this._seeking) {
>+      this._seeking = false;
>+
>+      if (this._seekingCallback) {
>+        this._seekingCallback(!forceStop);

It's pretty confusing to flip the boolean around like this.

I don't care which way the boolean goes, but can we make it consistent?

>+  _roundFrequency: function(frequencyInKHz) {
>+    if (frequencyInKHz < FM_BANDS[this._currentBand].lower ||
>+        frequencyInKHz > FM_BANDS[this._currentBand].upper) {
>+      return null;
>+    }
>+
>+    let partToBeRounded = frequencyInKHz - FM_BANDS[this._currentBand].lower;
>+    let roundedPart = Math.round(partToBeRounded / this._currentInterval) *
>+                        this._currentInterval;
>+    frequencyInKHz = FM_BANDS[this._currentBand].lower + roundedPart;
>+    return frequencyInKHz;

Just |return FM_BANDS[...] + roundedPart;|

>@@ -354,137 +380,144 @@
>     let ret = 0;
>     let self = this;
>     switch (aMessage.name) {

Like I said in the earlier review, I think this would be better if you moved
the large |case| bodies into their own functions, particularly because it would
let you do early returns instead of these long |else| clauses.  The small
|case| bodies are fine how they are.

>-      case "DOMFMRadioChild:enable": {
>-        let frequencyInKHz = roundFrequency(parseInt(msg.data * 1000));
>-        // If the FM radio is already enabled or it is being enabled currently or the
>-        // given frequency is out of range, return false.
>-        if (_isEnabled || _enabling || !frequencyInKHz) {
>-          self._sendMessage("DOMFMRadioChild:enable:Return", false, null, msg);
>+      case "DOMFMRadio:enable": {
>+        let frequencyInKHz = self._roundFrequency(msg.data * 1000);
>+
>+        // If the FM radio is already enabled or it is being enabled currently

s/being enabled currently/currently being enabled/

>+            // Update the current frequency without sending 'frequencyChange'
>+            // msg, to make sure the FM app will get the right frequecy when the
>+            // 'enabled' event is fired.

s/frequecy/frequency/

>       case "DOMFMRadio:seekDown":
>       case "DOMFMRadio:seekUp":

When you split out this code into a method, perhaps you can combine seekUp and
seekDown, if that makes sense.

r=me on the JS bits.  I'd like to take another look at the C++ parts.  And we
still need that review from a build peer.

Comment 113

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #111)
> Comment on attachment 660466 [details] [diff] [review]
> Interdiff v3 --> v4
> 
> These are comments on the C++ bits.  I'm working on the JS bits.
> 
> Please let me know in what form you'd prefer to get these English nits.  I'm
> happy to rewrite sentences, to provide explanations on points of grammar, or
> both, whatever you feel is most helpful.  My goal is to help you, not to be
> annoying or unhelpfully pedantic, so if I'm not accomplishing my goal, please
> let me know.
Thanks a lot for correcting my english written sentences. If you feel
re-writting the whole sensence is a lot easier for you, please do it.
Either way works for me. I am really appreciate.

> 
> 
> >diff -u b/dom/fm/nsIFMRadio.idl b/dom/fm/nsIFMRadio.idl
> >+[scriptable, uuid(c142387a-5488-454b-8b5a-91f0dbee833b)]
> >+interface nsIFMRadioSettings : nsISupports
> >+{
> >+    /* Upper limit in KHz */
> >+    attribute long upperLimit;
> >+    /* Lower limit in KHz */
> >+    attribute long lowerLimit;
> >+    /* Space type in KHz */
> >+    attribute long spaceType;
> 
> We decided to call this the channelInterval?
I think the interface is kind of a hal::FMRadioSettings "copy", i am trying to make them consistent.
BTW, in the WebFM idl, we call this "channelWidth", should we change it too? or call this "channelWidth" in anywhere?

> 
> If you want to be fancy, you could put "[infallible]" in front of
> "attribute".
> That would let you do
> 
>   nsCOMPtr<nsIFMRadioSettings> settings;
>   int32_t lowerLimit = settings->GetLowerLimit();
> 
Cool~

> >+    /**
> >+     * Current frequency in KHz
> >+     */
> >     readonly attribute long frequency;
> 
> I still don't get why we expose the frequency in KHz here and in MHz in the
> DOM
> interface, but it's not a big deal.
> 
I am trying to make it consistent with the hal part.

> 
> >+++ b/dom/fm/nsFMRadioSettings.h
> >@@ -0,0 +1,34 @@
> >+#include "nsIFMRadio.h"
> >+
> >+namespace mozilla {
> >+namespace dom {
> >+namespace fm {
> >+
> >+class nsFMRadioSettings : public nsIFMRadioSettings
> 
> The "ns" prefix is what we used before we had namespaces (and what we use now
> in places where we don't have namespaces, like interfaces).
> 
> I'd be OK if this class lived in the top-level namespace.  Otherwise, if you
> rename it to FMRadioSettings and leave it in mozilla::dom::fm that works too.
> 
Because there is hal::FMRadioSettings, it is easy to be confused, I prefer keep the "ns" prefix, and move it to the top-level namespace.

Comment 114

[Justin Lebar (not reading bugmail)]

> > We decided to call this the channelInterval?
> I think the interface is kind of a hal::FMRadioSettings "copy", i am trying to make them consistent.

Ah, I understand.  There are a lot of moving parts here!

I'm happy to land this how it is and then change everything all at once.

> BTW, in the WebFM idl, we call this "channelWidth", should we change it too? or call this 
> "channelWidth" in anywhere?

Oh, right.  :)  Either channelWidth or channelInterval is fine with me, but yeah, it would be good to be consistent.

Comment 115

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #114)
> > > We decided to call this the channelInterval?
> > I think the interface is kind of a hal::FMRadioSettings "copy", i am trying to make them consistent.
> 
> Ah, I understand.  There are a lot of moving parts here!
> 
> I'm happy to land this how it is and then change everything all at once.
> 
> > BTW, in the WebFM idl, we call this "channelWidth", should we change it too? or call this 
> > "channelWidth" in anywhere?
> 
> Oh, right.  :)  Either channelWidth or channelInterval is fine with me, but
> yeah, it would be good to be consistent.
Then i will rename them all to "channelWidth".
And what do you think about the comment #107 ?

Comment 116

[Justin Lebar (not reading bugmail)]

> I think we can introduce a setting, say "fmRadio.regionCode", to define the country code for the FM 
> radio (maybe there is already one setting pref for general use),

Sounds good so far...

> but the country code is defined as 
> an Enum type in C, which means, we should build a setting-to-enum map, so if the Enum type is 
> changed, the whole map will be rewritted, i think it's hard to be maintained. 

The problem is that if we ever changed the enum, existing settings would be broken?  Sounds like the solution is a big fat warning on the enum definition...

Comment 117

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #116)
> > I think we can introduce a setting, say "fmRadio.regionCode", to define the country code for the FM 
> > radio (maybe there is already one setting pref for general use),
> 
> Sounds good so far...
> 
> > but the country code is defined as 
> > an Enum type in C, which means, we should build a setting-to-enum map, so if the Enum type is 
> > changed, the whole map will be rewritted, i think it's hard to be maintained. 
> 
> The problem is that if we ever changed the enum, existing settings would be
> broken?  Sounds like the solution is a big fat warning on the enum
> definition...
Here is the enum definition for the country:
> +enum FMRadioCountry {
> +  FM_RADIO_COUNTRY_UNKNOWN = -1,
> +  FM_RADIO_COUNTRY_US,  //USA
> +  FM_RADIO_COUNTRY_EU,
> +  FM_RADIO_COUNTRY_JP_STANDARD,
> +  FM_RADIO_COUNTRY_JP_WIDE,
> +  FM_RADIO_COUNTRY_DE,  //Germany
So, we should map the "fmRadio.regionCode" setting like this:
  us -> 0, eu -> 1, de -> 4
If we change the enum, say insert FM_RADIO_COUNTRY_CN before FM_RADIO_COUNTRY_EU, the map will have to be changed like this:
  us -> 0, eu -> 2, de -> 5
unless we promise all the new values will be appended and no value will be removed.
If i am wrong, please correct me.

Comment 118

[Pin Zhang [:pzhang] (inactive)]

>diff -u b/dom/fm/nsIFMRadio.idl b/dom/fm/nsIFMRadio.idl
>>+[scriptable, uuid(c142387a-5488-454b-8b5a-91f0dbee833b)]
>>+interface nsIFMRadioSettings : nsISupports
>>+{
>>+    /* Upper limit in KHz */
>>+    attribute long upperLimit;
>>+    /* Lower limit in KHz */
>>+    attribute long lowerLimit;
>>+    /* Space type in KHz */
>>+    attribute long spaceType;
>
> We decided to call this the channelInterval?
> If you want to be fancy, you could put "[infallible]" in front of "attribute".
> That would let you do
>
>  nsCOMPtr<nsIFMRadioSettings> settings;
>  int32_t lowerLimit = settings->GetLowerLimit();

[infallible] attributes are only allowed on [builtinclass] interfaces, however, if i add [builtinclass] to nsIFMRadio, then the function FMRadio::enable(nsIFMRadioSettings settings) is not callable from JS, here is the error:
  [JavaScript Error: "NS_ERROR_FAILURE: Failure arg 0 [nsIFMRadio.enable]" {file: "resource://gre/modules/DOMFMRadioParent.jsm" line: 423}]

Comment 119

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

[builtinclass] can't be used on interfaces implemented in JS.

Comment 120

[Pin Zhang [:pzhang] (inactive)]

Attachment: part A - MOZ_B2G_FM option

Comment 121

[Pin Zhang [:pzhang] (inactive)]

Attachment: part B - nsIDOMFMRadio IDL, v1.1

Comment 122

[Pin Zhang [:pzhang] (inactive)]

Attachment: part C - mozFMRadio v5

Comment 123

[Justin Lebar (not reading bugmail)]

> +      ppmm.broadcastAsyncMessage("DOMFMRadio:signalStrengthChange", { });
>
> +    /* Fired when the signal strength changes. */
> +    attribute nsIDOMEventListener onsignalstrengthchange;

It worries me that every time the signal strength changes, we broadcast to all child processes.

Maybe if the signal strength were bucketed very coarsely (e.g. "no signal", "moderate signal", "good signal"), this would be OK.  But if we're reporting a fine-grained value, I'm afraid this is going to result in a /lot/ of useless messages, every two seconds.

Maybe we should hold off addressing this until we rewrite this whole thing in C++ -- in C++, this is easy to address, because it's just a matter of registering the only the observers you need with hal (I think?).

Comment 124

[Justin Lebar (not reading bugmail)]

Comment on attachment 660727 [details] [diff] [review]
part B - nsIDOMFMRadio IDL, v1.1

The only change here is the comment over addEventListener?  r=me, in that case.

> +     * These functions related to EventTarget are temporary hacks, i.e.:
> +     *   - addEventListener
> +     *   - removeEventListener
> +     *   - handleEvent

(I'd s/, i.e.:/:/ -- "i.e." doesn't make much sense here -- but that's a small point.  Most native speakers regularly use "i.e." less sensibly than this.  :)

Comment 125

[Justin Lebar (not reading bugmail)]

Comment on attachment 660728 [details] [diff] [review]
part C - mozFMRadio v5

>   /**
>+
>    * Seek the next channel with given direction.
>    * Only one seek action is allowed at once.
>    */

I don't think you meant to add this newline.

>+  _enableFMRadio: function(msg) {
>+    let frequencyInKHz = this._roundFrequency(msg.data * 1000);
>+
>+    // If the FM radio is already enabled or it is currently being enabled
>+    // or the given frequency is out of range, return false.
>+    if (this._isEnabled || this._enabling || !frequencyInKHz) {
>+      self._sendMessage("DOMFMRadio:enable:Return", false, null, msg);
>+    } else {

Part of the benefit of pulling these chunks into their own functions was that
"it would let you do early returns instead of these long |else| clauses."  That
is, you could do here

  if (...) { 
    sendMessage(...);
    return;
  }

  // The rest of the method.

It's not particularly important, but this is a common point of style in Gecko,
so you might as well familiarize yourself with it.  :)

>diff -U8 b/dom/fm/FMRadio.cpp b/dom/fm/FMRadio.cpp
>+#undef LOG
>+#if defined(MOZ_WIDGET_GONK)
>+#include <android/log.h>
> #define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "FMRadio" , ## args)
>+#else
>+#define BTDEBUG true
>+#define LOG(args...) if (BTDEBUG) printf(args);
>+#endif

Um...I have no idea where this BTDEBUG thing came from, but it's no good.
You're just defining LOG to printf for non-gonk systems.

A simple fix that I would be OK with is

#else
//#define LOG(args...) printf(args);
#define LOG(args...)
#endif

A better fix would be to use prlog.  See e.g. dom/ipc/ProcessPriorityManager.cpp.

>-  /* The range of signal strengh is [0-100], higher than 100 means the signal strengh is strong. */
>+  /**
>+   * The range of signal strengh is [0-100], higher than 100 means the signal 
>+   * strengh is strong. 
>+   */
>   GetFMRadioSignalStrength(&signalStrength);
>
>   *aSignalStrength = NS_MIN<uint32_t>(signalStrength, 100) / 100.0;
>
>   return NS_OK;
> }

See bug 749053 comment 111 -- apparently we /are/ getting dB's from the radio,
not a magic value between 1-100.  :)

Maybe the solution will be for hal to give us a value in dB, and then we'll
translate it into a value 0-5 here.  I'm not quite sure yet, but stay tuned, I
guess.

>diff -U8 b/dom/fm/nsIFMRadio.idl b/dom/fm/nsIFMRadio.idl
>- * If the WebFM API will be re-written in c++ some day, the interface will be useless.
>+ * If the WebFM API is re-written in c++ some day, the interface is useless.

Oh no, I wasn't clear at all!  It should be "If the WebFM API /is/ re-written
in C++ some day, this interface /will be/ useless."

(More correctly, you'd probably use the subjunctive here: "If the interface
were to be rewritten, this would be useless."  But most people ignore that...)

Sorry.  :(

Comment 126

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #124)
> Comment on attachment 660727 [details] [diff] [review]
> part B - nsIDOMFMRadio IDL, v1.1
> 
> The only change here is the comment over addEventListener?  r=me, in that
> case.
> 
Yes

Comment 127

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #125)
> Comment on attachment 660728 [details] [diff] [review]
> part C - mozFMRadio v5
> 
> >   /**
> >+
> >    * Seek the next channel with given direction.
> >    * Only one seek action is allowed at once.
> >    */
> 
> I don't think you meant to add this newline.
> 
> >+  _enableFMRadio: function(msg) {
> >+    let frequencyInKHz = this._roundFrequency(msg.data * 1000);
> >+
> >+    // If the FM radio is already enabled or it is currently being enabled
> >+    // or the given frequency is out of range, return false.
> >+    if (this._isEnabled || this._enabling || !frequencyInKHz) {
> >+      self._sendMessage("DOMFMRadio:enable:Return", false, null, msg);
> >+    } else {
> 
> Part of the benefit of pulling these chunks into their own functions was that
> "it would let you do early returns instead of these long |else| clauses." 
> That
> is, you could do here
> 
>   if (...) { 
>     sendMessage(...);
>     return;
>   }
> 
>   // The rest of the method.
> 
> It's not particularly important, but this is a common point of style in
> Gecko,
> so you might as well familiarize yourself with it.  :)
> 
> >diff -U8 b/dom/fm/FMRadio.cpp b/dom/fm/FMRadio.cpp
> >+#undef LOG
> >+#if defined(MOZ_WIDGET_GONK)
> >+#include <android/log.h>
> > #define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "FMRadio" , ## args)
> >+#else
> >+#define BTDEBUG true
> >+#define LOG(args...) if (BTDEBUG) printf(args);
> >+#endif
> 
> Um...I have no idea where this BTDEBUG thing came from, but it's no good.
> You're just defining LOG to printf for non-gonk systems.
> 
I copied them from dom/bluetooth/linux/BluetoothDBusService.cpp and dom/bluetooth/linux/BluetoothDBusUtils.cpp.

> A simple fix that I would be OK with is
> 
> #else
> //#define LOG(args...) printf(args);
> #define LOG(args...)
> #endif
> 
> A better fix would be to use prlog.  See e.g.
> dom/ipc/ProcessPriorityManager.cpp.
> 

> >-  /* The range of signal strengh is [0-100], higher than 100 means the signal strengh is strong. */
> >+  /**
> >+   * The range of signal strengh is [0-100], higher than 100 means the signal 
> >+   * strengh is strong. 
> >+   */
> >   GetFMRadioSignalStrength(&signalStrength);
> >
> >   *aSignalStrength = NS_MIN<uint32_t>(signalStrength, 100) / 100.0;
> >
> >   return NS_OK;
> > }
> 
> See bug 749053 comment 111 -- apparently we /are/ getting dB's from the
> radio,
> not a magic value between 1-100.  :)
> 
> Maybe the solution will be for hal to give us a value in dB, and then we'll
> translate it into a value 0-5 here.  I'm not quite sure yet, but stay tuned,
> I
> guess.
> 
Then we should change the type of signalStrength to "long", like this:
    /**
     * The signal strength.
     * The value returned is between 0 (no signal) and 5 (full signal), inclusive.
     */
    readonly attribute long signalStrength;
?

Comment 128

[Justin Lebar (not reading bugmail)]

I think the choice is between doing a 0-5 (or whatever) conversion or reporting the signal strength in dB's and letting the website figure out how to show that to the user.

If we did 0-5, it's not clear how to correctly convert dBm into that scale.  At best, I guess we'd listen to the radio under different conditions and qualitatively rate how good it sounds.  Then we could correlate these qualitative measurements with the signal strength in dBm.

If otoh we spit out dBm, then the FM radio app author is going to have to do this experiment him or herself, if she wants to show a meaningful signal strength indicator to the user.

OTOH, the app may be interested in the real signal strength, instead of our arbitrary number.  And it will be hard to spec our arbitrary scale in a meaningful way.

My gut feeling is that we should just report raw dBm, since that's a meaningful number.  We can always add a 0-5 (or whatever) scale if necessary.  And then we should make it a priority after landing this bug to get rid of that broadcast I mentioned earlier, since as written, it looks like we're going to broadcast the FM radio signal strength to all processes every 2 seconds (since I imagine the dBm value will be different every time we read it, if only by a small amount).

But I'm not wed to that idea.  What do you think?

Comment 129

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If we are talking about signal strength then dBm sounds great to me. That seems much more standard than any 0-5 scale we can invent.

Comment 130

[Justin Lebar (not reading bugmail)]

Um, Jonas, can you unset the security bit?

Comment 131

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #128)
> I think the choice is between doing a 0-5 (or whatever) conversion or
> reporting the signal strength in dB's and letting the website figure out how
> to show that to the user.
> 
> If we did 0-5, it's not clear how to correctly convert dBm into that scale. 
> At best, I guess we'd listen to the radio under different conditions and
> qualitatively rate how good it sounds.  Then we could correlate these
> qualitative measurements with the signal strength in dBm.
> 
> If otoh we spit out dBm, then the FM radio app author is going to have to do
> this experiment him or herself, if she wants to show a meaningful signal
> strength indicator to the user.
> 
> OTOH, the app may be interested in the real signal strength, instead of our
> arbitrary number.  And it will be hard to spec our arbitrary scale in a
> meaningful way.
> 
> My gut feeling is that we should just report raw dBm, since that's a
> meaningful number.  We can always add a 0-5 (or whatever) scale if
> necessary.  And then we should make it a priority after landing this bug to
> get rid of that broadcast I mentioned earlier, since as written, it looks
> like we're going to broadcast the FM radio signal strength to all processes
> every 2 seconds (since I imagine the dBm value will be different every time
> we read it, if only by a small amount).
> 
> But I'm not wed to that idea.  What do you think?
If we expose the raw value in dBm, i really doubt that the different FM chips won't have the same ranges, i check the spec of Si4709: 
  http://www.silabs.com/Support%20Documents/TechnicalDocs/Not-Rec-for-New-Designs%20Si4708-09-B.pdf
i search the keyword "rssi" in this spec, thought i don't know the exact range of the rssi, it seems that the rssi range won't be the same as the FM chip of the Otoro (i.e [-120, -20]).

If i will create a FM app, i think [0-5] is enough, i don't want to spend anytime to figure out the min and max value it should be, and how to transform it to a meaningful value. I don't quite understand why the app author should care about the accurate number in dBm, we even don't have a signal strength indicator in the FM app of Gaia.

So in my opinion, we should provide a relative signal strength at least, either an int value range from 0 to 5, or just keep it as a float value range from 0 to 1.

Comment 132

[Justin Lebar (not reading bugmail)]

Maybe we can just cut out the signal strength for now, if we don't need it for the FM app.  Then we wouldn't have to worry about scaling the logarithmic dBm units to more user-friendly units, and we wouldn't have to worry about that broadcast() call.

Comment 133

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #132)
> Maybe we can just cut out the signal strength for now, if we don't need it
> for the FM app.  Then we wouldn't have to worry about scaling the
> logarithmic dBm units to more user-friendly units, and we wouldn't have to
> worry about that broadcast() call.
Em, it's not a bad idea, Jonas, what do you think?

Comment 134

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds good

Comment 135

[Pin Zhang [:pzhang] (inactive)]

Attachment: part A - MOZ_B2G_FM option

rebased the lastest m-c

Comment 136

[Pin Zhang [:pzhang] (inactive)]

Attachment: part B - nsIDOMFMRadio IDL, v1.2

Removed signalStrength and onsignalstrengthchange

Comment 137

[Pin Zhang [:pzhang] (inactive)]

Attachment: part C - mozFMRadio v5.1

- Rebased the lastest m-c
- Removed signalStrength related codes
- Fixed nits based on the comment 124 and comment 125

Comment 138

[Pin Zhang [:pzhang] (inactive)]

(In reply to Ray Cheung [:pzhang] from comment #137)
> Created attachment 662062 [details] [diff] [review]
> part C - mozFMRadio v5.1
> 
> - Rebased the lastest m-c
> - Removed signalStrength related codes
> - Fixed nits based on the comment 124 and comment 125
- Used the interfaces of the latest patches of the bug 749053

Comment 139

[Pin Zhang [:pzhang] (inactive)]

(In reply to Ray Cheung [:pzhang] from comment #135)
> Created attachment 662058 [details] [diff] [review]
> part A - MOZ_B2G_FM option
> 
> rebased the lastest m-c
Hi, Kyle, do you have a plan to review this?

Comment 140

[Mike Hommey [:glandium]]

Comment on attachment 662058 [details] [diff] [review]
part A - MOZ_B2G_FM option

Review of attachment 662058 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +7346,5 @@
> +    MOZ_B2G_FM= )
> +if test -n "$MOZ_B2G_FM"; then
> +    AC_DEFINE(MOZ_B2G_FM)
> +fi
> +AC_SUBST(MOZ_B2G_FM)

Does this really need a configure option? It doesn't seem so. Please just AC_SUBST(MOZ_B2G_FM)

::: dom/Makefile.in
@@ +83,5 @@
>  
> +ifdef MOZ_B2G_FM
> +PARALLEL_DIRS += \
> +  fm \
> +  $(NULL)

There's not much point putting this on 3 lines. Just PARALLEL_DIRS += fm on one.

Comment 141

[Michael Wu [:mwu]]

Attachment: part A - MOZ_B2G_FM option v2

review comments addressed. AC_DEFINE was left in since there is code looking for that define.

Comment 142

[Michael Wu [:mwu]]

All patches were folded into one commit.

https://hg.mozilla.org/integration/mozilla-inbound/rev/75923725b2fc

Comment 143

[(no longer active)]

I backed out a range of patches including this one which caused a perma-leak in all of our test suites on debug builds:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e60144694d56

Comment 144

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/432033434b3a

Comment 145

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/432033434b3a

Comment 146

[David Bruant]

Here is the doc: https://developer.mozilla.org/en-US/docs/DOM/WebFM
I would love if someone could review it and say if something is obviously missing or wrong.

Thanks.

Comment 147

[Justin Lebar (not reading bugmail)]

> I would love if someone could review it and say if something is obviously missing or wrong.
>
> The WebFM API provides access to the radio.

This is a nit, but cell phones contain many radios (e.g. fm, gsm, wifi, bluetooth, nfc).  The WebFM API provides access to a device's /FM radio/.  I might additionally say in the introduction what you can do using the WebFM interface: "This interface lets you turn the FM radio on and off and tune it to different stations."

> Attributes [...]

We're missing the channelWidth attribute.

> Getting the frequency is a synchronous operation which may block the main thread and user interface.

I think this unfairly singles out the |frequency| attribute here in a way which may cause confusion.

Reading /any/ attribute in JS is a synchronous operation.  There is no such thing as an asynchronous attribute in JavaScript.

In general we care about synchronicity when it can cause disk I/O, because that's very slow.  That's why we don't like localStorage, for example.

It's true that reading the |frequency| attribute causes a synchronous call up to the parent process.  But as it turns out, we do a sync operation when reading all of the other attributes, as well.  The parent process does no I/O upon receiving one of these messages.

So either all of the attributes deserve a warning, or none of them does.  The issue isn't that a synchronous operation occurs, because that's how attributes work, but rather that we wait on a synchronous IPC message.  This isn't free, but it's a lot cheaper than sync I/O, so I'm not sure we want devs to be particularly scared of this.

Certainly you shouldn't expect getting the frequency (or any of the other attributes) to be fast, however.

> Minimum frequency up to which the seek method searches for radio stations

Perhaps "down to which"?  Really, this isn't sufficiently clear.  For example, can I manually set the FM radio frequency outside this bound?

> setFrequency() and cancelSeek()

What are the return values of these methods?  I think they return DOM requests, but we don't say so.

It looks good to me aside from these issues.  Thanks for your hard work here.

Comment 148

[David Bruant]

(In reply to Justin Lebar [:jlebar] from comment #147)
> > I would love if someone could review it and say if something is obviously missing or wrong.
> >
> > The WebFM API provides access to the radio.
> 
> This is a nit, but cell phones contain many radios (e.g. fm, gsm, wifi,
> bluetooth, nfc).  The WebFM API provides access to a device's /FM radio/.  I
> might additionally say in the introduction what you can do using the WebFM
> interface: "This interface lets you turn the FM radio on and off and tune it
> to different stations."
Yeah, I usually write the intro first and I should write it last instead, because at first, I don't know much about what's possible.
Taking your sentence as is. Thanks.

> > Attributes [...]
> 
> We're missing the channelWidth attribute.
Oops. Added. Thanks.
 
> > Getting the frequency is a synchronous operation which may block the main thread and user interface.
> 
> I think this unfairly singles out the |frequency| attribute here in a way
> which may cause confusion.
> 
> Reading /any/ attribute in JS is a synchronous operation.  There is no such
> thing as an asynchronous attribute in JavaScript.
Yes, I tend to use "synchronous" where I should talk about "blocking" and I really should stop doing that.

> It's true that reading the |frequency| attribute causes a synchronous call
> up to the parent process.  But as it turns out, we do a sync operation when
> reading all of the other attributes, as well.  The parent process does no
> I/O upon receiving one of these messages.
> 
> So either all of the attributes deserve a warning, or none of them does. 
> The issue isn't that a synchronous operation occurs, because that's how
> attributes work, but rather that we wait on a synchronous IPC message.  This
> isn't free, but it's a lot cheaper than sync I/O, so I'm not sure we want
> devs to be particularly scared of this.
> 
> Certainly you shouldn't expect getting the frequency (or any of the other
> attributes) to be fast, however.
Wow, that's super important! I removed the warning on the particular frequency attribute, but a general warning should be provided. I'll think about how to do that in a generic manner on the doc.
Taking this discussion to dev-webapi to not further pollute the bug.

> > Minimum frequency up to which the seek method searches for radio stations
> 
> Perhaps "down to which"?  Really, this isn't sufficiently clear.  For
> example, can I manually set the FM radio frequency outside this bound?
I've added a note to the setFrequency method.

> > setFrequency() and cancelSeek()
> 
> What are the return values of these methods?  I think they return DOM
> requests, but we don't say so.
fixed.

Thanks for your review, Justin.