870676 - [FMRadio] Rewrite the whole FMRadio API in C++

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::FMRadio, defect)

Comment 1

[Justin Lebar (not reading bugmail)]

This will probably be prioritized as a MemShrink:P2, but we like to leave MemShrink bugs un-prioritized when filing because we use triage during our MemShrink meetings as a time to talk about the bugs on the list and to make sure that the right people are aware of them.

Comment 2

[Pin Zhang [:pzhang] (inactive)]

Attachment: WIP Patch

I have implemented most of the features except the AudioChannelAgent related logics (Bug 862899), it passed all the tests I submitted to Bug 862672.

If the review cycle for this bug will not take too much time, I hope the patch for Bug 862899 could be rewritten based on this patch, :baku, Justin, what do you think?

Comment 3

[Pin Zhang [:pzhang] (inactive)]

About Bug 876597, I think we should handle it in Gecko layer, I checked the settings API, it seems that we can't read the settings synchronously, so I have a question, if the airplane mode is turned on, i.e. 'ril.radio.disabled' is set to True, how can I synchronous read the value of setting 'ril.radio.disabled' and refuse to turn on the FM radio HW if the app called `navigator.mozFMRadio.enable() in window.onload handler?

Comment 4

[Justin Lebar (not reading bugmail)]

You should be able to read the setting and then continue enabling the FM radio if and only if airplane mode is disabled.  That seems better than trying to do it synchronously; the settings API is probably async for a reason.

Comment 5

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #4)
> You should be able to read the setting and then continue enabling the FM
> radio if and only if airplane mode is disabled.  That seems better than
> trying to do it synchronously; the settings API is probably async for a
> reason.

OK, I got what you mean, thanks

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 754782 [details] [diff] [review]
WIP Patch

baku, would you mind doing a first pass on this patch?

Comment 7

[Pin Zhang [:pzhang] (inactive)]

Attachment: WIP whole-in-one Patch

Fix Bug 876597

Comment 8

[Andrea Marchesini [:baku]]

yes, I'm reviewing it.

Comment 9

[Andrea Marchesini [:baku]]

I would suggest to split this patch. At least in 3 parts: tests, IPDL, WebIDL.
I would like to see the IPDL part reviewed by jlebar or bent. For the rest I can help.

Comment 10

[Pin Zhang [:pzhang] (inactive)]

(In reply to Andrea Marchesini (:baku) from comment #9)
> I would suggest to split this patch. At least in 3 parts: tests, IPDL,
> WebIDL.
> I would like to see the IPDL part reviewed by jlebar or bent. For the rest I
> can help.

The tests are almost the same as what I submitted to Bug 862672, I just wrote one more test for Bug 876597, as Justin addressed in Bug 862672, most of the tests have problem under multi-process testing enviroment, so I think we don't have to review the tests in the first round.

Comment 11

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL

Comment 12

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part II: IPDL

Comment 13

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part III: Marionette tests

Comment 14

[Pin Zhang [:pzhang] (inactive)]

I splitted the patch into three parts as baku suggested in comment 9, hope it helps.

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 755850 [details] [diff] [review]
Part I: WebIDL

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

It looks good to me. Can you fix these comments and then I want to review it again. Thanks!

::: dom/base/Navigator.cpp
@@ +1149,5 @@
> +    mFMRadio = fmradio::FMRadio::CheckPermissionAndCreateInstance(win);
> +    NS_ENSURE_TRUE(mFMRadio, NS_OK);
> +  }
> +
> +  nsCOMPtr<nsISupports> fmRadio(mFMRadio);

NS_ADDREF(*aFMRadio = mFMRadio);

and remove these 2 lines.

::: dom/fmradio/FMRadio.cpp
@@ +69,5 @@
> +    UnregisterSwitchObserver(SWITCH_HEADPHONES, this);
> +  }
> +
> +  int32_t count = mRunnables.Length();
> +  for (int32_t index = 0; index < count; index++)

you should write:
for() {
 ...
}

and not
for()
{
  ...
}

the same for |if|, |switch|, etc.

@@ +83,5 @@
> +}
> +
> +void
> +FMRadio::Notify(const SwitchEvent& aEvent)
> +{

I would add MOZ_ASSERT(!mHasInternalAntenna)

@@ +97,5 @@
> +  switch(aType.type())
> +  {
> +    case FMRadioEventType::TFrequencyChangedEvent:
> +    {
> +      FrequencyChangedEvent event = aType;

You are not using this 'event' var, is it?

@@ +136,5 @@
> +
> +Nullable<double>
> +FMRadio::GetFrequency() const
> +{
> +  return Enabled() ? (Nullable<double>)(FMRadioService::Get()->GetFrequency())

I think this returns an internal value.

@@ +167,5 @@
> +  {
> +    return nullptr;
> +  }
> +
> +  nsRefPtr<DOMRequest> request = new DOMRequest(win);

Can FMRadioRequest be a DOMRequest? I'll comment it later.

::: dom/fmradio/FMRadio.h
@@ +93,5 @@
> +  bool mHasInternalAntenna;
> +  nsTArray<nsRefPtr<ReplyRunnable> > mRunnables;
> +};
> +
> +class FMRadioRequest MOZ_FINAL : public ReplyRunnable

Can this be:

class FMRadioRequest MOZ_FINAL : public ReplyRunnable
                               , public DOMRequest
{
  FMRadioRequest(nsPIDOMWindow* aWindow, FMRadio* aFMRadio)
    : DOMRequest(aWindow)
    ...

::: dom/fmradio/FMRadioService.cpp
@@ +4,5 @@
> + * 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 "FMRadioService.h"
> +#include "mozilla/dom/ContentParent.h"

you should not need this header in this patch.

@@ +117,5 @@
> +  delete sObserverList;
> +  sObserverList = nullptr;
> +}
> +
> +class EnableRunnable : public nsRunnable

MOZ_FINAL and no virtual ~EnableRunnable()

@@ +142,5 @@
> +    NS_ASSERTION(audioManager, "No AudioManager");
> +
> +    audioManager->SetFmRadioAudioEnabled(true);
> +
> +    // TODO apply path of bug 862899: AudioChannelAgent per process

Ok... so if we want to do so, I can take care of this, but we have to land these 2 bugs at the same time.
Otherwise I can propose a patch just for this in this bug, once all your patches have been fully reviewed.

@@ +162,5 @@
> +    : mFMRadioService(aFMRadioService) { }
> +  virtual ~ReadRilSettingTask() { }
> +
> +  NS_IMETHOD
> +  Handle(const nsAString & aName, const JS::Value & aResult)

nsAString& aName, const JS::Value& aResult

@@ +170,5 @@
> +
> +    if (!aResult.isBoolean())
> +    {
> +      mFMRadioService->mEnableRequest->SetReply(
> +          ErrorResponse(NS_ConvertASCIItoUTF16("Unexpected error")));

NS_LITERAL_STRING("Unexpected error")

@@ +189,5 @@
> +    }
> +    else
> +    {
> +      mFMRadioService->mEnableRequest->SetReply(ErrorResponse(
> +        NS_ConvertASCIItoUTF16("Airplane mode is enabled")));

NS_LITERAL_STRING

@@ +202,5 @@
> +  NS_IMETHOD
> +  HandleError(const nsAString& aName)
> +  {
> +    mFMRadioService->mEnableRequest->SetReply(ErrorResponse(
> +      NS_ConvertASCIItoUTF16("Unexpected error")));

NS_LITERAL_STRING

@@ +215,5 @@
> +};
> +
> +NS_IMPL_ISUPPORTS1(ReadRilSettingTask, nsISettingsServiceCallback)
> +
> +class DisableRunnable : public nsRunnable

MOZ_FINAL and no virtual ~DisableRunnable()

@@ +238,5 @@
> +    return NS_OK;
> +  }
> +};
> +
> +class SetFrequencyRunnable : public nsRunnable

see above.

@@ +248,5 @@
> +  virtual ~SetFrequencyRunnable() { }
> +
> +  NS_IMETHOD Run()
> +  {
> +    SetFMRadioFrequency(mFrequency);

Can SetFMRadioFrequency call FMRadioService->UpdateFrequency() ?

@@ +258,5 @@
> +  nsRefPtr<FMRadioService> mService;
> +  int32_t mFrequency;
> +};
> +
> +class SeekRunnable : public nsRunnable

Read above

@@ +381,5 @@
> +
> +  if (mDisabling)
> +  {
> +    aRunnable->SetReply(ErrorResponse(
> +      NS_ConvertASCIItoUTF16("It's disabling")));

NS_LITERAL_STRING. Update all of them.

::: dom/fmradio/FMRadioService.h
@@ +117,5 @@
> +};
> +
> +class ReadRilSettingTask;
> +
> +class FMRadioService : public IFMRadioService

MOZ_FINAL, if it is.

@@ +141,5 @@
> +  /* FMRadioObserver */
> +  virtual void Notify(const hal::FMRadioOperationInformation& info);
> +
> +  /* IFMRadioService */
> +  virtual bool IsEnabled();

MOZ_OVERRIDE everywhere here.

Comment 16

[Pin Zhang [:pzhang] (inactive)]

Thanks for your quick reply!

(In reply to Andrea Marchesini (:baku) from comment #15)
> @@ +136,5 @@
> > +
> > +Nullable<double>
> > +FMRadio::GetFrequency() const
> > +{
> > +  return Enabled() ? (Nullable<double>)(FMRadioService::Get()->GetFrequency())
> 
> I think this returns an internal value.

Did you mean:
  (Nullable<double>)(FMRadioService::Get()->GetFrequency())
is an internal value, and I can't return it?

But Nullable is just a template struct, the caller will get a copy of it, right?

> 
> @@ +167,5 @@
> > +  {
> > +    return nullptr;
> > +  }
> > +
> > +  nsRefPtr<DOMRequest> request = new DOMRequest(win);
> 
> Can FMRadioRequest be a DOMRequest? I'll comment it later.
> 
> ::: dom/fmradio/FMRadio.h
> @@ +93,5 @@
> > +  bool mHasInternalAntenna;
> > +  nsTArray<nsRefPtr<ReplyRunnable> > mRunnables;
> > +};
> > +
> > +class FMRadioRequest MOZ_FINAL : public ReplyRunnable
> 
> Can this be:
> 
> class FMRadioRequest MOZ_FINAL : public ReplyRunnable
>                                , public DOMRequest
> {
>   FMRadioRequest(nsPIDOMWindow* aWindow, FMRadio* aFMRadio)
>     : DOMRequest(aWindow)
>     ...
> 
It's a good idea, let me have a try.

> @@ +117,5 @@
> > +  delete sObserverList;
> > +  sObserverList = nullptr;
> > +}
> > +
> > +class EnableRunnable : public nsRunnable
> 
> MOZ_FINAL and no virtual ~EnableRunnable()
> 
Just curious, why Eclipse warn me this:
  Class 'EnableRunnable' has virtual method but non-virtual destructor
?

> @@ +142,5 @@
> > +    NS_ASSERTION(audioManager, "No AudioManager");
> > +
> > +    audioManager->SetFmRadioAudioEnabled(true);
> > +
> > +    // TODO apply path of bug 862899: AudioChannelAgent per process
> 
> Ok... so if we want to do so, I can take care of this, but we have to land
> these 2 bugs at the same time.
> Otherwise I can propose a patch just for this in this bug, once all your
> patches have been fully reviewed.

If you don't mind, I prefer the latter.

> 
> @@ +248,5 @@
> > +  virtual ~SetFrequencyRunnable() { }
> > +
> > +  NS_IMETHOD Run()
> > +  {
> > +    SetFMRadioFrequency(mFrequency);
> 
> Can SetFMRadioFrequency call FMRadioService->UpdateFrequency() ?
> 

I don't quite understand, :(

Comment 17

[Andrea Marchesini [:baku]]

> > Can SetFMRadioFrequency call FMRadioService->UpdateFrequency() ?
> I don't quite understand, :(

I mean, you call SetFMRadioFrequency(mFrequency) and then you do: mService->UpdateFrequency().
I was wondering if SetFMRadioFrequency() can  do: FMRadioService::Get()->UpdateFrequency()

Comment 18

[Pin Zhang [:pzhang] (inactive)]

(In reply to Andrea Marchesini (:baku) from comment #17)
> > > Can SetFMRadioFrequency call FMRadioService->UpdateFrequency() ?
> > I don't quite understand, :(
> 
> I mean, you call SetFMRadioFrequency(mFrequency) and then you do:
> mService->UpdateFrequency().
> I was wondering if SetFMRadioFrequency() can  do:
> FMRadioService::Get()->UpdateFrequency()

SetFMRadioFrequency() is defined in Hal layer, so I think we can't do this.

Comment 19

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL V2

Comment 20

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part II: IPDL

Comment 21

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part III: Marionette tests

Comment 22

[Pin Zhang [:pzhang] (inactive)]

(In reply to Andrea Marchesini (:baku) from comment #15)
> Comment on attachment 755850 [details] [diff] [review]
> Part I: WebIDL
> 
> Review of attachment 755850 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks good to me. Can you fix these comments and then I want to review it
> again. Thanks!
> 
> ::: dom/base/Navigator.cpp
> @@ +1149,5 @@
> > +    mFMRadio = fmradio::FMRadio::CheckPermissionAndCreateInstance(win);
> > +    NS_ENSURE_TRUE(mFMRadio, NS_OK);
> > +  }
> > +
> > +  nsCOMPtr<nsISupports> fmRadio(mFMRadio);
> 
> NS_ADDREF(*aFMRadio = mFMRadio);
> 
> and remove these 2 lines.

done

> ::: dom/fmradio/FMRadio.cpp
> @@ +69,5 @@
> > +    UnregisterSwitchObserver(SWITCH_HEADPHONES, this);
> > +  }
> > +
> > +  int32_t count = mRunnables.Length();
> > +  for (int32_t index = 0; index < count; index++)
> 
> you should write:
> for() {
>  ...
> }
> 
> and not
> for()
> {
>   ...
> }
> 
> the same for |if|, |switch|, etc.
>

done
 
> @@ +83,5 @@
> > +}
> > +
> > +void
> > +FMRadio::Notify(const SwitchEvent& aEvent)
> > +{
> 
> I would add MOZ_ASSERT(!mHasInternalAntenna)
> 

done

> @@ +97,5 @@
> > +  switch(aType.type())
> > +  {
> > +    case FMRadioEventType::TFrequencyChangedEvent:
> > +    {
> > +      FrequencyChangedEvent event = aType;
> 
> You are not using this 'event' var, is it?
> 

removed

> @@ +136,5 @@
> > +
> > +Nullable<double>
> > +FMRadio::GetFrequency() const
> > +{
> > +  return Enabled() ? (Nullable<double>)(FMRadioService::Get()->GetFrequency())
> 
> I think this returns an internal value.
> 
> @@ +167,5 @@
> > +  {
> > +    return nullptr;
> > +  }
> > +
> > +  nsRefPtr<DOMRequest> request = new DOMRequest(win);
> 
> Can FMRadioRequest be a DOMRequest? I'll comment it later.
> 
> ::: dom/fmradio/FMRadio.h
> @@ +93,5 @@
> > +  bool mHasInternalAntenna;
> > +  nsTArray<nsRefPtr<ReplyRunnable> > mRunnables;
> > +};
> > +
> > +class FMRadioRequest MOZ_FINAL : public ReplyRunnable
> 
> Can this be:
> 
> class FMRadioRequest MOZ_FINAL : public ReplyRunnable
>                                , public DOMRequest
> {
>   FMRadioRequest(nsPIDOMWindow* aWindow, FMRadio* aFMRadio)
>     : DOMRequest(aWindow)
>     ...
> 

done, please help to check if I used the CYCLE_COLLECTION MACROS correctly.

> ::: dom/fmradio/FMRadioService.cpp
> @@ +4,5 @@
> > + * 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 "FMRadioService.h"
> > +#include "mozilla/dom/ContentParent.h"
> 
> you should not need this header in this patch.
> 

Removed

> @@ +117,5 @@
> > +  delete sObserverList;
> > +  sObserverList = nullptr;
> > +}
> > +
> > +class EnableRunnable : public nsRunnable
> 
> MOZ_FINAL and no virtual ~EnableRunnable()
> 

done

> @@ +142,5 @@
> > +    NS_ASSERTION(audioManager, "No AudioManager");
> > +
> > +    audioManager->SetFmRadioAudioEnabled(true);
> > +
> > +    // TODO apply path of bug 862899: AudioChannelAgent per process
> 
> Ok... so if we want to do so, I can take care of this, but we have to land
> these 2 bugs at the same time.
> Otherwise I can propose a patch just for this in this bug, once all your
> patches have been fully reviewed.
> 
> @@ +162,5 @@
> > +    : mFMRadioService(aFMRadioService) { }
> > +  virtual ~ReadRilSettingTask() { }
> > +
> > +  NS_IMETHOD
> > +  Handle(const nsAString & aName, const JS::Value & aResult)
> 
> nsAString& aName, const JS::Value& aResult
> 

done

> @@ +170,5 @@
> > +
> > +    if (!aResult.isBoolean())
> > +    {
> > +      mFMRadioService->mEnableRequest->SetReply(
> > +          ErrorResponse(NS_ConvertASCIItoUTF16("Unexpected error")));
> 
> NS_LITERAL_STRING("Unexpected error")
> 
> @@ +189,5 @@
> > +    }
> > +    else
> > +    {
> > +      mFMRadioService->mEnableRequest->SetReply(ErrorResponse(
> > +        NS_ConvertASCIItoUTF16("Airplane mode is enabled")));
> 
> NS_LITERAL_STRING
> 
> @@ +202,5 @@
> > +  NS_IMETHOD
> > +  HandleError(const nsAString& aName)
> > +  {
> > +    mFMRadioService->mEnableRequest->SetReply(ErrorResponse(
> > +      NS_ConvertASCIItoUTF16("Unexpected error")));
> 
> NS_LITERAL_STRING
> 

done

> @@ +215,5 @@
> > +};
> > +
> > +NS_IMPL_ISUPPORTS1(ReadRilSettingTask, nsISettingsServiceCallback)
> > +
> > +class DisableRunnable : public nsRunnable
> 
> MOZ_FINAL and no virtual ~DisableRunnable()
> 
> @@ +238,5 @@
> > +    return NS_OK;
> > +  }
> > +};
> > +
> > +class SetFrequencyRunnable : public nsRunnable
> 
> see above.
> 
> @@ +248,5 @@
> > +  virtual ~SetFrequencyRunnable() { }
> > +
> > +  NS_IMETHOD Run()
> > +  {
> > +    SetFMRadioFrequency(mFrequency);
> 
> Can SetFMRadioFrequency call FMRadioService->UpdateFrequency() ?
> 
> @@ +258,5 @@
> > +  nsRefPtr<FMRadioService> mService;
> > +  int32_t mFrequency;
> > +};
> > +
> > +class SeekRunnable : public nsRunnable
> 
> Read above
> 
> @@ +381,5 @@
> > +
> > +  if (mDisabling)
> > +  {
> > +    aRunnable->SetReply(ErrorResponse(
> > +      NS_ConvertASCIItoUTF16("It's disabling")));
> 
> NS_LITERAL_STRING. Update all of them.
> 
> ::: dom/fmradio/FMRadioService.h
> @@ +117,5 @@
> > +};
> > +
> > +class ReadRilSettingTask;
> > +
> > +class FMRadioService : public IFMRadioService
> 
> MOZ_FINAL, if it is.
> 

done

> @@ +141,5 @@
> > +  /* FMRadioObserver */
> > +  virtual void Notify(const hal::FMRadioOperationInformation& info);
> > +
> > +  /* IFMRadioService */
> > +  virtual bool IsEnabled();
> 
> MOZ_OVERRIDE everywhere here.

done

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 756479 [details] [diff] [review]
Part I: WebIDL V2

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

::: dom/fmradio/FMRadio.cpp
@@ +17,5 @@
> +#undef LOG
> +#define LOG(args...) FM_LOG("FMRadio", args)
> +
> +// The pref indicates if the device has an internal antenna.
> +// If the pref is true, the antanna will be always available.

antenna. (This doesn't seem like a good use for a pref.)

@@ +50,5 @@
> +
> +  FMRadioService::Get()->RegisterHandler(this);
> +
> +  mHasInternalAntenna = Preferences::GetBool(DOM_FM_ANTENNA_INTERNAL_PREF,
> +                                             /* default = */ false);

Reading the pref for every window?

@@ +133,5 @@
> +
> +Nullable<double>
> +FMRadio::GetFrequency() const
> +{
> +  return Enabled() ? (Nullable<double>)(FMRadioService::Get()->GetFrequency())

No need for the parens around Nullable<double>

@@ +215,5 @@
> +already_AddRefed<DOMRequest>
> +FMRadio::SeekDown()
> +{
> +
> +  nsCOMPtr<nsPIDOMWindow> win = GetOwner();

Empty line

@@ +265,5 @@
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(FMRadioRequest, DOMRequest)
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(FMRadioRequest, DOMRequest)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END

Don't need those

::: dom/fmradio/FMRadio.h
@@ +117,5 @@
> +
> +    nsresult rv = NS_OK;
> +
> +    if (!mCanceled) {
> +      rv = CancelableRun();

rv is set but not used

::: dom/fmradio/FMRadioService.cpp
@@ +106,5 @@
> +FMRadioService::~FMRadioService()
> +{
> +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +  if (!obs || NS_FAILED(obs->RemoveObserver(mSettingsObserver,
> +    MOZSETTINGS_CHANGED_ID))) {

Indentation is off

@@ +272,5 @@
> +FMRadioService::UnregisterHandler(FMRadioEventObserver* aHandler)
> +{
> +  mObserverList->RemoveObserver(aHandler);
> +
> +  if (mObserverList->Length() == 0)

IsEmpty()

@@ +330,5 @@
> +  return 0;
> +}
> +
> +double
> +FMRadioService::GetFrequencyUpperBound()

A bunch of those can be const

@@ +351,5 @@
> +void
> +FMRadioService::Enable(double aFrequencyInMHz, ReplyRunnable* aRunnable)
> +{
> +  // We need to call EnableFMRadio() in main thread
> +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

MOZ_ASSERT

@@ +463,5 @@
> +
> +void
> +FMRadioService::SetFrequency(double aFrequencyInMHz, ReplyRunnable* aRunnable)
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Ditto. More below.

@@ +695,5 @@
> +    // The string that we're interested in will be a JSON string looks like:
> +    //  {"key":"ril.radio.disabled","value":true}
> +    mozilla::AutoSafeJSContext cx;
> +    nsDependentString dataStr(aData);
> +    JS::Value val;

JS::Rooted<JS::Value> val(cx);

@@ +696,5 @@
> +    //  {"key":"ril.radio.disabled","value":true}
> +    mozilla::AutoSafeJSContext cx;
> +    nsDependentString dataStr(aData);
> +    JS::Value val;
> +    if (!JS_ParseJSON(cx, dataStr.get(), dataStr.Length(), &val) ||

(You must not have bug 876639 in your tree yet.)

@@ +701,5 @@
> +        !val.isObject()) {
> +      return NS_OK;
> +    }
> +
> +    JSObject& obj(val.toObject());

JS::Rooted<JSObject*> obj(cx, val.toObject());

@@ +702,5 @@
> +      return NS_OK;
> +    }
> +
> +    JSObject& obj(val.toObject());
> +    JS::Value key;

Rooted

@@ +708,5 @@
> +        !key.isString()) {
> +      return NS_OK;
> +    }
> +
> +    JSString *jsKey = JS_ValueToString(cx, key);

JS::Rooted<JSString*> jsKey(cx, key.toString());

@@ +714,5 @@
> +    if (!keyStr.init(cx, jsKey)) {
> +      return NS_OK;
> +    }
> +
> +    JS::Value value;

Rooted

::: dom/fmradio/FMRadioService.h
@@ +85,5 @@
> +public:
> +  virtual ~IFMRadioService() { }
> +
> +  NS_IMETHOD_(nsrefcnt) AddRef(void);
> +  NS_IMETHOD_(nsrefcnt) Release(void);

NS_INLINE_DECL_REFCOUNTING or NS_INLINE_DECL_THREADSAFE_REFCOUNTING

::: dom/fmradio/Makefile.in
@@ +14,5 @@
> +FORCE_STATIC_LIB = 1
> +
> +include $(topsrcdir)/dom/dom-config.mk
> +
> +VPATH += $(srcdir)/ipc

Don't do this.

@@ +17,5 @@
> +
> +VPATH += $(srcdir)/ipc
> +
> +CPPSRCS += \
> +  FMRadio.cpp \

These should be in moz.build

::: dom/webidl/FMRadio.webidl
@@ +52,5 @@
> +  /**
> +   * Power the FM radio off.
> +   * The disabled event will be fired if this request completes successfully.
> +   */
> +  DOMRequest disable();

These can all return null, so should be 'DOMRequest?'

Comment 24

[Pin Zhang [:pzhang] (inactive)]

(In reply to :Ms2ger from comment #23)
> Comment on attachment 756479 [details] [diff] [review]
> Part I: WebIDL V2
> 
> Review of attachment 756479 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/fmradio/FMRadio.cpp
> @@ +17,5 @@
> > +#undef LOG
> > +#define LOG(args...) FM_LOG("FMRadio", args)
> > +
> > +// The pref indicates if the device has an internal antenna.
> > +// If the pref is true, the antanna will be always available.
> 
> antenna. (This doesn't seem like a good use for a pref.)

It's possible that some devices have internal antenna which means you don't
have to plug in the headphone to act as an antenna, so I think we should
make it customizable, and adding a pref for this is what I can imagine.

Any better choice?

> 
> @@ +50,5 @@
> > +
> > +  FMRadioService::Get()->RegisterHandler(this);
> > +
> > +  mHasInternalAntenna = Preferences::GetBool(DOM_FM_ANTENNA_INTERNAL_PREF,
> > +                                             /* default = */ false);
> 
> Reading the pref for every window?

Yes, otherwise, we have to read the pref through synchronous IPC message, and
receive the antenna-state-changed event from the parent process.

> @@ +265,5 @@
> > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(FMRadioRequest, DOMRequest)
> > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> > +
> > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(FMRadioRequest, DOMRequest)
> > +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> 
> Don't need those
> 

Why not?

If I removed these lines, I got linking error:

    ../../../prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/../lib/gcc/arm-linux-androideabi/4.4.3/../../../../arm-linux-androideabi/bin/ld: /home/pzhang/work/b2g/B2G-UNAGI/objdir-gecko/toolkit/library/../../dom/fmradio/FMRadio.o: in function mozilla::dom::fmradio::FMRadioRequest::FMRadioRequest(nsPIDOMWindow*, mozilla::dom::fmradio::FMRadio*):../../dist/include/nsTArray-inl.h:161: error: undefined reference to 'vtable for mozilla::dom::fmradio::FMRadioRequest'

I also removed NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED from the header file and recompiled, same error.

> 
> @@ +696,5 @@
> > +    //  {"key":"ril.radio.disabled","value":true}
> > +    mozilla::AutoSafeJSContext cx;
> > +    nsDependentString dataStr(aData);
> > +    JS::Value val;
> > +    if (!JS_ParseJSON(cx, dataStr.get(), dataStr.Length(), &val) ||
> 
> (You must not have bug 876639 in your tree yet.)

OK, I will rebase my WIP branch.

> 
> ::: dom/webidl/FMRadio.webidl
> @@ +52,5 @@
> > +  /**
> > +   * Power the FM radio off.
> > +   * The disabled event will be fired if this request completes successfully.
> > +   */
> > +  DOMRequest disable();
> 
> These can all return null, so should be 'DOMRequest?'

If we changed to 'DOMRequest?', it means we changed the WebFM API definition, I don't
think it's a good choice.

The only possible to return null is we can't get the DOMWindow object which means it's
an invalid call (window is destroyed?). 

I checked the non-webidl api implementation, e.g. DeviceStorage API, we just
return NS_ERROR_UNEXPECTED. Should I just throw an exception if GetOwer() returns null?

Comment 25

[Pin Zhang [:pzhang] (inactive)]

(In reply to :Ms2ger from comment #23)
> Comment on attachment 756479 [details] [diff] [review]
> Part I: WebIDL V2
> 
> Review of attachment 756479 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +272,5 @@
> > +FMRadioService::UnregisterHandler(FMRadioEventObserver* aHandler)
> > +{
> > +  mObserverList->RemoveObserver(aHandler);
> > +
> > +  if (mObserverList->Length() == 0)
> 
> IsEmpty()

No such a method.

> 
> ::: dom/fmradio/Makefile.in
> @@ +14,5 @@
> > +FORCE_STATIC_LIB = 1
> > +
> > +include $(topsrcdir)/dom/dom-config.mk
> > +
> > +VPATH += $(srcdir)/ipc
> 
> Don't do this.
> 

I tried to remove `VPATH += $(srcdir)/ipc`, It can't be compiled.
Actually, I stolen the codes from dom/bluetooth/Makefile.in

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Pin Zhang [:pzhang] from comment #25)
> (In reply to :Ms2ger from comment #23)
> > Comment on attachment 756479 [details] [diff] [review]
> > Part I: WebIDL V2
> > 
> > Review of attachment 756479 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > @@ +272,5 @@
> > > +FMRadioService::UnregisterHandler(FMRadioEventObserver* aHandler)
> > > +{
> > > +  mObserverList->RemoveObserver(aHandler);
> > > +
> > > +  if (mObserverList->Length() == 0)
> > 
> > IsEmpty()
> 
> No such a method.

I was going to double-check that there was one, but looks like I forgot. We should add it at some point, but that's something for another day.

> > 
> > ::: dom/fmradio/Makefile.in
> > @@ +14,5 @@
> > > +FORCE_STATIC_LIB = 1
> > > +
> > > +include $(topsrcdir)/dom/dom-config.mk
> > > +
> > > +VPATH += $(srcdir)/ipc
> > 
> > Don't do this.
> > 
> 
> I tried to remove `VPATH += $(srcdir)/ipc`, It can't be compiled.
> Actually, I stolen the codes from dom/bluetooth/Makefile.in

You'll need to add a moz.build in dom/fmradio/ipc for the files in that directory, and add

DIRS += [
    'ipc',
]

to dom/fmradio/moz.build.

Comment 27

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL V3

Comment 28

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part II: IPDL

Comment 29

[Pin Zhang [:pzhang] (inactive)]

(In reply to :Ms2ger from comment #23)
> Comment on attachment 756479 [details] [diff] [review]
> Part I: WebIDL V2
> 
> Review of attachment 756479 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +133,5 @@
> > +
> > +Nullable<double>
> > +FMRadio::GetFrequency() const
> > +{
> > +  return Enabled() ? (Nullable<double>)(FMRadioService::Get()->GetFrequency())
> 
> No need for the parens around Nullable<double>

Done

> 
> @@ +215,5 @@
> > +already_AddRefed<DOMRequest>
> > +FMRadio::SeekDown()
> > +{
> > +
> > +  nsCOMPtr<nsPIDOMWindow> win = GetOwner();
> 
> Empty line

Done

> 
> ::: dom/fmradio/FMRadio.h
> @@ +117,5 @@
> > +
> > +    nsresult rv = NS_OK;
> > +
> > +    if (!mCanceled) {
> > +      rv = CancelableRun();
> 
> rv is set but not used

Removed

> 
> ::: dom/fmradio/FMRadioService.cpp
> @@ +106,5 @@
> > +FMRadioService::~FMRadioService()
> > +{
> > +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> > +  if (!obs || NS_FAILED(obs->RemoveObserver(mSettingsObserver,
> > +    MOZSETTINGS_CHANGED_ID))) {
> 
> Indentation is off

Done

> 
> @@ +330,5 @@
> > +  return 0;
> > +}
> > +
> > +double
> > +FMRadioService::GetFrequencyUpperBound()
> 
> A bunch of those can be const

Done

> 
> @@ +351,5 @@
> > +void
> > +FMRadioService::Enable(double aFrequencyInMHz, ReplyRunnable* aRunnable)
> > +{
> > +  // We need to call EnableFMRadio() in main thread
> > +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> 
> MOZ_ASSERT

Done

>
> @@ +695,5 @@
> > +    // The string that we're interested in will be a JSON string looks like:
> > +    //  {"key":"ril.radio.disabled","value":true}
> > +    mozilla::AutoSafeJSContext cx;
> > +    nsDependentString dataStr(aData);
> > +    JS::Value val;
> 
> JS::Rooted<JS::Value> val(cx);

Done

> 
> @@ +696,5 @@
> > +    //  {"key":"ril.radio.disabled","value":true}
> > +    mozilla::AutoSafeJSContext cx;
> > +    nsDependentString dataStr(aData);
> > +    JS::Value val;
> > +    if (!JS_ParseJSON(cx, dataStr.get(), dataStr.Length(), &val) ||
> 
> (You must not have bug 876639 in your tree yet.)
> 
> @@ +701,5 @@
> > +        !val.isObject()) {
> > +      return NS_OK;
> > +    }
> > +
> > +    JSObject& obj(val.toObject());
> 
> JS::Rooted<JSObject*> obj(cx, val.toObject());

Done

> 
> @@ +702,5 @@
> > +      return NS_OK;
> > +    }
> > +
> > +    JSObject& obj(val.toObject());
> > +    JS::Value key;
> 
> Rooted
> 

Done

> @@ +708,5 @@
> > +        !key.isString()) {
> > +      return NS_OK;
> > +    }
> > +
> > +    JSString *jsKey = JS_ValueToString(cx, key);
> 
> JS::Rooted<JSString*> jsKey(cx, key.toString());
> 

Done

> @@ +714,5 @@
> > +    if (!keyStr.init(cx, jsKey)) {
> > +      return NS_OK;
> > +    }
> > +
> > +    JS::Value value;
> 
> Rooted

Done

> 
> ::: dom/fmradio/FMRadioService.h
> @@ +85,5 @@
> > +public:
> > +  virtual ~IFMRadioService() { }
> > +
> > +  NS_IMETHOD_(nsrefcnt) AddRef(void);
> > +  NS_IMETHOD_(nsrefcnt) Release(void);
> 
> NS_INLINE_DECL_REFCOUNTING or NS_INLINE_DECL_THREADSAFE_REFCOUNTING

Done

> 
> ::: dom/fmradio/Makefile.in
> @@ +14,5 @@
> > +FORCE_STATIC_LIB = 1
> > +
> > +include $(topsrcdir)/dom/dom-config.mk
> > +
> > +VPATH += $(srcdir)/ipc
> 
> Don't do this.

Add moz.build and Makefile.in in dom/fmradio/ipc

> 
> @@ +17,5 @@
> > +
> > +VPATH += $(srcdir)/ipc
> > +
> > +CPPSRCS += \
> > +  FMRadio.cpp \
> 
> These should be in moz.build

Done

Comment 30

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL V3.1

I found that we will receive FM_RADIO_OPERATION_ENABLE when we called DisableFMRadio(), there must be some problem in the Hal layer, so I double checked IsFMRadioOn() in the Observe() function.

Comment 31

[Pin Zhang [:pzhang] (inactive)]

(In reply to Pin Zhang [:pzhang] from comment #30)
> Created attachment 757763 [details] [diff] [review]
> Part I: WebIDL V3.1
> 
> I found that we will receive FM_RADIO_OPERATION_ENABLE when we called
> DisableFMRadio(), there must be some problem in the Hal layer, so I double
> checked IsFMRadioOn() in the Observe() function.

Hi, Steven, can you take a look at this?

Comment 32

[Justin Lebar (not reading bugmail)]

Comment on attachment 757397 [details] [diff] [review]
Part II: IPDL

This patch looks pretty good, although there are enough issues that I'd like to
have another look.

One of the goals I had for this rewrite was to reduce the number of pieces of
code involved in the FM radio.

In this respect I may have been overly optimistic; the rewrite seems to have
increased the number of pieces here.  A lot of this increase seems necessary,
but it's worth considering whether there are any pieces we can merge, and what
the cost of that would be.

>diff --git a/dom/fmradio/ipc/PFMRadio.ipdl b/dom/fmradio/ipc/PFMRadio.ipdl

>+union FMRadioEventType
>+{
>+  StateChangedEvent;
>+  FrequencyChangedEvent;
>+};

Maybe this should be called FMRadioEvent or maybe FMRadioEventArgs.  Type.  The
"Type" part makes it sound like this is an enum which can be either
StateChangedEvent or FrequencyChangedEvent.  But in fact that enum is
FMRadioEventType.type()!

>+union FMRadioRequestType
>+{
>+  EnableRequest;
>+  DisableRequest;
>+  SetFrequencyRequest;
>+  SeekRequest;
>+  CancelSeekRequest;
>+};

Similarly, FMRadioRequestArgs would make more sense to me here.  (I'd prefer
just FMRadioRequest, but that's already taken.)

>+parent:
>+  sync IsEnabled() returns (bool enabled);
>+  sync GetFrequency() returns (double frequency);
>+  sync GetSettings() returns (Settings settings);
>+
>+  PFMRadioRequest(FMRadioRequestType requestType);

A comment here explaining what's going on (and why we can't have separate
Enable/SetFrequency/etc. methods) would be helpful.

>+  /**
>+   * Sent when the FM HW is enabled/disabled or frequency is changed.
>+   */
>+  Notify(FMRadioEventType event);

Can we have separate NotifyEnabledChanged and NotifyFrequencyChanged methods?
That is, can we get rid of FMRadioEventType altogether?

>diff --git a/dom/fmradio/ipc/PFMRadioRequest.ipdl b/dom/fmradio/ipc/PFMRadioRequest.ipdl
>new file mode 100644
>index 0000000..d8691d1
>--- /dev/null
>+++ b/dom/fmradio/ipc/PFMRadioRequest.ipdl

>@@ -0,0 +1,38 @@
>+namespace mozilla {
>+namespace dom {
>+namespace fmradio {
>+
>+struct ErrorResponse
>+{
>+  nsString error;
>+};
>+
>+struct SuccessResponse
>+{
>+};
>+
>+union FMRadioResponseType
>+{
>+  ErrorResponse;
>+  SuccessResponse;
>+};
>+
>+async protocol PFMRadioRequest
>+{
>+  manager PFMRadio;
>+
>+child:
>+  __delete__(FMRadioResponseType response);
>+
>+};

Please add documentation explaining what PFMRadioRequest is for and that
__delete__ is how the parent passes the result of the request down to the
child.

>diff --git a/dom/fmradio/ipc/FMRadioChild.cpp b/dom/fmradio/ipc/FMRadioChild.cpp
>new file mode 100644
>index 0000000..9192a59
>--- /dev/null
>+++ b/dom/fmradio/ipc/FMRadioChild.cpp

FMRadioChild is supposed to be a singleton, right?  Perhaps we could merge it
with FMRadioChildService, which is also a singleton.

>+bool
>+FMRadioChild::RecvNotify(const FMRadioEventType& aType)
>+{
>+  FMRadioChildService::Get()->DistributeEvent(aType);

It would be helpful to keep the names parallel, so the code is easier to
follow.  If this function is "Notify" and it simply calls another function,
it's helpful if that function is also called "Notify", all things being equal.

>+bool
>+FMRadioChild::Recv__delete__()
>+{
>+  return true;
>+}

Do you need this one if you're just returning true?  I'm not sure it's
necessary, but it's fine if it is.

>diff --git a/dom/fmradio/ipc/FMRadioChildService.h b/dom/fmradio/ipc/FMRadioChildService.h
>new file mode 100644
>index 0000000..83c99b4
>--- /dev/null
>+++ b/dom/fmradio/ipc/FMRadioChildService.h

>@@ -0,0 +1,66 @@
>+/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
>+/* vim: set ts=2 et sw=2 tw=80: */
>+/* 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/. */
>+
>+#ifndef mozilla_dom_fmradio_ipc_fmradiochildservice_h__
>+#define mozilla_dom_fmradio_ipc_fmradiochildservice_h__
>+
>+#include "FMRadioCommon.h"
>+#include "DOMRequest.h"
>+#include "FMRadioService.h"
>+#include "mozilla/dom/fmradio/PFMRadio.h"
>+
>+BEGIN_FMRADIO_NAMESPACE
>+
>+class FMRadioChild;
>+class FMRadioRequestType;
>+
>+class FMRadioChildService MOZ_FINAL : public IFMRadioService
>+{
>+  friend class FMRadioChild;
>+
>+public:
>+  static FMRadioChildService* Get();

We have a convention in the DOM (which is not universally respected but is
fairly prevalent) that methods which start with "Get" can return null, while
methods which cannot return null don't start with "Get".

I think we should follow this convention here.  It would mean replacing "Get"
on this singleton and any others with something else, maybe "Singleton".

>+  /* IFMRadioService interfaces */

These are methods, not interfaces.  (An interface is a set of methods.)

>diff --git a/dom/fmradio/ipc/FMRadioChildService.cpp b/dom/fmradio/ipc/FMRadioChildService.cpp
>new file mode 100644
>index 0000000..c61e1ab
>--- /dev/null
>+++ b/dom/fmradio/ipc/FMRadioChildService.cpp

>+FMRadioChildService::FMRadioChildService()
>+  : mEnabled(false)
>+  , mFrequency(0)
>+  , mSettings(Settings())
>+{
>+  mChildEventObserverList = new FMRadioEventObserverList();
>+
>+  mFMRadioChild = new FMRadioChild();

You can use the initializer list for these two members.

>+  ContentChild::GetSingleton()->SendPFMRadioConstructor(mFMRadioChild);
>+}
>+
>+FMRadioChildService::~FMRadioChildService()
>+{
>+  // The FMRadioChild object will be released in ContentChild::DeallocPFMRadio
>+  mFMRadioChild = nullptr;

Since this is a raw pointer, setting it to null in the destructor doesn't do
anything.  Maybe you meant for it to be an nsRefPtr?  That's probably safer
anyway.

>+  delete mChildEventObserverList;

Since this member is deleted when this class is deleted, it should be an
nsAutoPtr<>.  But actually, I don't see wy mChildEventObserverList needs to be
a pointer at all; can we make it 

  |FMRadioEventObserverList mChildEventObserverList|?

>+}

>+void
>+FMRadioChildService::Init()
>+{
>+  mFMRadioChild->SendIsEnabled(&mEnabled);
>+  mFMRadioChild->SendGetFrequency(&mFrequency);
>+  mFMRadioChild->SendGetSettings(&mSettings);

This is three synchronous calls in a row.  Each sync call is pretty expensive
in the child and blocks the main thread of the child process.  It would be
better to combine these three sync calls into one.  Indeed, I don't think you
need IsEnabled, GetFrequency, and GetSettings except for this Init() call, so
then we could get rid of them, which would be good.

Maybe we can shove all of this info into the "settings" object.  Then we could 

In order to ensure that the FMRadioChildService doesn't miss any changes to
what's going on here, the parent needs to promise to send us notifications of
any changes which happen starting at some point before it receives the first of
these sync messages.  I /think/ we'll start receiving notifications in the
parent after it runs RecvPFMRadioConstructor, so I think we're good.  Do you
think so too?

>+void
>+FMRadioChildService::SetFrequency(double aFrequency,
>+                                       ReplyRunnable* aRunnable)

Indentation

>+{
>+  SendRequest(aRunnable, SetFrequencyRequest(aFrequency));
>+}
>+
>+void
>+FMRadioChildService::Seek(bool upward, ReplyRunnable* aRunnable)

aUpward

>+{
>+  SendRequest(aRunnable, SeekRequest(upward));
>+}
>+
>+void
>+FMRadioChildService::CancelSeek(ReplyRunnable* aRunnable)
>+{
>+  SendRequest(aRunnable, CancelSeekRequest());
>+}
>+
>+void
>+FMRadioChildService::DistributeEvent(const FMRadioEventType& aType)

"Distribute" isn't idiomatic.  "BroadcastEvent" or "NotifyEvent" would be a
better name.

>+{
>+  switch (aType.type()) {
>+    case FMRadioEventType::TFrequencyChangedEvent:
>+    {
>+      FrequencyChangedEvent event = aType;

I think the idiomatic way of doing this is

  const FrequencyChangedEvent& event = aType.get_FrequencyChangedEvent();

>+      mFrequency = event.frequency();
>+      break;
>+    }
>+    case FMRadioEventType::TStateChangedEvent:
>+    {
>+      StateChangedEvent event = aType;

Similarly per above.

>+      mEnabled = event.enabled();
>+      mFrequency = event.frequency();
>+      break;
>+    }
>+    default:
>+      NS_RUNTIMEABORT("not reached");
>+      break;
>+  }
>+
>+  mChildEventObserverList->Broadcast(aType);
>+}
>+
>+void
>+FMRadioChildService::RegisterHandler(FMRadioEventObserver* aHandler)

We should probably call this AddObserver (or maybe RegisterObserver), so that
the nomenclature is consistent.

>+{
>+  mChildEventObserverList->AddObserver(aHandler);
>+}
>+
>+void
>+FMRadioChildService::UnregisterHandler(FMRadioEventObserver* aHandler)
>+{
>+  mChildEventObserverList->RemoveObserver(aHandler);
>+
>+  if (mChildEventObserverList->Length() == 0) {
>+    sFMRadioChildService = nullptr;
>+    delete this;
>+  }

How do we trigger ContentChild::DeallocPFMRadio when we hit this path?  If we
don't, then mFMRadioChild will live on, which is bad...

I'm not sure that deleting the FM radio service when we have zero observers
makes a ton of sense.

Part of the reason that this idiom concerns me is that if we do something like:

  FMRadioChildService* svc = FMRadioChildService::Get();
  svc->UnregisterHandler(...);

At this point, we can't use |svc| anymore; it might have been deleted!  I'm
afraid that this will be a very easy mistake to make.

What would happen if instead we kept it FMRadioChildService around until
shutdown?  It's not using a lot of resources, as far as I can tell.

BTW sFMRadioChildService should probably be a StaticAutoPtr if it's an owning
pointer.

>+}
>+
>+void
>+FMRadioChildService::SendRequest(ReplyRunnable* aReplyRunnable,
>+                                 FMRadioRequestType aType)
>+{
>+  PFMRadioRequestChild* child = new FMRadioRequestChild(aReplyRunnable);

Please call |child| |request|, or something like that.  "Child" just refers to
the fact that this is the child-process half of this IPDL object.

>+  mFMRadioChild->SendPFMRadioRequestConstructor(child, aType);
>+}

>+// static
>+FMRadioChildService*
>+FMRadioChildService::Get()
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+
>+  if (sFMRadioChildService) {
>+    return sFMRadioChildService;
>+  }
>+
>+  MOZ_ASSERT(!sFMRadioChild);

Where sFMRadioChild is defined?  I don't see it in either of the two patches in
this bug.

>+
>+  sFMRadioChildService = new FMRadioChildService();
>+
>+  sFMRadioChildService->Init();

One of the only reasons to have a separate Init() method is when the object
in question is reference counted.  It's illegal (and very dangerous) to call
XPCOM methods on a refcounted object when its refcount is zero.

Here's why that gets us into trouble.  Suppose we have something like

  class Foo { // refcounted };

  Foo::Foo() {
    SomeFunction(this);
  }

  SomeFunction(Foo* aFoo)
  {
    nsRefPtr<Foo> foo = aFoo;
    ...
  }

During the Foo() constructor, the object's refcount is 0.  When SomeFunction
runs, it increments the refcount to 1.  When the function exits, the nsRefPtr
goes out of scope and the refcount gets decremented to 0.  That causes the
object to be delete'ed!  Thus |this| is a dangling pointer after the call to
SomeFunction() in Foo::Foo().

Since FMRadioChildService is not a refcounted object, we're not working around
this problem.  As a result, I don't think we need an Init() method separate
from the constructor.

>+  return sFMRadioChildService;
>+}

>diff --git a/dom/fmradio/ipc/FMRadioParent.cpp b/dom/fmradio/ipc/FMRadioParent.cpp
>new file mode 100644
>index 0000000..8b6079a
>--- /dev/null
>+++ b/dom/fmradio/ipc/FMRadioParent.cpp

I guess we can't merge this one into FMRadioService because FMRadioParent isn't
a singleton.

>+// The pref 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.fmradio.antenna.internal"

This is never used.

>+BEGIN_FMRADIO_NAMESPACE
>+
>+FMRadioParent::FMRadioParent()
>+{
>+  MOZ_COUNT_CTOR(FMRadioParent);
>+
>+  FMRadioService::Get()->RegisterHandler(this);
>+}
>+
>+FMRadioParent::~FMRadioParent()
>+{
>+  MOZ_COUNT_DTOR(FMRadioParent);
>+
>+  FMRadioService::Get()->UnregisterHandler(this);
>+}
>+
>+void
>+FMRadioParent::ActorDestroy(ActorDestroyReason aWhy)
>+{
>+  return;
>+}

I don't think you need this override if it's empty.

>+void
>+FMRadioParent::Notify(const FMRadioEventType& aType)
>+{
>+  switch (aType.type()) {
>+    case FMRadioEventType::TStateChangedEvent:
>+    case FMRadioEventType::TFrequencyChangedEvent:
>+    {
>+      unused << this->SendNotify(aType);
>+      break;
>+    }
>+    default:
>+      NS_RUNTIMEABORT("not reached");
>+      break;
>+  }

We should be consistent within one switch about whether we use braces inside
the case statements.  I'd probably not use them here, since we don't make any
declarations in this switch.

>+}

>diff --git a/dom/fmradio/ipc/FMRadioRequestChild.cpp b/dom/fmradio/ipc/FMRadioRequestChild.cpp
>new file mode 100644
>index 0000000..4c0049d
>--- /dev/null
>+++ b/dom/fmradio/ipc/FMRadioRequestChild.cpp

>+void
>+FMRadioRequestChild::ActorDestroy(ActorDestroyReason aWhy)
>+{
>+  if (!mReplyRunnable) {
>+    return;
>+  }
>+
>+  // If mReplyRunnable is not nullptr, it means we didn't receive __delete__
>+  // message normally, and we need cancel and dispatch it to make sure
>+  // it could remove itself from FMRadio::mRunnables.

s/could remove/removes/

>+  mReplyRunnable->Cancel();
>+  NS_DispatchToMainThread(mReplyRunnable);
>+}

>diff --git a/dom/fmradio/ipc/FMRadioRequestParent.h b/dom/fmradio/ipc/FMRadioRequestParent.h
>@@ -0,0 +1,101 @@

>+class FMRadioRequestParent MOZ_FINAL : public PFMRadioRequestParent
>+{
>+public:
>+  FMRadioRequestParent(const FMRadioRequestType& aRequestType);
>+  ~FMRadioRequestParent();
>+
>+  NS_IMETHOD_(nsrefcnt) AddRef();
>+  NS_IMETHOD_(nsrefcnt) Release();

The new hotness is to inherit from mozilla::RefCounted<FMRadioRequestParent,
AtomicRefCount>.  That takes care of the declaration and implementation and
AddRef/Release for you.

>+  void Dispatch();
>+
>+  virtual void ActorDestroy(ActorDestroyReason aWhy) MOZ_OVERRIDE;
>+
>+private:
>+  class ParentReplyRunnable MOZ_FINAL : public ReplyRunnable
>+  {
>+  public:
>+    ParentReplyRunnable(FMRadioRequestParent* aParent)
>+      : ReplyRunnable()

The explicit call to the superclass's no-args constructor isn't needed, but you
can leave it if you like it.

>+      , mParent(aParent)
>+    {
>+      mCanceled = !(mParent->AddRunnable(this));

Don't need the parens.  Also, we can set mCanceled in the initialization list.

>+    }
>+
>+    NS_IMETHOD Run()
>+    {
>+      NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>+
>+      nsresult rv = NS_OK;
>+
>+      if (!mCanceled) {
>+        rv = CancelableRun();

I don't understand in what way CancelableRun is "cancelable".  But it's one
line and not called from anywhere else; can we just inline it?

>+        mParent->RemoveRunnable(this);
>+      }
>+
>+      return rv;
>+    }
>+
>+    nsresult CancelableRun()
>+    {
>+      unused << mParent->Send__delete__(mParent, mResponseType);
>+      return NS_OK;
>+    }
>+
>+    NS_IMETHOD Cancel()
>+    {
>+      mCanceled = true;
>+      return NS_OK;
>+    }

You may be able to use nsCancelableRunnable from nsThreadUtils.h.

>+  private:
>+    nsRefPtr<FMRadioRequestParent> mParent;

The relationship of |this| to mParent is not "child to parent", so mParent
should be named something else, perhaps mRequest.

>+    bool mCanceled;
>+  };
>+
>+private:
>+  bool AddRunnable(ParentReplyRunnable* aRunnable)
>+  {
>+    MutexAutoLock lock(mMutex);
>+    if (mActorDestroyed)
>+      return false;
>+
>+    mRunnables.AppendElement(aRunnable);
>+    return true;
>+  }
>+
>+  void RemoveRunnable(ParentReplyRunnable* aRunnable)
>+  {
>+    MutexAutoLock lock(mMutex);
>+    mRunnables.RemoveElement(aRunnable);
>+  }
>+
>+private:
>+  nsAutoRefCnt mRefCnt;
>+  FMRadioRequestType mRequestType;
>+  Mutex mMutex;
>+  bool mActorDestroyed;
>+  nsTArray<nsRefPtr<ParentReplyRunnable> > mRunnables;

I don't understand why FMRadioRequestParent has a list of runnables.  Under
what circumstances do we have multiple things to dispatch here?

>+};
>+
>+END_FMRADIO_NAMESPACE

>diff --git a/dom/fmradio/ipc/FMRadioRequestParent.cpp b/dom/fmradio/ipc/FMRadioRequestParent.cpp
>new file mode 100644
>index 0000000..cc983cb
>--- /dev/null
>+++ b/dom/fmradio/ipc/FMRadioRequestParent.cpp

>@@ -0,0 +1,87 @@
>+FMRadioRequestParent::FMRadioRequestParent(const FMRadioRequestType& aType)
>+  : mRequestType(aType)
>+  , mMutex("FMRadioRequestParent::mMutex")
>+  , mActorDestroyed(false)
>+{
>+  MOZ_COUNT_CTOR(FMRadioRequestParent);
>+}
>+
>+void
>+FMRadioRequestParent::Dispatch()
>+{

I think we could simplify this a lot if we did something like the following:

  if (actorDestroyed)
    return;

  nsRefPtr<ReplyRunnable> replyRunnable = new ParentReplyRunnable(this);
  switch(mRequestType.type()) {
  ...
  default:
    NS_RUNTIMEABORT(...);
  }

  MutexAutoLock lock(mMutex);
  replyRunnables.AppendElement(replyRunnable);

Explicit is better than implicit, and this isn't any longer than what we have
now.  But I still don't understand why we would ever have more than one reply
runnable; it looks like Dispatch() is called only when the FMRadioRequestParent
is allocated.

If we never Dispatch() twice, maybe we can remove another layer of indirection
and simply pass the FMRadioRequestParent to the FMRadioService.  We could have
a Completed() (or whatever) method on the FMRadioRequestParent that the
FMRadioService could call when it's done, and that would create and dispatch a
runnable to the main thread.

>+  switch (mRequestType.type()) {
>+    case FMRadioRequestType::TEnableRequest:
>+    {
>+      EnableRequest request = mRequestType;

Again something like

  const EnableRequest& request = mRequestType.get_EnableRequest()

>+      nsRefPtr<ReplyRunnable> replyRunnable = new ParentReplyRunnable(this);
>+      FMRadioService::Get()->Enable(request.frequency(),
>+                                    replyRunnable.forget().get());

By passing replyRunnable.forget().get() here, we're leaking a refcnt on that
object.  It will now never be deleted, unless we manually NS_RELEASE it
somewhere.

FMRadioService::Enable() needs to addref the runnable, so assuming that
function is correct, you shouldn't need to forget() anything here.

>+      break;
>+    }
>+    case FMRadioRequestType::TDisableRequest:
>+    {
>+      nsRefPtr<ReplyRunnable> replyRunnable = new ParentReplyRunnable(this);
>+      FMRadioService::Get()->Disable(replyRunnable.forget().get());

Again, this is leaking the runnable.

>diff --git a/dom/fmradio/ipc/ipdl.mk b/dom/fmradio/ipc/ipdl.mk
>new file mode 100644
>index 0000000..fd304f8
>--- /dev/null
>+++ b/dom/fmradio/ipc/ipdl.mk

>+IPDLSRCS = \
>+  PFMRadio.ipdl \
>+  PFMRadioRequest.ipdl \
>+  $(NULL)

I wouldn't complain if you wanted to combine these two IPDL files.  That seems
to be how we do things with IPDL (unlike with interfaces, which we don't
usually combine).

Whatever you prefer is fine.

>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
>index 1cb8152..b334699 100644
>--- a/dom/ipc/ContentChild.cpp
>+++ b/dom/ipc/ContentChild.cpp

>+#ifdef MOZ_B2G_FM
>+#include "mozilla/dom/fmradio/FMRadioChild.h"
>+#endif
>+
> #ifdef MOZ_WEBSPEECH
> #include "mozilla/dom/PSpeechSynthesisChild.h"
> #endif

>@@ -115,6 +120,7 @@ using namespace base;
> using namespace mozilla;
> using namespace mozilla::docshell;
> using namespace mozilla::dom::bluetooth;
>+using namespace mozilla::dom::fmradio;

Does this compile even ifndef MOZ_B2G_FM?  (The namespace must exist.)

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
>index 32161ff..547d8d4 100644
>--- a/dom/ipc/ContentParent.cpp
>+++ b/dom/ipc/ContentParent.cpp

>@@ -2023,6 +2029,42 @@ ContentParent::RecvPBluetoothConstructor(PBluetoothParent* aActor)

>+PFMRadioParent*
>+ContentParent::AllocPFMRadio()
>+{
>+#ifdef MOZ_B2G_FM
>+    LOG("AllocPFMRadio");
>+    if (!AssertAppProcessPermission(this, "fmradio")) {
>+        return nullptr;
>+    }
>+    return new FMRadioParent();
>+#else
>+     MOZ_NOT_REACHED("No support for FMRadio on this platform!");

MOZ_NOT_REACHED is not well-documented; see bug 820686 where I've been trying
to get the documentation cleaned up for a long time.

The bottom line is: MOZ_NOT_REACHED triggers undefined behavior on some
platforms.  So it definitely shouldn't be used in places that might run.

In this case, I think a child process can cause this code to run in the parent,
and therefore the child process could cause undefined behavior.  That's bad.

We probably don't want MOZ_NOT_REACHED in the corresponding methods in the
child process either; I'd go with MOZ_CRASHED myself.  That's more of a
judgement call, though.

>+     return nullptr;
>+#endif
>+}
>+
>+bool
>+ContentParent::DeallocPFMRadio(PFMRadioParent* aActor)
>+{
>+#ifdef MOZ_B2G_FM
>+    LOG("DeallocPFMRadio");
>+    delete aActor;
>+    return true;
>+#else
>+     MOZ_NOT_REACHED("No support for FMRadio on this platform!");
>+     return false;

See above about MOZ_NOT_REACHED.

Comment 33

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #32)
> Comment on attachment 757397 [details] [diff] [review]
> Part II: IPDL
> 
> >+  /**
> >+   * Sent when the FM HW is enabled/disabled or frequency is changed.
> >+   */
> >+  Notify(FMRadioEventType event);
> 
> Can we have separate NotifyEnabledChanged and NotifyFrequencyChanged methods?
> That is, can we get rid of FMRadioEventType altogether?

Yes

> >diff --git a/dom/fmradio/ipc/FMRadioChild.cpp b/dom/fmradio/ipc/FMRadioChild.cpp
> >new file mode 100644
> >index 0000000..9192a59
> >--- /dev/null
> >+++ b/dom/fmradio/ipc/FMRadioChild.cpp
> 
> FMRadioChild is supposed to be a singleton, right?  Perhaps we could merge it
> with FMRadioChildService, which is also a singleton.
> 

You are right, let me have a try

> >+bool
> >+FMRadioChild::RecvNotify(const FMRadioEventType& aType)
> >+{
> >+  FMRadioChildService::Get()->DistributeEvent(aType);
> 
> It would be helpful to keep the names parallel, so the code is easier to
> follow.  If this function is "Notify" and it simply calls another function,
> it's helpful if that function is also called "Notify", all things being
> equal.
> 
> >+bool
> >+FMRadioChild::Recv__delete__()
> >+{
> >+  return true;
> >+}
> 
> Do you need this one if you're just returning true?  I'm not sure it's
> necessary, but it's fine if it is.

I tried to remove _delete__ from the protocol definition, it is not compiled.

> >+
> >+BEGIN_FMRADIO_NAMESPACE
> >+
> >+class FMRadioChild;
> >+class FMRadioRequestType;
> >+
> >+class FMRadioChildService MOZ_FINAL : public IFMRadioService
> >+{
> >+  friend class FMRadioChild;
> >+
> >+public:
> >+  static FMRadioChildService* Get();
> 
> We have a convention in the DOM (which is not universally respected but is
> fairly prevalent) that methods which start with "Get" can return null, while
> methods which cannot return null don't start with "Get".
> 
> I think we should follow this convention here.  It would mean replacing "Get"
> on this singleton and any others with something else, maybe "Singleton".
> 

Then let me replace it with Singleton.

> >+  /* IFMRadioService interfaces */
> 
> These are methods, not interfaces.  (An interface is a set of methods.)
> 
> >diff --git a/dom/fmradio/ipc/FMRadioChildService.cpp b/dom/fmradio/ipc/FMRadioChildService.cpp
> >new file mode 100644
> >index 0000000..c61e1ab
> >--- /dev/null
> >+++ b/dom/fmradio/ipc/FMRadioChildService.cpp
> 
> >+FMRadioChildService::FMRadioChildService()
> >+  : mEnabled(false)
> >+  , mFrequency(0)
> >+  , mSettings(Settings())
> >+{
> >+  mChildEventObserverList = new FMRadioEventObserverList();
> >+
> >+  mFMRadioChild = new FMRadioChild();
> 
> You can use the initializer list for these two members.
> 
> >+  ContentChild::GetSingleton()->SendPFMRadioConstructor(mFMRadioChild);
> >+}
> >+
> >+FMRadioChildService::~FMRadioChildService()
> >+{
> >+  // The FMRadioChild object will be released in ContentChild::DeallocPFMRadio
> >+  mFMRadioChild = nullptr;
> 
> Since this is a raw pointer, setting it to null in the destructor doesn't do
> anything.  Maybe you meant for it to be an nsRefPtr?  That's probably safer
> anyway.
> 

The FMRadioChild object will be released in ContentChild::DeallocPFMRadio, so I made
it a raw pointer here.

> >+  delete mChildEventObserverList;
> 
> Since this member is deleted when this class is deleted, it should be an
> nsAutoPtr<>.  But actually, I don't see wy mChildEventObserverList needs to
> be
> a pointer at all; can we make it 
> 
>   |FMRadioEventObserverList mChildEventObserverList|?
> 

I see, let me change it.

> >+}
> 
> >+void
> >+FMRadioChildService::Init()
> >+{
> >+  mFMRadioChild->SendIsEnabled(&mEnabled);
> >+  mFMRadioChild->SendGetFrequency(&mFrequency);
> >+  mFMRadioChild->SendGetSettings(&mSettings);
> 
> This is three synchronous calls in a row.  Each sync call is pretty expensive
> in the child and blocks the main thread of the child process.  It would be
> better to combine these three sync calls into one.  Indeed, I don't think you
> need IsEnabled, GetFrequency, and GetSettings except for this Init() call, so
> then we could get rid of them, which would be good.
> 
> Maybe we can shove all of this info into the "settings" object.  Then we
> could 
> 

I think a sync message to get the initial state of FM radio HW is necessory, so let
me shove all these info into one object to reduce the cost.

> In order to ensure that the FMRadioChildService doesn't miss any changes to
> what's going on here, the parent needs to promise to send us notifications of
> any changes which happen starting at some point before it receives the first
> of
> these sync messages.  I /think/ we'll start receiving notifications in the
> parent after it runs RecvPFMRadioConstructor, so I think we're good.  Do you
> think so too?
> 

Yes

> >+void
> >+FMRadioChildService::CancelSeek(ReplyRunnable* aRunnable)
> >+{
> >+  SendRequest(aRunnable, CancelSeekRequest());
> >+}
> >+
> >+void
> >+FMRadioChildService::DistributeEvent(const FMRadioEventType& aType)
> 
> "Distribute" isn't idiomatic.  "BroadcastEvent" or "NotifyEvent" would be a
> better name.
> 

FMRadioEventType will be removed from the IPDL definition, so I think I will
split it into two functions: NotifyEnabledChanged and NotifyFrequencyChanged.

> >+void
> >+FMRadioChildService::UnregisterHandler(FMRadioEventObserver* aHandler)
> >+{
> >+  mChildEventObserverList->RemoveObserver(aHandler);
> >+
> >+  if (mChildEventObserverList->Length() == 0) {
> >+    sFMRadioChildService = nullptr;
> >+    delete this;
> >+  }
> 
> How do we trigger ContentChild::DeallocPFMRadio when we hit this path?  If we
> don't, then mFMRadioChild will live on, which is bad...
> 

I don't think this will trigger ContentChild::DeallocPFMRadio.
As my understanding, ContentChild::DeallocPFMRadio will be executed when the app is closed, 
which also triggers FMRadio::Shutdown and FMRadioChildService::UnregisterHandler.
The FMRadioChild object will be deleted in ContentChild::DeallocPFMRadio, and we just set
mFMRadioChild with nullptr in FMRadioChildServcice::~FMRadioChildService.

> I'm not sure that deleting the FM radio service when we have zero observers
> makes a ton of sense.
> 
> Part of the reason that this idiom concerns me is that if we do something
> like:
> 
>   FMRadioChildService* svc = FMRadioChildService::Get();
>   svc->UnregisterHandler(...);
> 
> At this point, we can't use |svc| anymore; it might have been deleted!  I'm
> afraid that this will be a very easy mistake to make.
> 
> What would happen if instead we kept it FMRadioChildService around until
> shutdown?  It's not using a lot of resources, as far as I can tell.
> 

Did you mean we don't have to release the singleton object of FMRadioChildService?
and how about the singleton object of FMRadioService?

> BTW sFMRadioChildService should probably be a StaticAutoPtr if it's an owning
> pointer.
> 
> >+  return sFMRadioChildService;
> >+}
> 
> >diff --git a/dom/fmradio/ipc/FMRadioParent.cpp b/dom/fmradio/ipc/FMRadioParent.cpp
> >new file mode 100644
> >index 0000000..8b6079a
> >--- /dev/null
> >+++ b/dom/fmradio/ipc/FMRadioParent.cpp
> 
> I guess we can't merge this one into FMRadioService because FMRadioParent
> isn't
> a singleton.
> 
> >+// The pref 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.fmradio.antenna.internal"
> 
> This is never used.
> 

Did you mean we don't have to handle the case of devices that don't use headset as and antenna?

> >diff --git a/dom/fmradio/ipc/FMRadioRequestParent.h b/dom/fmradio/ipc/FMRadioRequestParent.h
> >@@ -0,0 +1,101 @@
> 
> >+class FMRadioRequestParent MOZ_FINAL : public PFMRadioRequestParent
> >+{
> >+public:
> >+  FMRadioRequestParent(const FMRadioRequestType& aRequestType);
> >+  ~FMRadioRequestParent();
> >+
> >+  NS_IMETHOD_(nsrefcnt) AddRef();
> >+  NS_IMETHOD_(nsrefcnt) Release();
> 
> The new hotness is to inherit from mozilla::RefCounted<FMRadioRequestParent,
> AtomicRefCount>.  That takes care of the declaration and implementation and
> AddRef/Release for you.
> 

Cool

> 
> >@@ -115,6 +120,7 @@ using namespace base;
> > using namespace mozilla;
> > using namespace mozilla::docshell;
> > using namespace mozilla::dom::bluetooth;
> >+using namespace mozilla::dom::fmradio;
> 
> Does this compile even ifndef MOZ_B2G_FM?  (The namespace must exist.)

I didn't try it, honestly, I wrote this because the upper line didn't ifdef MOZ_B2G_BT.

Comment 34

[Andrea Marchesini [:baku]]

Comment on attachment 757763 [details] [diff] [review]
Part I: WebIDL V3.1

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

I just proposed a different approach of the internal state machine of the FMRadioService.
Before continuing with the review, can you take a look at it?

::: dom/fmradio/FMRadio.h
@@ +116,5 @@
> +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +
> +    if (!mCanceled) {
> +      CancelableRun();
> +      mFMRadio->RemoveRunnable(this);

RemoveRunnable should not be called also when FireSuccess/FireError is called?
I'm not expert of DOMRequest, but it seems you remove this object only when cancelled. Is it right?

::: dom/fmradio/FMRadioService.cpp
@@ +164,5 @@
> +    if (!aResult.isBoolean()) {
> +      mFMRadioService->mEnableRequest->SetReply(
> +        ErrorResponse(NS_LITERAL_STRING("Unexpected error")));
> +      NS_DispatchToMainThread(mFMRadioService->mEnableRequest);
> +      mFMRadioService->mEnableRequest = nullptr;

mFMRadioService->SetState(mFMRadioService::Disabled);

then:

void
MFMRadioService::SetState(State aState)
{
  mState = aState;
  mPendingRequest = nullptr;
}

@@ +360,5 @@
> +    NS_DispatchToMainThread(aRunnable);
> +    return;
> +  }
> +
> +  if (mDisabling) {

I suspect you don't want to have mDisabling and mEnabling.
Probably you want:

enum {
  Disabled,
  Disabling,
  Enabling,
  Enabled,
  Seeking
};

Otherwise you can have mEnabling = true and mDisabling = true, and this seems buggy.

What about:

FMRadio::Enable(...)
{
switch (mState) {
  case Enabled:
    aRunnable->Setreply(ErrorResponse(NS_LITERAL_STRING("It's enabled")));
    NS_DispatchToMainThread(aRunnable);
    return;

  case Enabling:
    aRunnable->Setreply(ErrorResponse(NS_LITERAL_STRING("It's enabling")));
    NS_DispatchToMainThread(aRunnable);
    return;

  case Disabling:
    aRunnable->Setreply(ErrorResponse(NS_LITERAL_STRING("It's disabling")));
    NS_DispatchToMainThread(aRunnable);
    return;

  default:
    break;
}

mState = Enabling;
...

::: dom/fmradio/FMRadioService.h
@@ +178,5 @@
> +private:
> +  bool mEnabled;
> +  int32_t mFrequencyInKHz;
> +  /* Indicates if the FM radio is currently being disabled */
> +  bool mDisabling;

Get rid of mDisabling and mEnabling.

@@ +193,5 @@
> +  double mChannelWidthInMHz;
> +
> +  nsCOMPtr<nsIObserver> mSettingsObserver;
> +
> +  nsRefPtr<ReplyRunnable> mDisableRequest;

I don't like the idea that we can have a DisableRequest and a EnableRequest pending at the same time.
What about if you convert it to a nsRefPtr<ReplyRunnable> mPendingRequest;
and when you want to replace it you do:

if (mPendingRequest) {
  mPendingRequest->SetReply(...);
}

mPendingRequest = newRequest;

@@ +195,5 @@
> +  nsCOMPtr<nsIObserver> mSettingsObserver;
> +
> +  nsRefPtr<ReplyRunnable> mDisableRequest;
> +  nsRefPtr<ReplyRunnable> mEnableRequest;
> +  nsRefPtr<ReplyRunnable> mSeekRequest;

Also the SeekRequest is mutual exclusive. It should be handled by the mPendingRequest.

@@ +197,5 @@
> +  nsRefPtr<ReplyRunnable> mDisableRequest;
> +  nsRefPtr<ReplyRunnable> mEnableRequest;
> +  nsRefPtr<ReplyRunnable> mSeekRequest;
> +
> +  FMRadioEventObserverList* mObserverList;

I would like to see it as a nsRefPtr<FMRadioEventObserverList>

Comment 35

[Pin Zhang [:pzhang] (inactive)]

(In reply to Andrea Marchesini (:baku) from comment #34)
> Comment on attachment 757763 [details] [diff] [review]
> Part I: WebIDL V3.1
> 
> Review of attachment 757763 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I just proposed a different approach of the internal state machine of the
> FMRadioService.
> Before continuing with the review, can you take a look at it?
> 
> ::: dom/fmradio/FMRadio.h
> @@ +116,5 @@
> > +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> > +
> > +    if (!mCanceled) {
> > +      CancelableRun();
> > +      mFMRadio->RemoveRunnable(this);
> 
> RemoveRunnable should not be called also when FireSuccess/FireError is
> called?
> I'm not expert of DOMRequest, but it seems you remove this object only when
> cancelled. Is it right?
> 

If it's canceled which means the FMRadio::Shutdown() is called, it will be cleared
when the mFMRadio is released.

> ::: dom/fmradio/FMRadioService.cpp
> @@ +164,5 @@
> > +    if (!aResult.isBoolean()) {
> > +      mFMRadioService->mEnableRequest->SetReply(
> > +        ErrorResponse(NS_LITERAL_STRING("Unexpected error")));
> > +      NS_DispatchToMainThread(mFMRadioService->mEnableRequest);
> > +      mFMRadioService->mEnableRequest = nullptr;
> 
> mFMRadioService->SetState(mFMRadioService::Disabled);
> 
> then:
> 
> void
> MFMRadioService::SetState(State aState)
> {
>   mState = aState;
>   mPendingRequest = nullptr;
> }
> 
> @@ +360,5 @@
> > +    NS_DispatchToMainThread(aRunnable);
> > +    return;
> > +  }
> > +
> > +  if (mDisabling) {
> 
> I suspect you don't want to have mDisabling and mEnabling.
> Probably you want:
> 
> enum {
>   Disabled,
>   Disabling,
>   Enabling,
>   Enabled,
>   Seeking
> };
> 
> Otherwise you can have mEnabling = true and mDisabling = true, and this
> seems buggy.
> 
> What about:
> 
> FMRadio::Enable(...)
> {
> switch (mState) {
>   case Enabled:
>     aRunnable->Setreply(ErrorResponse(NS_LITERAL_STRING("It's enabled")));
>     NS_DispatchToMainThread(aRunnable);
>     return;
> 
>   case Enabling:
>     aRunnable->Setreply(ErrorResponse(NS_LITERAL_STRING("It's enabling")));
>     NS_DispatchToMainThread(aRunnable);
>     return;
> 
>   case Disabling:
>     aRunnable->Setreply(ErrorResponse(NS_LITERAL_STRING("It's disabling")));
>     NS_DispatchToMainThread(aRunnable);
>     return;
> 
>   default:
>     break;
> }
> 
> mState = Enabling;
> ...
> 
> ::: dom/fmradio/FMRadioService.h
> @@ +178,5 @@
> > +private:
> > +  bool mEnabled;
> > +  int32_t mFrequencyInKHz;
> > +  /* Indicates if the FM radio is currently being disabled */
> > +  bool mDisabling;
> 
> Get rid of mDisabling and mEnabling.
> 
> @@ +193,5 @@
> > +  double mChannelWidthInMHz;
> > +
> > +  nsCOMPtr<nsIObserver> mSettingsObserver;
> > +
> > +  nsRefPtr<ReplyRunnable> mDisableRequest;
> 
> I don't like the idea that we can have a DisableRequest and a EnableRequest
> pending at the same time.
> What about if you convert it to a nsRefPtr<ReplyRunnable> mPendingRequest;
> and when you want to replace it you do:
> 
> if (mPendingRequest) {
>   mPendingRequest->SetReply(...);
> }
> 
> mPendingRequest = newRequest;
> 
> @@ +195,5 @@
> > +  nsCOMPtr<nsIObserver> mSettingsObserver;
> > +
> > +  nsRefPtr<ReplyRunnable> mDisableRequest;
> > +  nsRefPtr<ReplyRunnable> mEnableRequest;
> > +  nsRefPtr<ReplyRunnable> mSeekRequest;
> 
> Also the SeekRequest is mutual exclusive. It should be handled by the
> mPendingRequest.
> 

Because enabling FM radio HW takes several seconds, if we want to disable it when
it's being enabled, we can't stop it immediately (not supported in Hal layer), we
have to wait for FM_RADIO_OPERATION_ENABLE signal from Hal and apply the disabling
action then, so physically, it could be disabling and enabling in the meantime.

Your proposal is reasonable and will be more clearer if it handles such a case well,
so let me have a try, thanks.

Comment 36

[StevenLee[:slee]]

(In reply to Pin Zhang [:pzhang] from comment #30)
> Created attachment 757763 [details] [diff] [review]
> Part I: WebIDL V3.1
> 
> I found that we will receive FM_RADIO_OPERATION_ENABLE when we called
> DisableFMRadio(), there must be some problem in the Hal layer, so I double
> checked IsFMRadioOn() in the Observe() function.
Can you tell me how to reproduce it and could you reproduce it every time? 
Thanks.

Comment 37

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 757763 [details] [diff] [review]
Part I: WebIDL V3.1

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

::: dom/fmradio/FMRadioService.cpp
@@ +577,5 @@
> +      if (!IsFMRadioOn())
> +      {
> +        LOG("FMRadio should not be off!!");
> +        return;
> +      }

Steven, every time I turn off the FMRadio, the log in the upper is printed. 

I don't think you have to apply my patch to check the issue, you can add some
log to the funtion in dom/fm/FMRadio.cpp:
  void FMRadio::Notify(const FMRadioOperationInformation& info)

BTW, I tested it on Unagi.

Comment 38

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #32)
> Comment on attachment 757397 [details] [diff] [review]
> Part II: IPDL
>
> >diff --git a/dom/fmradio/ipc/ipdl.mk b/dom/fmradio/ipc/ipdl.mk
> >new file mode 100644
> >index 0000000..fd304f8
> >--- /dev/null
> >+++ b/dom/fmradio/ipc/ipdl.mk
> 
> >+IPDLSRCS = \
> >+  PFMRadio.ipdl \
> >+  PFMRadioRequest.ipdl \
> >+  $(NULL)
> 
> I wouldn't complain if you wanted to combine these two IPDL files.  That
> seems
> to be how we do things with IPDL (unlike with interfaces, which we don't
> usually combine).
> 
> Whatever you prefer is fine.
> 

I got such a compling error:
  dom/fmradio/ipc/PFMRadio.ipdl:10: error: only one protocol definition per file

Comment 39

[StevenLee[:slee]]

(In reply to Pin Zhang [:pzhang] from comment #37)
> Steven, every time I turn off the FMRadio, the log in the upper is printed. 
> 
> I don't think you have to apply my patch to check the issue, you can add some
> log to the funtion in dom/fm/FMRadio.cpp:
>   void FMRadio::Notify(const FMRadioOperationInformation& info)
> 
> BTW, I tested it on Unagi.
Hi Pin,

I tested and found that the internal thread in hal gets TAVARUA_EVT_RADIO_READY when we're trying to disable FM radio. I will file a bug to solve it.

Comment 40

[Pin Zhang [:pzhang] (inactive)]

(In reply to StevenLee from comment #39)
> (In reply to Pin Zhang [:pzhang] from comment #37)
> > Steven, every time I turn off the FMRadio, the log in the upper is printed. 
> > 
> > I don't think you have to apply my patch to check the issue, you can add some
> > log to the funtion in dom/fm/FMRadio.cpp:
> >   void FMRadio::Notify(const FMRadioOperationInformation& info)
> > 
> > BTW, I tested it on Unagi.
> Hi Pin,
> 
> I tested and found that the internal thread in hal gets
> TAVARUA_EVT_RADIO_READY when we're trying to disable FM radio. I will file a
> bug to solve it.

Steven, thanks!

Comment 41

[Justin Lebar (not reading bugmail)]

> I got such a compling error:
>   dom/fmradio/ipc/PFMRadio.ipdl:10: error: only one protocol definition per file

Huh.  I guess that settles that, then!

Comment 42

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL V4

Comment 43

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part II: IPDL V2

Comment 44

[Pin Zhang [:pzhang] (inactive)]

Fixed issues addressed in comment 32 and comment 34.

Comment 45

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL V4.1

ifdef MOZ_B2G_FM for FMRadio.webidl in WebIDL.mk

Comment 46

[Justin Lebar (not reading bugmail)]

From the WebIDL patch:

>+class FMRadioRequest MOZ_FINAL : public ReplyRunnable
>+                               , public DOMRequest
>+{
>+public:
>+  NS_DECL_ISUPPORTS_INHERITED
>+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(FMRadioRequest, DOMRequest)
>+
>+  FMRadioRequest(nsPIDOMWindow* aWindow, FMRadio* aFMRadio)
>+    : DOMRequest(aWindow)
>+    , mFMRadio(aFMRadio)
>+    , mCanceled(false)
>+  {
>+    mFMRadio->AddRunnable(this);
>+  }
>+
>+  ~FMRadioRequest() { }
>+
>+  NS_IMETHOD Run()
>+  {
>+    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>+
>+    if (!mCanceled) {
>+      CancelableRun();
>+      mFMRadio->RemoveRunnable(this);
>+    }
>+
>+    return NS_OK;
>+  }
>+
>+  nsresult CancelableRun()

Like I said in my earlier review, "CancelableRun" doesn't make sense as a name
here.

>+  {
>+    switch (mResponseType.type()) {
>+      case FMRadioResponseType::TErrorResponse:
>+      {
>+        const ErrorResponse& response = mResponseType.get_ErrorResponse();
>+        FireError(response.error());
>+        break;
>+      }
>+      case FMRadioResponseType::TSuccessResponse:
>+        FireSuccess(JS::Rooted<JS::Value>(AutoJSContext(), JSVAL_VOID));
>+        break;
>+      default:
>+        NS_RUNTIMEABORT("not reached");
>+        break;
>+    }
>+    return NS_OK;
>+  }
>+
>+  NS_IMETHOD Cancel()
>+  {
>+    mCanceled = true;
>+    return NS_OK;

Don't we want to do mFMRadio->RemoveRunnable() here?

>+  }
>+
>+private:
>+  nsRefPtr<FMRadio> mFMRadio;
>+  bool mCanceled;
>+};

This whole bit where FMRadio keeps a list of runnables is not ideal, because
removing a runnable from this list is O(n) in the size of the list, and
webpages can enqueue arbitrarily-many runnables.

It seems that the only reason we have the list in the first place is so that we
can cancel all runnables when the FM radio shuts down.  But if this is all we 
want to do, we can accomplish this much more efficiently:

  * Make mFMRadio above a nsWeakPtr.
  * When we run the runnable, check if the nsWeakPtr is null.  If so, do
    nothing.
  * If the weak ptr is not null, check whether the FM radio has had
    Shutdown() called on it.
  * FireSuccess / FireError only if Shutdown hasn't been called.

See https://developer.mozilla.org/en-US/docs/Weak_reference for help on using
weak refs.

This is also a good change to make because by removing cyclic strong
references, we make it less likely that we'll accidentally leak memory.

>diff --git a/dom/fmradio/ipc/PFMRadio.ipdl b/dom/fmradio/ipc/PFMRadio.ipdl
>new file mode 100644
>index 0000000..88c1eeb
>--- /dev/null
>+++ b/dom/fmradio/ipc/PFMRadio.ipdl

>+union FMRadioRequestArgs
>+{
>+  EnableRequest;
>+  DisableRequest;
>+  SetFrequencyRequest;
>+  SeekRequest;
>+  CancelSeekRequest;
>+};

Sorry to make you do more renaming, but now that I see this, I think these
should be called EnableRequestArgs, DisableRequestArgs, and so on.

It's a little weird that not all of these guys actually have arguments, but the
majority of them do, and then it's parallel with "FMRadioRequestArgs".

>diff --git a/dom/fmradio/ipc/FMRadioChildService.h b/dom/fmradio/ipc/FMRadioChildService.h
>new file mode 100644
>index 0000000..d9fdae3
>--- /dev/null
>+++ b/dom/fmradio/ipc/FMRadioChildService.h

>+class FMRadioChildService MOZ_FINAL : public IFMRadioService
>+                                    , public PFMRadioChild

A comment explaining what this class does would be helpful.  It doesn't have to
be long.

Right now we have

  PFMRadio
  FMRadioChildService : PFMRadioChild
  FMRadioParent : PFMRadioParent

This doesn't match our convention that PFoo is implemented by classes FooChild
and FooParent.  This is a pretty important convention when you have lots of
IPC classes to keep track of.

I'd probably rename everything to PFMradio / FMRadioChild / FMRadioParent and
make sure that the comment on FMRadioChild indicates that it's a singleton.
You could also mention that FMRadioChild corresponds to FMRadioParent plus
FMRadioService in the parent.

>+  friend class FMRadioChild;

Does this class still exist?  I don't see it in either patch.

>+public:
>+  static FMRadioChildService* Singleton();
>+
>+  void SendRequest(ReplyRunnable* aReplyRunnable, FMRadioRequestArgs aArgs);

I don't think you can forward-declare FMRadioRequestArgs and then pass it by
value.  If this compiles, then the forward-declaration above is probably not
necessary.

>+  virtual void NotifyFrequencyChanged(double aFrequency) MOZ_OVERRIDE;
>+  virtual void NotifyEnabledChanged(bool aEnabled,
>+                                    double aFrequency) MOZ_OVERRIDE;

Why are these two methods on IFMRadioService?  I don't see a reason that they
should be public.

If we remove these methods from the interface and they're only called from one
place, I think), it may be appropriate to inline the implementation into the
caller.

>diff --git a/dom/fmradio/ipc/FMRadioChildService.cpp b/dom/fmradio/ipc/FMRadioChildService.cpp
>new file mode 100644
>index 0000000..b7d154b
>--- /dev/null
>+++ b/dom/fmradio/ipc/FMRadioChildService.cpp

>+#undef LOG
>+#define LOG(args...) FM_LOG("PFMRadioChildService", args)

Do you use this?

>+BEGIN_FMRADIO_NAMESPACE
>+
>+FMRadioChildService*
>+FMRadioChildService::sFMRadioChildService;

Please make this a StaticAutoPtr, to emphasize that this is an owning pointer.

>+FMRadioChildService::FMRadioChildService()
>+  : mEnabled(false)
>+  , mFrequency(0)
>+  , mObserverList(FMRadioEventObserverList())

This line doesn't make much sense to me; why do we need to make mObserverList
be a copy of an empty FMRadioEventObserverList?  I think we can just remove the
line altogether and use the default constructor for mObserverList.

>+void
>+FMRadioChildService::NotifyFrequencyChanged(double aFrequency)
>+{
>+  mFrequency = aFrequency;
>+  mObserverList.Broadcast(FMRadioEventArgs(FrequencyChanged,
>+                                           mEnabled,
>+                                           mFrequency));
>+}
>+
>+void
>+FMRadioChildService::NotifyEnabledChanged(bool aEnabled, double aFrequency)
>+{
>+  mEnabled = aEnabled;
>+  mFrequency = aFrequency;
>+  mObserverList.Broadcast(FMRadioEventArgs(EnabledChanged,
>+                                           aEnabled,
>+                                           aFrequency));
>+}

It's pretty confusing that we always send both mEnabled and mFrequency, even
when we're e.g. disabling the radio.

Maybe it would be simpler to broadcast only the enum value (i.e.
FrequencyChanged / EnabledChanged) and rely on the observer to call
GetFrequency / IsEnabled.

>+PFMRadioRequestChild*
>+FMRadioChildService::AllocPFMRadioRequest(const FMRadioRequestArgs& aArgs)
>+{
>+  MOZ_NOT_REACHED("Caller is supposed to manually construct a request");
>+  return nullptr;
>+}

Similarly to the other MOZ_NOT_REACHED(), let's make this a MOZ_CRASH() or
NS_RUNTIMEABORT().

>+// static
>+FMRadioChildService*
>+FMRadioChildService::Singleton()
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+
>+  if (sFMRadioChildService) {
>+    return sFMRadioChildService;
>+  }
>+
>+  sFMRadioChildService = new FMRadioChildService();
>+
>+  return sFMRadioChildService;

Nit: This could be a line or two shorter if it was:

    if (!sFMRadioChildService) {
      sFMRadioChildService = new FMRadioChildService();
    }

    return sFMRadioChildService;

>diff --git a/dom/fmradio/ipc/FMRadioParent.h b/dom/fmradio/ipc/FMRadioParent.h
>new file mode 100644
>index 0000000..cfecd5d
>--- /dev/null
>+++ b/dom/fmradio/ipc/FMRadioParent.h

>+class FMRadioParent MOZ_FINAL : public PFMRadioParent
>+                              , public FMRadioEventObserver

>+  /* Observer Interface */

It's probably better to say "FMRadioEventObserver", so there's no confusion
with nsIObserver, mozilla::Observer, and any other thing called "observer".

>+  virtual void Notify(const FMRadioEventArgs& aArgs) MOZ_OVERRIDE;
>+};

>diff --git a/dom/fmradio/ipc/FMRadioParent.cpp b/dom/fmradio/ipc/FMRadioParent.cpp
>new file mode 100644
>index 0000000..16f18a9
>--- /dev/null
>+++ b/dom/fmradio/ipc/FMRadioParent.cpp

>+#undef LOG
>+#define LOG(args...) FM_LOG("PFMRadioParent", args)

It doesn't look like you use this.

>+// The pref 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.fmradio.antenna.internal"

It looks like you use this in FMRadio.cpp, but not in this file.  Can you
remove this define?

>+void
>+FMRadioParent::Notify(const FMRadioEventArgs& aArgs)
>+{
>+  switch (aArgs.type) {
>+    case FrequencyChanged:
>+      unused << this->SendNotifyFrequencyChanged(aArgs.frequency);
>+      break;
>+    case EnabledChanged:
>+      unused << this->SendNotifyEnabledChanged(aArgs.enabled, aArgs.frequency);

What frequency do we send when the radio is disabled?  Is it valid to use that
frequency for anything?

Maybe we should have one method for enabling (which takes a frequency) and a
separate one method for disabling (which doesn't take a frequency).

>diff --git a/dom/fmradio/ipc/FMRadioRequestChild.cpp b/dom/fmradio/ipc/FMRadioRequestChild.cpp
>new file mode 100644
>index 0000000..b448594
>--- /dev/null
>+++ b/dom/fmradio/ipc/FMRadioRequestChild.cpp

>+void
>+FMRadioRequestChild::ActorDestroy(ActorDestroyReason aWhy)
>+{
>+  if (!mReplyRunnable) {
>+    return;
>+  }
>+
>+  // If mReplyRunnable is not nullptr, it means we didn't receive __delete__
>+  // message normally, and we need cancel and dispatch it to make sure
>+  // it removes itself from FMRadio::mRunnables.
>+  mReplyRunnable->Cancel();
>+  NS_DispatchToMainThread(mReplyRunnable);
>+}

If you cancel an nsCancelableRunnable and then dispatch it, it does nothing
when run, so I don't think this is right.

>diff --git a/dom/fmradio/ipc/FMRadioRequestParent.cpp b/dom/fmradio/ipc/FMRadioRequestParent.cpp
>new file mode 100644
>index 0000000..c6bf4f1
>--- /dev/null
>+++ b/dom/fmradio/ipc/FMRadioRequestParent.cpp

>+#undef LOG
>+#define LOG(args...) FM_LOG("PFMRadioRequestParent", args)

It doesn't look like we use this.

>+nsresult
>+FMRadioRequestParent::Run()
>+{
>+  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

This should be MOZ_ASSERT.  NS_ASSERTION is a pretty wimpy assertion; it just
prints a message but doesn't crash the program.  This is an important assertion
that should never be wrong.

>+  nsresult rv = NS_OK;
>+
>+  if (!mActorDestroyed) {
>+    unused << this->Send__delete__(this, mResponseType);
>+  }
>+
>+  return rv;

Just return NS_OK, without declaring |nsresult rv| above.

>+}

>diff --git a/dom/fmradio/ipc/Makefile.in b/dom/fmradio/ipc/Makefile.in
>new file mode 100644
>index 0000000..b1a1c41
>--- /dev/null
>+++ b/dom/fmradio/ipc/Makefile.in

We should have a build peer check your Makefile changes (I've been burned
before).  Can you please flag someone for review?

>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
>index 1cb8152..857c406 100644
>--- a/dom/ipc/ContentChild.cpp
>+++ b/dom/ipc/ContentChild.cpp

>+#undef LOG
>+#if defined(MOZ_WIDGET_GONK)
>+#include <android/log.h>
>+#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "FMRadio ContentChild" , ## args)
>+#else
>+#define LOG(args...)
>+#endif

We shouldn't call this "LOG" if it's in ContentChild but is just for the FM
radio.  FM_LOG or something would work.

>+PFMRadioChild*
>+ContentChild::AllocPFMRadio()
>+{
>+#ifdef MOZ_B2G_FM
>+    LOG("AllocPFMRadio");
>+    MOZ_NOT_REACHED("No one should be allocating PFMRadioChild actors");
>+    return nullptr;
>+#else
>+    MOZ_NOT_REACHED("No support for FMRadio on this platform!");
>+    return nullptr;
>+#endif
>+}

This is doing the same thing whether or not we have MOZ_B2G_FM.  It'd be
cleaner if we got rid of the ifdefs.  We don't need the log message here; we're
about to crash very loudly anyway.  (Well, we /want/ to crash; MOZ_NOT_REACHED
is actually undefined behavior.  See below.)

>+bool
>+ContentChild::DeallocPFMRadio(PFMRadioChild* aActor)
>+{
>+#ifdef MOZ_B2G_FM
>+    LOG("DeallocPFMRadio");
>+    delete aActor;
>+    return true;
>+#else
>+    MOZ_NOT_REACHED("No support for FMRadio on this platform!");
>+    return false;
>+#endif
>+}

Similarly, I don't feel like this log message is particularly useful.  But if
you like it, maybe we can move it into the FMRadioChild destructor?

The more I look at this, the more I'm convinced these MOZ_NOT_REACHEDs should
be MOZ_CRASH()'s or NS_RUNTIMEABORT()'s.  MOZ_NOT_REACHED is undefined
behavior, and we should strive to behave in a defined way when these functions
are called.

(The bluetooth code above does it wrong, but that's just because the author +
reviewer were not as enlightned as we are.  :)

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
>index 32161ff..f901f13 100644
>--- a/dom/ipc/ContentParent.cpp
>+++ b/dom/ipc/ContentParent.cpp

> #endif
> }
> 
>+#undef LOG
>+#if defined(MOZ_WIDGET_GONK)
>+#include <android/log.h>
>+#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "FMRadio ContentParent" , ## args)
>+#else
>+#define LOG(args...)
>+#endif

Same bit about the LOG marco here; if we don't get rid of it, let's call it
FM_LOG or something.

>+PFMRadioParent*
>+ContentParent::AllocPFMRadio()
>+{
>+#ifdef MOZ_B2G_FM
>+    LOG("AllocPFMRadio");
>+    if (!AssertAppProcessPermission(this, "fmradio")) {
>+        return nullptr;
>+    }
>+    return new FMRadioParent();
>+#else
>+    MOZ_CRASH();
>+    return nullptr;

This return is never reached; you /should/ be able to remove it, since
MOZ_CRASH() is noreturn.  If you can't, don't worry about it.

>+#endif
>+}
>+
>+bool
>+ContentParent::DeallocPFMRadio(PFMRadioParent* aActor)
>+{
>+#ifdef MOZ_B2G_FM
>+    LOG("DeallocPFMRadio");
>+    delete aActor;
>+    return true;
>+#else
>+    MOZ_CRASH();
>+    return false;

Similarly for this return.

>+#endif

Comment 47

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part II: IPDL V3

Fixed the issues addressed in comment 46.

@khuey, could you help checking the Makefile changes or reassign to someone else to take a look?

Comment 48

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL V5

Comment 49

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

Comment on attachment 760177 [details] [diff] [review]
Part I: WebIDL V5

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

Switching directory names from dom/fm to dom/fmradio seems unnecessary, but it doesn't hurt anything.

I just skimmed the non-build parts.  I can review them in detail if desired.

::: dom/fmradio/FMRadio.h
@@ +87,5 @@
> +  bool mIsShutdown;
> +};
> +
> +class FMRadioRequest MOZ_FINAL : public ReplyRunnable
> +                               , public DOMRequest

I'm not thrilled about having DOM objects also be runnables.  Is it possible to separate the two objects?  If you do that you may not even need to subclass DOMRequest.

@@ +91,5 @@
> +                               , public DOMRequest
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(FMRadioRequest, DOMRequest)

This isn't actually needed, since you don't have anything in the traverse/unlink hooks.

Comment 50

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

Comment on attachment 760176 [details] [diff] [review]
Part II: IPDL V3

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

I didn't look at the IPDL at all but I can if desired.

r=me on the build parts

Comment 51

[Justin Lebar (not reading bugmail)]

> I'm not thrilled about having DOM objects also be runnables.

Yeah, I've been thinking about this as well.  It's certainly non-idiomatic; we usually would use a callback function.  But maybe this is somehow different.  I reserve judgement until I have a chance to digest part I.

Comment 52

[Andrea Marchesini [:baku]]

Comment on attachment 760177 [details] [diff] [review]
Part I: WebIDL V5

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

::: dom/fmradio/FMRadioService.cpp
@@ +677,5 @@
> +    case FM_RADIO_OPERATION_DISABLE:
> +      // The signal we received might be triggered by disable request in other
> +      // process, we should check if `mState` is Disabling, if false, we should
> +      // skip it and just update the power state.
> +      if (mState == Disabling) {

Is it possible that FM_RADIO_OPERATION_DISABLE is called for other reasons?
I'm wondering if we should have a way to check the 'type' of pending request.

Comment 53

[Andrea Marchesini [:baku]]

Can you rebase your patch on top of mozilla-b2g18? I would like to write the AudioChannelAgent patch.

Comment 54

[Pin Zhang [:pzhang] (inactive)]

(In reply to Andrea Marchesini (:baku) from comment #53)
> Can you rebase your patch on top of mozilla-b2g18? I would like to write the
> AudioChannelAgent patch.

Hi Baku, we are preparing for MWC Asia[1], I am afraid I have no time to do this in this and next week, sorry for that, and I will finish it ASAP, thanks.

[1] http://www.mobileasiaexpo.com/

Comment 55

[Pin Zhang [:pzhang] (inactive)]

(In reply to Pin Zhang [:pzhang] from comment #54)
> (In reply to Andrea Marchesini (:baku) from comment #53)
> > Can you rebase your patch on top of mozilla-b2g18? I would like to write the
> > AudioChannelAgent patch.
> 
> Hi Baku, we are preparing for MWC Asia[1], I am afraid I have no time to do
> this in this and next week, sorry for that, and I will finish it ASAP,
> thanks.
> 
> [1] http://www.mobileasiaexpo.com/

I will probably finish it at around 30th June, if there is any problem, please let me know.

Comment 56

[Pin Zhang [:pzhang] (inactive)]

Attachment: Patch based on b2g18

Hi Baku, this is the patch rebased on b2g18, please check if it's OK for you to write the AudioChannelAgent patch.

Comment 57

[Justin Lebar (not reading bugmail)]

I hear that MWC Asia is about done now, so I'll try to have a look at these patches in the next day or so.  I hope I haven't inconvenienced you by waiting to look at these patches until you got back!  (I should have said that was what I was doing; sorry.)

Comment 58

[Justin Lebar (not reading bugmail)]

Comment on attachment 760177 [details] [diff] [review]
Part I: WebIDL V5

When attaching patches, please use eight lines of context (-U8).  (Also -M -C.)

If you use git bz, it will do this for you.

https://github.com/jlebar/git-bz-moz

No need to re-attach this patch; I can create the diff locally.

Comment 59

[Justin Lebar (not reading bugmail)]

>+class RilSettingsObserver MOZ_FINAL : public nsIObserver
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+
>+  NS_IMETHODIMP
>+  Observe(nsISupports * aSubject, const char * aTopic, const PRUnichar * aData)
>+  {
>+    MOZ_ASSERT(NS_IsMainThread());
>+    MOZ_ASSERT(sFMRadioService);
>+
>+    if (!strcmp(aTopic, MOZSETTINGS_CHANGED_ID)) {
>+      // The string that we're interested in will be a JSON string looks like:
>+      //  {"key":"ril.radio.disabled","value":true}
>+      mozilla::AutoSafeJSContext cx;
>+      nsDependentString dataStr(aData);
>+      JS::Rooted<JS::Value> val(cx);
>+      if (!JS_ParseJSON(cx, dataStr.get(), dataStr.Length(), &val) ||
>+          !val.isObject()) {
>+        return NS_OK;
>+      }
>+
>+      JS::Rooted<JSObject*> obj(cx, &val.toObject());
>+      JS::Rooted<JS::Value> key(cx);
>+      if (!JS_GetProperty(cx, obj, "key", key.address()) ||
>+          !key.isString()) {
>+        return NS_OK;
>+      }
>+
>+      JS::Rooted<JSString*> jsKey(cx, key.toString());
>+      nsDependentJSString keyStr;
>+      if (!keyStr.init(cx, jsKey)) {
>+        return NS_OK;
>+      }
>+
>+      JS::Rooted<JS::Value> value(cx);
>+      if (!JS_GetProperty(cx, obj, "value", value.address())) {
>+        return NS_OK;
>+      }
>+
>+      FMRadioService* fmRadioService =
>+        static_cast<FMRadioService*>(FMRadioService::Singleton());
>+
>+      if (keyStr.EqualsLiteral(SETTING_KEY_RIL_RADIO_DISABLED)) {
>+        if (!value.isBoolean()) {
>+          return NS_OK;
>+        }
>+
>+        fmRadioService->mRilDisabled = value.toBoolean();
>+
>+        // Disable the FM radio HW if Airplane mode is enabled.
>+        if (fmRadioService->mRilDisabled) {
>+          fmRadioService->Disable(nullptr);
>+        }
>+
>+        return NS_OK;
>+      }
>+    }
>+
>+    return NS_OK;
>+  }
>+};

Wow, that's pretty gross.

I'm not qualified to review jsapi.  khuey, can you sign off on this method?

Comment 60

[Justin Lebar (not reading bugmail)]

> Wow, that's pretty gross.

Sorry, didn't mean to suggest that this code is bad; it's just gross that you have to do it this way.  :)

Comment 61

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

I'm not qualified to review this either but smaug claims to be!

Comment 62

[Justin Lebar (not reading bugmail)]

Comment on attachment 760177 [details] [diff] [review]
Part I: WebIDL V5

This is a very good patch, but I have enough large concerns (scattered in with
the usual sampling of nits) that I'd like to have another look.

>diff --git a/dom/webidl/FMRadio.webidl b/dom/webidl/FMRadio.webidl
>index e69de29..a8fbca8 100644
>--- a/dom/webidl/FMRadio.webidl
>+++ b/dom/webidl/FMRadio.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.
>+   */

It seems like you're using one linebreak as a way to indicate that what follows
is not totally related to what came before, but not unrelated enough to warrant
a new paragraph (which is signified by a blank line).

This is an uncommon pattern in English.  Instead, please get rid of the newline
in the paragraph above, or add a blank line, so the two sentences make up two
paragraphs.

>+  readonly attribute double? frequency;
>+
>+  /* 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.

This doesn't really explain what's going on.  Maybe something like:

     The difference in frequency between two "adjacent" channels, in MHz.  That
     is, any two radio channels' frequencies differ by at least channelWidth
     MHz.

>+  /**
>+   * Power the FM radio off.
>+   * The disabled event will be fired if this request completes successfully.

No newline, or add a paragraph break, per above.

>+   */
>+  DOMRequest disable();
>+
>+  /**
>+   * Power the FM radio on, and tune 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.

Same.

>+   */
>+  DOMRequest enable(double frequency);
>+
>+  /**
>+   * Tune the FM radio to the given frequency.
>+   * This will fail if the given frequency is out of range.

Same.

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

No newline after the colon.

>+   */
>+  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.

No newline after the colon.

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

No newline, or separate into paragraphs.

In the rest of this IDL file you're very careful to say that we fire the "foo"
event, not the "onfoo" event.  We should do the same here.

>+   */
>+  DOMRequest cancelSeek();
>+};

I know I'm picky about the comments, but these are very good.  :)

>diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h
>index fb77156..cbad269 100644
>--- a/content/base/src/nsGkAtomList.h
>+++ b/content/base/src/nsGkAtomList.h

>@@ -634,16 +634,17 @@ GK_ATOM(omitXmlDeclaration, "omit-xml-declaration")
>+GK_ATOM(onantennaavailablechange, "onantennaavailablechange")

>@@ -704,16 +705,17 @@ GK_ATOM(ondraggesture, "ondraggesture")
>+GK_ATOM(onfrequencychange, "onfrequencychange")

>@@ -773,18 +775,16 @@ GK_ATOM(onscroll, "onscroll")
>-GK_ATOM(onantennastatechange, "onantennastatechange")
>-GK_ATOM(onseekcomplete, "onseekcomplete")

If we're changing the name of these events, we need to fix Gaia as well, right?

>diff --git a/dom/base/Navigator.cpp b/dom/base/Navigator.cpp
>index 2928e31..123d199 100644
>--- a/dom/base/Navigator.cpp
>+++ b/dom/base/Navigator.cpp

>+NS_IMETHODIMP
>+Navigator::GetMozFMRadio(nsISupports** aFMRadio)
>+{
>+  *aFMRadio = nullptr;
>+
>+  if (!mFMRadio) {
>+    nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow));
>+    NS_ENSURE_TRUE(win && win->GetDocShell(), NS_OK);
>+
>+    mFMRadio = fmradio::FMRadio::CheckPermissionAndCreateInstance(win);
>+    NS_ENSURE_TRUE(mFMRadio, NS_OK);
>+  }
>+
>+  NS_ADDREF(*aFMRadio = static_cast<nsIDOMEventTarget*>(mFMRadio));
>+
>+  return NS_OK;
>+}

Nit: We prefer |using namespace foo| to |foo::Foo|.  If you don't want to make
Navigator.cpp use the fmradio namespace, you could add a |using namespace
mozilla::fmradio| to the top of the GetMozFMRadio() function.

This function is correct but a bit scary, since if we run NS_ADDREF without
first ensuring that mFMRadio is non-null, we'll crash.  (The alternative that
doesn't crash is NS_IF_ADDREF.)

The raw addref/release macros are pretty easy to screw up, so in new code we
prefer to use nsRefPtr and nsCOMPtr to the same effect.  Something like:

  *aFMRadio = nullptr;
  if (!mFMRadio) {
    nsCOMPtr<nsPIDOMWindow> win(...);
    NS_ENSURE_TRUE(win && ...);
    mFMRadio = fmRadio::FMRadio::CheckPermissionAndCreateInstance(win);
  }

  nsRefPtr<FMRadio> radio = mFMRadio;
  radio.forget(aFMRadio);
  return NS_OK;
  
This isn't any more lines than the above, but it's also harder to screw up.

>diff --git a/dom/fmradio/FMRadio.h b/dom/fmradio/FMRadio.h
>--- a/dom/fmradio/FMRadio.h
>+++ b/dom/fmradio/FMRadio.h

>+class FMRadioRequest MOZ_FINAL : public ReplyRunnable
>+                               , public DOMRequest
>+{
>+public:
>+  NS_DECL_ISUPPORTS_INHERITED
>+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(FMRadioRequest, DOMRequest)
>+
>+  FMRadioRequest(nsPIDOMWindow* aWindow, nsWeakPtr aFMRadio)
>+    : DOMRequest(aWindow)
>+    , mFMRadio(aFMRadio) { }
>+
>+  ~FMRadioRequest() { }
>+
>+  NS_IMETHOD Run()
>+  {
>+    MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
>+
>+    nsCOMPtr<nsIDOMEventTarget> target = do_QueryReferent(mFMRadio);
>+    if (!target) {
>+      return NS_OK;
>+    }
>+
>+    FMRadio* fmRadio = static_cast<FMRadio*>(
>+      static_cast<nsIDOMEventTarget*>(target));
>+
>+    if (fmRadio->mIsShutdown) {
>+      return NS_OK;
>+    }
>+
>+    switch (mResponseType.type()) {
>+      case FMRadioResponseType::TErrorResponse:
>+        FireError(mResponseType.get_ErrorResponse().error());
>+        break;
>+      case FMRadioResponseType::TSuccessResponse:
>+        FireSuccess(JS::Rooted<JS::Value>(AutoJSContext(), JSVAL_VOID));

smaug (or someone) needs to check this, too.  I haven't reviewed a patch with
the new rooting stuff yet, so I don't know if this is right.

>+        break;
>+      default:
>+        NS_RUNTIMEABORT("not reached");
>+        break;

Nit: MOZ_CRASH() with no break; would be consistent with the rest of the code.

>+    }
>+
>+    return NS_OK;
>+  }

Is there some reason the Run() method needs to be implemented in the header
file?

>diff --git a/dom/fmradio/FMRadio.cpp b/dom/fmradio/FMRadio.cpp
>index e69de29..d1be0c6 100644
>--- a/dom/fmradio/FMRadio.cpp
>+++ b/dom/fmradio/FMRadio.cpp

>+// The pref 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.fmradio.antenna.internal"
>+
>+#define ENABLED_EVENT_NAME         NS_LITERAL_STRING("enabled")
>+#define DISABLED_EVENT_NAME        NS_LITERAL_STRING("disabled")
>+#define FREQUENCYCHANGE_EVENT_NAME NS_LITERAL_STRING("frequencychange")
>+#define ANTENNAAVAILABLECHANGE_EVENT_NAME \
>+  NS_LITERAL_STRING("antennaavailablechange")

Nit: I don't see a lot of value in these #defines if they're only used once,
but whatever you prefer is fine.

>+void
>+FMRadio::Notify(const FMRadioEventType& aType)
>+{
>+  switch (aType) {
>+    case FrequencyChanged:
>+      DispatchTrustedEvent(FREQUENCYCHANGE_EVENT_NAME);
>+      break;
>+    case EnabledChanged:
>+      if (FMRadioService::Singleton()->IsEnabled()) {
>+        DispatchTrustedEvent(ENABLED_EVENT_NAME);
>+      } else {
>+        DispatchTrustedEvent(DISABLED_EVENT_NAME);
>+      }
>+      break;
>+    default:
>+      MOZ_CRASH();
>+      break;

Nit: no |break;| after MOZ_CRASH().

>+  }
>+}

>+Nullable<double>
>+FMRadio::GetFrequency() const
>+{
>+  return Enabled() ? Nullable<double>(FMRadioService::Singleton()->GetFrequency())
>+                   : Nullable<double>();
>+}

Nit: When we wrap long lines, we wrap them with the operator at the end of the
line, not the beginning.  (I don't always like this myself, but that's how it
is.)

In this case it might make sense to wrap the line after "?" and just indent
both of the "Nullable"s by two spaces.  The the lines would also be less than
80 chars...

>+already_AddRefed<DOMRequest>
>+FMRadio::Enable(double aFrequency)
>+{
>+  nsCOMPtr<nsPIDOMWindow> win = GetOwner();
>+  if (!win) {
>+    return nullptr;
>+  }
>+
>+  // |FMRadio| inherits from |nsIDOMEventTarget| and |nsISupportsWeakReference|
>+  // which both inherits from nsISupports, so |nsISupports| is an ambiguous
>+  // base of |FMRadio|, we have to cast |this| to one of the base classes.
>+  nsWeakPtr weakFMRadio = do_GetWeakReference(
>+    static_cast<nsIDOMEventTarget*>(this));
>+  nsRefPtr<FMRadioRequest> r = new FMRadioRequest(win, weakFMRadio);

Would it make sense to have FMRadioRequest take an FMRadio* and convert it to
an nsWeakPtr itself?  That would save you some repetitive code, and it would
also give us strong typing -- right now you can pass any weak ref to the
FMRadioRequest constructor, which isn't great.

In addition to this, perhaps we should have a GetFMRadioRequest() function that
does this business for us?

>+  FMRadioService::Singleton()->Enable(aFrequency, r);
>+
>+  return r.forget();
>+}

>+already_AddRefed<DOMRequest>
>+FMRadio::SeekUp()
>+{
>+  nsCOMPtr<nsPIDOMWindow> win = GetOwner();
>+  if (!win) {
>+    return nullptr;
>+  }
>+
>+  nsWeakPtr weakFMRadio = do_GetWeakReference(
>+    static_cast<nsIDOMEventTarget*>(this));
>+  nsRefPtr<FMRadioRequest> r = new FMRadioRequest(win, weakFMRadio);
>+  FMRadioService::Singleton()->Seek(true, r);

Please do

  Seek(/* up */ true, r);

See http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html.

>+  return r.forget();
>+}
>+
>+already_AddRefed<DOMRequest>
>+FMRadio::SeekDown()
>+{
>+  nsCOMPtr<nsPIDOMWindow> win = GetOwner();
>+  if (!win) {
>+    return nullptr;
>+  }
>+
>+  nsWeakPtr weakFMRadio = do_GetWeakReference(
>+    static_cast<nsIDOMEventTarget*>(this));
>+  nsRefPtr<FMRadioRequest> r = new FMRadioRequest(win, weakFMRadio);
>+  FMRadioService::Singleton()->Seek(false, r);

Please comment "false" similarly.

>diff --git a/dom/fmradio/FMRadioCommon.h b/dom/fmradio/FMRadioCommon.h
>index e69de29..8df4d93 100644
>--- a/dom/fmradio/FMRadioCommon.h
>+++ b/dom/fmradio/FMRadioCommon.h

>+#undef FM_LOG
>+#if defined(MOZ_WIDGET_GONK)

Nit: This should probably be |if defined(ANDROID)| -- there's no reason not to
log to logcat on Android; certainly parts of the FM radio bits we've written
are usable there.

>+#include <android/log.h>
>+#define FM_LOG(FMRADIO_LOG_INFO, args...) \
>+  __android_log_print(ANDROID_LOG_INFO, \
>+                      FMRADIO_LOG_INFO, \
>+                      ## args)
>+#else
>+#define FM_LOG(args...)
>+#endif

Note that I'm racing against you in bug 881389 to change how we do this sort of
logging.

>diff --git a/dom/fmradio/FMRadioService.h b/dom/fmradio/FMRadioService.h
>index e69de29..9e535bf 100644
>--- a/dom/fmradio/FMRadioService.h
>+++ b/dom/fmradio/FMRadioService.h

>+BEGIN_FMRADIO_NAMESPACE
>+
>+class ReplyRunnable : public nsCancelableRunnable
>+{
>+public:
>+  ReplyRunnable() : mResponseType(SuccessResponse()) {}
>+  virtual ~ReplyRunnable() {}
>+
>+  void
>+  SetReply(const FMRadioResponseType& aResponseType)
>+  {
>+    mResponseType = aResponseType;
>+  }
>+
>+protected:
>+  FMRadioResponseType mResponseType;
>+};

By default, nsCancelableRunnable's Cancel method does nothing.  (See
nsThreadUtils.cpp)  I don't see you overriding the Cancel() method, so I'm
afraid it won't work right.

On the other hand, I don't see you calling Cancel() here or in the IPC patch.
Maybe this can inherit from plain nsRunnable?

Since you never expect anyone to create a ReplyRunnable -- it will always be a
FMRadioRequest or an FMRadioRequestParent -- the canonical way to do this would
be to inherit from nsICancelableRunnable or nsIRunnable.  The main difference
is that you'd have to implement Addref/Release on the classes which inherit
from ReplyRunnable, as opposed to "inheriting" addref/release from
ReplyRunnable.

The reason we do it this way is that if you inherit from two classes which
/both/ implement their own addref/release methods, you're in trouble: Which
ones do you use?

The only reason nsRunnable and nsCancelableRunnable exist -- note that they
violate this convention from above -- is because people often use those to make
very small classes that are never inherited from.

>+/**
>+ * The FMRadio Service Interface for FMRadio.
>+ *
>+ * All the requests coming from the content page will be redirected to the
>+ * concrete class object respectively.

This is confusing to me (particularly "respectively").  Perhaps get rid of
"respectively" and move this paragraph after the next paragraph, where you tell
me that there are two concrete classes which implement this interface.

>+ * There are two concrete classes which implement the interface:

s/the/this/

>+ *  - FMRadioService
>+ *    It's used in the main process, implements all the logics about FM Radio.
>+ *
>+ *  - FMRadioChildService
>+ *    It's used in OOP subprocess, it's a kind of proxy which just send all
>+ *    the requests to main process through IPC channel.

s/OOP// ("OOP subprocesses" is redundant)
s/just send/just sends/
s/through IPC channel/through an IPC channel/

s/,/;/ or s/,/./  Otherwise, this is a comma splice.
http://en.wikipedia.org/wiki/Comma_splice

I think we renamed FMRadioChildService to FMRadioChild?

It's too bad that we lost the "Service" part of this name, but I don't see how
to fix it without breaking more important conventions.  Oh well.

>+ * Consider navigator.mozFMRadio.enable(), here is the call sequences:

s/,/./
s/sequences/sequence/

>+ *  - OOP
>+ *    (1) Call navigator.mozFMRadio.enable().
>+ *    (2) Return a DOMRequest object, and call FMRadioChildService.Enable() with
>+ *        a ReplyRunnable object.
>+ *    (3) Send IPC message to main process.
>+ *    (4) Call FMRadioService::Enable() with a ReplyRunnable object.
>+ *    (5) Call hal::EnableFMRadio().
>+ *    (6) Notify FMRadioService object when FM radio HW is enabled.
>+ *    (7) Dispatch the ReplyRunnable object created in (4).
>+ *    (8) Send IPC message back to child process.
>+ *    (9) Dispatch the ReplyRunnable object created in (2).
>+ *   (10) Fire success callback of the DOMRequest Object created in (2).

This is a really helpful explanation, but it would be clearer if you could
annotate these steps with "Child: " or "Parent: ", as appropriate.

>+  virtual void Enable(double aFrequency, ReplyRunnable* aRunnable) = 0;

aReplyRunnable might be a better name here and elsewhere, since "aRunnable" is
kind of like calling something "aInt32".

>+  virtual void Disable(ReplyRunnable* aRunnable) = 0;
>+  virtual void SetFrequency(double frequency, ReplyRunnable* aRunnable) = 0;
>+  virtual void Seek(bool upward, ReplyRunnable* aRunnable) = 0;
>+  virtual void CancelSeek(ReplyRunnable* aRunnable) = 0;

Nit: Use the "a" prefix on all arg names.

>+  /**
>+   * Register handler to receive the FM Radio events, including:
>+   *   - StateChangedEvent
>+   *   - FrequencyChangedEvent
>+   *
>+   * Called by FMRadio and FMRadioParent in OOP model.
>+   */

s/OOP model/the OOP model/

It's not clear how "in OOP model" modifies this sentence.  Do you mean
"[FMRadio] and [FMRadioParent in the OOP model]"?  Then is FMRadio in-process
only, or does it exist in oop and in-process?

Or do you mean "[FMRadio and FMRadioParent] in the OOP model", in which case,
what about the in-process model?

Maybe it's clearer just to get rid of "in the OOP model".

>+  virtual void AddObserver(FMRadioEventObserver* aObserver) = 0;
>+  virtual void RemoveObserver(FMRadioEventObserver* aObserver) = 0;
>+};

>+class FMRadioService MOZ_FINAL : public IFMRadioService
>+                               , public hal::FMRadioObserver

FMRadioService is stored in a StaticRefPtr, so it must have AddRef and Release
methods, otherwise we'll get a compile error.  But I don't see where those
methods are coming from; neither IFMRadioService nor FMRadioObserver has those
methods, afaict.

I must be missing something if this compiles.  Help me out?

>+{
>+  friend class ReadRilSettingTask;
>+  friend class RilSettingsObserver;
>+  friend class SetFrequencyRunnable;
>+
>+public:
>+  /**
>+   * Static method to return the singleton instance.
>+   *
>+   * If it's in the child process, we will get an object of FMRadioChild.
>+   */
>+  static IFMRadioService* Singleton();

It's pretty confusing that we call FMRadioService::Singleton() to get either
an FMRadioService* or an FMRadioChild*.

How about we put this method on IFMRadioService?

You could then even have FMRadioService::Singleton() which asserted we were in
the main process and then returned an FMRadioService*.  This would let you get
rid of a lot of potentially-unsafe static casts.

>+  virtual void Enable(double aFrequency, ReplyRunnable* aRunnable) MOZ_OVERRIDE;
>+  virtual void Disable(ReplyRunnable* aRunnable) MOZ_OVERRIDE;
>+  virtual void SetFrequency(double frequency, ReplyRunnable* aRunnable) MOZ_OVERRIDE;
>+  virtual void Seek(bool upward, ReplyRunnable* aRunnable) MOZ_OVERRIDE;
>+  virtual void CancelSeek(ReplyRunnable* aRunnable) MOZ_OVERRIDE;

"a" prefix here too.

>+  virtual void AddObserver(FMRadioEventObserver* aObserver) MOZ_OVERRIDE;
>+  virtual void RemoveObserver(FMRadioEventObserver* aObserver) MOZ_OVERRIDE;
>+
>+  /* FMRadioObserver */
>+  void Notify(const hal::FMRadioOperationInformation& info) MOZ_OVERRIDE;

and here.

>+protected:
>+  FMRadioService();
>+
>+private:
>+  int32_t RoundFrequency(int32_t aFrequencyInKHz);
>+
>+  void NotifyFMRadioEvent(FMRadioEventType aType);
>+  void DoDisable();
>+  void SetState(FMRadioState aState);
>+  void UpdatePowerState();
>+  void UpdateFrequency();
>+
>+private:
>+  bool mEnabled;
>+
>+  int32_t mFrequencyInKHz;
>+
>+  FMRadioState mState;
>+
>+  bool mHasReadRilSetting;
>+  bool mRilDisabled;
>+
>+  double mUpperBoundInMHz;
>+  double mLowerBoundInMHz;
>+  double mChannelWidthInMHz;
>+
>+  nsCOMPtr<nsIObserver> mSettingsObserver;
>+
>+  nsRefPtr<ReplyRunnable> mPendingRequest;
>+
>+  FMRadioEventObserverList mObserverList;
>+
>+private:
>+  static StaticRefPtr<FMRadioService> sFMRadioService;
>+};
>+
>+END_FMRADIO_NAMESPACE
>+
>+#endif // mozilla_dom_fmradio_ipc_fmradioservice_h__
>+


>diff --git a/dom/fmradio/FMRadioService.cpp b/dom/fmradio/FMRadioService.cpp
>index e69de29..e331b2c 100644
>--- a/dom/fmradio/FMRadioService.cpp
>+++ b/dom/fmradio/FMRadioService.cpp

>+#define DOM_FMRADIO_BAND_PREF "dom.fmradio.band"
>+#define DOM_FMRADIO_CHANNEL_WIDTH_PREF "dom.fmradio.channelWidth"

These two are used only once, so you could inline them, if you like.

>+#define MOZSETTINGS_CHANGED_ID "mozsettings-changed"
>+#define SETTING_KEY_RIL_RADIO_DISABLED "ril.radio.disabled"
>+
>+using namespace mozilla::hal;
>+using mozilla::Preferences;
>+
>+BEGIN_FMRADIO_NAMESPACE
>+
>+class RilSettingsObserver MOZ_FINAL : public nsIObserver
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+
>+  NS_IMETHODIMP
>+  Observe(nsISupports * aSubject, const char * aTopic, const PRUnichar * aData)
>+  {
>+    MOZ_ASSERT(NS_IsMainThread());
>+    MOZ_ASSERT(sFMRadioService);
>+
>+    if (!strcmp(aTopic, MOZSETTINGS_CHANGED_ID)) {

We prefer early returns and less indentation:

      if (strcmp(aTopic, MOZSETTINGS_CHANGED_ID)) {
        return NS_OK;
      }

      ...

>+      // The string that we're interested in will be a JSON string looks like:
>+      //  {"key":"ril.radio.disabled","value":true}
>+      mozilla::AutoSafeJSContext cx;
>+      nsDependentString dataStr(aData);

Nit: const nsDependentString.

>+      JS::Rooted<JS::Value> val(cx);
>+      if (!JS_ParseJSON(cx, dataStr.get(), dataStr.Length(), &val) ||
>+          !val.isObject()) {
>+        return NS_OK;
>+      }
>+
>+      JS::Rooted<JSObject*> obj(cx, &val.toObject());
>+      JS::Rooted<JS::Value> key(cx);
>+      if (!JS_GetProperty(cx, obj, "key", key.address()) ||
>+          !key.isString()) {
>+        return NS_OK;
>+      }
>+
>+      JS::Rooted<JSString*> jsKey(cx, key.toString());
>+      nsDependentJSString keyStr;
>+      if (!keyStr.init(cx, jsKey)) {
>+        return NS_OK;
>+      }
>+
>+      JS::Rooted<JS::Value> value(cx);
>+      if (!JS_GetProperty(cx, obj, "value", value.address())) {
>+        return NS_OK;
>+      }
>+
>+      FMRadioService* fmRadioService =
>+        static_cast<FMRadioService*>(FMRadioService::Singleton());
>+      if (keyStr.EqualsLiteral(SETTING_KEY_RIL_RADIO_DISABLED)) {
>+        if (!value.isBoolean()) {
>+          return NS_OK;
>+        }
>+
>+        fmRadioService->mRilDisabled = value.toBoolean();
>+
>+        // Disable the FM radio HW if Airplane mode is enabled.
>+        if (fmRadioService->mRilDisabled) {
>+          fmRadioService->Disable(nullptr);
>+        }

Please move the declaration of fmRadioService closer to where it's used.

Could we just make FMRadioService inherit from nsIObserver?  Then we wouldn't
need this separate class at all.  That seems pretty reasonable to me...

>+StaticRefPtr<FMRadioService> FMRadioService::sFMRadioService;
>+
>+FMRadioService::FMRadioService()
>+  : mFrequencyInKHz(0)
>+  , mState(Disabled)
>+  , mHasReadRilSetting(false)
>+  , mRilDisabled(false)
>+  , mPendingRequest(nullptr)
>+  , mObserverList(FMRadioEventObserverList())

The last two initializer list entries here aren't necessary; we'll call the
default constructors automatically.  (In particular the second one is a verbose
way of saying |, mObserverList()|.)

>+{
>+  // read power state and frequency from Hal

Nit: Begin sentences with a capital letter and end them with a period.

>+  mEnabled = IsFMRadioOn();
>+  if (mEnabled) {
>+    mFrequencyInKHz = GetFMRadioFrequency();
>+    SetState(Enabled);
>+  }
>+
>+  switch (Preferences::GetInt(DOM_FMRADIO_BAND_PREF, BAND_87500_108000_kHz)) {
>+    case BAND_76000_90000_kHz:
>+      mUpperBoundInMHz = 90.0;
>+      mLowerBoundInMHz = 76.0;
>+      break;
>+    case BAND_76000_108000_kHz:
>+      mUpperBoundInMHz = 108.0;
>+      mLowerBoundInMHz = 76.0;
>+      break;
>+    case BAND_87500_108000_kHz:
>+    default:
>+      mUpperBoundInMHz = 108.0;
>+      mLowerBoundInMHz = 87.5;
>+      break;
>+  }
>+
>+  switch (Preferences::GetInt(DOM_FMRADIO_CHANNEL_WIDTH_PREF,
>+    CHANNEL_WIDTH_100KHZ)) {

Nit: Line up the "C" in "CHANNEL" with the "D" in "DOM".

>+    case CHANNEL_WIDTH_200KHZ:
>+      mChannelWidthInMHz = 0.2;
>+      break;
>+    case CHANNEL_WIDTH_50KHZ:
>+      mChannelWidthInMHz = 0.05;
>+      break;
>+    case CHANNEL_WIDTH_100KHZ:
>+    default:
>+      mChannelWidthInMHz = 0.1;
>+      break;
>+  }
>+
>+  mSettingsObserver = new RilSettingsObserver();
>+
>+  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
>+  MOZ_ASSERT(obs);

Nit: This MOZ_ASSERT is unnecessary; if !obs, we're definitely going to crash
on the next line, with or without the assert.

>+  if (NS_FAILED(obs->AddObserver(mSettingsObserver,
>+                                 MOZSETTINGS_CHANGED_ID,
>+                                 false))) {

s#false#/* useWeak */ false#

>+    NS_WARNING("Failed to add settings change observer!");
>+  }
>+
>+  RegisterFMRadioObserver(this);
>+}
>+
>+FMRadioService::~FMRadioService()
>+{
>+  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
>+  if (!obs || NS_FAILED(obs->RemoveObserver(mSettingsObserver,
>+                                            MOZSETTINGS_CHANGED_ID))) {
>+    NS_WARNING("Can't unregister observers, or already unregistered");
>+  }
>+  UnregisterFMRadioObserver(this);
>+}
>+
>+class EnableRunnable MOZ_FINAL : public nsRunnable

I was initially confused as to why this has to be in a runnable, but I think I
understand now that I've read more.  Good call.  :)

Could you add a comment explaining this so someone else doesn't mess it up?
Maybe the comment should go next to the places where you dispatch
EnableRunnable asynchronously.

>+{
>+public:
>+  EnableRunnable(int32_t aUpperLimit, int32_t aLowerLimit, int32_t aSpaceType)
>+    : mUpperLimit(aUpperLimit)
>+    , mLowerLimit(aLowerLimit)
>+    , mSpaceType(aSpaceType) { }
>+
>+  NS_IMETHOD Run()
>+  {
>+    FMRadioSettings info;
>+    info.upperLimit() = mUpperLimit;
>+    info.lowerLimit() = mLowerLimit;
>+    info.spaceType() = mSpaceType;
>+
>+    EnableFMRadio(info);
>+
>+    nsCOMPtr<nsIAudioManager> audioManager =
>+      do_GetService(NS_AUDIOMANAGER_CONTRACTID);
>+    MOZ_ASSERT(audioManager, "No AudioManager");
>+    audioManager->SetFmRadioAudioEnabled(true);

Another MOZ_ASSERT that's not helpful, IMO.

>+    // TODO apply path of bug 862899: AudioChannelAgent per process

s/path of/patch from/

>+    return NS_OK;
>+  }
>+
>+private:
>+  int32_t mUpperLimit;
>+  int32_t mLowerLimit;
>+  int32_t mSpaceType;
>+};
>+
>+class ReadRilSettingTask MOZ_FINAL : public nsISettingsServiceCallback
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+
>+  ReadRilSettingTask() { }
>+
>+  NS_IMETHOD
>+  Handle(const nsAString& aName, const JS::Value& aResult)
>+  {
>+    FMRadioService* fmRadioService =
>+      static_cast<FMRadioService*>(FMRadioService::Singleton());
>+
>+    fmRadioService->mHasReadRilSetting = true;

Should RilSettingsObserver::Observe() also set mHasReadRilSetting to true, if
it reads the setting successfully?

I guess we could also make fmRadioService inherit from
nsISettingsServiceCallback, but I agree that this way is cleaner.  It feels
different with an observer, which is persistent (like the FMRadioService) as
compared with a callback, which is ephemeral.

>+    if (!aResult.isBoolean()) {
>+      fmRadioService->mPendingRequest->SetReply(
>+        ErrorResponse(NS_LITERAL_STRING("Unexpected error")));
>+      NS_DispatchToMainThread(fmRadioService->mPendingRequest);
>+
>+      // Failed to read the setting value, set the state back to Disabled.
>+      fmRadioService->SetState(Disabled);
>+      return NS_OK;
>+    }
>+
>+    fmRadioService->mRilDisabled = aResult.toBoolean();
>+    if (!fmRadioService->mRilDisabled) {
>+      EnableRunnable* runnable =
>+        new EnableRunnable(fmRadioService->mUpperBoundInMHz * 1000,
>+                           fmRadioService->mLowerBoundInMHz * 1000,
>+                           fmRadioService->mChannelWidthInMHz * 1000);
>+      NS_DispatchToMainThread(runnable);

Please add a comment to the top of this class that we read the airplane-mode
setting and then, if airplane mode is not enabled, we enable the radio!  It's
not clear from the name that this class does more than simply read the setting.

>+    } else {
>+      fmRadioService->mPendingRequest->SetReply(ErrorResponse(
>+        NS_LITERAL_STRING("Airplane mode is enabled")));
>+      NS_DispatchToMainThread(fmRadioService->mPendingRequest);
>+
>+      // Airplane mode is enabled, set the state back to Disabled.
>+      fmRadioService->SetState(Disabled);
>+    }
>+
>+    return NS_OK;
>+  }

How confident are you that mPendingRequest cannot be null by the time this
thing runs?  I am not confident at all.  :)  Please convince me, or otherwise
add a nullcheck.

>+  NS_IMETHOD
>+  HandleError(const nsAString& aName)
>+  {
>+    FMRadioService* fmRadioService =
>+      static_cast<FMRadioService*>(FMRadioService::Singleton());
>+
>+    fmRadioService->mPendingRequest->SetReply(ErrorResponse(
>+      NS_LITERAL_STRING("Unexpected error")));
>+    NS_DispatchToMainThread(fmRadioService->mPendingRequest);
>+    fmRadioService->SetState(Disabled);
>+    return NS_OK;
>+  }

Similarly here re mPendingRequest.

>+};
>+
>+NS_IMPL_ISUPPORTS1(ReadRilSettingTask, nsISettingsServiceCallback)
>+
>+class DisableRunnable MOZ_FINAL : public nsRunnable
>+{
>+public:
>+  DisableRunnable() { }
>+
>+  NS_IMETHOD Run()
>+  {
>+    // Fix Bug 796733.
>+    // DisableFMRadio should be called before SetFmRadioAudioEnabled to prevent
>+    // the annoying beep sound.
>+    DisableFMRadio();

Comment newline nit from earlier.

>+    nsCOMPtr<nsIAudioManager> audioManager =
>+      do_GetService(NS_AUDIOMANAGER_CONTRACTID);
>+    MOZ_ASSERT(audioManager, "No AudioManager");

MOZ_ASSERT() nit from earlier.

>+    audioManager->SetFmRadioAudioEnabled(false);
>+
>+    return NS_OK;
>+  }
>+};

>+class SeekRunnable MOZ_FINAL : public nsRunnable
>+{
>+public:
>+  SeekRunnable(bool aUpward) : mUpward(aUpward) { }
>+
>+  NS_IMETHOD Run()
>+  {
>+    FMRadioSeek(mUpward ? FM_RADIO_SEEK_DIRECTION_UP
>+                        : FM_RADIO_SEEK_DIRECTION_DOWN);

If we already have this enum for UP/DOWN, is there a reason we don't use it
everywhere?  That would be preferable to the bool aUpward, IMO.

>+    return NS_OK;
>+  }
>+
>+private:
>+  bool mUpward;
>+};

>+void
>+FMRadioService::RemoveObserver(FMRadioEventObserver* aObserver)
>+{
>+  mObserverList.RemoveObserver(aObserver);
>+
>+  if (mObserverList.Length() == 0)
>+  {
>+    // No observer in the list means no app is using WebFM API, so we should
>+    // turn off the FM radio HW.
>+    if (IsFMRadioOn()) {
>+      LOG("Turn off the FM radio HW");

Nit: Maybe say "Turning off the FM radio HW because observer list is empty."

>+      NS_DispatchToMainThread(new DisableRunnable());
>+    }
>+  }
>+}

>+int32_t
>+FMRadioService::RoundFrequency(int32_t aFrequencyInKHz)
>+{
>+  int32_t lowerBoundInKHz = mLowerBoundInMHz * 1000;
>+  int32_t upperBoundInKHz = mUpperBoundInMHz * 1000;
>+  int32_t channelWidth = mChannelWidthInMHz * 1000;

This floating-point arithmetic works properly, but only barely.

When we say mChannelWidthInMHz = 0.2, what we're actually doing is saying
mChannelWidthInMHz = 2/10.  Since 10 is not a power of 2, 2/10 is not
exactly-representable as a double; there's some error in one direction or
another.

The trick we're relying on here is that (2/10) * 1000 == 20.000000000 or
20.000000001, and not 19.999999999.  If it's the latter, then when you convert
it to an int, you'll get 19, not 20, and that's obviously not what you want.

Apparently this error doesn't happen -- I checked all possible values in 0.001,
0.002, ... 0.999.

It seems that double multiplication is constructed not to have this problem.  I
guess it's because you're multiplying by 1000, and 1000 is divisible by the old
denominator.  I'm pretty surprised by this, to be honest.

Anyway, this seems like a really scary thing to rely on.

The FM radio accepts values as doubles in MHz.  There's going to be some
imprecision there, but that's OK because we're going to round to the nearest
channel boundary anyway.

But for working internally, can we just specify the bounds in KHz?

>+  if (aFrequencyInKHz < lowerBoundInKHz ||
>+      aFrequencyInKHz > upperBoundInKHz) {
>+    return 0;
>+  }
>+
>+  int32_t partToBeRounded = aFrequencyInKHz - lowerBoundInKHz;
>+  int32_t roundedPart = round(partToBeRounded / channelWidth) * channelWidth;

I don't think this is totally correct.

First, partToBeRounded and channelWidth are both ints, so you're doing integer
division here, and there's no reason to divide.  I agree with the intent of the
code here; I think we should be doing floating-point division and rounding.

Second, we round to the nearest channel boundary only after converting
the value to KHz.  But that conversion loses precision and could cause us to
round the wrong way.  So this method should probably take a double, not an
int32, as its param.

But also, suppose the user passes a value of 87.499999999MHz.  We should
probably round that up to 87.5, if mLowerBound == 87.5, particularly because,
if mLowerBound were 76.0, we'd happily round 87.4999999MHz up to 87.5MHz!

Alternatively, we could clamp an out-of-range frequency value to the lower or
upper bound, as appropriate.  Again, this avoids the rounding issues.

You'll also want to check that the frequency actually fits in an int before
converting it, otherwise I have no idea what will happen.  You can use
DoubleIsInt32() from mozilla/FloatingPoint.h.

>+  return lowerBoundInKHz + roundedPart;
>+}

>+double
>+FMRadioService::GetFrequency() const
>+{
>+  if (IsEnabled()) {
>+    int32_t frequencyInKHz = GetFMRadioFrequency();
>+    return frequencyInKHz / 1000.0;
>+  }
>+
>+  return 0;
>+}

I think you're not using mFrequencyInKHz here for a reason, but I don't
completely understand why.  Could you add a comment explaining this?

We should probably also put a comment on mFrequencyInKHz explaining what it is,
since it's different from e.g. mChannelWidthInMHz, which mirrors exactly the
value returned by GetChannelWidth().

>+void
>+FMRadioService::Enable(double aFrequencyInMHz, ReplyRunnable* aRunnable)
>+{
>+  // We need to call EnableFMRadio() in main thread
>+  MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
>+
>+  switch (mState) {
>+    case Enabled:
>+      aRunnable->SetReply(ErrorResponse(NS_LITERAL_STRING("It's enabled")));
>+      NS_DispatchToMainThread(aRunnable);
>+      return;
>+    case Disabling:
>+      aRunnable->SetReply(ErrorResponse(NS_LITERAL_STRING("It's disabling")));
>+      NS_DispatchToMainThread(aRunnable);
>+      return;
>+    case Enabling:
>+      aRunnable->SetReply(ErrorResponse(NS_LITERAL_STRING("It's enabling")));
>+      NS_DispatchToMainThread(aRunnable);
>+      return;

Thes strings are visible to the web page, so we should consider them carefully.
These error messages aren't great, because a developer won't necessarily know
that "it" refers to the FM radio.

Maybe "FM radio already enabled", "FM radio currently disabling", and "FM radio
currently enabling" would be better.

Also, it's not clear to me why it's an error to call Enable() while the radio
is disabling.  Shouldn't we just cancel the disable, or enqueue an event to run
Enable() once the disable finishes?  That can be a separate bug if you like.

>+  }
>+
>+  int32_t roundedFrequency = RoundFrequency(aFrequencyInMHz * 1000);
>+
>+  if (!roundedFrequency) {

It kind of seems like we should return -1 on error from RoundFrequency, so it's
clearer that it's an error.  Not a big deal, though.

>+    aRunnable->SetReply(ErrorResponse(
>+      NS_LITERAL_STRING("Frequency is out of range")));
>+    NS_DispatchToMainThread(aRunnable);
>+    return;
>+  }
>+
>+  if (mHasReadRilSetting && mRilDisabled) {
>+    aRunnable->SetReply(ErrorResponse(
>+      NS_LITERAL_STRING("Airplane mode is enabled")));
>+    NS_DispatchToMainThread(aRunnable);
>+    return;
>+  }
>+
>+  SetState(Enabling);
>+  // Cache the enable request just in case disable() is called
>+  // while the FM radio HW is being enabled.
>+  mPendingRequest = aRunnable;
>+
>+  // Cache the frequency value, and set it after the FM radio HW is enabled
>+  mFrequencyInKHz = roundedFrequency;

If mFrequencyInKHz is a cache-type thing, maybe we should change its name to
reflect that, like mPendingFrequencyInKHz, or something.

>+  if (!mHasReadRilSetting) {
>+    nsCOMPtr<nsISettingsService> settings =
>+      do_GetService("@mozilla.org/settingsService;1");
>+    MOZ_ASSERT(settings, "Can't create settings service");
>+
>+    nsCOMPtr<nsISettingsServiceLock> settingsLock;
>+    nsresult rv = settings->CreateLock(getter_AddRefs(settingsLock));
>+    MOZ_ASSERT(rv, "Can't create settings lock");

I think you mean MOZ_ASSERT(NS_SUCCEEDED(rv), ...);

But are you positive it's illegal for CreateLock to fail?  The settings service
is written in JS, so if SettingsService.js createLock throws an exception,
we'll crash in a debug build, and we'll behave incorrectly in a release build.

You should try to probably handle this failure properly instead of punting.

>+
>+    nsRefPtr<ReadRilSettingTask> callback = new ReadRilSettingTask();
>+    rv = settingsLock->Get(SETTING_KEY_RIL_RADIO_DISABLED, callback);
>+    MOZ_ASSERT(rv, "Can't get settings value");

Similarly to the above.

>+    return;
>+  }
>+
>+  NS_DispatchToMainThread(new EnableRunnable(mUpperBoundInMHz * 1000,
>+                                             mLowerBoundInMHz * 1000,
>+                                             mChannelWidthInMHz * 1000));
>+}
>+
>+void
>+FMRadioService::Disable(ReplyRunnable* aRunnable)
>+{
>+  MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");

Please document somewhere that it's OK to pass a null runnable to Disable(),
but not to Enable().  That might even be worth a MOZ_ASSERT() at the top of
Enable().

>+  switch (mState) {
>+    case Disabling:
>+      if (aRunnable) {
>+        aRunnable->SetReply(ErrorResponse(NS_LITERAL_STRING("It's disabling")));
>+        NS_DispatchToMainThread(aRunnable);
>+      }
>+      return;
>+    case Disabled:
>+      if (aRunnable) {
>+        aRunnable->SetReply(ErrorResponse(NS_LITERAL_STRING("It's disabled")));
>+        NS_DispatchToMainThread(aRunnable);
>+      }
>+      return;
>+  }

It's an error to enable while we're disabling, but not an error to disable
while we're enabling?  That seems weird, per above.  We can address separately,
if you like.

>+  // If the FM Radio is currently seeking, no fail-to-seek or similar
>+  // event will be fired, execute the seek callback manually.
>+  if (mState == Seeking) {
>+    mPendingRequest->SetReply(ErrorResponse(
>+      NS_LITERAL_STRING("It's canceled")));
>+    NS_DispatchToMainThread(mPendingRequest);
>+  }

Similarly with the error messages here.

>+  bool isEnabling = mState == Enabling;

Please just inline isEnabling.

>+  nsRefPtr<ReplyRunnable> enablingRequest = mPendingRequest;
>+  SetState(Disabling);
>+  mPendingRequest = aRunnable;
>+
>+  if (isEnabling) {
>+    // If the radio is currently enabling, we fire the error callback
>+    // immediately. When the radio finishes enabling, we'll fire the success
>+    // callback for the disable request.
>+    enablingRequest->SetReply(ErrorResponse(NS_LITERAL_STRING("It's canceled")));

Tweak error message.

>+    NS_DispatchToMainThread(enablingRequest);
>+    return;

I was originally pretty confused by the comment above, but I think what's
missing is:

  If the radio is currently enabling, we fire the error callback /on the enable
  request/ immediately.  When the radio finishes enabling, we'll /call
  DoDisable()/ and fire the success callback for the disable request.

>+  }
>+
>+  DoDisable();
>+}

>+void
>+FMRadioService::DoDisable()
>+{
>+  NS_DispatchToMainThread(new DisableRunnable());
>+}
>+
>+void
>+FMRadioService::SetFrequency(double aFrequencyInMHz, ReplyRunnable* aRunnable)
>+{
>+  MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");

Is anything in FMRadioService callable off the main thread?  If it's all
main-thread-only, we should have asserts everywhere or nowhere.  If there's
stuff that's callable off the main thread, we should mention that explicitly
(and I'll need to look very carefully in my next review to make sure that it's
all OK).

(Note that one can't even safely get the FMRadioService from off the main
thread, because there's no locking around the StaticRefPtr that initializes
it.)

>+  switch (mState) {
>+    case Disabled:
>+      aRunnable->SetReply(ErrorResponse(NS_LITERAL_STRING("It's disabled")));
>+      NS_DispatchToMainThread(aRunnable);
>+      return;
>+    case Enabling:
>+      aRunnable->SetReply(ErrorResponse(NS_LITERAL_STRING("It's enabling")));
>+      NS_DispatchToMainThread(aRunnable);
>+      return;
>+    case Disabling:
>+      aRunnable->SetReply(ErrorResponse(NS_LITERAL_STRING("It's disabling")));
>+      NS_DispatchToMainThread(aRunnable);
>+      return;
>+  }

Error messages again (I won't keep repeating this, but please apply this
elsewhere).

Nit: This switch could be more explicit if you had

  case Seeking:
    break;
  case Enabled:
    break;

and similarly elsewhere for cases that you want to fall through.

Speaking of being explicit about cases, what should happen if we SetFrequency
while we're seeking?

>+  int32_t roundedFrequency = RoundFrequency(aFrequencyInMHz * 1000);
>+
>+  if (!roundedFrequency) {
>+    aRunnable->SetReply(ErrorResponse(
>+      NS_LITERAL_STRING("Frequency is out of range")));
>+    NS_DispatchToMainThread(aRunnable);
>+    return;
>+  }
>+
>+  aRunnable->SetReply(SuccessResponse());
>+  NS_DispatchToMainThread(aRunnable);
>+
>+  NS_DispatchToMainThread(new SetFrequencyRunnable(this, roundedFrequency));
>+}
>+
>+void
>+FMRadioService::Seek(bool upward, ReplyRunnable* aRunnable)

Nit: aUpward.

>+void
>+FMRadioService::CancelSeek(ReplyRunnable* aRunnable)
>+{
>+  MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
>+
>+  // We accept canceling seek request only if it's currently seeking.
>+  if (mState != Seeking) {
>+    aRunnable->SetReply(ErrorResponse(NS_LITERAL_STRING("It's not seeking")));
>+    NS_DispatchToMainThread(aRunnable);
>+    return;
>+  }
>+
>+  // Cancel it immediately to prevent seeking complete
>+  CancelFMRadioSeek();

    // Cancel the seek immediately to prevent it from completing.

>+  mPendingRequest->SetReply(ErrorResponse(NS_LITERAL_STRING("It's canceled")));
>+  NS_DispatchToMainThread(mPendingRequest);
>+
>+  SetState(Enabled);
>+
>+  aRunnable->SetReply(SuccessResponse());
>+  NS_DispatchToMainThread(aRunnable);
>+}
>+
>+void
>+FMRadioService::NotifyFMRadioEvent(FMRadioEventType aType)
>+{
>+  mObserverList.Broadcast(aType);
>+}
>+
>+void
>+FMRadioService::Notify(const FMRadioOperationInformation& info)
>+{
>+  switch (info.operation()) {
>+    case FM_RADIO_OPERATION_ENABLE:
>+      // We will receive FM_RADIO_OPERATION_ENABLE if we call DisableFMRadio(),
>+      // there must be some problem with HAL layer, we need to double check
>+      // the FM radio HW status here.
>+      if (!IsFMRadioOn()) {
>+        LOG("FMRadio should not be off!!");
>+        return;
>+      }
>+
>+      // The signal we received might be triggered by enable request in other
>+      // process, we should check if `mState` is Disabling or Disabling, if
>+      // false, we should skip it and just update the power state and frequency.
>+      if (mState == Enabling || mState == Disabling) {
>+        // If we're disabling, go disable the radio right now.
>+        // We don't change the `mState` value here, because we have set it to
>+        // `Disabled` when Disable() is called.
>+        if (mState == Disabling) {
>+          DoDisable();
>+          return;
>+        }

This if structure is actually just

  if (mState == Disabling) {
    ...
    return;
  }

  if (mState == Enabling) {
    ...
  }

>+        // Fire success callback for the enable request.
>+        mPendingRequest->SetReply(SuccessResponse());
>+        NS_DispatchToMainThread(mPendingRequest);

How confident are you that mPendingRequest is non-null if mState == Enabling?
At the very least we need to make this an explicit invariant.

>+        SetState(Enabled);
>+
>+        // To make sure the FM app will get the right frequency after the FM
>+        // radio is enabled, we have to set the frequency first.
>+        SetFMRadioFrequency(mFrequencyInKHz);
>+      }
>+
>+      // Update the current frequency without distributing the
>+      // `FrequencyChangedEvent`, to make sure the FM app will get the right
>+      // frequency when the `StateChangedEvent` is fired.

I think the name is "the `frequencychange` event" and I think the other
event is called "enabled" now?

>+      mFrequencyInKHz = GetFMRadioFrequency();
>+      UpdatePowerState();
>+
>+      // The frequency is changed from '0' to some meaningful number, so we
>+      // should distribute the `FrequencyChangedEvent` manually.

s/is/was/
s/distribute/send/

Same as above re event name.

>+      NotifyFMRadioEvent(FrequencyChanged);
>+      break;
>+    case FM_RADIO_OPERATION_DISABLE:
>+      // The signal we received might be triggered by disable request in other
>+      // process, we should check if `mState` is Disabling, if false, we should
>+      // skip it and just update the power state.

>       // The signal we received might have been triggered by a disable request in another
>       // process, so we should check if `mState` is Disabling.  If false, we should
>       // skip notifying the pending request and just update the power state.

I don't understand this comment or the code below.  We're the main process, and
we're only process that's sending disable requests to the hardware, right?

Also, if we don't notify the pending request now, when do we notify it?

>+      if (mState == Disabling) {
>+        if (mPendingRequest) {
>+          mPendingRequest->SetReply(SuccessResponse());
>+          NS_DispatchToMainThread(mPendingRequest);
>+        }
>+
>+        SetState(Disabled);
>+      }
>+
>+      UpdatePowerState();
>+      break;
>+    case FM_RADIO_OPERATION_SEEK:
>+      // The signal we received might be triggered by disable request in other
>+      // process, we should check if `mState` is Seeking, if false, we should
>+      // skip it and just update the frequency.

Similar changes to the comment.

>+      if (mState == Seeking) {
>+        mPendingRequest->SetReply(SuccessResponse());
>+        NS_DispatchToMainThread(mPendingRequest);
>+
>+        SetState(Enabled);
>+      }
>+
>+      UpdateFrequency();
>+      break;

Similar question about the code from above.

>+    default:
>+      MOZ_CRASH();
>+      return;

Nit: No return after MOZ_CRASH().

>+  }
>+}

>+
>+bool
>+IsMainProcess()
>+{
>+  return XRE_GetProcessType() == GeckoProcessType_Default;
>+}

This is used in only one place; please just inline it.

Comment 63

[Justin Lebar (not reading bugmail)]

smaug, search for your name in the comment above for another small JSAPI thing to check out (I don't want to get this stuff wrong).

Comment 64

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #62)
> 
> >diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h
> >index fb77156..cbad269 100644
> >--- a/content/base/src/nsGkAtomList.h
> >+++ b/content/base/src/nsGkAtomList.h
> 
> >@@ -634,16 +634,17 @@ GK_ATOM(omitXmlDeclaration, "omit-xml-declaration")
> >+GK_ATOM(onantennaavailablechange, "onantennaavailablechange")
> 
> >@@ -704,16 +705,17 @@ GK_ATOM(ondraggesture, "ondraggesture")
> >+GK_ATOM(onfrequencychange, "onfrequencychange")
> 
> >@@ -773,18 +775,16 @@ GK_ATOM(onscroll, "onscroll")
> >-GK_ATOM(onantennastatechange, "onantennastatechange")
> >-GK_ATOM(onseekcomplete, "onseekcomplete")
> 
> If we're changing the name of these events, we need to fix Gaia as well,
> right?
> 

The events, i.e |onseekcomplete| and |onantennastatechange|, are not the events we defined
in the WebIDL, they are just kind of internal events which are used in DOMFMRadioChild.js.

> >+    FMRadio* fmRadio = static_cast<FMRadio*>(
> >+      static_cast<nsIDOMEventTarget*>(target));
> >+
> >+    if (fmRadio->mIsShutdown) {
> >+      return NS_OK;
> >+    }
> >+
> >+    switch (mResponseType.type()) {
> >+      case FMRadioResponseType::TErrorResponse:
> >+        FireError(mResponseType.get_ErrorResponse().error());
> >+        break;
> >+      case FMRadioResponseType::TSuccessResponse:
> >+        FireSuccess(JS::Rooted<JS::Value>(AutoJSContext(), JSVAL_VOID));
> 
> smaug (or someone) needs to check this, too.  I haven't reviewed a patch with
> the new rooting stuff yet, so I don't know if this is right.
> 

Hi Justin, I found there are some differences between m-c and b2g18 when I rebased the patch on
top of b2g18, the build system has been changed a lot and the rooting stuff is also unavailable
in b2g18, will this patch be landed in m-c or b2g18?

Comment 65

[Justin Lebar (not reading bugmail)]

I don't think we need this on b2g18, and I think we'd have a heck of a time convincing release drivers to take it, based on how reluctant they have been to take much safer things.

Comment 66

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #65)
> I don't think we need this on b2g18, and I think we'd have a heck of a time
> convincing release drivers to take it, based on how reluctant they have been
> to take much safer things.

Hi Baku, if we won't land this patch on b2g18, could you write the AudioChannelAgent patch based on the version for m-c instead of b2g18?

Comment 67

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Justin Lebar [:jlebar] from comment #62)
> >diff --git a/dom/fmradio/FMRadio.h b/dom/fmradio/FMRadio.h
> >--- a/dom/fmradio/FMRadio.h
> >+++ b/dom/fmradio/FMRadio.h
> 
> >+class FMRadioRequest MOZ_FINAL : public ReplyRunnable
> >+                               , public DOMRequest
> >+{
> >+public:
> >+  NS_DECL_ISUPPORTS_INHERITED
> >+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(FMRadioRequest, DOMRequest)
> >+
> >+  FMRadioRequest(nsPIDOMWindow* aWindow, nsWeakPtr aFMRadio)
> >+    : DOMRequest(aWindow)
> >+    , mFMRadio(aFMRadio) { }
> >+
> >+  ~FMRadioRequest() { }
> >+
> >+  NS_IMETHOD Run()
> >+  {
> >+    MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
> >+
> >+    nsCOMPtr<nsIDOMEventTarget> target = do_QueryReferent(mFMRadio);
> >+    if (!target) {
> >+      return NS_OK;
> >+    }
> >+
> >+    FMRadio* fmRadio = static_cast<FMRadio*>(
> >+      static_cast<nsIDOMEventTarget*>(target));
> >+
> >+    if (fmRadio->mIsShutdown) {
> >+      return NS_OK;
> >+    }
> >+
> >+    switch (mResponseType.type()) {
> >+      case FMRadioResponseType::TErrorResponse:
> >+        FireError(mResponseType.get_ErrorResponse().error());
> >+        break;
> >+      case FMRadioResponseType::TSuccessResponse:
> >+        FireSuccess(JS::Rooted<JS::Value>(AutoJSContext(), JSVAL_VOID));
> 
> smaug (or someone) needs to check this, too.  I haven't reviewed a patch with
> the new rooting stuff yet, so I don't know if this is right.

Should be JS::UndefinedHandleValue.

> >+Nullable<double>
> >+FMRadio::GetFrequency() const
> >+{
> >+  return Enabled() ? Nullable<double>(FMRadioService::Singleton()->GetFrequency())
> >+                   : Nullable<double>();
> >+}
> 
> Nit: When we wrap long lines, we wrap them with the operator at the end of
> the
> line, not the beginning.  (I don't always like this myself, but that's how it
> is.)

No we don't... This is the correct style.

> >+      JS::Rooted<JS::Value> val(cx);
> >+      if (!JS_ParseJSON(cx, dataStr.get(), dataStr.Length(), &val) ||
> >+          !val.isObject()) {
> >+        return NS_OK;

Please check with bholley if you can leave this exception on the context.

Comment 68

[Justin Lebar (not reading bugmail)]

> No we don't... This is the correct style.

$ git grep '^\s*? '  -- '*.cpp' | wc -l
502
$ git grep '?\s*$'  -- '*.cpp'  | grep -v '//' | wc -l
941

If you exclude JS, which has its own weird style, that tilts the scales even more in favor of what I suggested:

$ git grep '^\s*?'  -- '*.cpp'  | grep -v '^js' | wc -l
357
$ git grep '?\s*$'  -- '*.cpp'  | grep -v '^js' | grep -v '//' | wc -l
902

There are some multiline comments in the latter, but it's much less than half.  So what I recommended is the prevailing style, as far as I can tell.  It also matches what we do with other operators

Comment 69

[Justin Lebar (not reading bugmail)]

Comment on attachment 760176 [details] [diff] [review]
Part II: IPDL V3

I'm not done with this review, but I wanted to get this comment out there, before I head out for the long weekend.

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
>index 3ec2626..8276cbf 100644
>--- a/dom/ipc/ContentParent.cpp
>+++ b/dom/ipc/ContentParent.cpp

>@@ -2015,16 +2021,42 @@ ContentParent::RecvPBluetoothConstructor(PBluetoothParent* aActor)
>+PFMRadioParent*
>+ContentParent::AllocPFMRadio()
>+{
>+#ifdef MOZ_B2G_FM
>+    if (!AssertAppProcessPermission(this, "fmradio")) {
>+        return nullptr;
>+    }
>+    return new FMRadioParent();
>+#else
>+    NS_RUNTIMEABORT("No support for FMRadio on this platform!");
>+    return nullptr;
>+#endif
>+}
>+
>+bool
>+ContentParent::DeallocPFMRadio(PFMRadioParent* aActor)
>+{
>+#ifdef MOZ_B2G_FM
>+    delete aActor;
>+    return true;
>+#else
>+    NS_RUNTIMEABORT("No support for FMRadio on this platform!");
>+    return false;
>+#endif
>+}

As I understand the code, a child process could call AllocPFMRadio even if
MOZ_B2G_FM is disabled.  (It couldn't go through the code in ContentChild, but
that's not going to stop an exploited process from calling the raw IPC
methods.)

We don't want an exploited child process to be able to cause the main process
to crash.

I think all you need to do is get rid of the NS_RUNTIMEABORTs here.  You could
replace them with NS_WARNINGs, if you like.

Comment 70

[Pin Zhang [:pzhang] (inactive)]

> >+class FMRadioService MOZ_FINAL : public IFMRadioService
> >+                               , public hal::FMRadioObserver
> 
> FMRadioService is stored in a StaticRefPtr, so it must have AddRef and
> Release
> methods, otherwise we'll get a compile error.  But I don't see where those
> methods are coming from; neither IFMRadioService nor FMRadioObserver has
> those
> methods, afaict.
> 
> I must be missing something if this compiles.  Help me out?
> 

|AddRef| and |Release| are defined in IFMRadioService:
 NS_INLINE_DECL_THREADSAFE_REFCOUNTING(IFMRadioService)

Comment 71

[Pin Zhang [:pzhang] (inactive)]

> >+      NS_DispatchToMainThread(fmRadioService->mPendingRequest);
> >+
> >+      // Airplane mode is enabled, set the state back to Disabled.
> >+      fmRadioService->SetState(Disabled);
> >+    }
> >+
> >+    return NS_OK;
> >+  }
> 
> How confident are you that mPendingRequest cannot be null by the time this
> thing runs?  I am not confident at all.  :)  Please convince me, or otherwise
> add a nullcheck.
> 
The mPendingRequest is set to nullptr only in SetState(). If ReadRilSettingTask
runs, it means the state is changed to |Enabling|, at this time, only |Disable|
could change the state to |Disabling|, other functions which can change the state
won't be qualified to run because we check the state.

But you reminds me that when this thing runs, the |mPendingRequest| could be 
disabling request which is not handled, in the case like this:
 - call Enable()
 - call Disable()
 - ReadRilSettingTask::Handle() is called back

Comment 72

[Pin Zhang [:pzhang] (inactive)]

> >+    default:
> >+      mChannelWidthInMHz = 0.1;
> >+      break;
> >+  }
> >+
> >+  mSettingsObserver = new RilSettingsObserver();
> >+
> >+  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> >+  MOZ_ASSERT(obs);
> 
> Nit: This MOZ_ASSERT is unnecessary; if !obs, we're definitely going to crash
> on the next line, with or without the assert.

I'm OK with it, but in the codebase, almost every time we fetched a service, it's followed by an service-is-not-null ASSERTION, isn't this a convention?

Comment 73

[Justin Lebar (not reading bugmail)]

> I'm OK with it, but in the codebase, almost every time we fetched a service, it's 
> followed by an service-is-not-null ASSERTION, isn't this a convention?

It's a silly convention if so.  :)

But also I'm not sure it's widespread; it may just be one or two people who are doing it.  This is a pretty rough approximation, but:

$ git grep do_GetService | wc -l
1639
$ git grep -C5 do_GetService | grep ASSERT | wc -l
149

So fewer than 10% of do_GetService calls have a MOZ_ASSERT or NS_ASSERTION within 5 lines of them.

Comment 74

[Justin Lebar (not reading bugmail)]

> |AddRef| and |Release| are defined in IFMRadioService:

Ah, right where I least expected it.  :)

> NS_INLINE_DECL_THREADSAFE_REFCOUNTING(IFMRadioService)

When do we addref / release an IFMRadioService off the main thread?

Comment 75

[Justin Lebar (not reading bugmail)]

> So fewer than 10% of do_GetService calls have a MOZ_ASSERT or NS_ASSERTION within 5 lines 
> of them.

But note that

$ git grep -C1 do_GetService | grep ENSURE | wc -l
391

That is, we frequently follow a do_GetService with an NS_ENSURE_TRUE.  That's primarily because do_GetService returns null during shutdown.

Comment 76

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #74)
> > |AddRef| and |Release| are defined in IFMRadioService:
> 
> Ah, right where I least expected it.  :)
> 
> > NS_INLINE_DECL_THREADSAFE_REFCOUNTING(IFMRadioService)
> 
> When do we addref / release an IFMRadioService off the main thread?
We use the concrete class object by directly calling the functions of FMRadioService::Singleton() directly, won't hold any references.

Comment 77

[Pin Zhang [:pzhang] (inactive)]

> >+
> >+      JS::Rooted<JS::Value> value(cx);
> >+      if (!JS_GetProperty(cx, obj, "value", value.address())) {
> >+        return NS_OK;
> >+      }
> >+
> >+      FMRadioService* fmRadioService =
> >+        static_cast<FMRadioService*>(FMRadioService::Singleton());
> >+      if (keyStr.EqualsLiteral(SETTING_KEY_RIL_RADIO_DISABLED)) {
> >+        if (!value.isBoolean()) {
> >+          return NS_OK;
> >+        }
> >+
> >+        fmRadioService->mRilDisabled = value.toBoolean();
> >+
> >+        // Disable the FM radio HW if Airplane mode is enabled.
> >+        if (fmRadioService->mRilDisabled) {
> >+          fmRadioService->Disable(nullptr);
> >+        }
> 
> Please move the declaration of fmRadioService closer to where it's used.
> 
> Could we just make FMRadioService inherit from nsIObserver?  Then we wouldn't
> need this separate class at all.  That seems pretty reasonable to me...
> 
In the previous version, I made FMRadioService inherit from nsIObserver, what I
did is add FMRadioService itself as the setting observer in the constructor and
remove it in the destructor, and I tried to delete the refconnted static 
FMRadioService object (sFMRadioService) when its mObserverList is empty, then
the result is there is no chance to delete the FMRadioService object..., so I create
the RilSettingsObserver class.

But in the latter version, we decide to just keep the static FMRadioService object
not renewed, so maybe I can make FMRadioService inherit from nsIObserver again ...

Comment 78

[Justin Lebar (not reading bugmail)]

>> When do we addref / release an IFMRadioService off the main thread?
> We use the concrete class object by directly calling the functions of 
> FMRadioService::Singleton() directly, won't hold any references.

Indeed, that is true.  But I don't understand what that has to do with addref/release'ing IFMRadioService off the main thread.

If we call Singleton() off the main thread, then we would in fact need thread-safe refcounting.  But in addition to possibly a lot of other problems, there's a race condition in Singleton(): two threads might race and create two IFMRadioService objects.

Comment 79

[Justin Lebar (not reading bugmail)]

> But in the latter version, we decide to just keep the static FMRadioService object
> not renewed, so maybe I can make FMRadioService inherit from nsIObserver again ...

I'm not sure what you mean by "not renewed".

> I tried to delete the refconnted static 
> FMRadioService object (sFMRadioService) when its mObserverList is empty, then
> the result is there is no chance to delete the FMRadioService object...

I'm also not sure what you mean by "there is no chance to delete the FMRadioService object".  If a refcounted object's refcount goes to zero, it is automatically deleted.

But maybe this is academic, since we both seem to think you can make FMRadioService inherit from nsIObserver now.  :)

Comment 80

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #78)
> >> When do we addref / release an IFMRadioService off the main thread?
> > We use the concrete class object by directly calling the functions of 
> > FMRadioService::Singleton() directly, won't hold any references.
> 
> Indeed, that is true.  But I don't understand what that has to do with
> addref/release'ing IFMRadioService off the main thread.
> 
> If we call Singleton() off the main thread, then we would in fact need
> thread-safe refcounting.  But in addition to possibly a lot of other
> problems, there's a race condition in Singleton(): two threads might race
> and create two IFMRadioService objects.

How about NS_INLINE_DECL_REFCOUNTING?

Comment 81

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL V6

Comment 82

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part II: IPDL V4

Fix issues addressed in comment 62/67, and rebased the code on top of the latest m-c.

Comment 83

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL V6 (-U8)

Comment 84

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part II: IPDL V4 (-U8)

Comment 85

[Pin Zhang [:pzhang] (inactive)]

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

> Also, it's not clear to me why it's an error to call Enable() while the radio
> is disabling.  Shouldn't we just cancel the disable, or enqueue an event to
> run
> Enable() once the disable finishes?  That can be a separate bug if you like.
>
> >+      return;
> >+    case Disabled:
> >+      if (aRunnable) {
> >+        aRunnable->SetReply(ErrorResponse(NS_LITERAL_STRING("It's disabled")));
> >+        NS_DispatchToMainThread(aRunnable);
> >+      }
> >+      return;
> >+  }
> 
> It's an error to enable while we're disabling, but not an error to disable
> while we're enabling?  That seems weird, per above.  We can address
> separately, if you like.
I prefer filing a separate bug.

> and similarly elsewhere for cases that you want to fall through.
> 
> Speaking of being explicit about cases, what should happen if we SetFrequency
> while we're seeking?

I think seeking action should be cancelled and then set frequency with the passed value.


> 
> >+        // Fire success callback for the enable request.
> >+        mPendingRequest->SetReply(SuccessResponse());
> >+        NS_DispatchToMainThread(mPendingRequest);
> 
> How confident are you that mPendingRequest is non-null if mState == Enabling?
> At the very least we need to make this an explicit invariant.
> 
There is only one line code to call SetState(Enabling) which is followed by mPendingRequest value.

setting.

Comment 86

[Justin Lebar (not reading bugmail)]

> How about NS_INLINE_DECL_REFCOUNTING?

Sure, that'd be fine.

Comment 87

[Justin Lebar (not reading bugmail)]

I'm sorry for taking so long here, Pin.  I've been traveling, and now I've been saddled with some drop-everything-else B2G bugs.  I really appreciate your patience, and I'm sorry again.  I promise I'll get to these reviews as soon as I can.

Comment 88

[Pin Zhang [:pzhang] (inactive)]

That's OK, I can imagine how busy you are, :)

Comment 89

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

(In reply to :Ms2ger from comment #67)

> > >+    switch (mResponseType.type()) {
> > >+      case FMRadioResponseType::TErrorResponse:
> > >+        FireError(mResponseType.get_ErrorResponse().error());
> > >+        break;
> > >+      case FMRadioResponseType::TSuccessResponse:
> > >+        FireSuccess(JS::Rooted<JS::Value>(AutoJSContext(), JSVAL_VOID));
> > 
> > smaug (or someone) needs to check this, too.  I haven't reviewed a patch with
> > the new rooting stuff yet, so I don't know if this is right.
> 
> Should be JS::UndefinedHandleValue.
Indeed.

Comment 90

[Pin Zhang [:pzhang] (inactive)]

Hi, Justin, do you have plan to start the next round review?

Comment 91

[Justin Lebar (not reading bugmail)]

Comment on attachment 771550 [details] [diff] [review]
Part I: WebIDL V6 (-U8)

I'm still underwater, but khuey has said he'll try to finish up this review.

I'm really sorry about how I've handled this bug; it shouldn't have sat for so long.

Comment 92

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

Comment on attachment 771550 [details] [diff] [review]
Part I: WebIDL V6 (-U8)

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

This looks pretty good.  I made enough comments that I want to see it again before giving r+, but I don't think there's anything fundamentally wrong with this patch.

The method of exposing properties on window.navigator has changed since you wrote this patch, unfortunately.  If you search for "NavigatorProperty" in https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings you can see the new way.  I left comments inline too.

::: dom/base/nsDOMClassInfo.cpp
@@ +1296,5 @@
>      DOM_CLASSINFO_MAP_ENTRY(nsIDOMMozNavigatorSms)
>      DOM_CLASSINFO_MAP_ENTRY(nsIDOMMozNavigatorMobileMessage)
> +#ifdef MOZ_B2G_FM
> +    DOM_CLASSINFO_MAP_ENTRY(nsIDOMNavigatorFMRadio)
> +#endif

This is all gone now.  It has been replaced by WebIDL annotations.

::: dom/bindings/Bindings.conf
@@ +389,5 @@
>  },
>  
> +'FMRadio': {
> +    'nativeType': 'mozilla::dom::fmradio::FMRadio',
> +    'headerFile': 'FMRadio.h'

Can we name the type mozilla::dom::FMRadio and put the header in mozilla/dom?

::: dom/fmradio/FMRadio.cpp
@@ +135,5 @@
> +    case FrequencyChanged:
> +      DispatchTrustedEvent(NS_LITERAL_STRING("frequencychange"));
> +      break;
> +    case EnabledChanged:
> +      if (IFMRadioService::Singleton()->IsEnabled()) {

Just use Enabled().

@@ +146,5 @@
> +      MOZ_CRASH();
> +  }
> +}
> +
> +bool

/* static */ bool

::: dom/fmradio/FMRadio.h
@@ +27,5 @@
> +  friend class FMRadioRequest;
> +
> +public:
> +  FMRadio();
> +  ~FMRadio();

The constructor and the destructor should be private (this is only constructed from FMRadio::CheckPermissionAndCreateInstance and only destroyed from reference counting).

@@ +41,5 @@
> +  virtual void Notify(const hal::SwitchEvent& aEvent) MOZ_OVERRIDE;
> +  /* FMRadioEventObserver */
> +  virtual void Notify(const FMRadioEventType& aType) MOZ_OVERRIDE;
> +
> +  nsPIDOMWindow * GetParentObject() const

super nit: * on the left, not in the middle

@@ +49,5 @@
> +
> +  virtual JSObject* WrapObject(JSContext* aCx,
> +                               JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  bool Enabled() const;

This should be static.

@@ +88,5 @@
> +  bool mIsShutdown;
> +};
> +
> +class FMRadioRequest MOZ_FINAL : public ReplyRunnable
> +                               , public DOMRequest

This doesn't need to be in the header.  Move it into the .cpp file and then you can drop the inclusion of DOMRequest.h and FMRadioService.h from this header.

::: dom/fmradio/FMRadioCommon.h
@@ +22,5 @@
> +
> +#define BEGIN_FMRADIO_NAMESPACE \
> +  namespace mozilla { namespace dom { namespace fmradio {
> +#define END_FMRADIO_NAMESPACE \
> +  } /* namespace fmradio */ } /* namespace dom */ } /* namespace mozilla */

Let's just do mozilla::dom here.  Especially since you prefix everything with FMRadio anyways (mozilla::dom::fmradio::FMRadioEventType is a little repetitive).

::: dom/fmradio/FMRadioService.cpp
@@ +108,5 @@
> +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +  if (!obs || NS_FAILED(obs->RemoveObserver(this,
> +                                            MOZSETTINGS_CHANGED_ID))) {
> +    NS_WARNING("Can't unregister observers, or already unregistered");
> +  }

The observer service is holding a strong reference to the FMRadioService, so it cannot still be registered when the FMRadioService is being destroyed.

@@ +162,5 @@
> +    FMRadioService* fmRadioService = FMRadioService::Singleton();
> +
> +    fmRadioService->mHasReadRilSetting = true;
> +
> +    if (!fmRadioService->mPendingRequest) {

Could you store the pending request as a member variable in this class (in DEBUG builds), and then assert that fmRadioService->mPendingRequest is the request we expect here?

@@ +173,5 @@
> +      NS_DispatchToMainThread(fmRadioService->mPendingRequest);
> +
> +      // Failed to read the setting value, set the state back to Disabled.
> +      fmRadioService->SetState(Disabled);
> +      return NS_OK;

You do this pattern all over the place:

mPendingRequest->SetReply(something);
NS_DispatchToMainThread(mPendingRequest);
SetState(somethingElse);

I think you should break this into a function that takes 'something' and 'somethingElse', the response and the next state.  This will ensure that we don't dispatch the pending request and then forget to clear it by calling SetState.  Call it TransitionState or something?

@@ +189,5 @@
> +        NS_LITERAL_STRING("Airplane mode currently enabled")));
> +      NS_DispatchToMainThread(fmRadioService->mPendingRequest);
> +
> +      // Airplane mode is enabled, set the state back to Disabled.
> +      fmRadioService->SetState(Disabled);

Then you could use that function here.

@@ +202,5 @@
> +    FMRadioService* fmRadioService = FMRadioService::Singleton();
> +
> +    if (!fmRadioService->mPendingRequest) {
> +      return NS_OK;
> +    }

Same comment about asserting the request is what you expect.

@@ +207,5 @@
> +
> +    fmRadioService->mPendingRequest->SetReply(ErrorResponse(
> +      NS_LITERAL_STRING("Unexpected error")));
> +    NS_DispatchToMainThread(fmRadioService->mPendingRequest);
> +    fmRadioService->SetState(Disabled);

And here.

@@ +424,5 @@
> +
> +  SetState(Enabling);
> +  // Cache the enable request just in case disable() is called
> +  // while the FM radio HW is being enabled.
> +  mPendingRequest = aReplyRunnable;

After this point we need to null out mPendingRequest if we fail aReplyRunnable, right?  I think you should use mPendingRequest everywhere you use aReplyRunnable after this point too.

@@ +438,5 @@
> +    nsresult rv = settings->CreateLock(getter_AddRefs(settingsLock));
> +    if (NS_FAILED(rv)) {
> +      aReplyRunnable->SetReply(ErrorResponse(
> +        NS_LITERAL_STRING("Can't create settings lock")));
> +      NS_DispatchToMainThread(aReplyRunnable);

So this should probably use that function I mentioned earlier (TransitionState) to clear mPendingRequest (and set the state back to Disabled, I think).

@@ +446,5 @@
> +    rv = settingsLock->Get(SETTING_KEY_RIL_RADIO_DISABLED, callback);
> +    if (NS_FAILED(rv)) {
> +      aReplyRunnable->SetReply(ErrorResponse(
> +        NS_LITERAL_STRING("Can't get settings value")));
> +      NS_DispatchToMainThread(aReplyRunnable);

same here.

@@ +491,5 @@
> +  // event will be fired, execute the seek callback manually.
> +  if (mState == Seeking) {
> +    mPendingRequest->SetReply(ErrorResponse(
> +      NS_LITERAL_STRING("Seek action is cancelled")));
> +    NS_DispatchToMainThread(mPendingRequest);

This should use TransitionState too, probably.

@@ +504,5 @@
> +    // If the radio is currently enabling, we fire the error callback on the
> +    // enable request immediately. When the radio finishes enabling, we'll call
> +    // DoDisable and fire the success callback on the disable request.
> +    enablingRequest->SetReply(
> +      ErrorResponse(NS_LITERAL_STRING("Seek action is cancelled")));

This isn't a seek, is it?  Didn't we just handle that above?

@@ +508,5 @@
> +      ErrorResponse(NS_LITERAL_STRING("Seek action is cancelled")));
> +    NS_DispatchToMainThread(enablingRequest);
> +
> +    // If we havn't read the ril settings yet which means we won't enable
> +    // the FM radio HW, fail the disable request immediately.

If we haven't read the ril settings yet we won't enable the FM radio HW, so fail the disable request immediately.

@@ +518,5 @@
> +        NS_DispatchToMainThread(aReplyRunnable);
> +      }
> +    }
> +
> +    return;

We don't call DoDisable() if we were Enabling before?

What should mPendingRequest be when we leave this function?

@@ +565,5 @@
> +      CancelFMRadioSeek();
> +      mPendingRequest->SetReply(
> +        ErrorResponse(NS_LITERAL_STRING("Seek action is cancelled")));
> +      NS_DispatchToMainThread(mPendingRequest);
> +      SetState(Enabled);

TransitionState.

@@ +583,5 @@
> +
> +  aReplyRunnable->SetReply(SuccessResponse());
> +  NS_DispatchToMainThread(aReplyRunnable);
> +
> +  NS_DispatchToMainThread(new SetFrequencyRunnable(this, roundedFrequency));

We should probably do these in the opposite order ... set the frequency before telling the reply runnable that we succeeded, even if we know that setting the frequency can't fail.

@@ +645,5 @@
> +  mPendingRequest->SetReply(
> +    ErrorResponse(NS_LITERAL_STRING("Seek action is cancelled")));
> +  NS_DispatchToMainThread(mPendingRequest);
> +
> +  SetState(Enabled);

TransitionState.

@@ +659,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(sFMRadioService);
> +
> +  if (strcmp(aTopic, MOZSETTINGS_CHANGED_ID)) {

I know this is pedantic, but let's do if (strcmp(aTopic, MOZSETTINGS_CHANGED_ID) != 0).

I think it's clearer that we're only interested in MOZSETTINGS_CHANGED_ID that way.

@@ +734,5 @@
> +      // Fire success callback on the enable request.
> +      mPendingRequest->SetReply(SuccessResponse());
> +      NS_DispatchToMainThread(mPendingRequest);
> +
> +      SetState(Enabled);

TransitionState.

@@ +758,5 @@
> +        mPendingRequest->SetReply(SuccessResponse());
> +        NS_DispatchToMainThread(mPendingRequest);
> +      }
> +
> +      SetState(Disabled);

TransitionState.

@@ +768,5 @@
> +      if (mState == Seeking) {
> +        mPendingRequest->SetReply(SuccessResponse());
> +        NS_DispatchToMainThread(mPendingRequest);
> +
> +        SetState(Enabled);

TransitionState.

::: dom/fmradio/FMRadioService.h
@@ +86,5 @@
> + */
> +class IFMRadioService
> +{
> +public:
> +  virtual ~IFMRadioService() { }

The destructor should be protected.

@@ +88,5 @@
> +{
> +public:
> +  virtual ~IFMRadioService() { }
> +
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(IFMRadioService)

This isn't actually used.  FMRadioService implements nsISupports and those AddRef/Release will shadow this.  FMRadioChild is alloc/dealloced by IPDL and isn't refcounted.

@@ +193,5 @@
> +  nsRefPtr<ReplyRunnable> mPendingRequest;
> +
> +  FMRadioEventObserverList mObserverList;
> +
> +private:

Don't need this extra 'private:'.

::: dom/fmradio/nsIDOMNavigatorFMRadio.idl
@@ +4,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(c96434a5-7001-47e8-ae6b-d997bdeb83ba)]
> +interface nsIDOMNavigatorFMRadio : nsISupports

And this interface can go away.

::: dom/webidl/FMRadio.webidl
@@ +1,5 @@
> +/* 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/. */
> +
> +interface FMRadio : EventTarget {

And above the interface declaration you want [NavigatorProperty="mozFMRadio"] or whatever it is supposed to be.

Comment 93

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

Comment on attachment 771551 [details] [diff] [review]
Part II: IPDL V4 (-U8)

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

r=me with the nits addressed.

::: dom/fmradio/ipc/FMRadioChild.h
@@ +7,5 @@
> +#ifndef mozilla_dom_fmradio_ipc_fmradiochild_h__
> +#define mozilla_dom_fmradio_ipc_fmradiochild_h__
> +
> +#include "FMRadioCommon.h"
> +#include "DOMRequest.h"

You don't need to include DOMRequest.h here.

::: dom/fmradio/ipc/FMRadioParent.h
@@ +8,5 @@
> +#define mozilla_dom_fmradio_ipc_fmradioparent_h__
> +
> +#include "FMRadioCommon.h"
> +#include "mozilla/dom/fmradio/PFMRadioParent.h"
> +#include "mozilla/dom/ContentChild.h"

You don't need to include ContentChild.h here.

Comment 94

[Pin Zhang [:pzhang] (inactive)]

> @@ +49,5 @@
> > +
> > +  virtual JSObject* WrapObject(JSContext* aCx,
> > +                               JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> > +
> > +  bool Enabled() const;
> 
> This should be static.
Why? 

> ::: dom/fmradio/FMRadioService.cpp
> @@ +108,5 @@
> > +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> > +  if (!obs || NS_FAILED(obs->RemoveObserver(this,
> > +                                            MOZSETTINGS_CHANGED_ID))) {
> > +    NS_WARNING("Can't unregister observers, or already unregistered");
> > +  }
> 
> The observer service is holding a strong reference to the FMRadioService, so
> it cannot still be registered when the FMRadioService is being destroyed.
> 
Did you mean we don't have to remove observer here?

> @@ +518,5 @@
> > +        NS_DispatchToMainThread(aReplyRunnable);
> > +      }
> > +    }
> > +
> > +    return;
> 
> We don't call DoDisable() if we were Enabling before?
> 
Currently, we cann't stop it when the FM radio HW is enabling, we have to wait for ENABLED event from HAL part, and disable it then.

> What should mPendingRequest be when we leave this function?
> 
|mPendingRequest| should be the |aReplyResponse|

BTW, I hate rebase...

Comment 95

[Pin Zhang [:pzhang] (inactive)]

> ::: dom/fmradio/FMRadio.cpp
> @@ +135,5 @@
> > +    case FrequencyChanged:
> > +      DispatchTrustedEvent(NS_LITERAL_STRING("frequencychange"));
> > +      break;
> > +    case EnabledChanged:
> > +      if (IFMRadioService::Singleton()->IsEnabled()) {
> 
> Just use Enabled().

If changed to Enabled, it conflicts with the the enum FMRadioState::Enabled, any ideas?

Comment 96

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

(In reply to Pin Zhang [:pzhang] from comment #94)
> > @@ +49,5 @@
> > > +
> > > +  virtual JSObject* WrapObject(JSContext* aCx,
> > > +                               JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> > > +
> > > +  bool Enabled() const;
> > 
> > This should be static.
> Why? 

Because it can be.  You're not using anything from the class so why not make it static.

> > ::: dom/fmradio/FMRadioService.cpp
> > @@ +108,5 @@
> > > +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> > > +  if (!obs || NS_FAILED(obs->RemoveObserver(this,
> > > +                                            MOZSETTINGS_CHANGED_ID))) {
> > > +    NS_WARNING("Can't unregister observers, or already unregistered");
> > > +  }
> > 
> > The observer service is holding a strong reference to the FMRadioService, so
> > it cannot still be registered when the FMRadioService is being destroyed.
> > 
> Did you mean we don't have to remove observer here?

Yes.

> > @@ +518,5 @@
> > > +        NS_DispatchToMainThread(aReplyRunnable);
> > > +      }
> > > +    }
> > > +
> > > +    return;
> > 
> > We don't call DoDisable() if we were Enabling before?
> > 
> Currently, we cann't stop it when the FM radio HW is enabling, we have to
> wait for ENABLED event from HAL part, and disable it then.

Hmm, ok.

> > What should mPendingRequest be when we leave this function?
> > 
> |mPendingRequest| should be the |aReplyResponse|
> 
> BTW, I hate rebase...

Yeah, I know.  I promise I will review the updated patch quickly.

(In reply to Pin Zhang [:pzhang] from comment #95)
> > ::: dom/fmradio/FMRadio.cpp
> > @@ +135,5 @@
> > > +    case FrequencyChanged:
> > > +      DispatchTrustedEvent(NS_LITERAL_STRING("frequencychange"));
> > > +      break;
> > > +    case EnabledChanged:
> > > +      if (IFMRadioService::Singleton()->IsEnabled()) {
> > 
> > Just use Enabled().
> 
> If changed to Enabled, it conflicts with the the enum FMRadioState::Enabled,
> any ideas?

Rename the function to IsEnabled?

Comment 97

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL V7

Comment 98

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part II: IPDL V5

Addressed issues mentioned in comment 92 and comment 93.

Comment 99

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

Comment on attachment 794689 [details] [diff] [review]
Part II: IPDL V5

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

r=me

Comment 100

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

Comment on attachment 794686 [details] [diff] [review]
Part I: WebIDL V7

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

Some of these assertions you added don't compile ... please build with --enable-debug and test this before landing.

r=me with the following changes.

::: b2g/installer/package-manifest.in
@@ +177,1 @@
>  #endif

We can drop this entirely since there are no XPIDL interfaces left, right?

::: dom/bindings/Bindings.conf
@@ +411,5 @@
>  
> +'FMRadio': {
> +    'nativeType': 'mozilla::dom::FMRadio',
> +    'headerFile': 'mozilla/dom/FMRadio.h'
> +},

These are the default values, so I think you can remove FMRadio from Bindings.conf entirely (entries are only needed to override the defaults).

::: dom/fmradio/FMRadio.cpp
@@ +287,5 @@
> +
> +NS_IMPL_ADDREF_INHERITED(FMRadio, nsDOMEventTargetHelper)
> +NS_IMPL_RELEASE_INHERITED(FMRadio, nsDOMEventTargetHelper)
> +
> +NS_IMPL_ISUPPORTS_INHERITED0(FMRadioRequest, DOMRequest)

Move this up to be with the FMRadioRequest class.

::: dom/fmradio/FMRadio.h
@@ +9,5 @@
> +#include "FMRadioCommon.h"
> +#include "nsDOMEventTargetHelper.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "mozilla/HalTypes.h"
> +#include "DOMRequest.h"

You can remove DOMRequest.h now that you moved FMRadioRequest out of this header.  Just forward declare DOMRequest.

::: dom/fmradio/FMRadioService.cpp
@@ +153,5 @@
> +  Handle(const nsAString& aName, const JS::Value& aResult)
> +  {
> +    MOZ_ASSERT(mPendingRequest == fmRadioService->mPendingRequest);
> +
> +    FMRadioService* fmRadioService = FMRadioService::Singleton();

I would be very curious to know how this compiles ;-)

You should rearrange these.

@@ +185,5 @@
> +  HandleError(const nsAString& aName)
> +  {
> +    MOZ_ASSERT(mPendingRequest == fmRadioService->mPendingRequest);
> +
> +    FMRadioService* fmRadioService = FMRadioService::Singleton();

And here.

::: dom/fmradio/moz.build
@@ +7,5 @@
> +DIRS += [
> +    'ipc',
> +]
> +
> +XPIDL_MODULE = 'dom_fmradio'

There are no longer any XPIDL interfaces here, so you can drop this.

Comment 101

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL for check-in

Rebased again, this is the patch for check-in.

Comment 102

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part II: IPDL for check-in

Comment 103

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 795297 [details] [diff] [review]
Part I: WebIDL for check-in

Fixed issues listed in comment 100.

Comment 104

[Pin Zhang [:pzhang] (inactive)]

:khuey, I have passed all the tests (attachment 756482 [details] [diff] [review]) on device I worte, those tests only works on device, since we don't have any radio backend for Emulator (bug 872417), and as comment 10 described, the tests might have problems under multi-process testing enviroment, and it seems that we don't run tests in multi-process (comment 38 of Bug 862672), I prefer checking the tests first and open another bug to track multi-process-support issue for those tests, what do you think?

Comment 105

[Pin Zhang [:pzhang] (inactive)]

/builds/slave/try-osx64-d-000000000000000000/build/dom/ipc/PContent.ipdl:14: error: can't locate include file `PFMRadio.ipdl'

I seems that there are problem with the build path for other mozapps like browser, :khuey, any ideas?

Comment 106

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

(In reply to Pin Zhang [:pzhang] from comment #104)
> :khuey, I have passed all the tests (attachment 756482 [details] [diff] [review]
> [review]) on device I worte, those tests only works on device, since we
> don't have any radio backend for Emulator (bug 872417), and as comment 10
> described, the tests might have problems under multi-process testing
> enviroment, and it seems that we don't run tests in multi-process (comment
> 38 of Bug 862672), I prefer checking the tests first and open another bug to
> track multi-process-support issue for those tests, what do you think?

That's fine.

(In reply to Pin Zhang [:pzhang] from comment #105)
> /builds/slave/try-osx64-d-000000000000000000/build/dom/ipc/PContent.ipdl:14:
> error: can't locate include file `PFMRadio.ipdl'
> 
> I seems that there are problem with the build path for other mozapps like
> browser, :khuey, any ideas?

Ah, I probably should have caught this during review.  The IPDL for FMRadio needs to be built regardless of whether MOZ_B2G_FM is set.  Look at bluetooth (http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/moz.build) for an example of what the moz.build should look like with that.

Comment 107

[Pin Zhang [:pzhang] (inactive)]

The TBPL[1] shows there are 4 ORANGEs, for the first three, I didn't aware of the connection between the failures and the patches I submitted, and for the last one:
  ########################B2GRunner TEST-UNEXPECTED-FAIL | /tests/dom/permission/tests/test_fmradio.html | application timed out after None seconds with no output
I have no idea what happened, and [2] is the what I modified on test_fmradio.html.

:khuey, could you take a look?

[1] https://tbpl.mozilla.org/?tree=Try&rev=35ed03cc586d
[2] https://hg.mozilla.org/try/rev/1d1efce653ef#l28.8

Comment 108

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

The others are probably unrelated.

As for the test_fmradio failure:

21:25:07     INFO -  08-27 04:18:12.240   694   694 I GeckoDump: 9968 INFO TEST-START | /tests/dom/permission/tests/test_fmradio.html
21:25:07     INFO -  08-27 04:18:13.130   694   694 I GeckoDump: 9969 INFO TEST-PASS | /tests/dom/permission/tests/test_fmradio.html | Doesn't have fmradio
21:25:07     INFO -  08-27 04:18:13.170   694   694 I GeckoDump: 9970 INFO TEST-PASS | /tests/dom/permission/tests/test_fmradio.html | Did not receive proper object
21:25:07     INFO -  08-27 04:18:13.470   694   694 I GeckoDump: 9971 INFO TEST-PASS | /tests/dom/permission/tests/test_fmradio.html | Has fmradio
21:25:07     INFO -  08-27 04:18:13.490   694   694 I FMRadio : FMRadio is initialized.
21:25:07     INFO -  08-27 04:18:13.500   656   656 I Gecko   : Security problem: Content process does not have `fmradio'.  It will be killed.

So for some reason Gecko thinks the subframe doesn't have the fmradio permission.  It's not clear to me how the permission checking changed though ...

There is a permissions related flag in these tests[0], try flipping that and see what happens?

[0] http://mxr.mozilla.org/mozilla-central/search?string=needParentPerm&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 110

[Pin Zhang [:pzhang] (inactive)]

I am trying to re-test with needParentPerm=true on the failed platform, I choose B2G platforms to run those tests, but it[1] seems that the platform |B2G Emu (VM) opt| is not selected, I can't find it either on TryChooser[2], do you know the platform id for |B2G Emu (VM) opt|?

[1] https://tbpl.mozilla.org/?tree=Try&rev=94c5f10c2275
[2] http://trychooser.pub.build.mozilla.org/

Comment 111

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

No idea.  I'd just check all of the b2g platforms (the last 4 checkboxes underneath 'platform') :-P

Comment 112

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

Oh, "B2G Emu (VM) opt" is just tests run on the build from "B2G Emu opt" so it won't appear until the tests start running.  You've got the "B2G Emu opt" build though so you chose the right options.

Comment 113

[Pin Zhang [:pzhang] (inactive)]

I see, thanks, let me have a cup of tea and see what happens, :)

Comment 114

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

(In reply to Pin Zhang [:pzhang] from comment #113)
> I see, thanks, let me have a cup of tea and see what happens, :)

I kicked off extra test runs too.  4 of the 6 went green, the other 2 are known intermittent failures.  I'll file a separate bug on figuring out why we need this change.

Comment 115

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

Filed bug 909823 on that.

Comment 116

[Pin Zhang [:pzhang] (inactive)]

There are 4 orange and 1 blue[1] which I think are unrelated, let me upload the latest patches for checking in.


[1] https://tbpl.mozilla.org/?tree=Try&rev=771dc045d703

Comment 117

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL for check-in

Comment 118

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part II: IPDL for check-in

Comment 119

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part III: Marionette tests

Comment 120

[Pin Zhang [:pzhang] (inactive)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #100)
> 
> ::: dom/bindings/Bindings.conf
> @@ +411,5 @@
> >  
> > +'FMRadio': {
> > +    'nativeType': 'mozilla::dom::FMRadio',
> > +    'headerFile': 'mozilla/dom/FMRadio.h'
> > +},
> 
> These are the default values, so I think you can remove FMRadio from
> Bindings.conf entirely (entries are only needed to override the defaults).
> 

The patch still contains this ... let me remove it and submit again, sorry

Comment 121

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL for check-in

Comment 122

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL for check-in

Comment 123

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part III: Marionette tests

Comment 124

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

Comment on attachment 796409 [details] [diff] [review]
Part III: Marionette tests

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

I don't think I'm qualified to review these tests, since I know nothing about marionette or the API being tested ;-)

Comment 125

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 796409 [details] [diff] [review]
Part III: Marionette tests

Hi Tim, can you review the tests? Please check comment 10 and comment 104, thanks.

Comment 126

[Pin Zhang [:pzhang] (inactive)]

I think Bug 867831 is the tracking bug for multi-process support of Marionette.

Comment 127

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 796409 [details] [diff] [review]
Part III: Marionette tests

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

I don't think I'm qualified to review these tests either. Sorry about that.

Comment 128

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 796409 [details] [diff] [review]
Part III: Marionette tests

Hi Justin, I am sorry to come back to you, I can't find anyone else who might be able to review this patch.

Comment 129

[Pin Zhang [:pzhang] (inactive)]

:khuey, can we just check in the r+ patches first?

Comment 130

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

(In reply to Pin Zhang [:pzhang] from comment #129)
> :khuey, can we just check in the r+ patches first?

Yes, please :-)

Comment 131

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

I'll land parts 1 and 2 once I get to the office.

Comment 132

[Justin Lebar (not reading bugmail)]

(In reply to Pin Zhang [:pzhang] from comment #128)
> Comment on attachment 796409 [details] [diff] [review]
> Part III: Marionette tests
> 
> Hi Justin, I am sorry to come back to you, I can't find anyone else who
> might be able to review this patch.

Okay; you've got me for three more days!  :)

Comment 133

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

https://hg.mozilla.org/integration/b2g-inbound/rev/05529a089e45
https://hg.mozilla.org/integration/b2g-inbound/rev/8ed8e41fb68c

Comment 134

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

Apparently hg doesn't like adding files ...

I backed out and tried again:

https://hg.mozilla.org/integration/b2g-inbound/rev/8870aa561d0b
https://hg.mozilla.org/integration/b2g-inbound/rev/44d7d636eb34
https://hg.mozilla.org/integration/b2g-inbound/rev/d495cd4206fa

Comment 135

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

And then I realized I screwed up the patch author so I landed it again :-P

https://hg.mozilla.org/integration/b2g-inbound/rev/bd27a92ce16a
https://hg.mozilla.org/integration/b2g-inbound/rev/cd99d10fc665
https://hg.mozilla.org/integration/b2g-inbound/rev/5a71d3bf48ce

The third time is the charm right?

Comment 136

[Pin Zhang [:pzhang] (inactive)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #135)
> And then I realized I screwed up the patch author so I landed it again :-P
> 
> https://hg.mozilla.org/integration/b2g-inbound/rev/bd27a92ce16a
> https://hg.mozilla.org/integration/b2g-inbound/rev/cd99d10fc665
> https://hg.mozilla.org/integration/b2g-inbound/rev/5a71d3bf48ce
> 
> The third time is the charm right?

Thanks man!

Comment 137

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #132)
> (In reply to Pin Zhang [:pzhang] from comment #128)
> > Comment on attachment 796409 [details] [diff] [review]
> > Part III: Marionette tests
> > 
> > Hi Justin, I am sorry to come back to you, I can't find anyone else who
> > might be able to review this patch.
> 
> Okay; you've got me for three more days!  :)

Justin, thanks!

Comment 138

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

(In reply to Pin Zhang [:pzhang] from comment #136)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #135)
> > And then I realized I screwed up the patch author so I landed it again :-P
> > 
> > https://hg.mozilla.org/integration/b2g-inbound/rev/bd27a92ce16a
> > https://hg.mozilla.org/integration/b2g-inbound/rev/cd99d10fc665
> > https://hg.mozilla.org/integration/b2g-inbound/rev/5a71d3bf48ce
> > 
> > The third time is the charm right?
> 
> Thanks man!

No problem.  Thanks for sticking with this.

Comment 139

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cd99d10fc665
https://hg.mozilla.org/mozilla-central/rev/5a71d3bf48ce

Comment 140

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

Pin, can you take before and after measurements to see how much memory this saved?

Something like https://bugzilla.mozilla.org/show_bug.cgi?id=864932#c28

Comment 141

[Pin Zhang [:pzhang] (inactive)]

Attachment: baseline memory reports

Comment 142

[Phil Ringnalda (:philor)]

Bug 870676, meet bug 875934 (in merges across four trees).

https://hg.mozilla.org/mozilla-central/rev/a9ffc1cdb1ae

Comment 143

[Pin Zhang [:pzhang] (inactive)]

Attachment: rewrite memory reports

I got the reports by |get_about_memory.py -m| after Unagi rebooted and the FM radio app openned, and here is the diff result:

Main Process
-0.57 MB ── gfx-surface-image
 0.14 MB ── heap-allocated
 0.57 MB ── heap-committed
   1.79% ── heap-overhead-ratio
-1.16 MB ── images-content-used-uncompressed
      -9 ── page-faults-hard
   3,819 ── page-faults-soft
 1.91 MB ── resident
 1.73 MB ── resident-unique
 0.30 MB ── storage-sqlite
 0.69 MB ── vsize

FM Radio
   -0.01 MB ── heap-allocated
-0.21 MB ── heap-committed
  -3.75% ── heap-overhead-ratio
     317 ── page-faults-soft
-0.86 MB ── resident
-1.02 MB ── resident-unique
-4.05 MB ── vsize

I seems that FM radio app memory usage decreased, however main process memory usage increased.

Comment 144

[Justin Lebar (not reading bugmail)]

Maybe we can hope that this is all noise.  :)  I didn't expect this to have a big effect on memory usage, but if the main process's memory usage /increased/, that would be pretty unexpected.

Comment 145

[Marco Castelluccio [:marco]]

I think I'm seeing build problems due to this patch: https://tbpl.mozilla.org/?tree=Try&rev=48c266b6804b

Comment 146

[Pin Zhang [:pzhang] (inactive)]

(In reply to Marco Castelluccio [:marco] from comment #145)
> I think I'm seeing build problems due to this patch:
> https://tbpl.mozilla.org/?tree=Try&rev=48c266b6804b
see comment 142?

Comment 147

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #144)
> Maybe we can hope that this is all noise.  :)  I didn't expect this to have
> a big effect on memory usage, but if the main process's memory usage
> /increased/, that would be pretty unexpected.

I collected the memory reports twice from the same rewrite-build, and diff shows:
Main process
-0.61 MB ── gfx-surface-image
 1.12 MB ── gralloc
-0.53 MB ── heap-allocated
-0.45 MB ── heap-committed
   0.64% ── heap-overhead-ratio
-0.62 MB ── images-content-used-uncompressed
      -2 ── page-faults-hard
 -32,564 ── page-faults-soft
-0.49 MB ── resident
-0.39 MB ── resident-unique
 3.82 MB ── vsize

FM Radio
-0.21 MB ── heap-allocated
-0.17 MB ── heap-committed
   1.08% ── heap-overhead-ratio
      -1 ── page-faults-hard
  -8,230 ── page-faults-soft
-0.34 MB ── resident
-0.16 MB ── resident-unique
 3.68 MB ── vsize

it's kind of disappointed, :(, I think we can't make any conclusion that we improved the memory usage or make it worse by rewriting the WebFM in c++, so maybe as you said, this is all noise.

Comment 148

[Ed Morley [:emorley]]

This needs rebasing on top of the changes in bug 875934; attempts at doing so as trees were merged resulted in breakage (eg https://tbpl.mozilla.org/php/getParsedLog.php?id=27162455&tree=Mozilla-Central), so I had to back this out:

remote:   https://hg.mozilla.org/mozilla-central/rev/fea293e03def
remote:   https://hg.mozilla.org/mozilla-central/rev/b548dc7b068a
remote:   https://hg.mozilla.org/mozilla-central/rev/c101b643f160
remote:   https://hg.mozilla.org/mozilla-central/rev/ac628666aa2a
remote:   https://hg.mozilla.org/mozilla-central/rev/0e1efad50b9f

Comment 149

[Justin Lebar (not reading bugmail)]

Comment on attachment 796409 [details] [diff] [review]
Part III: Marionette tests

We need to make sure that all of these tests run twice, once in the main
process and once in a child process.

>diff --git a/dom/fmradio/test/marionette/manifest.ini b/dom/fmradio/test/marionette/manifest.ini
>new file mode 100644
>--- /dev/null
>+++ b/dom/fmradio/test/marionette/manifest.ini

>@@ -0,0 +1,13 @@
>+[DEFAULT]
>+b2g = true
>+browser = false
>+qemu = false

Nit: Maybe add a comment here explaining why we can't use QEMU?

>diff --git a/dom/fmradio/test/marionette/test_bug862672.js b/dom/fmradio/test/marionette/test_bug862672.js
>new file mode 100644
>--- /dev/null
>+++ b/dom/fmradio/test/marionette/test_bug862672.js

>@@ -0,0 +1,46 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+MARIONETTE_TIMEOUT = 10000;
>+
>+SpecialPowers.addPermission("fmradio", true, document);
>+
>+let FMRadio = window.navigator.mozFMRadio;
>+
>+function verifyInitialState() {
>+  log("Verifying initial state.");
>+  ok(FMRadio, "FMRadio");
>+  is(FMRadio.enabled, false, "FMRadio.enabled");

In our other test harnesses, when a test fails it tells us which line it failed
on.  If that's true in Marionette, I'm not sure that the strings here which
just repeat the condition are useful.

>+  enableThenDisable();
>+}
>+
>+function enableThenDisable() {
>+  log("Enable FM Radio and disable it immediately.");
>+  var request = FMRadio.enable(90);
>+  ok(request, "request should not be null.");
>+
>+  request.onsuccess = function() {
>+    ok(false, "Enable request should fail.");
>+  };
>+
>+  var failedToEnable = false;
>+  request.onerror = function() {
>+    failedToEnable = true;
>+  };
>+
>+  var disableReq = FMRadio.disable();
>+  ok(disableReq, "Disable request should not be null.");
>+
>+  disableReq.onsuccess = function() {
>+    ok(failedToEnable, "Enable request failed.");
>+    ok(!FMRadio.enabled, "FMRadio.enabled is false.");
>+    finish();
>+  };
>+
>+  disableReq.onerror = function() {
>+    ok(false, "Disable request should not fail.");
>+  };
>+}

I still have the concern about these tests from bug 862672 comment 26.  Unless
the implementation has changed significantly, I don't think this is correct.

Last time I looked at this code, the following ordering was possible:

  website: FMRadio.enable();
  gecko: Start enabling FM radio on separate thread.
  gecko: Finish enabling FM radio on separate thread, post "enable successful" message to the main thread's event loop.
  website: FMRadio.disable();
  gecko: Post "disable successful" message to main thread's event loop.

I think this is also possible if this test runs in a child process:

  website: FMRadio.enable();
  gecko child: Send "enable FM radio" message to parent
  gecko parent: Send "FM radio enabled" message to child
  gecko child: Post "enable successful" message to the main thread's event loop.
  website: FMRadio.disable();
  (and so on)

If this ordering occurs, this test will fail.

It seems that the only thing we can check here is that either

  - enable fails (as tested here), or
  - enable's onsuccess fires before disable's onsuccess

But this is still tricky, because when enable's onsuccess fires, should
FMRadio.enabled be true?  As currently written, it might not be.  I'm not sure
whether we care.

>diff --git a/dom/fmradio/test/marionette/test_bug876597.js b/dom/fmradio/test/marionette/test_bug876597.js
>new file mode 100644
>--- /dev/null
>+++ b/dom/fmradio/test/marionette/test_bug876597.js

This one looks good.

>+function cleanUp() {

We should ask the marionette folks if there's a way to ensure that this code
gets run, even when the test fails or times out.  As is, we won't run this
code, and that creates a situation where many FM radio tests after this one
might also fail.

>diff --git a/dom/fmradio/test/marionette/test_cancel_seek.js b/dom/fmradio/test/marionette/test_cancel_seek.js
>new file mode 100644
>--- /dev/null
>+++ b/dom/fmradio/test/marionette/test_cancel_seek.js

I think this has a similar race condition.

>diff --git a/dom/fmradio/test/marionette/test_enable_disable.js b/dom/fmradio/test/marionette/test_enable_disable.js
>new file mode 100644
>--- /dev/null
>+++ b/dom/fmradio/test/marionette/test_enable_disable.js

>@@ -0,0 +1,97 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+MARIONETTE_TIMEOUT = 10000;
>+
>+SpecialPowers.addPermission("fmradio", true, document);
>+
>+let FMRadio = window.navigator.mozFMRadio;
>+
>+function verifyInitialState() {
>+  log("Verifying initial state.");
>+  ok(FMRadio, "FMRadio");
>+  is(FMRadio.enabled, false, "FMRadio.enabled");
>+  verifyAttributesWhenDisabled();
>+  enableFMRadio();
>+}
>+
>+function verifyAttributesWhenDisabled() {

This function is used only once; maybe it should be inlined?

>+  log("Verifying attributes when disabled.");
>+  is(FMRadio.frequency, null, "FMRadio.frequency == null");
>+  ok(FMRadio.frequencyLowerBound, "FMRadio.frequencyLowerBound");
>+  ok(FMRadio.frequencyUpperBound, "FMRadio.frequencyUpperBound");
>+  ok(FMRadio.frequencyUpperBound > FMRadio.frequencyLowerBound,
>+    "FMRadio.frequencyUpperBound > FMRadio.frequencyLowerBound");
>+  ok(FMRadio.channelWidth, "FMRadio.channelWidth")
>+}

>+function enableFMRadio() {
>+  log("Verifying behaviors when enabled.");
>+  var request = FMRadio.enable(90);
>+  ok(request, "FMRadio.enable(90) returns request");
>+
>+  request.onsuccess = function() {
>+    ok(FMRadio.enabled, "FMRadio.enabled");
>+    ok(!isNaN(FMRadio.frequency), "FMRadio.frequency is a number");

This doesn't quite get what you want, because isNaN(null) is false.

>+    ok(FMRadio.frequency > FMRadio.frequencyLowerBound,
>+      "FMRadio.frequency > FMRadio.frequencyLowerBound");

>= ?

>+  var seekRequest = FMRadio.seekUp();
>+
>+  seekRequest.onerror = function() {
>+    results.seekErrorFired = true;
>+    checkResults();
>+  };
>+
>+  seekRequest.onsuccess = function() {
>+    ok(false, "Seek request should fail after FMRadio.disable is called.");
>+  };
>+
>+  FMRadio.disable();
>+  FMRadio.ondisabled = function() {
>+    FMRadio.ondisabled = null;
>+    results.onDisabledEventFired = true;
>+    ok(!FMRadio.enabled, "FMRadio.enabled is false");
>+    checkResults();
>+  };
>+}

Similarly I think this part with the seek is racy.

I'm going to stop reviewing here, since it seems there's enough to work through.

Comment 150

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part I: WebIDL for check-in, rebased on top of bug 875934

Comment 151

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part II: IPDL for check-in, rebased on top of bug 875934

Hi, :khuey, I fixed the build settings and added some missed header files for bug 875934, didn't push the new changesets to TryServer yet, but I would be happy to do that if you thought it's necessary.

Comment 152

[Pin Zhang [:pzhang] (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #149)
> Comment on attachment 796409 [details] [diff] [review]
> Part III: Marionette tests
> 
> Similarly I think this part with the seek is racy.
> 
> I'm going to stop reviewing here, since it seems there's enough to work
> through.

Let me open another bug on the marionette tests for WebFM.

Comment 153

[Pin Zhang [:pzhang] (inactive)]

(In reply to Pin Zhang [:pzhang] from comment #151)
> Created attachment 797715 [details] [diff] [review]
> Part II: IPDL for check-in, rebased on top of bug 875934
> 
> Hi, :khuey, I fixed the build settings and added some missed header files
> for bug 875934, didn't push the new changesets to TryServer yet, but I would
> be happy to do that if you thought it's necessary.

BTW, I renamed mozilla::dom::ReplyRunnable to mozilla::dom::FMRadioReplyRunnable.

Comment 154

[Pin Zhang [:pzhang] (inactive)]

(In reply to Pin Zhang [:pzhang] from comment #153)
> (In reply to Pin Zhang [:pzhang] from comment #151)
> > Created attachment 797715 [details] [diff] [review]
> > Part II: IPDL for check-in, rebased on top of bug 875934
> > 
> > Hi, :khuey, I fixed the build settings and added some missed header files
> > for bug 875934, didn't push the new changesets to TryServer yet, but I would
> > be happy to do that if you thought it's necessary.
> 
> BTW, I renamed mozilla::dom::ReplyRunnable to
> mozilla::dom::FMRadioReplyRunnable.

Hi, :khuey, any plan to review the rebased patches?

Comment 155

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

Hi Pin,

Yesterday was a holiday in the US.  I will review and land them today.

Comment 156

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

https://hg.mozilla.org/integration/b2g-inbound/rev/8c448e11da5c
https://hg.mozilla.org/integration/b2g-inbound/rev/5f0cdd85e5a4

Comment 157

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8c448e11da5c
https://hg.mozilla.org/mozilla-central/rev/5f0cdd85e5a4