If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

B2G RIL: refactor RILContentHelper callback registrations

RESOLVED FIXED in mozilla22

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: vicamo, Assigned: vicamo)

Tracking

unspecified
mozilla22
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-)

Details

Attachments

(7 attachments, 20 obsolete attachments)

21.53 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
42.96 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
18.59 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
8.14 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
7.38 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
48.96 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
8.96 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Depends on bug 833278 to adopt path changes and blocks bug 833229 because new interfaces added here will then be reused.
Blocks: 833229
Depends on: 833278
(Assignee)

Updated

5 years ago
Depends on: 834193
(Assignee)

Updated

5 years ago
Depends on: 835137
(Assignee)

Updated

5 years ago
Depends on: 777271
(Assignee)

Comment 2

5 years ago
Created attachment 707600 [details] [diff] [review]
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.
Attachment #707600 - Flags: review?(mounir)
Attachment #707600 - Flags: feedback?(htsai)
Attachment #707600 - Flags: feedback?(allstars.chh)
(Assignee)

Updated

5 years ago
Assignee: nobody → vyang
(Assignee)

Comment 3

5 years ago
Created attachment 707689 [details] [diff] [review]
Part 2: MobileConnection : WIP
(Assignee)

Comment 4

5 years ago
Created attachment 707691 [details] [diff] [review]
Part 3: Cell Broadcast : WIP
(Assignee)

Comment 5

5 years ago
Created attachment 707692 [details] [diff] [review]
Part 4: Voicemail : WIP
(Assignee)

Comment 6

5 years ago
Created attachment 707693 [details] [diff] [review]
Part 5: Telephony : WIP
(Assignee)

Comment 7

5 years ago
Created attachment 707694 [details] [diff] [review]
Part 6: ICC : WIP
(Assignee)

Updated

5 years ago
Attachment #707694 - Attachment description: Part 6: ICC → Part 6: ICC : WIP
(Assignee)

Comment 8

5 years ago
Created attachment 708595 [details] [diff] [review]
Part 5/7: Telephony : WIP2

Rebase
Attachment #707693 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 708598 [details] [diff] [review]
Part 7/7: RILContentHelper & RadioInterfaceLayer : WIP
Attachment #708598 - Flags: feedback?(htsai)
Attachment #708598 - Flags: feedback?(allstars.chh)
(Assignee)

Updated

5 years ago
Blocks: 814581
(Assignee)

Updated

5 years ago
blocking-b2g: --- → leo?
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.
Attachment #707600 - Flags: feedback?(htsai) → feedback+
Comment on attachment 708598 [details] [diff] [review]
Part 7/7: RILContentHelper & RadioInterfaceLayer : WIP

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

Looks good!
Attachment #708598 - Flags: feedback?(htsai) → feedback+
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.
Attachment #707600 - Flags: feedback?(allstars.chh) → feedback+
Attachment #708598 - Flags: feedback?(allstars.chh) → feedback+
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?
Attachment #707600 - Flags: review?(mounir) → feedback+
(Assignee)

Comment 14

5 years ago
(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.
Blocks: 843452
(Assignee)

Comment 15

5 years ago
Created attachment 716453 [details] [diff] [review]
Part 1: interface changes

1) rename to nsIFooEventListener instead.
2) change uuid of every interface modified.
Attachment #707600 - Attachment is obsolete: true
Attachment #716453 - Flags: review?(htsai)
Attachment #716453 - Flags: review?(allstars.chh)
Attachment #716453 - Flags: feedback+
(Assignee)

Comment 16

5 years ago
Created attachment 716454 [details] [diff] [review]
Part 2/7: MobileConnection

Re-implement mobile connection event listening with callback instead of observers.
Attachment #707689 - Attachment is obsolete: true
Attachment #716454 - Flags: review?(bugs)
Attachment #716454 - Flags: feedback?(allstars.chh)
(Assignee)

Comment 17

5 years ago
Created attachment 716455 [details] [diff] [review]
Part 3/7: Cell Broadcast

migrate to nsICellBroadcastProvider & nsICellBroadcastEventListener
Attachment #707691 - Attachment is obsolete: true
Attachment #716455 - Flags: review?(mounir)
Attachment #716455 - Flags: feedback?(htsai)
(Assignee)

Comment 18

5 years ago
Created attachment 716456 [details] [diff] [review]
Part 4/7: Voicemail

migrate to nsIVoicemailProvider & nsIVoicemailEventListener
Attachment #707692 - Attachment is obsolete: true
Attachment #716456 - Flags: review?(bugs)
Attachment #716456 - Flags: feedback?(htsai)
(Assignee)

Comment 19

5 years ago
Created attachment 716457 [details] [diff] [review]
Part 5/7: Telephony

migrate to nsITelephonyProvider & nsITelephonyEventListener
Attachment #708595 - Attachment is obsolete: true
Attachment #716457 - Flags: review?(echou)
Attachment #716457 - Flags: review?(bugs)
Attachment #716457 - Flags: feedback?(htsai)
(Assignee)

Comment 20

5 years ago
Created attachment 716458 [details] [diff] [review]
Part 6/7: ICC

migrate to nsIIccProvider & nsIIccEventListener
Attachment #707694 - Attachment is obsolete: true
Attachment #716458 - Flags: review?(bugs)
Attachment #716458 - Flags: feedback?(allstars.chh)
(Assignee)

Comment 21

5 years ago
Created attachment 716460 [details] [diff] [review]
Part 7/7: RILContentHelper & RadioInterfaceLayer

1) Rebase
2) handle "RIL:RegisterIccMsg" as well
3) Use "EventListener" rather than ambiguous "Callback".
Attachment #708598 - Attachment is obsolete: true
Attachment #716460 - Flags: review?(htsai)
Attachment #716460 - Flags: review?(allstars.chh)
(Assignee)

Comment 22

5 years ago
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?
(Assignee)

Comment 24

5 years ago
(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.
Attachment #716453 - Flags: review?(allstars.chh) → review+
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.
Attachment #716458 - Flags: feedback?(allstars.chh) → feedback+
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.
Attachment #716453 - Flags: review?(htsai) → review+
Attachment #716460 - Flags: review?(allstars.chh) → review+
Comment on attachment 716457 [details] [diff] [review]
Part 5/7: Telephony

For those modifications under dom/Bluetooth, looks good.
Attachment #716457 - Flags: review?(echou) → review+
Attachment #716454 - Flags: feedback?(allstars.chh) → feedback+
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.
Attachment #716455 - Flags: review?(mounir) → review+
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.
Attachment #716454 - Flags: review?(bugs) → review-
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?
Attachment #716456 - Flags: review?(bugs) → review-
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
Attachment #716457 - Flags: review?(bugs) → review-
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.
Attachment #716458 - Flags: review?(bugs) → review-
(Assignee)

Comment 33

5 years ago
(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.

Updated

5 years ago
Attachment #716455 - Flags: feedback?(htsai) → feedback+

Updated

5 years ago
Attachment #716456 - Flags: feedback?(htsai) → feedback+

Updated

5 years ago
Attachment #716457 - Flags: feedback?(htsai) → feedback+
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.
Attachment #716460 - Flags: review?(htsai)
(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!
(Assignee)

Comment 37

5 years ago
(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.
blocking-b2g: leo? → -
(Assignee)

Comment 39

5 years ago
Created attachment 721084 [details] [diff] [review]
Part 1/7: interface changes : v2

Use Listener rather than EventListener. See comments #31
Attachment #716453 - Attachment is obsolete: true
Attachment #721084 - Flags: review+
(Assignee)

Comment 40

5 years ago
Created attachment 721086 [details] [diff] [review]
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>
Attachment #716454 - Attachment is obsolete: true
Attachment #721086 - Flags: review?(bugs)
(Assignee)

Comment 41

5 years ago
Created attachment 721087 [details] [diff] [review]
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>
Attachment #716455 - Attachment is obsolete: true
Attachment #721087 - Flags: review+
(Assignee)

Comment 42

5 years ago
Created attachment 721089 [details] [diff] [review]
Part 4/7: Voicemail : v2

1) Use Listener rather than EventListener.
2) Move comments to document |class Listener;| instead.
3) MOZ_ASSERT & DebugOnly<nsresult>
Attachment #716456 - Attachment is obsolete: true
Attachment #721089 - Flags: review?(bugs)
(Assignee)

Comment 43

5 years ago
Created attachment 721090 [details] [diff] [review]
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
Attachment #716457 - Attachment is obsolete: true
Attachment #721090 - Flags: review?(bugs)
(Assignee)

Comment 44

5 years ago
Created attachment 721092 [details] [diff] [review]
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>
Attachment #716458 - Attachment is obsolete: true
Attachment #721092 - Flags: review?(bugs)
(Assignee)

Comment 45

5 years ago
Created attachment 721094 [details] [diff] [review]
Part 7/7: RILContentHelper & RadioInterfaceLayer : v2

1) Use Listener rather than EventListener.
5) Rebase after bug 840780
Attachment #716460 - Attachment is obsolete: true
Attachment #721094 - Flags: review+
(Assignee)

Comment 46

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=f93e4319be35
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/
Attachment #721086 - Flags: review?(bugs) → review+
Comment on attachment 721089 [details] [diff] [review]
Part 4/7: Voicemail : v2

>+  void Disable()
>+  {
>+    MOZ_ASSERT(mVoicemail);
>+    mVoicemail = nullptr;
>+  }
Disconnect()
Attachment #721089 - Flags: review?(bugs) → review+
Comment on attachment 721090 [details] [diff] [review]
Part 5/7: Telephony : v2

s/Disable/Disconnect/
Attachment #721090 - Flags: review?(bugs) → review+
Comment on attachment 721092 [details] [diff] [review]
Part 6/7: ICC : v2

s/Disable/Disconnect/
Attachment #721092 - Flags: review?(bugs) → review+
(Assignee)

Comment 51

5 years ago
Another full try with review comments addressed: https://tbpl.mozilla.org/?tree=Try&rev=efc32fe20662
(Assignee)

Comment 52

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

Comment 53

5 years ago
Created attachment 721622 [details] [diff] [review]
Part 2/7: MobileConnection : v3

s/Disable/Disconnect/
Attachment #721086 - Attachment is obsolete: true
Attachment #721622 - Flags: review+
(Assignee)

Comment 54

5 years ago
Created attachment 721623 [details] [diff] [review]
Part 3/7: Cell Broadcast : v3

s/Disable/Disconnect/
Attachment #721087 - Attachment is obsolete: true
Attachment #721623 - Flags: review+
(Assignee)

Comment 55

5 years ago
Created attachment 721625 [details] [diff] [review]
Part 4/7: Voicemail : v3

s/Disable/Disconnect/
Attachment #721089 - Attachment is obsolete: true
Attachment #721625 - Flags: review+
(Assignee)

Comment 56

5 years ago
Created attachment 721626 [details] [diff] [review]
Part 5/7: Telephony : v3

s/Disable/Disconnect/
Attachment #721090 - Attachment is obsolete: true
Attachment #721626 - Flags: review+
(Assignee)

Comment 57

5 years ago
Created attachment 721627 [details] [diff] [review]
Part 6/7: ICC : v3

s/Disable/Disconnect/
Attachment #721092 - Attachment is obsolete: true
Attachment #721627 - Flags: review+
(Assignee)

Comment 58

5 years ago
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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
(Assignee)

Updated

4 years ago
Blocks: 904084
You need to log in before you can comment on or make changes to this bug.