Closed
Bug 870676
Opened 11 years ago
Closed 10 years ago
[FMRadio] Rewrite the whole FMRadio API in C++
Categories
(Firefox OS Graveyard :: Gaia::FMRadio, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: pzhang, Assigned: pzhang)
References
Details
(Whiteboard: [MemShrink:P3])
Attachments
(5 files, 30 obsolete files)
14.76 KB,
patch
|
justin.lebar+bug
:
review-
|
Details | Diff | Splinter Review |
1.35 MB,
text/plain
|
Details | |
1.36 MB,
text/plain
|
Details | |
117.46 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
31.51 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•11 years ago
|
Comment 1•11 years ago
|
||
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.
Whiteboard: [MemShrink:P2] → [MemShrink]
Updated•11 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P3]
Assignee | ||
Comment 2•11 years ago
|
||
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?
Attachment #754782 -
Flags: feedback?(justin.lebar+bug)
Attachment #754782 -
Flags: feedback?(amarchesini)
Assignee | ||
Comment 3•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
(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
Updated•11 years ago
|
Summary: [MemShrink] [FMRadio] Rewrite the whole FMRadio API in C++ → [FMRadio] Rewrite the whole FMRadio API in C++
Comment 6•11 years ago
|
||
Comment on attachment 754782 [details] [diff] [review] WIP Patch baku, would you mind doing a first pass on this patch?
Assignee | ||
Comment 7•11 years ago
|
||
Fix Bug 876597
Attachment #754782 -
Attachment is obsolete: true
Attachment #754782 -
Flags: feedback?(justin.lebar+bug)
Attachment #754782 -
Flags: feedback?(amarchesini)
Attachment #755742 -
Flags: review?(justin.lebar+bug)
Attachment #755742 -
Flags: review?(amarchesini)
Comment 8•11 years ago
|
||
yes, I'm reviewing it.
Comment 9•11 years ago
|
||
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.
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Assignee | ||
Comment 11•11 years ago
|
||
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Comment 13•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #755742 -
Attachment description: WIP Patch With Bug 876597 Fixing → WIP whole-in-one Patch
Assignee | ||
Comment 14•11 years ago
|
||
I splitted the patch into three parts as baku suggested in comment 9, hope it helps.
Comment 15•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #755742 -
Flags: review?(amarchesini)
Assignee | ||
Comment 16•11 years ago
|
||
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•11 years ago
|
||
> > 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()
Assignee | ||
Comment 18•11 years ago
|
||
(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.
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #755742 -
Attachment is obsolete: true
Attachment #755850 -
Attachment is obsolete: true
Attachment #755742 -
Flags: review?(justin.lebar+bug)
Attachment #756479 -
Flags: review?(amarchesini)
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #755852 -
Attachment is obsolete: true
Attachment #756480 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #755853 -
Attachment is obsolete: true
Assignee | ||
Comment 22•11 years ago
|
||
(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•11 years ago
|
||
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?'
Assignee | ||
Comment 24•11 years ago
|
||
(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?
Assignee | ||
Comment 25•11 years ago
|
||
(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•11 years ago
|
||
(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.
Assignee | ||
Comment 27•11 years ago
|
||
Attachment #756479 -
Attachment is obsolete: true
Attachment #756479 -
Flags: review?(amarchesini)
Attachment #757395 -
Flags: review?(amarchesini)
Assignee | ||
Comment 28•11 years ago
|
||
Attachment #756480 -
Attachment is obsolete: true
Attachment #756480 -
Flags: review?(justin.lebar+bug)
Attachment #757397 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 29•11 years ago
|
||
(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
Assignee | ||
Comment 30•11 years ago
|
||
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.
Attachment #757395 -
Attachment is obsolete: true
Attachment #757395 -
Flags: review?(amarchesini)
Attachment #757763 -
Flags: review?(amarchesini)
Assignee | ||
Comment 31•11 years ago
|
||
(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•11 years ago
|
||
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.
Attachment #757397 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 33•11 years ago
|
||
(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•11 years ago
|
||
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>
Attachment #757763 -
Flags: review?(amarchesini)
Assignee | ||
Comment 35•11 years ago
|
||
(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•11 years ago
|
||
(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.
Assignee | ||
Comment 37•11 years ago
|
||
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.
Assignee | ||
Comment 38•11 years ago
|
||
(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•11 years ago
|
||
(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.
Assignee | ||
Comment 40•11 years ago
|
||
(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•11 years ago
|
||
> 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!
Assignee | ||
Comment 42•11 years ago
|
||
Attachment #757763 -
Attachment is obsolete: true
Attachment #759133 -
Flags: review?(amarchesini)
Assignee | ||
Comment 43•11 years ago
|
||
Attachment #757397 -
Attachment is obsolete: true
Attachment #759134 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 44•11 years ago
|
||
Fixed issues addressed in comment 32 and comment 34.
Assignee | ||
Comment 45•11 years ago
|
||
ifdef MOZ_B2G_FM for FMRadio.webidl in WebIDL.mk
Attachment #759133 -
Attachment is obsolete: true
Attachment #759133 -
Flags: review?(amarchesini)
Attachment #759629 -
Flags: review?(amarchesini)
Comment 46•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #759134 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 47•11 years ago
|
||
Fixed the issues addressed in comment 46. @khuey, could you help checking the Makefile changes or reassign to someone else to take a look?
Attachment #759629 -
Attachment is obsolete: true
Attachment #759629 -
Flags: review?(amarchesini)
Attachment #760176 -
Flags: review?(khuey)
Attachment #760176 -
Flags: review?(justin.lebar+bug)
Attachment #760176 -
Flags: review?(amarchesini)
Assignee | ||
Updated•11 years ago
|
Attachment #760176 -
Attachment description: Part I: WebIDL V5 → Part II: IPDL V3
Attachment #760176 -
Flags: review?(khuey)
Attachment #760176 -
Flags: review?(amarchesini)
Assignee | ||
Comment 48•11 years ago
|
||
Attachment #759134 -
Attachment is obsolete: true
Attachment #760177 -
Flags: review?(khuey)
Attachment #760177 -
Flags: review?(justin.lebar+bug)
Attachment #760177 -
Flags: review?(amarchesini)
Assignee | ||
Updated•11 years ago
|
Attachment #760176 -
Flags: review?(khuey)
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.
Attachment #760177 -
Flags: review?(khuey) → review+
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
Attachment #760176 -
Flags: review?(khuey) → review+
Comment 51•11 years ago
|
||
> 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•11 years ago
|
||
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.
Attachment #760177 -
Flags: review?(amarchesini) → review+
Comment 53•11 years ago
|
||
Can you rebase your patch on top of mozilla-b2g18? I would like to write the AudioChannelAgent patch.
Assignee | ||
Comment 54•11 years ago
|
||
(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/
Assignee | ||
Comment 55•11 years ago
|
||
(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.
Assignee | ||
Comment 56•11 years ago
|
||
Hi Baku, this is the patch rebased on b2g18, please check if it's OK for you to write the AudioChannelAgent patch.
Comment 57•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
>+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?
Flags: needinfo?(khuey)
Comment 60•11 years ago
|
||
> 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. :)
I'm not qualified to review this either but smaug claims to be!
Flags: needinfo?(khuey)
Comment 62•11 years ago
|
||
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.
Attachment #760177 -
Flags: review?(justin.lebar+bug) → review-
Comment 63•11 years ago
|
||
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).
Flags: needinfo?(bugs)
Assignee | ||
Comment 64•11 years ago
|
||
(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•11 years ago
|
||
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.
Assignee | ||
Comment 66•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
> 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•11 years ago
|
||
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.
Assignee | ||
Comment 70•11 years ago
|
||
> >+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)
Assignee | ||
Comment 71•11 years ago
|
||
> >+ 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
Assignee | ||
Comment 72•11 years ago
|
||
> >+ 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•11 years ago
|
||
> 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•11 years ago
|
||
> |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•11 years ago
|
||
> 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.
Assignee | ||
Comment 76•11 years ago
|
||
(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.
Assignee | ||
Comment 77•11 years ago
|
||
> >+
> >+ 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•11 years ago
|
||
>> 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•11 years ago
|
||
> 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. :)
Assignee | ||
Comment 80•11 years ago
|
||
(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?
Assignee | ||
Comment 81•11 years ago
|
||
Attachment #771547 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 82•11 years ago
|
||
Fix issues addressed in comment 62/67, and rebased the code on top of the latest m-c.
Attachment #771549 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 83•11 years ago
|
||
Attachment #771547 -
Attachment is obsolete: true
Attachment #771547 -
Flags: review?(justin.lebar+bug)
Attachment #771550 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 84•11 years ago
|
||
Attachment #771549 -
Attachment is obsolete: true
Attachment #771549 -
Flags: review?(justin.lebar+bug)
Attachment #771551 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 85•11 years ago
|
||
(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•11 years ago
|
||
> How about NS_INLINE_DECL_REFCOUNTING?
Sure, that'd be fine.
Updated•11 years ago
|
Attachment #760176 -
Attachment is obsolete: true
Attachment #760176 -
Flags: review?(justin.lebar+bug)
Updated•11 years ago
|
Attachment #760177 -
Attachment is obsolete: true
Comment 87•11 years ago
|
||
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.
Assignee | ||
Comment 88•11 years ago
|
||
That's OK, I can imagine how busy you are, :)
Comment 89•11 years ago
|
||
(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.
Flags: needinfo?(bugs)
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 90•10 years ago
|
||
Hi, Justin, do you have plan to start the next round review?
Comment 91•10 years ago
|
||
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.
Attachment #771550 -
Flags: review?(justin.lebar+bug) → review?(khuey)
Updated•10 years ago
|
Attachment #771551 -
Flags: review?(justin.lebar+bug) → review?(khuey)
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.
Attachment #771550 -
Flags: review?(khuey) → review-
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.
Attachment #771551 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 94•10 years ago
|
||
> @@ +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...
Assignee | ||
Comment 95•10 years ago
|
||
> ::: 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?
(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?
Assignee | ||
Comment 97•10 years ago
|
||
Attachment #771550 -
Attachment is obsolete: true
Attachment #794686 -
Flags: review?(khuey)
Assignee | ||
Comment 98•10 years ago
|
||
Addressed issues mentioned in comment 92 and comment 93.
Attachment #771551 -
Attachment is obsolete: true
Attachment #794689 -
Flags: review?(khuey)
Comment on attachment 794689 [details] [diff] [review] Part II: IPDL V5 Review of attachment 794689 [details] [diff] [review]: ----------------------------------------------------------------- r=me
Attachment #794689 -
Flags: review?(khuey) → review+
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.
Attachment #794686 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 101•10 years ago
|
||
Rebased again, this is the patch for check-in.
Attachment #794686 -
Attachment is obsolete: true
Assignee | ||
Comment 102•10 years ago
|
||
Attachment #794689 -
Attachment is obsolete: true
Attachment #795299 -
Flags: review?(khuey)
Assignee | ||
Updated•10 years ago
|
Attachment #795297 -
Flags: review?(khuey)
Assignee | ||
Comment 103•10 years ago
|
||
Comment on attachment 795297 [details] [diff] [review] Part I: WebIDL for check-in Fixed issues listed in comment 100.
Assignee | ||
Comment 104•10 years ago
|
||
: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?
Assignee | ||
Comment 105•10 years ago
|
||
/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?
Attachment #795299 -
Flags: review?(khuey) → review+
Attachment #795297 -
Flags: review?(khuey) → review+
(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.
Assignee | ||
Comment 107•10 years ago
|
||
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
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
Assignee | ||
Comment 110•10 years ago
|
||
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/
No idea. I'd just check all of the b2g platforms (the last 4 checkboxes underneath 'platform') :-P
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.
Assignee | ||
Comment 113•10 years ago
|
||
I see, thanks, let me have a cup of tea and see what happens, :)
(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.
Depends on: 909823
Filed bug 909823 on that.
Assignee | ||
Comment 116•10 years ago
|
||
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
Assignee | ||
Comment 117•10 years ago
|
||
Attachment #769552 -
Attachment is obsolete: true
Attachment #795297 -
Attachment is obsolete: true
Attachment #796391 -
Flags: review?(khuey)
Assignee | ||
Comment 118•10 years ago
|
||
Attachment #796392 -
Flags: review?(khuey)
Assignee | ||
Comment 119•10 years ago
|
||
Attachment #756482 -
Attachment is obsolete: true
Attachment #795299 -
Attachment is obsolete: true
Attachment #796394 -
Flags: review?(khuey)
Assignee | ||
Updated•10 years ago
|
Attachment #796392 -
Attachment filename: 0002-Bug-870676-Part-II-IPDL-r-khuey.patch → Part II: IPDL for check-in
Assignee | ||
Updated•10 years ago
|
Attachment #796392 -
Attachment description: 0002-Bug-870676-Part-II-IPDL-r-khuey.patch → Part II: IPDL for check-in
Attachment #796392 -
Attachment filename: Part II: IPDL for check-in → 0002-Bug-870676-Part-II-IPDL-r-khuey.patch
Assignee | ||
Comment 120•10 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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
Assignee | ||
Comment 121•10 years ago
|
||
Attachment #796391 -
Attachment is obsolete: true
Attachment #796391 -
Flags: review?(khuey)
Attachment #796404 -
Flags: review?(khuey)
Assignee | ||
Comment 122•10 years ago
|
||
Attachment #796404 -
Attachment is obsolete: true
Attachment #796404 -
Flags: review?(khuey)
Attachment #796408 -
Flags: review?(khuey)
Assignee | ||
Comment 123•10 years ago
|
||
Attachment #796409 -
Flags: review?(khuey)
Assignee | ||
Updated•10 years ago
|
Attachment #796394 -
Attachment is obsolete: true
Attachment #796394 -
Flags: review?(khuey)
Attachment #796408 -
Flags: review?(khuey) → review+
Attachment #796392 -
Flags: review?(khuey) → review+
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 ;-)
Attachment #796409 -
Flags: review?(khuey)
Assignee | ||
Comment 125•10 years ago
|
||
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.
Attachment #796409 -
Flags: review?(timdream)
Assignee | ||
Comment 126•10 years ago
|
||
I think Bug 867831 is the tracking bug for multi-process support of Marionette.
Comment 127•10 years ago
|
||
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.
Attachment #796409 -
Flags: review?(timdream)
Assignee | ||
Comment 128•10 years ago
|
||
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.
Attachment #796409 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 129•10 years ago
|
||
:khuey, can we just check in the r+ patches first?
(In reply to Pin Zhang [:pzhang] from comment #129) > :khuey, can we just check in the r+ patches first? Yes, please :-)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
I'll land parts 1 and 2 once I get to the office.
Keywords: checkin-needed
Comment 132•10 years ago
|
||
(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! :)
https://hg.mozilla.org/integration/b2g-inbound/rev/05529a089e45 https://hg.mozilla.org/integration/b2g-inbound/rev/8ed8e41fb68c
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
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?
Assignee | ||
Comment 136•10 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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!
Assignee | ||
Comment 137•10 years ago
|
||
(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!
(In reply to Pin Zhang [:pzhang] from comment #136) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cd99d10fc665 https://hg.mozilla.org/mozilla-central/rev/5a71d3bf48ce
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
Assignee | ||
Comment 141•10 years ago
|
||
Comment 142•10 years ago
|
||
Bug 870676, meet bug 875934 (in merges across four trees). https://hg.mozilla.org/mozilla-central/rev/a9ffc1cdb1ae
Assignee | ||
Comment 143•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
I think I'm seeing build problems due to this patch: https://tbpl.mozilla.org/?tree=Try&rev=48c266b6804b
Assignee | ||
Comment 146•10 years ago
|
||
(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?
Assignee | ||
Comment 147•10 years ago
|
||
(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•10 years ago
|
||
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 149•10 years ago
|
||
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.
Attachment #796409 -
Flags: review?(justin.lebar+bug) → review-
Assignee | ||
Comment 150•10 years ago
|
||
Attachment #796408 -
Attachment is obsolete: true
Attachment #797712 -
Flags: review?(khuey)
Assignee | ||
Comment 151•10 years ago
|
||
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.
Attachment #796392 -
Attachment is obsolete: true
Attachment #797715 -
Flags: review?(khuey)
Assignee | ||
Comment 152•10 years ago
|
||
(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.
Assignee | ||
Comment 153•10 years ago
|
||
(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.
Assignee | ||
Comment 154•10 years ago
|
||
(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?
Hi Pin, Yesterday was a holiday in the US. I will review and land them today.
Attachment #797712 -
Flags: review?(khuey) → review+
Attachment #797715 -
Flags: review?(khuey) → review+
https://hg.mozilla.org/integration/b2g-inbound/rev/8c448e11da5c https://hg.mozilla.org/integration/b2g-inbound/rev/5f0cdd85e5a4
Comment 157•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8c448e11da5c https://hg.mozilla.org/mozilla-central/rev/5f0cdd85e5a4
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Blocks: slim-fast
You need to log in
before you can comment on or make changes to this bug.
Description
•