The RILContentHelper now implements two interfaces: nsIMobileConnectionProvider and nsIRILContentHelper. Separating interfaces for different components is actually a good idea and we should follow this way in all other components like telephony, voicemail, and cellbroadcast. At the same time, registerXxxMsg() and registerXxxCallback() should be integrated into one single call. That would be much more handy.

Depends on bug 833278 to adopt path changes and blocks bug 833229 because new interfaces added here will then be reused.

Hi Mounir, could you help review this patch? This bug is our first step to remove RILContentHelper. I tear down nsIRILContentHelper into multiple function provider interfaces so that we can later rewrite IPC code with ipdl component by component. Every provider interface has a corresponding callback interface and DOM components register to a certain provider with that callback for event delivery and then we'll never have to rely on broadcasting observer events.

Rebase

Comment on attachment 707600 [details] [diff] [review] Part 1: interface changes Review of attachment 707600 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/interfaces/nsIMobileConnectionProvider.idl @@ +22,5 @@ > + in boolean sessionEnded); > + void notifyDataError(in DOMString message); > + void notifyIccCardLockError(in DOMString lockType, > + in unsigned long retryCount); > + void notifyCFStateChange(in boolean success, Abbreviation 'CF' isn't that clear to me. If we do want to leave this abbreviation, please have a comment explaining that. ::: dom/telephony/nsITelephonyProvider.idl @@ +12,5 @@ > + * > + * @param callIndex > + * Call identifier assigned by the RIL. > + * @param callState > + * One of the nsIRadioInterfaceLayer::CALL_STATE_* values. s/nsIRadioInterfaceLayer/nsITelephonyProvider @@ +24,5 @@ > + in AString number, > + in boolean isActive); > + > + /** > + * Called when nsIRILContentHelper is asked to enumerate the current s/nsIRILContentHelper/nsITelephonyProvider @@ +25,5 @@ > + in boolean isActive); > + > + /** > + * Called when nsIRILContentHelper is asked to enumerate the current > + * telephony call state (nsIRILContentHelper::enumerateCalls). This is ditto.

Comment on attachment 708598 [details] [diff] [review] Part 7/7: RILContentHelper & RadioInterfaceLayer : WIP Review of attachment 708598 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

Comment on attachment 707600 [details] [diff] [review] Part 1: interface changes Review of attachment 707600 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/interfaces/nsIIccProvider.idl @@ +6,5 @@ > + > +interface nsIDOMWindow; > + > +[scriptable, uuid(2cb8e811-7eaf-4cb9-8aa8-581e7a245edc)] > +interface nsIIccCallback : nsISupports Not sure Callback is a good naming here, Callback may be refer to asynchronous operations, but in these cases, those events are sent by ICC itself. @@ +19,5 @@ > +[scriptable, uuid(1bfb62cf-544c-4adb-b91c-c75e5d0dce29)] > +interface nsIIccProvider : nsISupports > +{ > + /** > + * Called when a content process registers receiving unsolicited messages from 'Called' seems a bit strange to me.

Comment on attachment 707600 [details] [diff] [review] Part 1: interface changes Review of attachment 707600 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for cleaning up a bit :) The changes look good, I would just go a bit deeper and cleanup the names given that are in specific interfaces, it's likely that simple names are enough. I don't know enough the RIL code to know for sure so the decision is up to you. BTW, regarding my poor knowledge of the RIL code, I think you should ask a proper RIL peer to r+ the patch, f=me, though ;) ::: dom/cellbroadcast/interfaces/nsICellBroadcastProvider.idl @@ +30,5 @@ > + * RadioInterfaceLayer in the chrome process. Only a content process that has > + * the 'cellbroadcast' permission is allowed to register. > + */ > + void registerCellBroadcastMsg(in nsICellBroadcastCallback callback); > + void unregisterCellBroadcastMsg(in nsICellBroadcastCallback callback); Why not calling those methods register/unregister? ::: dom/icc/interfaces/nsIIccProvider.idl @@ +24,5 @@ > + * RadioInterfaceLayer in the chrome process. Only a content process that has > + * the 'mobileconnection' permission is allowed to register. > + */ > + void registerIccMsg(in nsIIccCallback callback); > + void unregisterIccMsg(in nsIIccCallback callback); register/unregister? @@ +35,5 @@ > + in boolean helpRequested); > + void sendStkTimerExpiration(in nsIDOMWindow window, > + in jsval timer); > + void sendStkEventDownload(in nsIDOMWindow window, > + in jsval event); I guess you could remove the 'Stk' part in those methods: sendResponse / sendMenuSelection / sendTimerExpiration / sendEventDownload. ::: dom/voicemail/nsIVoicemailProvider.idl @@ +30,5 @@ > + * RadioInterfaceLayer in the chrome process. Only a content process that has > + * the 'voicemail' permission is allowed to register. > + */ > + void registerVoicemailMsg(in nsIVoicemailCallback callback); > + void unregisterVoicemailMsg(in nsIVoicemailCallback callback); Why not register/unregister? @@ +34,5 @@ > + void unregisterVoicemailMsg(in nsIVoicemailCallback callback); > + > + readonly attribute nsIDOMMozVoicemailStatus voicemailStatus; > + readonly attribute DOMString voicemailNumber; > + readonly attribute DOMString voicemailDisplayName; Why not status / number / displayName?

(In reply to Mounir Lamouri (:mounir) from comment #13) > ::: dom/cellbroadcast/interfaces/nsICellBroadcastProvider.idl > @@ +30,5 @@ > > + * RadioInterfaceLayer in the chrome process. Only a content process that has > > + * the 'cellbroadcast' permission is allowed to register. > > + */ > > + void registerCellBroadcastMsg(in nsICellBroadcastCallback callback); > > + void unregisterCellBroadcastMsg(in nsICellBroadcastCallback callback); > > Why not calling those methods register/unregister? RILContentHelper implements multiple provider interfaces now, so I have to name these register/unregister functions differently. Other namings you mentioned have the same situation. We can rename them to exclude STK/Voicemail prefix after re-implementing the provider interfaces out of RILContentHelper.

1) rename to nsIFooEventListener instead. 2) change uuid of every interface modified.

Re-implement mobile connection event listening with callback instead of observers.

migrate to nsICellBroadcastProvider & nsICellBroadcastEventListener

migrate to nsIVoicemailProvider & nsIVoicemailEventListener

migrate to nsITelephonyProvider & nsITelephonyEventListener

migrate to nsIIccProvider & nsIIccEventListener

1) Rebase 2) handle "RIL:RegisterIccMsg" as well 3) Use "EventListener" rather than ambiguous "Callback".

try https://tbpl.mozilla.org/?tree=Try&rev=64d9a24b2c33

Hi Vicamo, why should this block 1.1 - why would we hold the release back for this refactoring?

(In reply to Dietrich Ayala (:dietrich) from comment #23) > Hi Vicamo, why should this block 1.1 - why would we hold the > release back for this refactoring? This is for multisim. And I was previously told multisim should be available v1.1.

Comment on attachment 716458 [details] [diff] [review] Part 6/7: ICC Review of attachment 716458 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/src/IccManager.cpp @@ +61,5 @@ > > // Not being able to acquire the provider isn't fatal since we check > // for it explicitly below. > if (!mProvider) { > + NS_WARNING("Could not acquire nsIIccProvider!"); return early.

Comment on attachment 716453 [details] [diff] [review] Part 1: interface changes Review of attachment 716453 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/nsITelephonyProvider.idl @@ +12,5 @@ > + * > + * @param callIndex > + * Call identifier assigned by the RIL. > + * @param callState > + * One of the nsIRadioInterfaceLayer::CALL_STATE_* values. s/nsIRadioInterfaceLayer/nsIRILTelephonyProvider @@ +24,5 @@ > + in AString number, > + in boolean isActive); > + > + /** > + * Called when nsIRILContentHelper is asked to enumerate the current s/nsIRILContentHelper/nsIRILTelephonyProvider @@ +25,5 @@ > + in boolean isActive); > + > + /** > + * Called when nsIRILContentHelper is asked to enumerate the current > + * telephony call state (nsIRILContentHelper::enumerateCalls). This is ditto.

Comment on attachment 716457 [details] [diff] [review] Part 5/7: Telephony For those modifications under dom/Bluetooth, looks good.

Comment on attachment 716455 [details] [diff] [review] Part 3/7: Cell Broadcast Review of attachment 716455 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/cellbroadcast/src/CellBroadcast.cpp @@ +29,5 @@ > > + EventListener(CellBroadcast* aCellBroadcast) > + : mCellBroadcast(aCellBroadcast) > + { > + MOZ_ASSERT(mCellBroadcast, "Null pointer!"); nit: you don't have to put a comment in the MOZ_ASSERT. @@ +34,5 @@ > + } > + > + void Disable() > + { > + NS_ASSERTION(mCellBroadcast, "Disable called more than once!"); MOZ_ASSERT @@ +74,2 @@ > > + nsresult rv = mProvider->RegisterCellBroadcastMsg(mEventListener); DebugOnly<nsresult> rv = @@ +78,5 @@ > } > > CellBroadcast::~CellBroadcast() > { > + MOZ_ASSERT(mProvider && mEventListener, "Null pointer!"); nit: you don't have to put a comment in the MOZ_ASSERT. ::: dom/cellbroadcast/src/CellBroadcast.h @@ +18,5 @@ > > class CellBroadcast MOZ_FINAL : public nsDOMEventTargetHelper > , public nsIDOMMozCellBroadcast > { > + class EventListener; Please, add a comment.

Comment on attachment 716454 [details] [diff] [review] Part 2/7: MobileConnection > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(MobileConnection, > nsDOMEventTargetHelper) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > > NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(MobileConnection, > nsDOMEventTargetHelper) > tmp->mProvider = nullptr; >+ tmp->mEventListener = nullptr; > tmp->mIccManager = nullptr; > NS_IMPL_CYCLE_COLLECTION_UNLINK_END Why doesn't this thing leak? What if mEventListener keeps MobileConnection alive? You should probably traverse the same objects you're unlinking. Also, why not use the macros for unlinking? Or, if mEventListener, mPrivider and mIccManager doesn't need to be traversed, at least add comment to traverse why they don't need to be handled there. Also, I'd prefer to not call the thing eventlistener since it is not DOM event listener, not anything which deals with event loop. We're using the word "event" for many things enough already now.

Comment on attachment 716456 [details] [diff] [review] Part 4/7: Voicemail >+class Voicemail::EventListener : public nsIVoicemailEventListener Please don't use "EventListener", if just possible >+{ >+ Voicemail* mVoicemail; >+Voicemail::Voicemail(nsPIDOMWindow* aWindow, nsIVoicemailProvider* aProvider) >+ : mProvider(aProvider) > { > BindToOwner(aWindow); > >- mRILVoicemailCallback = new RILVoicemailCallback(this); >+ mEventListener = new EventListener(this); I don't quite understand the setup here. Voicemail effectively owns EventListener always, and mEventListener is registered to mProvider. Why isn't Voicemail itself some kind of listener which is registered to mProvider? >+ /** >+ * Class Voicemail doesn't actually inherit nsIVoicemailEventListener. So, why not?

Comment on attachment 716457 [details] [diff] [review] Part 5/7: Telephony > BluetoothHfpManager::OnConnectSuccess() > { >- nsCOMPtr<nsIRILContentHelper> ril = >+ nsCOMPtr<nsITelephonyProvider> provider = > do_GetService(NS_RILCONTENTHELPER_CONTRACTID); Should we change NS_RILCONTENTHELPER_CONTRACTID at some point too? >+class BluetoothTelephonyEventListener : public nsITelephonyEventListener Please, no "EventListener", just *Listener or *Callback or something >+class Telephony::EventListener : public nsITelephonyEventListener Ditto

Comment on attachment 716458 [details] [diff] [review] Part 6/7: ICC Same things with EventListener and add comments to traverse why everything doesn't need to be traversed.

(In reply to Olli Pettay [:smaug] from comment #30) > Please don't use "EventListener", if just possible Any suggestions? > I don't quite understand the setup here. Voicemail effectively > owns EventListener always, and mEventListener is registered to > mProvider. Why isn't Voicemail itself some kind of listener > which is registered to mProvider? See bug 775997 comment #51. All these DOM components fall in the same trap because they all register to RILContentHelper, which is now implemented by javascript. We may only remove these strange behaviours after removing RILContentHelper in bug 815526 and implement all these provider interfaces by C++.

Ah, bug 775997 comment #51 is clear. Thanks. Could you just call the interfaces and classes *Listener or *Observer. nsICellBroadcasObserver, nsIIccObserver? Doesn't sounds too bad to me.

Comment on attachment 716460 [details] [diff] [review] Part 7/7: RILContentHelper & RadioInterfaceLayer Review of attachment 716460 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RILContentHelper.js @@ +657,5 @@ > + _mobileConnectionEventListeners: null, > + _telephonyEventListeners: null, > + _cellBroadcastEventListeners: null, > + _voicemailEventListeners: null, > + _iccEventListeners: null, Please also consider bugs' concern about the naming here. @@ +707,3 @@ > if (index != -1) { > + listeners.splice(index, 1); > + if (DEBUG) debug("Unregistered telephony listener: " + listener); The comment shouldn't be restricted to only 'telephony listener' @@ +1184,4 @@ > return; > } > > + let listeners = thisListeners.slice(); Check whether |listerners| is null before moving on. @@ +1185,5 @@ > } > > + let listeners = thisListeners.slice(); > + for each (let listener in listeners) { > + if (thisListeners.indexOf(listener) == -1) { This |thisListeners.indexOf(listener) == -1| seems redundant, right? If yes, would be a good timing to remove it by your patch.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #35) > Comment on attachment 716460 [details] [diff] [review] > Part 7/7: RILContentHelper & RadioInterfaceLayer > > Review of attachment 716460 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/RILContentHelper.js > @@ +657,5 @@ > > + _mobileConnectionEventListeners: null, > > + _telephonyEventListeners: null, > > + _cellBroadcastEventListeners: null, > > + _voicemailEventListeners: null, > > + _iccEventListeners: null, > > Please also consider bugs' concern about the naming here. > > @@ +707,3 @@ > > if (index != -1) { > > + listeners.splice(index, 1); > > + if (DEBUG) debug("Unregistered telephony listener: " + listener); > > The comment shouldn't be restricted to only 'telephony listener' > > @@ +1184,4 @@ > > return; > > } > > > > + let listeners = thisListeners.slice(); > > Check whether |listerners| is null before moving on. > > @@ +1185,5 @@ > > } > > > > + let listeners = thisListeners.slice(); > > + for each (let listener in listeners) { > > + if (thisListeners.indexOf(listener) == -1) { > > This |thisListeners.indexOf(listener) == -1| seems redundant, right? If yes, > would be a good timing to remove it by your patch. Let's keep it as our offline discussion, Vicamo. Thanks!

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #35) > Comment on attachment 716460 [details] [diff] [review] > Part 7/7: RILContentHelper & RadioInterfaceLayer > ----------------------------------------------------------------- > ::: dom/system/gonk/RILContentHelper.js > @@ +1184,4 @@ > > return; > > } > > > > + let listeners = thisListeners.slice(); > > Check whether |listerners| is null before moving on. The for-each loop in the next line will check it. :) > @@ +1185,5 @@ > > } > > > > + let listeners = thisListeners.slice(); > > + for each (let listener in listeners) { > > + if (thisListeners.indexOf(listener) == -1) { > > This |thisListeners.indexOf(listener) == -1| seems redundant, right? If yes, > would be a good timing to remove it by your patch. Just for keeping some thoughts we had :) The reason we have this line is unknown. It have been there since ice age I guess. I can only know that "might" help avoid some problems when some web content event listener closes an iframe. So it sounds still useful for me.

It's not clear why this blocks 1.1. Please re-nominate with blocking reasoning explained.

Use Listener rather than EventListener. See comments #31

1) Use Listener rather than EventListener. 2) Move comments to document |class Listener;| instead. 3) Use NS_DECL_ISUPPORTS_INHERITED instead of NS_DECL_ISUPPORTS 4) As discussed on IRC, we don't really have to cc mProvider & mListener. Add comments to address this. 5) Use formal traverse/unlink macros instead. 6) MOZ_ASSERT & DebugOnly<nsresult>

1) Use Listener rather than EventListener. 2) Move comments to document |class Listener;| instead. 3) Use NS_DECL_ISUPPORTS_INHERITED instead of NS_DECL_ISUPPORTS 4) Like voicemail, we don't need CC here. 5) MOZ_ASSERT & DebugOnly<nsresult>

1) Use Listener rather than EventListener. 2) Move comments to document |class Listener;| instead. 3) MOZ_ASSERT & DebugOnly<nsresult>

1) Use Listener rather than EventListener. 2) As discussed on IRC, we don't really have to cc mProvider & mListener. Add comments to address this. 3) MOZ_ASSERT

1) Use Listener rather than EventListener. 2) Move comments to document |class Listener;| instead. 3) Use NS_DECL_ISUPPORTS_INHERITED instead of NS_DECL_ISUPPORTS 4) Like voicemail, we don't need CC here. 5) Rebase after bug 840780 6) MOZ_ASSERT & DebugOnly<nsresult>

1) Use Listener rather than EventListener. 5) Rebase after bug 840780

Comment on attachment 721086 [details] [diff] [review] Part 2/7: MobileConnection : v2 >+ void Disable() >+ { >+ MOZ_ASSERT(mMobileConnection); >+ mMobileConnection = nullptr; >+ } Usually this kind of method is called Disconnect() in Gecko > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(MobileConnection, > nsDOMEventTargetHelper) >+ // Don't traverse mListener because it doesn't keep any reference to s/any/strong/

Comment on attachment 721089 [details] [diff] [review] Part 4/7: Voicemail : v2 >+ void Disable() >+ { >+ MOZ_ASSERT(mVoicemail); >+ mVoicemail = nullptr; >+ } Disconnect()

Comment on attachment 721090 [details] [diff] [review] Part 5/7: Telephony : v2 s/Disable/Disconnect/

Comment on attachment 721092 [details] [diff] [review] Part 6/7: ICC : v2 s/Disable/Disconnect/

Another full try with review comments addressed: https://tbpl.mozilla.org/?tree=Try&rev=efc32fe20662

Yet another full try after bug 847683 has been landed: https://tbpl.mozilla.org/?tree=Try&rev=5bbad57efab3

s/Disable/Disconnect/

s/Disable/Disconnect/

s/Disable/Disconnect/

s/Disable/Disconnect/

s/Disable/Disconnect/

https://hg.mozilla.org/integration/mozilla-inbound/rev/099a31538e9f https://hg.mozilla.org/integration/mozilla-inbound/rev/99d2237bfe5a https://hg.mozilla.org/integration/mozilla-inbound/rev/364825af90d4 https://hg.mozilla.org/integration/mozilla-inbound/rev/2c663057f534 https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4ce0c7040a https://hg.mozilla.org/integration/mozilla-inbound/rev/4b3d1c5c0e4e https://hg.mozilla.org/integration/mozilla-inbound/rev/c77fdc51aa60

https://hg.mozilla.org/mozilla-central/rev/099a31538e9f https://hg.mozilla.org/mozilla-central/rev/99d2237bfe5a https://hg.mozilla.org/mozilla-central/rev/364825af90d4 https://hg.mozilla.org/mozilla-central/rev/2c663057f534 https://hg.mozilla.org/mozilla-central/rev/2b4ce0c7040a https://hg.mozilla.org/mozilla-central/rev/4b3d1c5c0e4e https://hg.mozilla.org/mozilla-central/rev/c77fdc51aa60

https://hg.mozilla.org/mozilla-central/rev/099a31538e9f https://hg.mozilla.org/mozilla-central/rev/99d2237bfe5a https://hg.mozilla.org/mozilla-central/rev/364825af90d4 https://hg.mozilla.org/mozilla-central/rev/2c663057f534 https://hg.mozilla.org/mozilla-central/rev/2b4ce0c7040a https://hg.mozilla.org/mozilla-central/rev/4b3d1c5c0e4e https://hg.mozilla.org/mozilla-central/rev/c77fdc51aa60