834160 - B2G RIL: refactor RILContentHelper callback registrations

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: interface changes

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.

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: MobileConnection : WIP

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: Cell Broadcast : WIP

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4: Voicemail : WIP

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 5: Telephony : WIP

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 6: ICC : WIP

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 5/7: Telephony : WIP2

Rebase

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 7/7: RILContentHelper & RadioInterfaceLayer : WIP

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 708598 [details] [diff] [review]
Part 7/7: RILContentHelper & RadioInterfaceLayer : WIP

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

Looks good!

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 13

[Mounir Lamouri (:mounir)]

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?

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: interface changes

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

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2/7: MobileConnection

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

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3/7: Cell Broadcast

migrate to nsICellBroadcastProvider & nsICellBroadcastEventListener

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4/7: Voicemail

migrate to nsIVoicemailProvider & nsIVoicemailEventListener

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 5/7: Telephony

migrate to nsITelephonyProvider & nsITelephonyEventListener

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 6/7: ICC

migrate to nsIIccProvider & nsIIccEventListener

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 7/7: RILContentHelper & RadioInterfaceLayer

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

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 23

[Dietrich Ayala (:dietrich)]

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

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

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

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 26

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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 27

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 716457 [details] [diff] [review]
Part 5/7: Telephony

For those modifications under dom/Bluetooth, looks good.

Comment 28

[Mounir Lamouri (:mounir)]

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 29

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

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 30

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

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 31

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

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 32

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

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.

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 34

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

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 35

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 36

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 38

[Dietrich Ayala (:dietrich)]

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

Comment 39

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1/7: interface changes : v2

Use Listener rather than EventListener. See comments #31

Comment 40

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2/7: MobileConnection : v2

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>

Comment 41

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3/7: Cell Broadcast : v2

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>

Comment 42

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4/7: Voicemail : v2

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

Comment 43

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 5/7: Telephony : v2

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

Comment 44

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 6/7: ICC : v2

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>

Comment 45

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 7/7: RILContentHelper & RadioInterfaceLayer : v2

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

Comment 46

[Vicamo Yang [:vicamo][:vyang]]

https://tbpl.mozilla.org/?tree=Try&rev=f93e4319be35

Comment 47

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

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 48

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

Comment on attachment 721089 [details] [diff] [review]
Part 4/7: Voicemail : v2

>+  void Disable()
>+  {
>+    MOZ_ASSERT(mVoicemail);
>+    mVoicemail = nullptr;
>+  }
Disconnect()

Comment 49

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

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

s/Disable/Disconnect/

Comment 50

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

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

s/Disable/Disconnect/

Comment 51

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 52

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 53

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2/7: MobileConnection : v3

s/Disable/Disconnect/

Comment 54

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3/7: Cell Broadcast : v3

s/Disable/Disconnect/

Comment 55

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4/7: Voicemail : v3

s/Disable/Disconnect/

Comment 56

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 5/7: Telephony : v3

s/Disable/Disconnect/

Comment 57

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 6/7: ICC : v3

s/Disable/Disconnect/

Comment 58

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 59

[Ed Morley [:emorley]]

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

Comment 60

[Ed Morley [:emorley]]

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