B2G RIL: use ipdl as IPC in MozMobileConnection

RESOLVED FIXED in 2.1 S4 (12sep)

Status

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: edgar, Assigned: edgar)

Tracking

unspecified
2.1 S4 (12sep)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(feature-b2g:2.2+, tracking-b2g:backlog)

Details

(Whiteboard: [p=13])

Attachments

(20 attachments, 85 obsolete attachments)

1.54 KB, patch
gwagner
: review+
Details | Diff | Splinter Review
37.32 KB, patch
edgar
: review+
pgravel
: feedback-
Details | Diff | Splinter Review
4.20 KB, patch
edgar
: review+
Details | Diff | Splinter Review
403.80 KB, patch
smaug
: feedback+
Details | Diff | Splinter Review
2.50 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
3.68 KB, patch
Details | Diff | Splinter Review
3.30 KB, patch
edgar
: review+
Details | Diff | Splinter Review
1.69 KB, patch
edgar
: review+
Details | Diff | Splinter Review
2.87 KB, patch
edgar
: review+
Details | Diff | Splinter Review
14.44 KB, patch
Details | Diff | Splinter Review
8.76 KB, patch
edgar
: review+
Details | Diff | Splinter Review
7.61 KB, patch
edgar
: review+
Details | Diff | Splinter Review
39.90 KB, patch
edgar
: review+
Details | Diff | Splinter Review
39.34 KB, patch
edgar
: review+
Details | Diff | Splinter Review
67.82 KB, patch
edgar
: review+
Details | Diff | Splinter Review
21.69 KB, patch
edgar
: review+
Details | Diff | Splinter Review
1.87 KB, patch
edgar
: review+
Details | Diff | Splinter Review
170.49 KB, patch
edgar
: review+
Details | Diff | Splinter Review
12.01 KB, patch
edgar
: review+
Details | Diff | Splinter Review
5.10 KB, patch
edgar
: review+
Details | Diff | Splinter Review
Assignee

Description

6 years ago
To deprecate RILContentHelper, we need to use IPDL to replace current architecture of MozMobileConnection.
Assignee

Updated

6 years ago
Assignee

Updated

6 years ago
Assignee: nobody → echen
Assignee

Updated

6 years ago
Depends on: 857414
Blocks: 879710
Assignee

Updated

6 years ago
Depends on: 898445
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Assignee

Updated

6 years ago
Blocks: 898445
No longer depends on: 898445
Assignee

Comment 2

6 years ago
Currently, '$GECKO_ROOT/dom/network' contains some different modules, like NetworkStats, MobileConnection, TCPSocket. To make the architecture more clear, I would like to create a new folder 'mobileconneciton' under '$GECKO_ROOT/dom' and move mobileConnection related *.idl/*.js/*.c/*.h into '$GECKO_ROOT/dom/mobileconnection'. Does this sound a good idea?
Assignee

Comment 3

6 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #2)
> Currently, '$GECKO_ROOT/dom/network' contains some different modules, like
> NetworkStats, MobileConnection, TCPSocket. To make the architecture more
> clear, I would like to create a new folder 'mobileconneciton' under
> '$GECKO_ROOT/dom' and move mobileConnection related *.idl/*.js/*.c/*.h into
> '$GECKO_ROOT/dom/mobileconnection'. Does this sound a good idea?

And it seems we don't need to put MobileConnection into an extra namespacing, 'mozilla/dom/network', maybe we can just stick these in 'mozilla/dom'.
Assignee

Comment 4

6 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #3)
> (In reply to Edgar Chen [:edgar][:echen] from comment #2)
> > Currently, '$GECKO_ROOT/dom/network' contains some different modules, like
> > NetworkStats, MobileConnection, TCPSocket. To make the architecture more
> > clear, I would like to create a new folder 'mobileconneciton' under
> > '$GECKO_ROOT/dom' and move mobileConnection related *.idl/*.js/*.c/*.h into
> > '$GECKO_ROOT/dom/mobileconnection'. Does this sound a good idea?
> 
> And it seems we don't need to put MobileConnection into an extra
> namespacing, 'mozilla/dom/network', maybe we can just stick these in
> 'mozilla/dom'.

Hi smaug:

Before using ipdl as IPC, I would like to do below two things first,
1). Move mobileConnection related *.idl/*.js/*.c/*.h into '$GECKO_ROOT/dom/mobileconnection'.
2). Move mobileConnection from namespace 'mozilla/dom/network' to 'mozilla/dom'.

Do you think it is ok to have these changes?
Thank you. :)
Flags: needinfo?(bugs)
What all files would be moved to dom/mobileconnection?
If there just very few files, does it harm to keep them in dom/network ?

mozilla::dom namespaceing should be fine.
(I don't like too deep namespace hierarchies)
Flags: needinfo?(bugs)
Assignee

Comment 6

6 years ago
(In reply to Olli Pettay [:smaug] from comment #5)
> What all files would be moved to dom/mobileconnection?
> If there just very few files, does it harm to keep them in dom/network ?
Thanks for you reply.
Currently, there are not so many files would be moved to dom/mobileconnection,
- dom/network/interfaces/nsIDOMMobileConnection.idl
- dom/network/interfaces/nsIMobileConnectionProvider.idl
- dom/network/src/MobileConnection.cpp
- dom/network/src/MobileConnection.h
- dom/network/src/MobileConnectionArray.cpp
- dom/network/src/MobileConnectionArray.h
- dom/network/tests/marionette/
(After we move to Webidl event generator, we won't need below *Event.idl interfaces anymore)
- dom/network/interfaces/nsIDOMMozEmergencyCbModeEvent.idl
- dom/network/interfaces/nsIDOMMozOtaStatusEvent.idl
- dom/network/interfaces/nsIDOMUSSDReceivedEvent.idl
- dom/network/interfaces/nsIDOMDataErrorEvent.idl
- dom/network/interfaces/nsIDOMCFStateChangeEvent.idl

But there are 15+ files will be created to implement the IPDL implementation [1].
So I would like to move them all to dom/mobileconnection to make the file hierarchies more clear.

[1] You could reference the working branch in comment#1.
Assignee

Comment 7

6 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #6)
> (After we move to Webidl event generator, we won't need below *Event.idl
> interfaces anymore)
> - dom/network/interfaces/nsIDOMMozEmergencyCbModeEvent.idl
> - dom/network/interfaces/nsIDOMMozOtaStatusEvent.idl
> - dom/network/interfaces/nsIDOMUSSDReceivedEvent.idl
> - dom/network/interfaces/nsIDOMDataErrorEvent.idl
> - dom/network/interfaces/nsIDOMCFStateChangeEvent.idl

Bug 956240 was filed to address this.
Assignee

Updated

6 years ago
Depends on: 956655
Assignee

Updated

5 years ago
Depends on: 970812
Assignee

Updated

5 years ago
Assignee

Updated

5 years ago
No longer depends on: 970812
Assignee

Updated

5 years ago
See Also: → 970812
Assignee

Updated

5 years ago
Depends on: 946589

Comment 8

5 years ago
Edgar, this is a really good change. However, this is a big change and a risky one for 1.4 FC date coming up in few weeks. Wondering if we can make that change post once 1.4 is branched off?
Flags: needinfo?(echen)
Assignee

Comment 9

5 years ago
(In reply to Anshul from comment #8)
> Edgar, this is a really good change. However, this is a big change and a
> risky one for 1.4 FC date coming up in few weeks. Wondering if we can make
> that change post once 1.4 is branched off?

Yep, indeed this is a big change and risky for 1.4, I will wait until 1.4 is branched. Thanks for your reminding, Anshul.
Flags: needinfo?(echen)
Assignee

Updated

5 years ago
No longer blocks: 898445
Depends on: 898445
Put this bug into backlog.
blocking-b2g: --- → backlog
Assignee

Updated

5 years ago
Depends on: 995109
Assignee

Updated

5 years ago
Depends on: 929701
Assignee

Updated

5 years ago
No longer blocks: 999307
Assignee

Updated

5 years ago
Whiteboard: [p=13]
feature-b2g: --- → 2.1
Assignee

Comment 17

5 years ago
Attachment #8429098 - Attachment is obsolete: true
Assignee

Comment 19

5 years ago
Attachment #8430613 - Attachment is obsolete: true
Assignee

Comment 21

5 years ago
Attachment #8430614 - Attachment is obsolete: true
Assignee

Comment 23

5 years ago
Attachment #8433823 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Duplicate of this bug: 970812
Assignee

Comment 32

5 years ago
Attachment #8433850 - Attachment is obsolete: true
Assignee

Comment 35

5 years ago
Attachment #8428630 - Attachment is obsolete: true
Assignee

Comment 36

5 years ago
Comment on attachment 8438868 [details] [diff] [review]
Part 1-1: WEBIDL changes for MobileConnection IPDL, v2

Hi Hsinyi, Olli, could you help to review this?

1). Remove ChromeConstructor/Constructor of MozMobileNetworkInfo/DOMMMIError given that now we create the instance directly in C++.
2). Make MozMmiResult clearer.

Thank you.
Attachment #8438868 - Flags: review?(htsai)
Attachment #8438868 - Flags: review?(bugs)
Assignee

Comment 39

5 years ago
Comment on attachment 8438964 [details] [diff] [review]
Part 1-2: Mobile*Info implements both webidl and idl interfaces, v3

In this patch, MozMobileCellInfo and MozMobileConnectionInfo implement both webidl and idl interface.

Hi Olli, could you help to review this? Thank you.
Attachment #8438964 - Flags: review?(bugs)
Comment on attachment 8438868 [details] [diff] [review]
Part 1-1: WEBIDL changes for MobileConnection IPDL, v2

>-dictionary MozMMIResult
>+dictionary MozMmiResult
Why this change. Since most of the MMI stuff has MMI and not Mmi, I'd prefer the old name

>   /**
>    * Some MMI requests like call forwarding or PIN/PIN2/PUK/PUK2 related
>    * requests provide extra information along with the status message, this
>-   * information can be a number, a string key or an array of string keys.
>+   * information can be a number, an array of string keys or an array of
>+   * MozCallForwardingOptions.
>+   *
>+   * And it should be
>+   * (unsigned short or sequence<DOMString> or sequence<MozCallForwardingOptions>)
>+   * But we cannot yet use sequences as union member types (please see bug 767924)
>+   * ,so we use object here.
>    */
>-  any additionalInformation;
>+  (unsigned short or object) additionalInformation;
So additionalInformation is never just a string key?
Attachment #8438868 - Flags: review?(bugs) → review+
Attachment #8438964 - Flags: review?(bugs) → review+
Comment on attachment 8438868 [details] [diff] [review]
Part 1-1: WEBIDL changes for MobileConnection IPDL, v2

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

::: dom/webidl/MozMobileConnection.webidl
@@ +650,5 @@
>     */
>    DOMString? newPin;
>  };
>  
> +dictionary MozMmiResult

I'd prefer letting it be consistent with the names used elsewhere, i.e. stay capital letters. Thank you!
Attachment #8438868 - Flags: review?(htsai) → review+
Assignee

Comment 42

5 years ago
(In reply to Olli Pettay [:smaug] from comment #40)
> Comment on attachment 8438868 [details] [diff] [review]
> Part 1-1: WEBIDL changes for MobileConnection IPDL, v2
> >   /**
> >    * Some MMI requests like call forwarding or PIN/PIN2/PUK/PUK2 related
> >    * requests provide extra information along with the status message, this
> >-   * information can be a number, a string key or an array of string keys.
> >+   * information can be a number, an array of string keys or an array of
> >+   * MozCallForwardingOptions.
> >+   *
> >+   * And it should be
> >+   * (unsigned short or sequence<DOMString> or sequence<MozCallForwardingOptions>)
> >+   * But we cannot yet use sequences as union member types (please see bug 767924)
> >+   * ,so we use object here.
> >    */
> >-  any additionalInformation;
> >+  (unsigned short or object) additionalInformation;
> So additionalInformation is never just a string key?

No, it is never just a string key. The old doc wasn't up to date.
Assignee

Comment 43

5 years ago
Thanks for your review, will keep the old name, "MozMMIResult", in next version.

(In reply to Olli Pettay [:smaug] from comment #40)
> Comment on attachment 8438868 [details] [diff] [review]
> Part 1-1: WEBIDL changes for MobileConnection IPDL, v2
> 
> >-dictionary MozMMIResult
> >+dictionary MozMmiResult
> Why this change. Since most of the MMI stuff has MMI and not Mmi, I'd prefer
> the old name

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #41)
> Comment on attachment 8438868 [details] [diff] [review]
> Part 1-1: WEBIDL changes for MobileConnection IPDL, v2
> 
> Review of attachment 8438868 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/MozMobileConnection.webidl
> @@ +650,5 @@
> >     */
> >    DOMString? newPin;
> >  };
> >  
> > +dictionary MozMmiResult
> 
> I'd prefer letting it be consistent with the names used elsewhere, i.e. stay
> capital letters. Thank you!

Comment 44

5 years ago
Comment on attachment 8430675 [details] [diff] [review]
Part 2-1: MobileConnectionProvider interface changes for IPDL, v3

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

Please avoid using jsval in public interfaces (such as in nsIMobileConnectionCallback). Makes life harder for native code implementations.
Attachment #8430675 - Flags: feedback-
Assignee

Comment 45

5 years ago
(In reply to pgravel from comment #44)
> Comment on attachment 8430675 [details] [diff] [review]
> Part 2-1: MobileConnectionProvider interface changes for IPDL, v3
> 
> Review of attachment 8430675 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please avoid using jsval in public interfaces (such as in
> nsIMobileConnectionCallback). Makes life harder for native code
> implementations.

> +  void notifySuccessCallForwarding(in jsval results /* Array of MozCallForwardingOptions */);
Hi pgravel:

In native implementation, you could use MozCallForwardingOptions which is a WebIDL dictionary and simply create a JS::Value for it by using "ToJSValue" [1].

[1] https://groups.google.com/forum/#!searchin/mozilla.dev.platform/ToJSValue/mozilla.dev.platform/Y23hkRFJJwY/hdmay-jfQhkJ

Comment 46

5 years ago
Unfortunately using "mozilla::dom::" outside of of the gecko code-base is problematic. We've run into several compilation complications in the past due to changes in JSAPI-related interfaces and would much prefer to avoid running into more of the same.

Having strongly-typed interfaces is generally preferable in the long run regardless.
Assignee

Comment 47

5 years ago
1). Rebase
2). Address review comment #40 and comment #41,
    - Keep the old name: MozMMIResult
Attachment #8438868 - Attachment is obsolete: true
Attachment #8444215 - Flags: review+
Assignee

Comment 49

5 years ago
Attachment #8430675 - Attachment is obsolete: true
Assignee

Comment 50

5 years ago
Comment on attachment 8444220 [details] [diff] [review]
Part 2-1.a: Rename MobileConnectionProvider to MobileConnectionService, v1

This patch renames the MobileConnectionProvider to MobileConnectionService in order to be consistent with the naming of Telephony and Sms.
Attachment #8444220 - Flags: review?(htsai)
Assignee

Comment 51

5 years ago
Comment on attachment 8444221 [details] [diff] [review]
Part 2-1.b: MobileConnectionService interface changes for IPDL, v4

There are some changes for service interface:
1). s/notifyCFStateChange/notifyCFStateChanged/
2). Add three new callbacks in nsIMobileConnectionListener
    - notifyLastKnownNetworkChanged()
    - notifyLastKnownHomeNetworkChanged()
    - notifyNetworkSelectionModeChanged()
    It is because we would like to prevent using synced message in ipc protocol
    , so the child side needs to keep the latest data and also needs a
    notification for notifying the data is changed.
3). Introduce nsIMobileConnectionCallback and use it for notifying the result
    of asynced request in MobileConnectionService.
Attachment #8444221 - Flags: review?(htsai)
Comment on attachment 8444220 [details] [diff] [review]
Part 2-1.a: Rename MobileConnectionProvider to MobileConnectionService, v1

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

::: dom/mobileconnection/interfaces/moz.build
@@ +6,5 @@
>  
>  XPIDL_SOURCES += [
>      'nsIMobileCellInfo.idl',
>      'nsIMobileConnectionInfo.idl',
> +    'nsIMobileConnectionService.idl',

Theoretically, we need a build peer's review. But the change here is trivial, so r=me.
Attachment #8444220 - Flags: review?(htsai) → review+
Comment on attachment 8444221 [details] [diff] [review]
Part 2-1.b: MobileConnectionService interface changes for IPDL, v4

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

::: dom/mobileconnection/interfaces/nsIMobileConnectionService.idl
@@ +120,4 @@
>  };
>  
> +[scriptable, builtinclass, uuid(10118f19-9f49-4a8b-9bed-29f154d27820)]
> +interface nsIMobileConnectionCallback : nsISupports

We have two groups of "notifySuccess"; one is categorized according to 'type of result' e.g. notifySuccessString and notifySuccessBoolean, while the other is according to the original requests, e.g. notifySuccessMmi and notifySuccessClirStatus.

I would like to have a consistent naming pattern only if the pattern fits all cases and is intelligible. And in this interface, I am considering adopting various patterns. How about the below? Looks better to you?

@@ +126,5 @@
> +   * notifySuccess*() will be called, when request is succeed.
> +   */
> +  void notifySuccess();
> +
> +  void notifySuccessString(in DOMString result);

notifySuccessWithString

@@ +128,5 @@
> +  void notifySuccess();
> +
> +  void notifySuccessString(in DOMString result);
> +
> +  void notifySuccessBoolean(in boolean result);

notifySuccessWithBoolean

@@ +130,5 @@
> +  void notifySuccessString(in DOMString result);
> +
> +  void notifySuccessBoolean(in boolean result);
> +
> +  void notifySuccessNetworks(in uint32_t count,

notifyGetNetworksSuccess

@@ +134,5 @@
> +  void notifySuccessNetworks(in uint32_t count,
> +                             [array, size_is(count)] in nsIMobileNetworkInfo networks);
> +
> +  [optional_argc]
> +  void notifySuccessMmi(in DOMString serviceCode, in DOMString statusMessage,

notifySendCancelMmiSuccess

@@ +137,5 @@
> +  [optional_argc]
> +  void notifySuccessMmi(in DOMString serviceCode, in DOMString statusMessage,
> +                        [optional] in jsval additionalInformation);
> +
> +  void notifySuccessCallForwarding(in jsval results /* Array of MozCallForwardingOptions */);

notifyGetCallForwardingSuccess

@@ +139,5 @@
> +                        [optional] in jsval additionalInformation);
> +
> +  void notifySuccessCallForwarding(in jsval results /* Array of MozCallForwardingOptions */);
> +
> +  void notifySuccessCallBarring(in unsigned short program, in boolean enabled,

notifyGetCallBarringSuccess

@@ +142,5 @@
> +
> +  void notifySuccessCallBarring(in unsigned short program, in boolean enabled,
> +                                in unsigned short serviceClass);
> +
> +  void notifySuccessClirStatus(in unsigned short n, in unsigned short m);

notifyGetClirStatusSuccess
Attachment #8444221 - Flags: review?(htsai)
Assignee

Comment 54

5 years ago
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #53)
> Comment on attachment 8444221 [details] [diff] [review]
> Part 2-1.b: MobileConnectionService interface changes for IPDL, v4
> 
> Review of attachment 8444221 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobileconnection/interfaces/nsIMobileConnectionService.idl
> @@ +120,4 @@
> >  };
> >  
> > +[scriptable, builtinclass, uuid(10118f19-9f49-4a8b-9bed-29f154d27820)]
> > +interface nsIMobileConnectionCallback : nsISupports
> 
> We have two groups of "notifySuccess"; one is categorized according to 'type
> of result' e.g. notifySuccessString and notifySuccessBoolean, while the
> other is according to the original requests, e.g. notifySuccessMmi and
> notifySuccessClirStatus.
> 
> I would like to have a consistent naming pattern only if the pattern fits
> all cases and is intelligible. And in this interface, I am considering
> adopting various patterns. How about the below? Looks better to you?

I like your proposal, will address them in next version. Thank you. :)
Assignee

Comment 55

5 years ago
Address review comment#53:
- s/notifySuccessString/notifySuccessWithString/
- s/notifySuccessBoolean/notifySuccessWithBoolean/
- s/notifySuccessNetworks/notifyGetNetworksSuccess/
- s/notifySuccessMmi/notifySendCancelMmiSuccess/
- s/notifySuccessCallForwarding/notifyGetCallForwardingSuccess/
- s/notifySuccessCallBarring/notifyGetCallBarringSuccess/
- s/notifySuccessClirStatus/notifyGetClirStatusSuccess/
Attachment #8444221 - Attachment is obsolete: true
Assignee

Comment 58

5 years ago
Attachment #8433821 - Attachment is obsolete: true
Assignee

Comment 59

5 years ago
Attachment #8433849 - Attachment is obsolete: true
Assignee

Comment 60

5 years ago
Attachment #8433982 - Attachment is obsolete: true
Assignee

Comment 61

5 years ago
Attachment #8433862 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8449179 - Attachment description: Part 5-2: [GPS] Get voiceInfo via MobileConnection, v1 → Part 4-2: [GPS] Get voiceInfo via MobileConnectionService, v2
Assignee

Updated

5 years ago
Attachment #8433864 - Attachment is obsolete: true
Assignee

Comment 63

5 years ago
Attachment #8433865 - Attachment is obsolete: true
Assignee

Comment 64

5 years ago
Attachment #8433866 - Attachment is obsolete: true
Assignee

Comment 65

5 years ago
Attachment #8433869 - Attachment is obsolete: true
Assignee

Comment 67

5 years ago
Attachment #8434021 - Attachment is obsolete: true
Assignee

Comment 68

5 years ago
Attachment #8434025 - Attachment is obsolete: true
Assignee

Comment 69

5 years ago
Comment on attachment 8449147 [details] [diff] [review]
Part 3-1: IPDL for MobileConnection, v3

Hi Olli, may I have your feedback for ipdl protocol?

Some explanation about the design:
1). I would like to reuse the dictionaries (defined in 
    MozMobileConnection.webidl) in ipdl, but the structure used in ipdl must 
    has equal operator implemented. So I implement the equal operator and
    also write serialize/deserialize function for them.

    For example:

    If there is a dictionary "Foo" defined in Foo.webidl,
    we do following things,

    Struct nsFoo : Foo {
      bool operator==(const nsFoo& aOther) const
      {
        ....
      }
    }

    and

    template <>
    struct ParamTraits<nsFoo>
    {
      static void Write(...)
      {
        ...
      }

      static bool Read(...)
      {
        ...
      }
    }

2). There are some synced IPC call here. Those synced call only be called
    once per child process for initialization. I had tried to use async way
    for initialization (it seems we prefer not use sync protocol/message),
    but using async way will change the behaviour and may break something :(. 
    The problem of async way we have is api consumer may have a chance to
    get a data that is not initialized yet. To solve this, looks like we
    need to provide an additional information for the readiness of data
    (maybe dispatch an event when it's ready?) and api consumer also needs
    do some checks (or wait an event) first before accessing.
    In current design, we don't have such problems because we use sync
    message in cpmm/ppmm [1] for initialization. Given that I would like 
    to follow the current design first and file a follow-up bug for async
    things. Any suggestions?

Thank you in advance. :)

[1] http://hg.mozilla.org/mozilla-central/file/7084a82a629d/dom/system/gonk/RILContentHelper.js#l581
Attachment #8449147 - Flags: feedback?(bugs)
Comment on attachment 8449180 [details] [diff] [review]
Part 4-3: [MMS] Get voiceInfo via MobileConnectionService, v2

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +316,5 @@
>     *
>     * @return true if voice call is roaming.
>     */
>    isVoiceRoaming: function() {
> +    let voice = gMobileConnectionService.getVoiceConnectionInfo(this.serviceId);

Just curious that, should we or could we check if 'voice' is valid or not, after calling getVoiceConnectionInfo(), and before using voice.roaming?
Assignee

Comment 71

5 years ago
(In reply to Kevin Hu [:khu] from comment #70)
> Comment on attachment 8449180 [details] [diff] [review]
> Part 4-3: [MMS] Get voiceInfo via MobileConnectionService, v2
> 
> Review of attachment 8449180 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/gonk/MmsService.js
> @@ +316,5 @@
> >     *
> >     * @return true if voice call is roaming.
> >     */
> >    isVoiceRoaming: function() {
> > +    let voice = gMobileConnectionService.getVoiceConnectionInfo(this.serviceId);
> 
> Just curious that, should we or could we check if 'voice' is valid or not,
> after calling getVoiceConnectionInfo(), and before using voice.roaming?

No, we don't need to check, getVoiceConnectionInfo() always returns a valid nsIMobileConnectionInfo object if we pass a valid serviceId.
(In reply to Edgar Chen [:edgar][:echen] from comment #69)
>     If there is a dictionary "Foo" defined in Foo.webidl,
>     we do following things,
> 
>     Struct nsFoo : Foo {
No ns-prefix, but make it be Foo in ipc namespace?




> 2). There are some synced IPC call here. Those synced call only be called
>     once per child process for initialization. I had tried to use async way
>     for initialization (it seems we prefer not use sync protocol/message),
>     but using async way will change the behaviour and may break something
> :(. 
The issue with sync is that it slows down app initialization _significantly_.
We have tried to get rid of all those, but there are still some left, 
like gfx initialization and bug 1003848.
(Recently fixed cases are for example bug 1003041 and bug 1003046)


>     The problem of async way we have is api consumer may have a chance to
>     get a data that is not initialized yet. To solve this, looks like we
>     need to provide an additional information for the readiness of data
>     (maybe dispatch an event when it's ready?) and api consumer also needs
>     do some checks (or wait an event) first before accessing.
>     In current design, we don't have such problems because we use sync
>     message in cpmm/ppmm [1] for initialization. Given that I would like 
>     to follow the current design first and file a follow-up bug for async
>     things. Any suggestions?
Can we postpone the app initialization until parent has the needed data, and
then when initializing the app process send data from parent to child?

In general I think we should not do any sync child->parent messaging during app initialization.
It just causes child process to wait for tens or hundreds of milliseconds parent to send data back.
Attachment #8449147 - Flags: feedback?(bugs)
Assignee

Comment 73

5 years ago
(In reply to Olli Pettay [:smaug] from comment #72)
> (In reply to Edgar Chen [:edgar][:echen] from comment #69)
> >     If there is a dictionary "Foo" defined in Foo.webidl,
> >     we do following things,
> > 
> >     Struct nsFoo : Foo {
> No ns-prefix, but make it be Foo in ipc namespace?
> 
Even better!

> 
> > 2). There are some synced IPC call here. Those synced call only be called
> >     once per child process for initialization. I had tried to use async way
> >     for initialization (it seems we prefer not use sync protocol/message),
> >     but using async way will change the behaviour and may break something
> > :(. 
> The issue with sync is that it slows down app initialization _significantly_.
> We have tried to get rid of all those, but there are still some left, 
> like gfx initialization and bug 1003848.
> (Recently fixed cases are for example bug 1003041 and bug 1003046)
> 
I will take a look at those fixed cases first. Thank you. :)
Assignee

Comment 74

5 years ago
(In reply to Olli Pettay [:smaug] from comment #72)
> (In reply to Edgar Chen [:edgar][:echen] from comment #69)
> > 2). There are some synced IPC call here. Those synced call only be called
> >     once per child process for initialization. I had tried to use async way
> >     for initialization (it seems we prefer not use sync protocol/message),
> >     but using async way will change the behaviour and may break something
> > :(. 
> The issue with sync is that it slows down app initialization _significantly_.
> We have tried to get rid of all those, but there are still some left, 
> like gfx initialization and bug 1003848.
> (Recently fixed cases are for example bug 1003041 and bug 1003046)
> 
> 
> >     The problem of async way we have is api consumer may have a chance to
> >     get a data that is not initialized yet. To solve this, looks like we
> >     need to provide an additional information for the readiness of data
> >     (maybe dispatch an event when it's ready?) and api consumer also needs
> >     do some checks (or wait an event) first before accessing.
> >     In current design, we don't have such problems because we use sync
> >     message in cpmm/ppmm [1] for initialization. Given that I would like 
> >     to follow the current design first and file a follow-up bug for async
> >     things. Any suggestions?
> Can we postpone the app initialization until parent has the needed data, and
> then when initializing the app process send data from parent to child?
> 
> In general I think we should not do any sync child->parent messaging during
> app initialization.
> It just causes child process to wait for tens or hundreds of milliseconds
> parent to send data back.

((Off-line discussed with Olli))
PMobileConnection is constructed along with navigator.mozMobileconnections[x] when app has proper permission and the first time access navigator.mozMobileconnections[x]. Using synced call for initialization here seems not be an issue. But we still can have some improvement, for example just have one for all information.

Comment 75

5 years ago
Wesley, are you guys targeting this bug for 2.1? Could you please add an ETA so we can be prepared on our side as this is a pretty big change.
Flags: needinfo?(whuang)
Hi Anshul, 
Yes, this will be included in v2.1.
The feature should be landed before Aug29 which is the FL date.
As Edgar is already working on this for a while, ni? him if he can reply a more detailed ETA.
Flags: needinfo?(whuang) → needinfo?(echen)
Assignee

Comment 77

5 years ago
Attachment #8445077 - Attachment is obsolete: true
Flags: needinfo?(echen)
Assignee

Comment 79

5 years ago
Comment on attachment 8455231 [details] [diff] [review]
Part 2-1.b: MobileConnectionService interface changes for IPDL, v6

Address review comment#53:
- s/notifySuccessString/notifySuccessWithString/
- s/notifySuccessBoolean/notifySuccessWithBoolean/
- s/notifySuccessNetworks/notifyGetNetworksSuccess/
- s/notifySuccessMmi/notifySendCancelMmiSuccess/
- s/notifySuccessCallForwarding/notifyGetCallForwardingSuccess/
- s/notifySuccessCallBarring/notifyGetCallBarringSuccess/
- s/notifySuccessClirStatus/notifyGetClirStatusSuccess/

Hi Hsinyi, could you help to review it again? Thank you.
Attachment #8455231 - Flags: review?(htsai)
Comment on attachment 8455231 [details] [diff] [review]
Part 2-1.b: MobileConnectionService interface changes for IPDL, v6

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

Looks good, thank you.
Attachment #8455231 - Flags: review?(htsai) → review+
Assignee

Comment 81

5 years ago
Rebase
Attachment #8444220 - Attachment is obsolete: true
Attachment #8456046 - Flags: review+
Assignee

Comment 82

5 years ago
Rebase
Attachment #8445078 - Attachment is obsolete: true
Assignee

Comment 83

5 years ago
Comment on attachment 8456047 [details] [diff] [review]
Part 2-2: Bluetooth changes for MobileConnectionService interface, v3

Hi Eric, we change the mobileconnection internal interface in patch part2.1 for IPDL, so bluetooth needs do corresponding changes as well. May I have your review? Thank you.
Attachment #8456047 - Flags: review?(echou)
Assignee

Comment 85

5 years ago
Use AutoJSAPI instead.
Attachment #8456059 - Attachment is obsolete: true
Assignee

Comment 86

5 years ago
Comment on attachment 8456753 [details] [diff] [review]
Part 2-3: MobileConnection DOM changes for MobileConnectionService interface, v7

Hi Olli,

In patch part2-1*, we redesign the internal interface for IPDL implementation,
- Rename nsIMobileConnectionProvider to nsIMobileConnectionService.
- Add some new callbacks into nsIMobileConnectionListener.
- Introduce nsIMobileConnectionCallback as a callback of async request.
- Asynced function of nsIMobileConnectionService doesn't return a DOMRequest any more, but takes nsIMobileConnectionCallback as one of it's argument, and the aynced result will be notified via nsIMobileConectionCallback.

And MobileConnection needs to do corresponding change as well. 
May I have your review for the changes in this patch?

Thank you.
Attachment #8456753 - Flags: review?(bugs)
Assignee

Comment 87

5 years ago
1). Address comment #72
    - no ns prefix, but put it IPC namespace.
2). Address comment #74
    - Combine all synced protocol into one.
Attachment #8449147 - Attachment is obsolete: true
Assignee

Comment 88

5 years ago
Attachment #8449150 - Attachment is obsolete: true
Assignee

Comment 89

5 years ago
Attachment #8449159 - Attachment is obsolete: true
Comment on attachment 8456047 [details] [diff] [review]
Part 2-2: Bluetooth changes for MobileConnectionService interface, v3

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

LGTM since it looks like this patch only does renaming.
Attachment #8456047 - Flags: review?(echou) → review+
Comment on attachment 8456753 [details] [diff] [review]
Part 2-3: MobileConnection DOM changes for MobileConnectionService interface, v7

>+MobileConnectionCallback::NotifySuccessWithString(const nsAString& aResult)
>+{
>+  AutoJSAPI jsapi;
>+  if (!NS_WARN_IF(jsapi.Init(mWindow))) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  JSContext *cx = jsapi.cx();
>+  JS::Rooted<JS::Value> jsResult(cx);
>+
>+  if (!ToJSValue(cx, aResult, &jsResult)) {
Shouldn't you clear the possible exception?


>+MobileConnectionCallback::NotifySuccessWithBoolean(bool aResult)
>+{
>+  AutoJSAPI jsapi;
>+  if (!NS_WARN_IF(jsapi.Init(mWindow))) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  JSContext *cx = jsapi.cx();
>+  JS::Rooted<JS::Value> jsResult(cx, aResult ? JSVAL_TRUE : JSVAL_FALSE);
>+
>+  return NotifySuccess(jsResult);
Just use TrueHandleValue/FalseHandleValue ?


>+MobileConnectionCallback::NotifyGetNetworksSuccess(uint32_t aCount,
>+                                                   nsIMobileNetworkInfo** aNetworks)
>+{
>+  nsTArray<nsRefPtr<MobileNetworkInfo>> results;
>+  for (uint32_t i = 0; i < aCount; i++)
>+  {
>+    nsRefPtr<MobileNetworkInfo> networkInfo = new MobileNetworkInfo(mWindow);
>+    networkInfo->Update(aNetworks[i]);
>+    results.AppendElement(networkInfo);
>+  }
>+
>+  AutoJSAPI jsapi;
>+  if (!NS_WARN_IF(jsapi.Init(mWindow))) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  JSContext *cx = jsapi.cx();
>+  JS::Rooted<JS::Value> jsResult(cx);
>+
>+  if (!ToJSValue(cx, results, &jsResult)) {
Again, I'd imagine you want to clear the exception here.
(I wish bug 1023295 was fixed.)

>+MobileConnectionCallback::NotifyGetCallBarringSuccess(uint16_t aProgram,
>+                                                      bool aEnabled,
>+                                                      uint16_t aServiceClass)
>+{
>+  AutoJSAPI jsapi;
>+  if (!NS_WARN_IF(jsapi.Init(mWindow))) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  JSContext *cx = jsapi.cx();
>+  RootedDictionary<MozCallBarringOptions> result(cx);
>+  result.mProgram.Construct().SetValue(aProgram);
>+  result.mEnabled.Construct().SetValue(aEnabled);
>+  result.mServiceClass.Construct().SetValue(aServiceClass);
>+
>+  JS::Rooted<JS::Value> jsResult(cx);
>+  if (!ToJSValue(cx, result, &jsResult)) {
ditto

>+MobileConnectionCallback::NotifyGetClirStatusSuccess(uint16_t aN, uint16_t aM)
>+{
>+  AutoJSAPI jsapi;
>+  if (!NS_WARN_IF(jsapi.Init(mWindow))) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  JSContext *cx = jsapi.cx();
>+  RootedDictionary<MozClirStatus> result(cx);
>+  result.mN.Construct(aN);
>+  result.mM.Construct(aM);
Why do you need RootedDictionary here?
Isn't MozClirStatus keeping just some simple integer data alive.
No JS stuff anywhere


>+
>+  JS::Rooted<JS::Value> jsResult(cx);
>+  if (!ToJSValue(cx, result, &jsResult)) {
exception handling?
>+class MobileConnectionCallback : public nsIMobileConnectionCallback
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIMOBILECONNECTIONCALLBACK
>+
>+  MobileConnectionCallback(nsPIDOMWindow* aWindow, DOMRequest* aRequest);
>+
>+private:
>+  nsresult
>+  NotifySuccess(JS::Handle<JS::Value> aResult);
>+
>+  nsCOMPtr<nsPIDOMWindow> mWindow;
>+  nsRefPtr<DOMRequest> mRequest;
>+};
So need to be careful to not keep MobileConnectionCallback objects alive too long.
What is the lifetime management? Could you add some comment to this file


I could take a look at the updated patch.
Attachment #8456753 - Flags: review?(bugs) → review-
Assignee

Comment 92

5 years ago
Thanks for the review, Olli!!

(In reply to Olli Pettay [:smaug] from comment #91)
> Comment on attachment 8456753 [details] [diff] [review]
> Part 2-3: MobileConnection DOM changes for MobileConnectionService
> interface, v7
> 
> >+MobileConnectionCallback::NotifySuccessWithString(const nsAString& aResult)
> >+{
> >+  AutoJSAPI jsapi;
> >+  if (!NS_WARN_IF(jsapi.Init(mWindow))) {
> >+    return NS_ERROR_FAILURE;
> >+  }
> >+
> >+  JSContext *cx = jsapi.cx();
> >+  JS::Rooted<JS::Value> jsResult(cx);
> >+
> >+  if (!ToJSValue(cx, aResult, &jsResult)) {
> Shouldn't you clear the possible exception?
Yes, we should suppress the exception which comes from ToJSValue here.
Will address them in next version.

> 
> 
> >+MobileConnectionCallback::NotifySuccessWithBoolean(bool aResult)
> >+{
> >+  AutoJSAPI jsapi;
> >+  if (!NS_WARN_IF(jsapi.Init(mWindow))) {
> >+    return NS_ERROR_FAILURE;
> >+  }
> >+
> >+  JSContext *cx = jsapi.cx();
> >+  JS::Rooted<JS::Value> jsResult(cx, aResult ? JSVAL_TRUE : JSVAL_FALSE);
> >+
> >+  return NotifySuccess(jsResult);
> Just use TrueHandleValue/FalseHandleValue ?
Will do this.

> 
> >+MobileConnectionCallback::NotifyGetClirStatusSuccess(uint16_t aN, uint16_t aM)
> >+{
> >+  AutoJSAPI jsapi;
> >+  if (!NS_WARN_IF(jsapi.Init(mWindow))) {
> >+    return NS_ERROR_FAILURE;
> >+  }
> >+
> >+  JSContext *cx = jsapi.cx();
> >+  RootedDictionary<MozClirStatus> result(cx);
> >+  result.mN.Construct(aN);
> >+  result.mM.Construct(aM);
> Why do you need RootedDictionary here?
> Isn't MozClirStatus keeping just some simple integer data alive.
> No JS stuff anywhere
Ah, yes, just two simple integer data.
Will remove RootedDictionary.
Assignee

Comment 93

5 years ago
Address review comment #91.
- exception handling for ToJSValue
- use TrueHandleValue/FalseHandleValue instead
- no need RootedDictionary if no JS stuff
- add some comment for the lifetime of MobileConnectionCallback

Hi Olli, could you help to take look at again?

Thank you.
Attachment #8456753 - Attachment is obsolete: true
Attachment #8458497 - Flags: review?(bugs)
Assignee

Comment 94

5 years ago
Just revise some comments.
Attachment #8456780 - Attachment is obsolete: true
Assignee

Comment 95

5 years ago
Comment on attachment 8458499 [details] [diff] [review]
Part 3-1: IPDL for MobileConnection, v5

Address comment #72:
- put the structure used for ipdl in ipc namespace.

Address comment #74:
- Use only one sync call for initialization. I did not merge it with SendPMobileConnectionChildConstructor is because we maintain the data in MobileConnectionChild (please see part3-2), put the sync call into PMobileConnection makes the implementation easier.

Hi Olli, may I have your review for the ipdl things?

Thank you.
Attachment #8458499 - Flags: review?(bugs)
Assignee

Comment 96

5 years ago
Changes since v4,
- exception handling
- don't need RootedDictionary if no JS stuff
- Add some comments about child/parent actor
Attachment #8456795 - Attachment is obsolete: true
Assignee

Comment 97

5 years ago
Comment on attachment 8458514 [details] [diff] [review]
Part 3-2: IPDL implementation for MobileConnection, v5

Here is the implementation of PMobileConnection.
Attachment #8458514 - Flags: review?(bugs)
Assignee

Updated

5 years ago
Attachment #8456797 - Flags: review?(bugs)
Assignee

Comment 98

5 years ago
Just rebase
Attachment #8449162 - Attachment is obsolete: true
Assignee

Comment 99

5 years ago
Comment on attachment 8458520 [details] [diff] [review]
Part 4-1: Interface changes for gonk backend, v3

Hi Hsinyi, may I have your review for the interface of gonk backend? Thank you.
Attachment #8458520 - Flags: review?(htsai)
Assignee

Comment 100

5 years ago
Comment on attachment 8449179 [details] [diff] [review]
Part 4-2: [GPS] Get voiceInfo via MobileConnectionService, v2

After moving to IPDL, we don't expose voiceInfo in nsIRilContext any more. But other modules still can get it from nsIMobileConnectionService. This patch is for the changes of GPS.
Hi Kan-Ru, may I have your review? Thank you
Attachment #8449179 - Flags: review?(kchen)
Assignee

Comment 101

5 years ago
Comment on attachment 8449180 [details] [diff] [review]
Part 4-3: [MMS] Get voiceInfo via MobileConnectionService, v2

Hi Vicamo, could you help to review the changes in MMSService? Thank you.
Attachment #8449180 - Flags: review?(vyang)
Assignee

Comment 102

5 years ago
Rebase only
Attachment #8449181 - Attachment is obsolete: true
Comment on attachment 8449179 [details] [diff] [review]
Part 4-2: [GPS] Get voiceInfo via MobileConnectionService, v2

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

This is great!

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +34,5 @@
>  #include "nsIDOMIccInfo.h"
>  #include "nsIMobileConnectionInfo.h"
>  #include "nsIMobileCellInfo.h"
>  #include "nsIRadioInterfaceLayer.h"
> +#include "nsIMobileConnectionService.h"

Sort them in alphabetical order

@@ +513,4 @@
>      nsCOMPtr<nsIMobileConnectionInfo> voice;
> +    // TODO: Bug 878748 - B2G GPS: acquire correct RadioInterface instance in
> +    // MultiSIM configuration
> +    service->GetVoiceConnectionInfo(0 /* Client Id */,getter_AddRefs(voice));

nit: one space after comma
Attachment #8449179 - Flags: review?(kchen) → review+
Attachment #8449180 - Flags: review?(vyang) → review+
Comment on attachment 8458520 [details] [diff] [review]
Part 4-1: Interface changes for gonk backend, v3

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

Looks good, thank you.

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ -38,3 @@
>    readonly attribute DOMString cardState;
>  
> -  readonly attribute long retryCount;

Why removing this? Shouldn't we need this for IccWebAPI?
Attachment #8458520 - Flags: review?(htsai) → review+
Assignee

Comment 105

5 years ago
Comment on attachment 8458536 [details] [diff] [review]
Part 4-4: PhoneNumberUtils changes for MobileConnection ipdl, v3

Hi Gregor, could you help to review this?
- Rename the nsIMobileConnectionProvider to nsIMobileConnectionService (please see part 2.1)
- Change the contract id given that nsIMobileConnectionService is not implemented by RILContentHelper, but MobileConnectionGonkService.

Thank you
Attachment #8458536 - Flags: review?(anygregor)
Assignee

Comment 106

5 years ago
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #104)
> Comment on attachment 8458520 [details] [diff] [review]
> Part 4-1: Interface changes for gonk backend, v3
> 
> Review of attachment 8458520 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, thank you.
> 
> ::: dom/system/gonk/nsIRadioInterfaceLayer.idl
> @@ -38,3 @@
> >    readonly attribute DOMString cardState;
> >  
> > -  readonly attribute long retryCount;
> 
> Why removing this? Shouldn't we need this for IccWebAPI?

Now we use getCardLockRetryCount() to get retry count. The retryCount should be removed in bug 886239, this one seems a missing. So it is safe to remove this.
Assignee

Comment 107

5 years ago
Comment on attachment 8449182 [details] [diff] [review]
Part 4-5.a: Refactor otaStatus updating, v2

Just refactor the ota status updating. (Will squash with part4-5.b/c/d for landing)
Hi Hsinyi, could you help to review this? Thank you
Attachment #8449182 - Flags: review?(htsai)
Assignee

Comment 108

5 years ago
Rebase only
Attachment #8449183 - Attachment is obsolete: true
Assignee

Comment 109

5 years ago
Comment on attachment 8458578 [details] [diff] [review]
Part 4-5.b: Combine GECKO_RADIOSTATE_* and  GECKO_DETAILED_RADIOSTATE_*, v3

((Will squash with part4-5.x for landing))
Attachment #8458578 - Flags: review?(htsai)
Assignee

Comment 110

5 years ago
Attachment #8449184 - Attachment is obsolete: true
Assignee

Comment 111

5 years ago
Attachment #8449185 - Attachment is obsolete: true
Assignee

Comment 112

5 years ago
Comment on attachment 8458582 [details] [diff] [review]
Part 4-5.c: MobileConnectionGonkService for gonk backend, v3

Quick summarise for this patch:
- MobileConnectionProvider represent a sim slot (clientId) and the data of each sim slot is maintained in corresponding MobileConnectionProvider.
- MobileConectionGonkService implements nsIMobileConnectionGonkService. It creates all MobileConnectionProviders and dispatches request to one of MobileConnectionProviders based on clientId. 
- I keep the original logic/flow of data/voice/operator/signal updating as much as possible to prevent regression.

Hi Hsinyi, could you help to take a look at this? Thank you
Attachment #8458582 - Flags: review?(htsai)
Assignee

Comment 113

5 years ago
Comment on attachment 8458585 [details] [diff] [review]
Part 4-5.d: gRadioEnabledController changes, v3

The request of radio enable/disable is handled in gRadioEnabledController now, so we have some special handling for this in this patch. Basically we catch the "setRadioEnabled" message in sendWorkerMessage() and sent it to gRadioEnabledController. I also did some refactor to make it easier to understand.

Hi Hsinyi, could you help to review this? Thank you.
Attachment #8458585 - Flags: review?(htsai)
Attachment #8449182 - Flags: review?(htsai) → review+
Attachment #8458578 - Flags: review?(htsai) → review+
Comment on attachment 8456797 [details] [diff] [review]
Part 3-3: MobileConnectionIPCService for content process, v4


>+class MobileConnectionServiceFactory
>+{
>+public:
>+  static already_AddRefed<nsIMobileConnectionService>
>+  Create() {
>+    nsCOMPtr<nsIMobileConnectionService> mService;
>+
>+    if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+      mService = mobileconnection::MobileConnectionIPCService::GetSingleton();
>+    }
>+
>+    return mService.forget();
>+  }
>+};
A bit ugly to have this file and class just for this case.
Couldn't you just have simple method in LayoutModule.cpp
which calls mobileconnection::MobileConnectionIPCService::GetSingleton(); and
perhaps use NS_GENERIC_FACTORY_CONSTRUCTOR


>+bool
>+MobileConnectionChild::SendRequest(MobileConnectionRequest aRequest,
>+                                   nsIMobileConnectionCallback* aRequestCallback)
>+{
>+  NS_ENSURE_TRUE(mLive, false);
>+
>+  // Deallocated in MobileConnectionChild::DeallocPMobileConnectionRequestChild().
>+  MobileConnectionRequestChild* actor = new MobileConnectionRequestChild(aRequestCallback);
>+  SendPMobileConnectionRequestConstructor(actor, aRequest);


>+MobileConnectionIPCService::GetVoiceConnectionInfo(uint32_t aClientId,
>+                                                   nsIMobileConnectionInfo** aInfo)
>+{
>+  if (aClientId >= mClients.Length()) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  nsCOMPtr<nsIMobileConnectionInfo> info = mClients[aClientId]->GetVoiceInfo();
>+  NS_IF_ADDREF(*aInfo = info);
forget(), not NS_IF_ADDREF


>+MobileConnectionIPCService::GetDataConnectionInfo(uint32_t aClientId,
>+                                                  nsIMobileConnectionInfo** aInfo)
>+{
>+  if (aClientId >= mClients.Length()) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  nsCOMPtr<nsIMobileConnectionInfo> info = mClients[aClientId]->GetDataInfo();
>+  NS_IF_ADDREF(*aInfo = info);
forget(), not NS_IF_ADDREF


>+private:
>+  nsTArray<MobileConnectionChild*> mClients;
I don't understand the lifetime management of mClients.
What guarantees the array doesn't ever contain pointers to deleted objects?
Attachment #8456797 - Flags: review?(bugs) → review-
Comment on attachment 8458497 [details] [diff] [review]
Part 2-3: MobileConnection DOM changes for MobileConnectionService interface, v8

Don't use NS_LITERAL_STRING("") but EmptyString()
Attachment #8458497 - Flags: review?(bugs) → review+
Comment on attachment 8458499 [details] [diff] [review]
Part 3-1: IPDL for MobileConnection, v5

>+MobileCellInfo::MobileCellInfo(int32_t aGsmLocationAreaCode,
>+                               int64_t aGsmCellId,
>+                               int32_t aCdmaBaseStationId,
>+                               int32_t aCdmaBaseStationLatitude,
>+                               int32_t aCdmaBaseStationLongitude,
>+                               int32_t aCdmaSystemId,
>+                               int32_t aCdmaNetworkId)
>+  : mGsmLocationAreaCode(aGsmLocationAreaCode)
>+  , mGsmCellId(aGsmCellId)
>+  , mCdmaBaseStationId(aCdmaBaseStationId)
>+  , mCdmaBaseStationLatitude(aCdmaBaseStationLatitude)
>+  , mCdmaBaseStationLongitude(aCdmaBaseStationLongitude)
>+  , mCdmaSystemId(aCdmaSystemId)
>+  , mCdmaNetworkId(aCdmaNetworkId)
>+{
>+  SetIsDOMBinding();
>+}
What will be the parent object when MobileCellInfo is created this way?
Or is this object not exposed to JS then?


>+MobileConnectionInfo::MobileConnectionInfo(const nsAString& aState,
>+                                           bool aConnected,
>+                                           bool aEmergencyCallsOnly,
>+                                           bool aRoaming,
>+                                           nsIMobileNetworkInfo* aNetworkInfo,
>+                                           const nsAString& aType,
>+                                           const Nullable<int32_t>& aSignalStrength,
>+                                           const Nullable<uint16_t>& aRelSignalStrength,
>+                                           nsIMobileCellInfo* aCellInfo)
>+  : mConnected(aConnected)
>+  , mEmergencyCallsOnly(aEmergencyCallsOnly)
>+  , mRoaming(aRoaming)
>+  , mSignalStrength(aSignalStrength)
>+  , mRelSignalStrength(aRelSignalStrength)
>+{
ditto


>+MobileNetworkInfo::MobileNetworkInfo(const nsAString& aShortName,
>+                                     const nsAString& aLongName,
>+                                     const nsAString& aMcc,
>+                                     const nsAString& aMnc,
>+                                     const nsAString& aState)
>+  : mShortName(aShortName)
>+  , mLongName(aLongName)
>+  , mMcc(aMcc)
>+  , mMnc(aMnc)
>+  , mState(aState)
>+{
And here


>+  /**
>+   * Sync call only be called once per child actor for initialization.
>+   */
>+  sync Init()
>+    returns (nsMobileConnectionInfo aVoice, nsMobileConnectionInfo aData,
>+             nsString aLastKnownNetwork, nsString aLastKnownHomeNetwork,
>+             nsString aIccId, nsString aNetworkSelectionMode,
>+             nsString aRadioState, nsString[] aSupportedNetworkTypes);
Why doesn't the PMobileConnection in PContent return these?
Why you need Init() ?
Attachment #8458499 - Flags: review?(bugs) → review-
Comment on attachment 8458514 [details] [diff] [review]
Part 3-2: IPDL implementation for MobileConnection, v5

>+  nsresult NotifyError(const nsAString& aName)
>+  {
>+    return NotifyError(aName, NS_LITERAL_STRING(""), NS_LITERAL_STRING(""), 0,
EmptyString() here and elsewhere



>+                       0);
>+  }
>+  // non-virtual so it won't affect the vtable
>+  nsresult NotifyError(const nsAString& aName,
>+                       const nsAString& aMessage)
>+  {
>+    return NotifyError(aName, aMessage, NS_LITERAL_STRING(""), 0, 1);
>+  }
>+  // non-virtual so it won't affect the vtable
>+  nsresult NotifyError(const nsAString& aName,
>+                       const nsAString& aMessage,
>+                       const nsAString& aServiceCode)
>+  {
>+    return NotifyError(aName, aMessage, aServiceCode, 0, 2);

All these 0, x numbers as params are really mysterious.
Use some consts or enum

>+  } else {
>+    // Note: The resulting nsIVariant dupes both the array and its elements.
>+    const char16_t **array = reinterpret_cast<const char16_t **>
>+                               (NS_Alloc(arrayLen * sizeof(char16_t *)));


const char16_t*** array

>+MobileConnectionRequestChild::DoReply(const MobileConnectionReplySuccessNetworks& aReply)
>+{
>+  uint32_t count = aReply.results().Length();
>+  // The |aReply.results()| has not been |AddRef|ed, we manage it with an
>+  // nsCOMPtr here.
Why nsCOMPtr is needed. Is it because those nsIMobileNetworkInfo instances have refcnt 0 ?
Couldn't IPC stuff AddRef?

>+  uint16_t type;
>+  nsIID iid;
>+  uint32_t count;
>+  void *data;
void* data;

You should check the return value of nsAutoJSString::init


>+      // IPC::MozCallForwardingOptions[]
>+      nsTArray<IPC::MozCallForwardingOptions> infos;
>+      for (uint32_t i = 0; i < length; i++) {
>+        RootedDictionary<IPC::MozCallForwardingOptions> info(cx);
>+        if (!JS_GetElement(cx, object, i, &value) || !info.Init(cx, value)) {
>+          return NS_ERROR_TYPE_ERR;
>+        }
>+
>+        infos.AppendElement(info);
What keeps 'infos' alive? Only info is rooted.


>+  nsTArray<IPC::MozCallForwardingOptions> results;
>+
>+  if (!JS_IsArrayObject(cx, object) ||
>+      !JS_GetArrayLength(cx, object, &length)) {
>+    return NS_ERROR_TYPE_ERR;
>+  }
>+
>+  for (uint32_t i = 0; i < length; i++) {
>+    JS::Rooted<JS::Value> entry(cx);
>+    RootedDictionary<IPC::MozCallForwardingOptions> info(cx);
Similar here
Attachment #8458514 - Flags: review?(bugs) → review-
Assignee

Comment 118

5 years ago
(In reply to Olli Pettay [:smaug] from comment #114)
> Comment on attachment 8456797 [details] [diff] [review]
> Part 3-3: MobileConnectionIPCService for content process, v4
> 
> >+MobileConnectionIPCService::GetDataConnectionInfo(uint32_t aClientId,
> >+                                                  nsIMobileConnectionInfo** aInfo)
> >+{
> >+  if (aClientId >= mClients.Length()) {
> >+    return NS_ERROR_FAILURE;
> >+  }
> >+
> >+  nsCOMPtr<nsIMobileConnectionInfo> info = mClients[aClientId]->GetDataInfo();
> >+  NS_IF_ADDREF(*aInfo = info);
> forget(), not NS_IF_ADDREF
> 
> 
> >+private:
> >+  nsTArray<MobileConnectionChild*> mClients;
> I don't understand the lifetime management of mClients.
> What guarantees the array doesn't ever contain pointers to deleted objects?

Ah, thanks for pointing this out.
MobileConnectionIPCService may not aware the MobileConnectionChild was deleted in ContentChild::DeallocPMobileConnectionChild. And the array will contain pointers to deleted objects after that.

We could have two different way to solve this:
1. Make MobileConnectionChild reference-counted and let MobileConnectionIPCService hold a reference.
2. Notify MobileConnectionIPCService the child is deleted in MobileConnectionChild::ActorDestroy().

Thank you
Comment on attachment 8458582 [details] [diff] [review]
Part 4-5.c: MobileConnectionGonkService for gonk backend, v3

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

Sorry for being late :(

Overall structure looks good, thank you!  Please remember to invite build peer for the review!

::: dom/mobileconnection/src/gonk/MobileConnectionGonkService.js
@@ +192,5 @@
> +    return supportedNetworkTypes;
> +  },
> +
> +  /**
> +   * Helper for guarding us again invalid reason values for call forwarding.

nit:s/again/against/

@@ +196,5 @@
> +   * Helper for guarding us again invalid reason values for call forwarding.
> +   */
> +  _isValidCallForwardingReason: function(aReason) {
> +    switch (aReason) {
> +      case RIL.CALL_FORWARD_REASON_UNCONDITIONAL:

As the requests are coming from nsIMobileConnectionService, let's use Ci.nsIMobileConnectionService.* though the values should be the same as RIL.* now.

@@ +209,5 @@
> +    }
> +  },
> +
> +  /**
> +   * Helper for guarding us again invalid action values for call forwarding.

ditto.

@@ +213,5 @@
> +   * Helper for guarding us again invalid action values for call forwarding.
> +   */
> +  _isValidCallForwardingAction: function(aAction) {
> +    switch (aAction) {
> +      case RIL.CALL_FORWARD_ACTION_DISABLE:

ditto.

@@ +228,5 @@
> +   * Helper for guarding us against invalid program values for call barring.
> +   */
> +  _isValidCallBarringProgram: function(aProgram) {
> +    switch (aProgram) {
> +      case RIL.CALL_BARRING_PROGRAM_ALL_OUTGOING:

ditto.

@@ +262,5 @@
> +   * Helper for guarding us against invalid mode for clir.
> +   */
> +  _isValidClirMode: function(aMode) {
> +    switch (aMode) {
> +      case RIL.CLIR_DEFAULT:

ditto.

@@ +321,5 @@
> +    let isUpdated = false;
> +    for (let key in aSrcInfo) {
> +      if ((key !== "cell") && (aDestInfo[key] !== aSrcInfo[key])) {
> +        isUpdated = true;
> +        aDestInfo[key] = aSrcInfo[key];

This is kinda duplicate of _updateInfo.

Can we rewrite as:

let isUpdated = falsefor (let key in aScrInfo) {
    if (key === "cell") {
        if (aSrcInfo.cell && !aDestInfo.cell) {
            aDestInfo.cell = new MobileCellInfo();
        }
    }
    isUpdated = this._updateInfo(aDestInfo[key], aSrcInfo[key]);
}

@@ +468,5 @@
> +  },
> +
> +  updateSignalInfo: function(aNewInfo, aBatch = false) {
> +    // If the voice is not registered, need not to update signal information.
> +    if (this.voiceInfo.state === RIL.GECKO_MOBILE_CONNECTION_STATE_REGISTERED) {

I know it's simply copied from the current code, but sometime we check voiceInfo.network (e.g. line 460) and sometimes check voiceInfo.state. Wondering if we do want to have separate conditions? Shouldn't they be align?

@@ +474,5 @@
> +        this.deliverListenerEvent("notifyVoiceChanged");
> +      }
> +    }
> +
> +    // If the data is not registered, need not to update signal information.

nit: s/need not/no need/

@@ +829,5 @@
> +    // Checking valid PIN for supplementary services. See TS.22.004 clause 5.2.
> +    if (aOptions.pin == null || !aOptions.pin.match(/^\d{4}$/) ||
> +        aOptions.newPin == null || !aOptions.newPin.match(/^\d{4}$/)) {
> +      this._dispatchNotifyError(aCallback, "InvalidPassword");
> +      return false;

Why return false? I thought the return value of the API is void.

::: dom/mobileconnection/src/moz.build
@@ +44,5 @@
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
> +    EXTRA_COMPONENTS += [
> +        'gonk/MobileConnectionGonkService.js',
> +        'gonk/MobileConnectionGonkService.manifest',
> +    ]

Don't we need to modify dom/mobileconnection/interfaces/moz.build, too?
Attachment #8458582 - Flags: review?(htsai)
Comment on attachment 8458585 [details] [diff] [review]
Part 4-5.d: gRadioEnabledController changes, v3

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

Thank you!
Attachment #8458585 - Flags: review?(htsai) → review+
Assignee

Comment 121

5 years ago
Hi Olli,

Thanks for the review, please see my below comments.

(In reply to Olli Pettay [:smaug] from comment #116)
> Comment on attachment 8458499 [details] [diff] [review]
> Part 3-1: IPDL for MobileConnection, v5
> 
> >+MobileCellInfo::MobileCellInfo(int32_t aGsmLocationAreaCode,
> >+                               int64_t aGsmCellId,
> >+                               int32_t aCdmaBaseStationId,
> >+                               int32_t aCdmaBaseStationLatitude,
> >+                               int32_t aCdmaBaseStationLongitude,
> >+                               int32_t aCdmaSystemId,
> >+                               int32_t aCdmaNetworkId)
> >+  : mGsmLocationAreaCode(aGsmLocationAreaCode)
> >+  , mGsmCellId(aGsmCellId)
> >+  , mCdmaBaseStationId(aCdmaBaseStationId)
> >+  , mCdmaBaseStationLatitude(aCdmaBaseStationLatitude)
> >+  , mCdmaBaseStationLongitude(aCdmaBaseStationLongitude)
> >+  , mCdmaSystemId(aCdmaSystemId)
> >+  , mCdmaNetworkId(aCdmaNetworkId)
> >+{
> >+  SetIsDOMBinding();
> >+}
> What will be the parent object when MobileCellInfo is created this way?
> Or is this object not exposed to JS then?

No parent object, mWindow is null in this way.
And it won't be exposed to JS.
So in such case, should we call SetIsDOMBinding()?

> 
> >+  /**
> >+   * Sync call only be called once per child actor for initialization.
> >+   */
> >+  sync Init()
> >+    returns (nsMobileConnectionInfo aVoice, nsMobileConnectionInfo aData,
> >+             nsString aLastKnownNetwork, nsString aLastKnownHomeNetwork,
> >+             nsString aIccId, nsString aNetworkSelectionMode,
> >+             nsString aRadioState, nsString[] aSupportedNetworkTypes);
> Why doesn't the PMobileConnection in PContent return these?
> Why you need Init() ?

It is because we maintain these data in MobileConnectionChild (please see part3-2), having a sync call here makes the implementation easier.
Flags: needinfo?(bugs)
Assignee

Comment 122

5 years ago
(In reply to Olli Pettay [:smaug] from comment #117)
> Comment on attachment 8458514 [details] [diff] [review]
> Part 3-2: IPDL implementation for MobileConnection, v5
> 
> >+      // IPC::MozCallForwardingOptions[]
> >+      nsTArray<IPC::MozCallForwardingOptions> infos;
> >+      for (uint32_t i = 0; i < length; i++) {
> >+        RootedDictionary<IPC::MozCallForwardingOptions> info(cx);
> >+        if (!JS_GetElement(cx, object, i, &value) || !info.Init(cx, value)) {
> >+          return NS_ERROR_TYPE_ERR;
> >+        }
> >+
> >+        infos.AppendElement(info);
> What keeps 'infos' alive? Only info is rooted.

Ah, MozCallForwardingOptions contains no JS stuff (only string and short) [1], so we don't need to use RootedDictionary here actually. Am I right?

Thank you.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozMobileConnection.webidl?from=MozMobileConnection.webidl&case=true#574
In that case Rooting isn't indeed needed, right.
Flags: needinfo?(bugs)
Attachment #8458536 - Flags: review?(anygregor) → review+
Assignee

Comment 124

5 years ago
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #119)
> Comment on attachment 8458582 [details] [diff] [review]
> Part 4-5.c: MobileConnectionGonkService for gonk backend, v3
> 
> Review of attachment 8458582 [details] [diff] [review]:
> ::: dom/mobileconnection/src/gonk/MobileConnectionGonkService.js 
> @@ +196,5 @@
> > +   * Helper for guarding us again invalid reason values for call forwarding.
> > +   */
> > +  _isValidCallForwardingReason: function(aReason) {
> > +    switch (aReason) {
> > +      case RIL.CALL_FORWARD_REASON_UNCONDITIONAL:
> 
> As the requests are coming from nsIMobileConnectionService, let's use
> Ci.nsIMobileConnectionService.* though the values should be the same as
> RIL.* now.

Ah, thanks for catching this.
RIL.* here was from very old code. We don't even use RIL.* in current code (RILContentHelper).  :(
I guess I should check latest code again to make sure everything is up to date.

> 
> @@ +321,5 @@
> > +    let isUpdated = false;
> > +    for (let key in aSrcInfo) {
> > +      if ((key !== "cell") && (aDestInfo[key] !== aSrcInfo[key])) {
> > +        isUpdated = true;
> > +        aDestInfo[key] = aSrcInfo[key];
> 
> This is kinda duplicate of _updateInfo.
> 
> Can we rewrite as:
> 
> let isUpdated = falsefor (let key in aScrInfo) {
>     if (key === "cell") {
>         if (aSrcInfo.cell && !aDestInfo.cell) {
>             aDestInfo.cell = new MobileCellInfo();
>         }
>     }
>     isUpdated = this._updateInfo(aDestInfo[key], aSrcInfo[key]);
> }

The |_updateInfo| now only supports taking objects as argument. aDestInfo[key] or aSrcInfo[key] could be just a primitive type. So |this._updateInfo(aDestInfo[key], aSrcInfo[key])| may not work as expected.
But I think I still can try to merge _updateInfo and _updateConnectionInfo together by a more generic way.
Thank you.

> 
> @@ +468,5 @@
> > +  },
> > +
> > +  updateSignalInfo: function(aNewInfo, aBatch = false) {
> > +    // If the voice is not registered, need not to update signal information.
> > +    if (this.voiceInfo.state === RIL.GECKO_MOBILE_CONNECTION_STATE_REGISTERED) {
> 
> I know it's simply copied from the current code, but sometime we check
> voiceInfo.network (e.g. line 460) and sometimes check voiceInfo.state.
> Wondering if we do want to have separate conditions? Shouldn't they be align?

Yes, it is better to have a consistent way for checking (although they point to the same thing).
Will do this.

>
> 
> @@ +829,5 @@
> > +    // Checking valid PIN for supplementary services. See TS.22.004 clause 5.2.
> > +    if (aOptions.pin == null || !aOptions.pin.match(/^\d{4}$/) ||
> > +        aOptions.newPin == null || !aOptions.newPin.match(/^\d{4}$/)) {
> > +      this._dispatchNotifyError(aCallback, "InvalidPassword");
> > +      return false;
> 
> Why return false? I thought the return value of the API is void.

Ah, yes, should not return false here.
Thanks for catching this.

> 
> ::: dom/mobileconnection/src/moz.build
> @@ +44,5 @@
> > +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
> > +    EXTRA_COMPONENTS += [
> > +        'gonk/MobileConnectionGonkService.js',
> > +        'gonk/MobileConnectionGonkService.manifest',
> > +    ]
> 
> Don't we need to modify dom/mobileconnection/interfaces/moz.build, too?

No, we don't need. We already modify dom/mobileconnection/interfaces/moz.build in part 4-1 (Adding nsIMobileConnectionGonkService.idl).
Assignee

Updated

5 years ago
Target Milestone: --- → 2.1 S2 (15aug)
Assignee

Comment 125

5 years ago
(In reply to Olli Pettay [:smaug] from comment #117)
> Comment on attachment 8458514 [details] [diff] [review]
> Part 3-2: IPDL implementation for MobileConnection, v5
> 
> >+                       0);
> >+  }
> >+  // non-virtual so it won't affect the vtable
> >+  nsresult NotifyError(const nsAString& aName,
> >+                       const nsAString& aMessage)
> >+  {
> >+    return NotifyError(aName, aMessage, NS_LITERAL_STRING(""), 0, 1);
> >+  }
> >+  // non-virtual so it won't affect the vtable
> >+  nsresult NotifyError(const nsAString& aName,
> >+                       const nsAString& aMessage,
> >+                       const nsAString& aServiceCode)
> >+  {
> >+    return NotifyError(aName, aMessage, aServiceCode, 0, 2);
> 
> All these 0, x numbers as params are really mysterious.
> Use some consts or enum

The 0 here is dummy parameter for the case that has no additionalInformation passing.
I will add a consts for this.

The 2 here is the number of optional arguments (for the interface attribute, optional_argc). Having a consts or enum for this seems unnecessary. But I can add some comments here to make it easier to understand.

Thank you.
Assignee

Comment 127

5 years ago
Rebase
Attachment #8456047 - Attachment is obsolete: true
Attachment #8464497 - Flags: review+
Assignee

Comment 128

5 years ago
Address review comment #115:
- Use EmptyString() instead.
Attachment #8458497 - Attachment is obsolete: true
Attachment #8464499 - Flags: review+
Assignee

Comment 129

5 years ago
1. Rebase
2. Address review comment #116:
   - Don't need |SetIsDOMBinding()| given that the object won't expose to JS.
   - Please see comment #121 for the reason that PMobileConnection in PContent  
     doesn't return the initial data, but have a sync call, Init().
3. Do AddRef in IPC serializer. (address the review comment #117)

Hi Olli, may I have your review again? Thank you.
Attachment #8458499 - Attachment is obsolete: true
Attachment #8464549 - Flags: review?(bugs)
Assignee

Comment 130

5 years ago
Address review comment #117:
- Use EmptyString() instead.
- Add const and comments for mysterious number. (Please see comment #125)
- s/char16_t */const char16_t*** array/
- Use dont_AddRef given that those instances are AddRefed in IPCSerializer. 
  (Please see part 3-1)
- s/void *data/void* data/
- check the return value of nsAutoJSString::init
- Don't need RootedDictionary given that no JS stuff. (Please see comment #122)

Hi Olli, may I have your review again? Thank you.
Attachment #8458514 - Attachment is obsolete: true
Attachment #8464626 - Flags: review?(bugs)
Assignee

Comment 131

5 years ago
Address review comment #114:
- Move mobileconnection::MobileConnectionIPCService::GetSingleton() things to
  LayoutModule.cpp, then we don't need MobileConnectionServiceFactory.
- Use forget(), not NS_IF_ADDREF.
- Correct the lifetime management of mClients. (MobileConnectionChild is
  refcounted now, please see part 3-2)

Hi Olli, may I have your review again? Thank you.
Attachment #8456797 - Attachment is obsolete: true
Attachment #8464631 - Flags: review?(bugs)
Assignee

Updated

5 years ago
Attachment #8464632 - Attachment description: bug_843452_part4-1_idl_gonk_backend_v4_r=hsinyi.patch → Part 4-1: Interface changes for gonk backend, v4, r=hsinyi
Attachment #8464632 - Flags: review+
Assignee

Updated

5 years ago
Attachment #8458520 - Attachment is obsolete: true
Assignee

Comment 133

5 years ago
Address review comment #103:
- Sort #incldue in alphabetical order
- Address nit
Attachment #8449179 - Attachment is obsolete: true
Attachment #8464634 - Flags: review+
Comment on attachment 8464549 [details] [diff] [review]
Part 3-1: IPDL for MobileConnection, v6

Could you add to 
MobileCellInfo::WrapObject and to MobileConnectionInfo::WrapObject
MOZ_ASSERT(IsDOMBinding);
Attachment #8464549 - Flags: review?(bugs) → review+
Er, MOZ_ASSERT(IsDOMBinding());
Though, I can't recall now whether we even call that method if IsDOMBinding() returns false.
Comment on attachment 8464549 [details] [diff] [review]
Part 3-1: IPDL for MobileConnection, v6

># HG changeset patch
># User Edgar Chen <echen@mozilla.com>
># Date 1404209348 -28800
>#      Tue Jul 01 18:09:08 2014 +0800
># Node ID 92fb882c371ef0466a73286d46f23b0d1a8cea0d
># Parent  ac6c7613cfcdf916c135c7c7d82dd7fa28f1cdb0
>Bug 843452 - Part 3-1: IPDL for MobileConnection. r=smaug
>
>diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl
>--- a/dom/ipc/PContent.ipdl
>+++ b/dom/ipc/PContent.ipdl
>@@ -17,16 +17,17 @@ include protocol PExternalHelperApp;
> include protocol PDeviceStorageRequest;
> include protocol PFileDescriptorSet;
> include protocol PFMRadio;
> include protocol PFileSystemRequest;
> include protocol PHal;
> include protocol PImageBridge;
> include protocol PIndexedDB;
> include protocol PMemoryReportRequest;
>+include protocol PMobileConnection;
> include protocol PNecko;
> include protocol PScreenManager;
> include protocol PSharedBufferManager;
> include protocol PSms;
> include protocol PSpeechSynthesis;
> include protocol PStorage;
> include protocol PTelephony;
> include protocol PTestShell;
>@@ -305,16 +306,17 @@ intr protocol PContent
>     manages PDeviceStorageRequest;
>     manages PFileSystemRequest;
>     manages PExternalHelperApp;
>     manages PFileDescriptorSet;
>     manages PFMRadio;
>     manages PHal;
>     manages PIndexedDB;
>     manages PMemoryReportRequest;
>+    manages PMobileConnection;
>     manages PNecko;
>     manages PScreenManager;
>     manages PSms;
>     manages PSpeechSynthesis;
>     manages PStorage;
>     manages PTelephony;
>     manages PTestShell;
>     manages PJavaScript;
>@@ -487,16 +489,18 @@ parent:
> 
>     sync IsSecureURI(uint32_t type, URIParams uri, uint32_t flags)
>         returns (bool isSecureURI);
> 
>     PHal();
> 
>     PIndexedDB();
> 
>+    PMobileConnection(uint32_t clientId);
>+
>     PNecko();
> 
>     sync PScreenManager()
>         returns (uint32_t numberOfScreens,
>                  float systemDefaultScale,
>                  bool success);
> 
>     PSms();
>diff --git a/dom/mobileconnection/src/MobileCellInfo.cpp b/dom/mobileconnection/src/MobileCellInfo.cpp
>--- a/dom/mobileconnection/src/MobileCellInfo.cpp
>+++ b/dom/mobileconnection/src/MobileCellInfo.cpp
>@@ -28,16 +28,33 @@ MobileCellInfo::MobileCellInfo(nsPIDOMWi
>   , mCdmaBaseStationLatitude(-1)
>   , mCdmaBaseStationLongitude(-1)
>   , mCdmaSystemId(-1)
>   , mCdmaNetworkId(-1)
> {
>   SetIsDOMBinding();
> }
> 
>+MobileCellInfo::MobileCellInfo(int32_t aGsmLocationAreaCode,
>+                               int64_t aGsmCellId,
>+                               int32_t aCdmaBaseStationId,
>+                               int32_t aCdmaBaseStationLatitude,
>+                               int32_t aCdmaBaseStationLongitude,
>+                               int32_t aCdmaSystemId,
>+                               int32_t aCdmaNetworkId)
>+  : mGsmLocationAreaCode(aGsmLocationAreaCode)
>+  , mGsmCellId(aGsmCellId)
>+  , mCdmaBaseStationId(aCdmaBaseStationId)
>+  , mCdmaBaseStationLatitude(aCdmaBaseStationLatitude)
>+  , mCdmaBaseStationLongitude(aCdmaBaseStationLongitude)
>+  , mCdmaSystemId(aCdmaSystemId)
>+  , mCdmaNetworkId(aCdmaNetworkId)
>+{
>+}
>+
> void
> MobileCellInfo::Update(nsIMobileCellInfo* aInfo)
> {
>   if (!aInfo) {
>     return;
>   }
> 
>   aInfo->GetGsmLocationAreaCode(&mGsmLocationAreaCode);
>diff --git a/dom/mobileconnection/src/MobileCellInfo.h b/dom/mobileconnection/src/MobileCellInfo.h
>--- a/dom/mobileconnection/src/MobileCellInfo.h
>+++ b/dom/mobileconnection/src/MobileCellInfo.h
>@@ -19,16 +19,21 @@ class MobileCellInfo MOZ_FINAL : public 
> {
> public:
>   NS_DECL_NSIMOBILECELLINFO
>   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(MobileCellInfo)
> 
>   MobileCellInfo(nsPIDOMWindow* aWindow);
> 
>+  MobileCellInfo(int32_t aGsmLocationAreaCode, int64_t aGsmCellId,
>+                 int32_t aCdmaBaseStationId, int32_t aCdmaBaseStationLatitude,
>+                 int32_t aCdmaBaseStationLongitude, int32_t aCdmaSystemId,
>+                 int32_t aCdmaNetworkId);
>+
>   void
>   Update(nsIMobileCellInfo* aInfo);
> 
>   nsPIDOMWindow*
>   GetParentObject() const
>   {
>     return mWindow;
>   }
>diff --git a/dom/mobileconnection/src/MobileConnectionInfo.cpp b/dom/mobileconnection/src/MobileConnectionInfo.cpp
>--- a/dom/mobileconnection/src/MobileConnectionInfo.cpp
>+++ b/dom/mobileconnection/src/MobileConnectionInfo.cpp
>@@ -52,16 +52,48 @@ MobileConnectionInfo::MobileConnectionIn
>   : mConnected(false)
>   , mEmergencyCallsOnly(false)
>   , mRoaming(false)
>   , mWindow(aWindow)
> {
>   SetIsDOMBinding();
> }
> 
>+MobileConnectionInfo::MobileConnectionInfo(const nsAString& aState,
>+                                           bool aConnected,
>+                                           bool aEmergencyCallsOnly,
>+                                           bool aRoaming,
>+                                           nsIMobileNetworkInfo* aNetworkInfo,
>+                                           const nsAString& aType,
>+                                           const Nullable<int32_t>& aSignalStrength,
>+                                           const Nullable<uint16_t>& aRelSignalStrength,
>+                                           nsIMobileCellInfo* aCellInfo)
>+  : mConnected(aConnected)
>+  , mEmergencyCallsOnly(aEmergencyCallsOnly)
>+  , mRoaming(aRoaming)
>+  , mSignalStrength(aSignalStrength)
>+  , mRelSignalStrength(aRelSignalStrength)
>+{
>+  // Update mState and mType
>+  CONVERT_STRING_TO_NULLABLE_ENUM(aState, MobileConnectionState, mState);
>+  CONVERT_STRING_TO_NULLABLE_ENUM(aType, MobileConnectionType, mType);
>+
>+  // Update mNetworkInfo
>+  if (aNetworkInfo) {
>+    mNetworkInfo = new MobileNetworkInfo(mWindow);
>+    mNetworkInfo->Update(aNetworkInfo);
>+  }
>+
>+  // Update mCellInfo
>+  if (aCellInfo) {
>+    mCellInfo = new MobileCellInfo(mWindow);
>+    mCellInfo->Update(aCellInfo);
>+  }
>+}
>+
> void
> MobileConnectionInfo::Update(nsIMobileConnectionInfo* aInfo)
> {
>   if (!aInfo) {
>     return;
>   }
> 
>   aInfo->GetConnected(&mConnected);
>diff --git a/dom/mobileconnection/src/MobileConnectionInfo.h b/dom/mobileconnection/src/MobileConnectionInfo.h
>--- a/dom/mobileconnection/src/MobileConnectionInfo.h
>+++ b/dom/mobileconnection/src/MobileConnectionInfo.h
>@@ -22,16 +22,24 @@ class MobileConnectionInfo MOZ_FINAL : p
> {
> public:
>   NS_DECL_NSIMOBILECONNECTIONINFO
>   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(MobileConnectionInfo)
> 
>   MobileConnectionInfo(nsPIDOMWindow* aWindow);
> 
>+  MobileConnectionInfo(const nsAString& aState, bool aConnected,
>+                       bool aEmergencyCallsOnly, bool aRoaming,
>+                       nsIMobileNetworkInfo* aNetworkInfo,
>+                       const nsAString& aType,
>+                       const Nullable<int32_t>& aSignalStrength,
>+                       const Nullable<uint16_t>& aRelSignalStrength,
>+                       nsIMobileCellInfo* aCellInfo);
>+
>   void
>   Update(nsIMobileConnectionInfo* aInfo);
> 
>   nsPIDOMWindow*
>   GetParentObject() const
>   {
>     return mWindow;
>   }
>diff --git a/dom/mobileconnection/src/MobileNetworkInfo.cpp b/dom/mobileconnection/src/MobileNetworkInfo.cpp
>--- a/dom/mobileconnection/src/MobileNetworkInfo.cpp
>+++ b/dom/mobileconnection/src/MobileNetworkInfo.cpp
>@@ -20,16 +20,29 @@ NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(
> NS_INTERFACE_MAP_END
> 
> MobileNetworkInfo::MobileNetworkInfo(nsPIDOMWindow* aWindow)
>   : mWindow(aWindow)
> {
>   SetIsDOMBinding();
> }
> 
>+MobileNetworkInfo::MobileNetworkInfo(const nsAString& aShortName,
>+                                     const nsAString& aLongName,
>+                                     const nsAString& aMcc,
>+                                     const nsAString& aMnc,
>+                                     const nsAString& aState)
>+  : mShortName(aShortName)
>+  , mLongName(aLongName)
>+  , mMcc(aMcc)
>+  , mMnc(aMnc)
>+  , mState(aState)
>+{
>+}
>+
> void
> MobileNetworkInfo::Update(nsIMobileNetworkInfo* aInfo)
> {
>   if (!aInfo) {
>     return;
>   }
> 
>   aInfo->GetShortName(mShortName);
>diff --git a/dom/mobileconnection/src/MobileNetworkInfo.h b/dom/mobileconnection/src/MobileNetworkInfo.h
>--- a/dom/mobileconnection/src/MobileNetworkInfo.h
>+++ b/dom/mobileconnection/src/MobileNetworkInfo.h
>@@ -22,16 +22,20 @@ class MobileNetworkInfo MOZ_FINAL : publ
> {
> public:
>   NS_DECL_NSIMOBILENETWORKINFO
>   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(MobileNetworkInfo)
> 
>   MobileNetworkInfo(nsPIDOMWindow* aWindow);
> 
>+  MobileNetworkInfo(const nsAString& aShortName, const nsAString& aLongName,
>+                    const nsAString& aMcc, const nsAString& aMnc,
>+                    const nsAString& aState);
>+
>   void
>   Update(nsIMobileNetworkInfo* aInfo);
> 
>   nsPIDOMWindow*
>   GetParentObject() const
>   {
>     return mWindow;
>   }
>diff --git a/dom/mobileconnection/src/ipc/MobileConnectionIPCSerializer.h b/dom/mobileconnection/src/ipc/MobileConnectionIPCSerializer.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/mobileconnection/src/ipc/MobileConnectionIPCSerializer.h
>@@ -0,0 +1,737 @@
>+/* 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 dom_mobileconnection_src_ipc_MobileConnectionIPCSerialiser_h
>+#define dom_mobileconnection_src_ipc_MobileConnectionIPCSerialiser_h
>+
>+#include "ipc/IPCMessageUtils.h"
>+#include "mozilla/dom/MobileCellInfo.h"
>+#include "mozilla/dom/MobileConnectionInfo.h"
>+#include "mozilla/dom/MobileNetworkInfo.h"
>+#include "MozMobileConnectionBinding.h"
>+#include "nsCxPusher.h"
>+
>+typedef nsIMobileCellInfo* nsMobileCellInfo;
>+typedef nsIMobileConnectionInfo* nsMobileConnectionInfo;
>+typedef nsIMobileNetworkInfo* nsMobileNetworkInfo;
>+
>+using namespace mozilla;
>+using namespace mozilla::dom;
>+
>+namespace IPC {
>+
>+struct MozCallForwardingOptions : public mozilla::dom::MozCallForwardingOptions
>+{
>+  bool operator==(const MozCallForwardingOptions& aOther) const
>+  {
>+    return // Compare mActive
>+           ((!mActive.WasPassed() && !aOther.mActive.WasPassed()) ||
>+            (mActive.WasPassed() && aOther.mActive.WasPassed() &&
>+             mActive.Value() == aOther.mActive.Value())) &&
>+           // Compare mAction
>+           ((!mAction.WasPassed() && !aOther.mAction.WasPassed()) ||
>+            (mAction.WasPassed() && aOther.mAction.WasPassed() &&
>+             mAction.Value() == aOther.mAction.Value())) &&
>+           // Compare mReason
>+           ((!mReason.WasPassed() && !aOther.mReason.WasPassed()) ||
>+            (mReason.WasPassed() && aOther.mReason.WasPassed() &&
>+             mReason.Value() == aOther.mReason.Value())) &&
>+           // Compare mNumber
>+           ((!mNumber.WasPassed() && !aOther.mNumber.WasPassed()) ||
>+            (mNumber.WasPassed() && aOther.mNumber.WasPassed() &&
>+             mNumber.Value() == aOther.mNumber.Value())) &&
>+           // Compare mTimeSeconds
>+           ((!mTimeSeconds.WasPassed() && !aOther.mTimeSeconds.WasPassed()) ||
>+            (mTimeSeconds.WasPassed() && aOther.mTimeSeconds.WasPassed() &&
>+             mTimeSeconds.Value() == aOther.mTimeSeconds.Value())) &&
>+           // Compare mServiceClass
>+           ((!mServiceClass.WasPassed() && !aOther.mServiceClass.WasPassed()) ||
>+            (mServiceClass.WasPassed() && aOther.mServiceClass.WasPassed() &&
>+             mServiceClass.Value() == aOther.mServiceClass.Value()));
>+  };
>+};
>+
>+struct MozCallBarringOptions : mozilla::dom::MozCallBarringOptions
>+{
>+  bool operator==(const MozCallBarringOptions& aOther) const
>+  {
>+    return // Compare mEnabled
>+           ((!mEnabled.WasPassed() && !aOther.mEnabled.WasPassed()) ||
>+            (mEnabled.WasPassed() && aOther.mEnabled.WasPassed() &&
>+             mEnabled.Value() == aOther.mEnabled.Value())) &&
>+           // Compare mPassword
>+           ((!mPassword.WasPassed() && !aOther.mPassword.WasPassed()) ||
>+            (mPassword.WasPassed() && aOther.mPassword.WasPassed() &&
>+             mPassword.Value() == aOther.mPassword.Value())) &&
>+           // Compare mProgram
>+           ((!mProgram.WasPassed() && !aOther.mProgram.WasPassed()) ||
>+            (mProgram.WasPassed() && aOther.mProgram.WasPassed() &&
>+             mProgram.Value() == aOther.mProgram.Value())) &&
>+           // Compare mServiceClass
>+           ((!mServiceClass.WasPassed() && !aOther.mServiceClass.WasPassed()) ||
>+            (mServiceClass.WasPassed() && aOther.mServiceClass.WasPassed() &&
>+             mServiceClass.Value() == aOther.mServiceClass.Value()));
>+  };
>+};
>+
>+/**
>+ * nsIMobileNetworkInfo Serialize/De-serialize.
>+ */
>+template <>
>+struct ParamTraits<nsIMobileNetworkInfo*>
>+{
>+  typedef nsIMobileNetworkInfo* paramType;
>+
>+  // Function to serialize a MobileNetworkInfo.
>+  static void Write(Message *aMsg, const paramType& aParam)
>+  {
>+    bool isNull = !aParam;
>+    WriteParam(aMsg, isNull);
>+    // If it is a null object, then we are done.
>+    if (isNull) {
>+      return;
>+    }
>+
>+    nsString pString;
>+    aParam->GetShortName(pString);
>+    WriteParam(aMsg, pString);
>+
>+    aParam->GetLongName(pString);
>+    WriteParam(aMsg, pString);
>+
>+    aParam->GetMcc(pString);
>+    WriteParam(aMsg, pString);
>+
>+    aParam->GetMnc(pString);
>+    WriteParam(aMsg, pString);
>+
>+    aParam->GetState(pString);
>+    WriteParam(aMsg, pString);
>+  }
>+
>+  // Function to de-serialize a MobileNetworkInfo.
>+  static bool Read(const Message *aMsg, void **aIter, paramType* aResult)
>+  {
>+    // Check if is the null pointer we have transfered.
>+    bool isNull;
>+    if (!ReadParam(aMsg, aIter, &isNull)) {
>+      return false;
>+    }
>+
>+    if (isNull) {
>+      *aResult = nullptr;
>+      return true;
>+    }
>+
>+    nsString shortName;
>+    nsString longName;
>+    nsString mcc;
>+    nsString mnc;
>+    nsString state;
>+
>+    // It's not important to us where it fails, but rather if it fails
>+    if (!(ReadParam(aMsg, aIter, &shortName) &&
>+          ReadParam(aMsg, aIter, &longName) &&
>+          ReadParam(aMsg, aIter, &mcc) &&
>+          ReadParam(aMsg, aIter, &mnc) &&
>+          ReadParam(aMsg, aIter, &state))) {
>+      return false;
>+    }
>+
>+    *aResult = new MobileNetworkInfo(shortName,
>+                                     longName,
>+                                     mcc,
>+                                     mnc,
>+                                     state);
>+    // We release this ref after receiver finishes processing.
>+    NS_IF_ADDREF(*aResult);
>+
>+    return true;
>+  }
>+};
>+
>+/**
>+ * nsIMobileCellInfo Serialize/De-serialize.
>+ */
>+template <>
>+struct ParamTraits<nsIMobileCellInfo*>
>+{
>+  typedef nsIMobileCellInfo* paramType;
>+
>+  // Function to serialize a MobileCellInfo.
>+  static void Write(Message *aMsg, const paramType& aParam)
>+  {
>+    bool isNull = !aParam;
>+    WriteParam(aMsg, isNull);
>+    // If it is a null object, then we are done.
>+    if (isNull) {
>+      return;
>+    }
>+
>+    int32_t pLong;
>+    int64_t pLongLong;
>+
>+    aParam->GetGsmLocationAreaCode(&pLong);
>+    WriteParam(aMsg, pLong);
>+
>+    aParam->GetGsmCellId(&pLongLong);
>+    WriteParam(aMsg, pLongLong);
>+
>+    aParam->GetCdmaBaseStationId(&pLong);
>+    WriteParam(aMsg, pLong);
>+
>+    aParam->GetCdmaBaseStationLatitude(&pLong);
>+    WriteParam(aMsg, pLong);
>+
>+    aParam->GetCdmaBaseStationLongitude(&pLong);
>+    WriteParam(aMsg, pLong);
>+
>+    aParam->GetCdmaSystemId(&pLong);
>+    WriteParam(aMsg, pLong);
>+
>+    aParam->GetCdmaNetworkId(&pLong);
>+    WriteParam(aMsg, pLong);
>+  }
>+
>+  // Function to de-serialize a MobileCellInfo.
>+  static bool Read(const Message *aMsg, void **aIter, paramType* aResult)
>+  {
>+    // Check if is the null pointer we have transfered.
>+    bool isNull;
>+    if (!ReadParam(aMsg, aIter, &isNull)) {
>+      return false;
>+    }
>+
>+    if (isNull) {
>+      *aResult = nullptr;
>+      return true;
>+    }
>+
>+    int32_t gsmLac;
>+    int64_t gsmCellId;
>+    int32_t cdmaBsId;
>+    int32_t cdmaBsLat;
>+    int32_t cdmaBsLong;
>+    int32_t cdmaSystemId;
>+    int32_t cdmaNetworkId;
>+
>+    // It's not important to us where it fails, but rather if it fails
>+    if (!(ReadParam(aMsg, aIter, &gsmLac) &&
>+          ReadParam(aMsg, aIter, &gsmCellId) &&
>+          ReadParam(aMsg, aIter, &cdmaBsId) &&
>+          ReadParam(aMsg, aIter, &cdmaBsLat) &&
>+          ReadParam(aMsg, aIter, &cdmaBsLong) &&
>+          ReadParam(aMsg, aIter, &cdmaSystemId) &&
>+          ReadParam(aMsg, aIter, &cdmaNetworkId))) {
>+      return false;
>+    }
>+
>+    *aResult = new MobileCellInfo(gsmLac, gsmCellId, cdmaBsId, cdmaBsLat,
>+                                  cdmaBsLong, cdmaSystemId, cdmaNetworkId);
>+    // We release this ref after receiver finishes processing.
>+    NS_IF_ADDREF(*aResult);
>+
>+    return true;
>+  }
>+};
>+
>+/**
>+ * nsIMobileConnectionInfo Serialize/De-serialize.
>+ */
>+template <>
>+struct ParamTraits<nsIMobileConnectionInfo*>
>+{
>+  typedef nsIMobileConnectionInfo* paramType;
>+
>+  // Function to serialize a MobileConnectionInfo.
>+  static void Write(Message *aMsg, const paramType& aParam)
>+  {
>+    bool isNull = !aParam;
>+    WriteParam(aMsg, isNull);
>+    // If it is a null object, then we are done.
>+    if (isNull) {
>+      return;
>+    }
>+
>+    AutoJSContext cx;
>+    nsString pString;
>+    bool pBool;
>+    nsCOMPtr<nsIMobileNetworkInfo> pNetworkInfo;
>+    nsCOMPtr<nsIMobileCellInfo> pCellInfo;
>+    JS::Rooted<JS::Value> pJsval(cx);
>+    int32_t pInt32;
>+
>+    aParam->GetState(pString);
>+    WriteParam(aMsg, pString);
>+
>+    aParam->GetConnected(&pBool);
>+    WriteParam(aMsg, pBool);
>+
>+    aParam->GetEmergencyCallsOnly(&pBool);
>+    WriteParam(aMsg, pBool);
>+
>+    aParam->GetRoaming(&pBool);
>+    WriteParam(aMsg, pBool);
>+
>+    aParam->GetType(pString);
>+    WriteParam(aMsg, pString);
>+
>+    aParam->GetNetwork(getter_AddRefs(pNetworkInfo));
>+    WriteParam(aMsg, pNetworkInfo.get());
>+
>+    aParam->GetCell(getter_AddRefs(pCellInfo));
>+    WriteParam(aMsg, pCellInfo.get());
>+
>+    // Serialize jsval signalStrength
>+    aParam->GetSignalStrength(&pJsval);
>+    isNull = !pJsval.isInt32();
>+    WriteParam(aMsg, isNull);
>+
>+    if (!isNull) {
>+      pInt32 = pJsval.toInt32();
>+      WriteParam(aMsg, pInt32);
>+    }
>+
>+    // Serialize jsval relSignalStrength
>+    aParam->GetRelSignalStrength(&pJsval);
>+    isNull = !pJsval.isInt32();
>+    WriteParam(aMsg, isNull);
>+
>+    if (!isNull) {
>+      pInt32 = pJsval.toInt32();
>+      WriteParam(aMsg, pInt32);
>+    }
>+  }
>+
>+  // Function to de-serialize a MobileConectionInfo.
>+  static bool Read(const Message* aMsg, void **aIter, paramType* aResult)
>+  {
>+    // Check if is the null pointer we have transfered.
>+    bool isNull;
>+    if (!ReadParam(aMsg, aIter, &isNull)) {
>+      return false;
>+    }
>+
>+    if (isNull) {
>+      *aResult = nullptr;
>+      return true;
>+    }
>+
>+    AutoSafeJSContext cx;
>+    nsString state;
>+    bool connected;
>+    bool emergencyOnly;
>+    bool roaming;
>+    nsString type;
>+    nsIMobileNetworkInfo* networkInfo = nullptr;
>+    nsIMobileCellInfo* cellInfo = nullptr;
>+    Nullable<int32_t> signalStrength;
>+    Nullable<uint16_t> relSignalStrength;
>+
>+    // It's not important to us where it fails, but rather if it fails
>+    if (!(ReadParam(aMsg, aIter, &state) &&
>+          ReadParam(aMsg, aIter, &connected) &&
>+          ReadParam(aMsg, aIter, &emergencyOnly) &&
>+          ReadParam(aMsg, aIter, &roaming) &&
>+          ReadParam(aMsg, aIter, &type) &&
>+          ReadParam(aMsg, aIter, &networkInfo) &&
>+          ReadParam(aMsg, aIter, &cellInfo))) {
>+      return false;
>+    }
>+
>+    // De-serialize jsval signalStrength
>+    if (!ReadParam(aMsg, aIter, &isNull)) {
>+      return false;
>+    }
>+
>+    if (!isNull) {
>+      int32_t value;
>+
>+      if (!ReadParam(aMsg, aIter, &value)) {
>+        return false;
>+      }
>+
>+      signalStrength.SetValue(value);
>+    }
>+
>+    // De-serialize jsval relSignalStrength
>+    if (!ReadParam(aMsg, aIter, &isNull)) {
>+      return false;
>+    }
>+
>+    if (!isNull) {
>+      int32_t value;
>+
>+      if (!ReadParam(aMsg, aIter, &value)) {
>+        return false;
>+      }
>+
>+      relSignalStrength.SetValue(uint16_t(value));
>+    }
>+
>+    *aResult = new MobileConnectionInfo(state,
>+                                        connected,
>+                                        emergencyOnly,
>+                                        roaming,
>+                                        networkInfo,
>+                                        type,
>+                                        signalStrength,
>+                                        relSignalStrength,
>+                                        cellInfo);
>+    // We release this ref after receiver finishes processing.
>+    NS_IF_ADDREF(*aResult);
>+    // We already clone the data into MobileConnectionInfo, so release the ref
>+    // of networkInfo and cellInfo here.
>+    NS_IF_RELEASE(networkInfo);
>+    NS_IF_RELEASE(cellInfo);
>+
>+    return true;
>+  }
>+};
>+
>+/**
>+ * MozCallForwardingOptions Serialize/De-serialize.
>+ */
>+template <>
>+struct ParamTraits<MozCallForwardingOptions>
>+{
>+  typedef MozCallForwardingOptions paramType;
>+
>+  // Function to serialize a MozCallForwardingOptions.
>+  static void Write(Message *aMsg, const paramType& aParam)
>+  {
>+    bool wasPassed = false;
>+    bool isNull = false;
>+
>+    // Write mActive
>+    wasPassed = aParam.mActive.WasPassed();
>+    WriteParam(aMsg, wasPassed);
>+    if (wasPassed) {
>+      isNull = aParam.mActive.Value().IsNull();
>+      WriteParam(aMsg, isNull);
>+      if (!isNull) {
>+        WriteParam(aMsg, aParam.mActive.Value().Value());
>+      }
>+    }
>+
>+    // Write mAction
>+    wasPassed = aParam.mAction.WasPassed();
>+    WriteParam(aMsg, wasPassed);
>+    if (wasPassed) {
>+      isNull = aParam.mAction.Value().IsNull();
>+      WriteParam(aMsg, isNull);
>+      if (!isNull) {
>+        WriteParam(aMsg, aParam.mAction.Value().Value());
>+      }
>+    }
>+
>+    // Write mReason
>+    wasPassed = aParam.mReason.WasPassed();
>+    WriteParam(aMsg, wasPassed);
>+    if (wasPassed) {
>+      isNull = aParam.mReason.Value().IsNull();
>+      WriteParam(aMsg, isNull);
>+      if (!isNull) {
>+        WriteParam(aMsg, aParam.mReason.Value().Value());
>+      }
>+    }
>+
>+    // Write mNumber
>+    wasPassed = aParam.mNumber.WasPassed();
>+    WriteParam(aMsg, wasPassed);
>+    if (wasPassed) {
>+      WriteParam(aMsg, aParam.mNumber.Value());
>+    }
>+
>+    // Write mTimeSeconds
>+    wasPassed = aParam.mTimeSeconds.WasPassed();
>+    WriteParam(aMsg, wasPassed);
>+    if (wasPassed) {
>+      isNull = aParam.mTimeSeconds.Value().IsNull();
>+      WriteParam(aMsg, isNull);
>+      if (!isNull) {
>+        WriteParam(aMsg, aParam.mTimeSeconds.Value().Value());
>+      }
>+    }
>+
>+    // Write mServiceClass
>+    wasPassed = aParam.mServiceClass.WasPassed();
>+    WriteParam(aMsg, wasPassed);
>+    if (wasPassed) {
>+      isNull = aParam.mServiceClass.Value().IsNull();
>+      WriteParam(aMsg, isNull);
>+      if (!isNull) {
>+        WriteParam(aMsg, aParam.mServiceClass.Value().Value());
>+      }
>+    }
>+  }
>+
>+  // Function to de-serialize a MozCallForwardingOptions.
>+  static bool Read(const Message *aMsg, void **aIter, paramType* aResult)
>+  {
>+    bool wasPassed = false;
>+    bool isNull = false;
>+
>+    // Read mActive
>+    if (!ReadParam(aMsg, aIter, &wasPassed)) {
>+      return false;
>+    }
>+    if (wasPassed) {
>+      aResult->mActive.Construct();
>+      if (!ReadParam(aMsg, aIter, &isNull)) {
>+        return false;
>+      }
>+
>+      if (!isNull) {
>+        if (!ReadParam(aMsg, aIter, &aResult->mActive.Value().SetValue())) {
>+          return false;
>+        }
>+      }
>+    }
>+
>+    // Read mAction
>+    if (!ReadParam(aMsg, aIter, &wasPassed)) {
>+      return false;
>+    }
>+    if (wasPassed) {
>+      aResult->mAction.Construct();
>+      if (!ReadParam(aMsg, aIter, &isNull)) {
>+        return false;
>+      }
>+
>+      if (!isNull) {
>+        if (!ReadParam(aMsg, aIter, &aResult->mAction.Value().SetValue())) {
>+          return false;
>+        }
>+      }
>+    }
>+
>+    // Read mReason
>+    if (!ReadParam(aMsg, aIter, &wasPassed)) {
>+      return false;
>+    }
>+    if (wasPassed) {
>+      aResult->mReason.Construct();
>+      if (!ReadParam(aMsg, aIter, &isNull)) {
>+        return false;
>+      }
>+
>+      if (!isNull) {
>+        if (!ReadParam(aMsg, aIter, &aResult->mReason.Value().SetValue())) {
>+          return false;
>+        }
>+      }
>+    }
>+
>+    // Read mNumber
>+    if (!ReadParam(aMsg, aIter, &wasPassed)) {
>+      return false;
>+    }
>+    if (wasPassed) {
>+      if (!ReadParam(aMsg, aIter, &aResult->mNumber.Construct())) {
>+        return false;
>+      }
>+    }
>+
>+    // Read mTimeSeconds
>+    if (!ReadParam(aMsg, aIter, &wasPassed)) {
>+      return false;
>+    }
>+    if (wasPassed) {
>+      aResult->mTimeSeconds.Construct();
>+      if (!ReadParam(aMsg, aIter, &isNull)) {
>+        return false;
>+      }
>+
>+      if (!isNull) {
>+        if (!ReadParam(aMsg, aIter, &aResult->mTimeSeconds.Value().SetValue())) {
>+          return false;
>+        }
>+      }
>+    }
>+
>+    // Read mServiceClass
>+    if (!ReadParam(aMsg, aIter, &wasPassed)) {
>+      return false;
>+    }
>+    if (wasPassed) {
>+      aResult->mServiceClass.Construct();
>+      if (!ReadParam(aMsg, aIter, &isNull)) {
>+        return false;
>+      }
>+
>+      if (!isNull) {
>+        if (!ReadParam(aMsg, aIter, &aResult->mServiceClass.Value().SetValue())) {
>+          return false;
>+        }
>+      }
>+    }
>+
>+    return true;
>+  }
>+};
>+
>+/**
>+ * MozCallBarringOptions Serialize/De-serialize.
>+ */
>+template <>
>+struct ParamTraits<MozCallBarringOptions>
>+{
>+  typedef MozCallBarringOptions paramType;
>+
>+  // Function to serialize a MozCallBarringOptions.
>+  static void Write(Message *aMsg, const paramType& aParam)
>+  {
>+    bool wasPassed = false;
>+    bool isNull = false;
>+
>+    // Write mProgram
>+    wasPassed = aParam.mProgram.WasPassed();
>+    WriteParam(aMsg, wasPassed);
>+    if (wasPassed) {
>+      isNull = aParam.mProgram.Value().IsNull();
>+      WriteParam(aMsg, isNull);
>+      if (!isNull) {
>+        WriteParam(aMsg, aParam.mProgram.Value().Value());
>+      }
>+    }
>+
>+    // Write mEnabled
>+    wasPassed = aParam.mEnabled.WasPassed();
>+    WriteParam(aMsg, wasPassed);
>+    if (wasPassed) {
>+      isNull = aParam.mEnabled.Value().IsNull();
>+      WriteParam(aMsg, isNull);
>+      if (!isNull) {
>+        WriteParam(aMsg, aParam.mEnabled.Value().Value());
>+      }
>+    }
>+
>+    // Write mPassword
>+    wasPassed = aParam.mPassword.WasPassed();
>+    WriteParam(aMsg, wasPassed);
>+    if (wasPassed) {
>+      WriteParam(aMsg, aParam.mPassword.Value());
>+    }
>+
>+    // Write mServiceClass
>+    wasPassed = aParam.mServiceClass.WasPassed();
>+    WriteParam(aMsg, wasPassed);
>+    if (wasPassed) {
>+      isNull = aParam.mServiceClass.Value().IsNull();
>+      WriteParam(aMsg, isNull);
>+      if (!isNull) {
>+        WriteParam(aMsg, aParam.mServiceClass.Value().Value());
>+      }
>+    }
>+
>+    // Write mPin
>+    wasPassed = aParam.mPin.WasPassed();
>+    WriteParam(aMsg, wasPassed);
>+    if (wasPassed) {
>+      WriteParam(aMsg, aParam.mPin.Value());
>+    }
>+
>+    // Write mNewPin
>+    wasPassed = aParam.mNewPin.WasPassed();
>+    WriteParam(aMsg, wasPassed);
>+    if (wasPassed) {
>+      WriteParam(aMsg, aParam.mNewPin.Value());
>+    }
>+  }
>+
>+  // Function to de-serialize a MozCallBarringOptions.
>+  static bool Read(const Message *aMsg, void **aIter, paramType* aResult)
>+  {
>+    bool wasPassed = false;
>+    bool isNull = false;
>+
>+    // Read mProgram
>+    if (!ReadParam(aMsg, aIter, &wasPassed)) {
>+      return false;
>+    }
>+    if (wasPassed) {
>+      aResult->mProgram.Construct();
>+      if (!ReadParam(aMsg, aIter, &isNull)) {
>+        return false;
>+      }
>+
>+      if (!isNull) {
>+        if (!ReadParam(aMsg, aIter, &aResult->mProgram.Value().SetValue())) {
>+          return false;
>+        }
>+      }
>+    }
>+
>+    // Read mEnabled
>+    if (!ReadParam(aMsg, aIter, &wasPassed)) {
>+      return false;
>+    }
>+    if (wasPassed) {
>+      aResult->mEnabled.Construct();
>+      if (!ReadParam(aMsg, aIter, &isNull)) {
>+        return false;
>+      }
>+
>+      if (!isNull) {
>+        if (!ReadParam(aMsg, aIter, &aResult->mEnabled.Value().SetValue())) {
>+          return false;
>+        }
>+      }
>+    }
>+
>+    // Read mPassword
>+    if (!ReadParam(aMsg, aIter, &wasPassed)) {
>+      return false;
>+    }
>+    if (wasPassed) {
>+      if (!ReadParam(aMsg, aIter, &aResult->mPassword.Construct())) {
>+        return false;
>+      }
>+    }
>+
>+    // Read mServiceClass
>+    if (!ReadParam(aMsg, aIter, &wasPassed)) {
>+      return false;
>+    }
>+    if (wasPassed) {
>+      aResult->mServiceClass.Construct();
>+      if (!ReadParam(aMsg, aIter, &isNull)) {
>+        return false;
>+      }
>+
>+      if (!isNull) {
>+        if (!ReadParam(aMsg, aIter, &aResult->mServiceClass.Value().SetValue())) {
>+          return false;
>+        }
>+      }
>+    }
>+
>+    // Read mPin
>+    if (!ReadParam(aMsg, aIter, &wasPassed)) {
>+      return false;
>+    }
>+    if (wasPassed) {
>+      if (!ReadParam(aMsg, aIter, &aResult->mPin.Construct())) {
>+        return false;
>+      }
>+    }
>+
>+    // Read mNewPin
>+    if (!ReadParam(aMsg, aIter, &wasPassed)) {
>+      return false;
>+    }
>+    if (wasPassed) {
>+      if (!ReadParam(aMsg, aIter, &aResult->mNewPin.Construct())) {
>+        return false;
>+      }
>+    }
>+
>+    return true;
>+  }
>+};
>+
>+} // namespace IPC
>+
>+#endif // dom_mobileconnection_src_ipc_MobileConnectionIPCSerialiser_h
>diff --git a/dom/mobileconnection/src/ipc/PMobileConnection.ipdl b/dom/mobileconnection/src/ipc/PMobileConnection.ipdl
>new file mode 100644
>--- /dev/null
>+++ b/dom/mobileconnection/src/ipc/PMobileConnection.ipdl
>@@ -0,0 +1,188 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set sw=2 ts=8 et ft=cpp : */
>+/* 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/. */
>+
>+include protocol PContent;
>+include protocol PMobileConnectionRequest;
>+include PMobileConnectionTypes;
>+
>+namespace mozilla {
>+namespace dom {
>+
>+sync protocol PMobileConnection
>+{
>+  manager PContent;
>+  manages PMobileConnectionRequest;
>+
>+child:
>+  NotifyVoiceInfoChanged(nsMobileConnectionInfo aInfo);
>+  NotifyDataInfoChanged(nsMobileConnectionInfo aInfo);
>+  NotifyUssdReceived(nsString aMessage, bool aSessionEnd);
>+  NotifyDataError(nsString aMessage);
>+  NotifyCFStateChanged(bool aSuccess, uint16_t aAction, uint16_t aReason,
>+                       nsString aNumber, uint16_t aTimeSeconds,
>+                       uint16_t aServiceClass);
>+  NotifyEmergencyCbModeChanged(bool aActive, uint32_t aTimeoutMs);
>+  NotifyOtaStatusChanged(nsString aStatus);
>+  NotifyIccChanged(nsString aIccId);
>+  NotifyRadioStateChanged(nsString aRadioState);
>+  NotifyClirModeChanged(uint32_t aMode);
>+  NotifyLastNetworkChanged(nsString aNetwork);
>+  NotifyLastHomeNetworkChanged(nsString aNetwork);
>+  NotifyNetworkSelectionModeChanged(nsString aMode);
>+
>+parent:
>+  /**
>+   * Send when child no longer needs to use PMobileConnection.
>+   */
>+  __delete__();
>+
>+  /**
>+   * Sent when the child makes an asynchronous request to the parent.
>+   */
>+  PMobileConnectionRequest(MobileConnectionRequest aRequest);
>+
>+  /**
>+   * Sync call only be called once per child actor for initialization.
>+   */
>+  sync Init()
>+    returns (nsMobileConnectionInfo aVoice, nsMobileConnectionInfo aData,
>+             nsString aLastKnownNetwork, nsString aLastKnownHomeNetwork,
>+             nsString aIccId, nsString aNetworkSelectionMode,
>+             nsString aRadioState, nsString[] aSupportedNetworkTypes);
>+};
>+
>+/**
>+ * MobileConnectionRequest
>+ */
>+struct GetNetworksRequest
>+{
>+};
>+
>+struct SelectNetworkRequest
>+{
>+  nsMobileNetworkInfo network;
>+};
>+
>+struct SelectNetworkAutoRequest
>+{
>+};
>+
>+struct SetPreferredNetworkTypeRequest
>+{
>+  nsString type;
>+};
>+
>+struct GetPreferredNetworkTypeRequest
>+{
>+};
>+
>+struct SetRoamingPreferenceRequest
>+{
>+  nsString mode;
>+};
>+
>+struct GetRoamingPreferenceRequest
>+{
>+};
>+
>+struct SetVoicePrivacyModeRequest
>+{
>+  bool enabled;
>+};
>+
>+struct GetVoicePrivacyModeRequest
>+{
>+};
>+
>+struct SendMmiRequest
>+{
>+  nsString mmi;
>+};
>+
>+struct CancelMmiRequest
>+{
>+};
>+
>+struct SetCallForwardingRequest
>+{
>+  MozCallForwardingOptions options;
>+};
>+
>+struct GetCallForwardingRequest
>+{
>+  int16_t reason;
>+};
>+
>+struct SetCallBarringRequest
>+{
>+  MozCallBarringOptions options;
>+};
>+
>+struct GetCallBarringRequest
>+{
>+  MozCallBarringOptions options;
>+};
>+
>+struct ChangeCallBarringPasswordRequest
>+{
>+  MozCallBarringOptions options;
>+};
>+
>+struct SetCallWaitingRequest
>+{
>+  bool enabled;
>+};
>+
>+struct GetCallWaitingRequest
>+{
>+};
>+
>+struct SetCallingLineIdRestrictionRequest
>+{
>+  uint16_t mode;
>+};
>+
>+struct GetCallingLineIdRestrictionRequest
>+{
>+};
>+
>+struct ExitEmergencyCbModeRequest
>+{
>+};
>+
>+struct SetRadioEnabledRequest
>+{
>+  bool enabled;
>+};
>+
>+union MobileConnectionRequest
>+{
>+  GetNetworksRequest;
>+  SelectNetworkRequest;
>+  SelectNetworkAutoRequest;
>+  SetPreferredNetworkTypeRequest;
>+  GetPreferredNetworkTypeRequest;
>+  SetRoamingPreferenceRequest;
>+  GetRoamingPreferenceRequest;
>+  SetVoicePrivacyModeRequest;
>+  GetVoicePrivacyModeRequest;
>+  SendMmiRequest;
>+  CancelMmiRequest;
>+  SetCallForwardingRequest;
>+  GetCallForwardingRequest;
>+  SetCallBarringRequest;
>+  GetCallBarringRequest;
>+  ChangeCallBarringPasswordRequest;
>+  SetCallWaitingRequest;
>+  GetCallWaitingRequest;
>+  SetCallingLineIdRestrictionRequest;
>+  GetCallingLineIdRestrictionRequest;
>+  ExitEmergencyCbModeRequest;
>+  SetRadioEnabledRequest;
>+};
>+
>+} // namespace mozilla
>+} // namespace dom
>diff --git a/dom/mobileconnection/src/ipc/PMobileConnectionRequest.ipdl b/dom/mobileconnection/src/ipc/PMobileConnectionRequest.ipdl
>new file mode 100644
>--- /dev/null
>+++ b/dom/mobileconnection/src/ipc/PMobileConnectionRequest.ipdl
>@@ -0,0 +1,103 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set sw=2 ts=8 et ft=cpp : */
>+/* 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/. */
>+
>+include protocol PMobileConnection;
>+include PMobileConnectionTypes;
>+
>+namespace mozilla {
>+namespace dom {
>+
>+protocol PMobileConnectionRequest
>+{
>+  manager PMobileConnection;
>+
>+child:
>+  /**
>+   * Send when asynchronous request has completed.
>+   */
>+  __delete__(MobileConnectionReply aResponse);
>+};
>+
>+/**
>+ * MobileConnectionReply
>+ */
>+// Success
>+struct MobileConnectionReplySuccess
>+{
>+};
>+
>+struct MobileConnectionReplySuccessString
>+{
>+  nsString result;
>+};
>+
>+struct MobileConnectionReplySuccessBoolean
>+{
>+  bool result;
>+};
>+
>+struct MobileConnectionReplySuccessNetworks
>+{
>+  nsMobileNetworkInfo[] results;
>+};
>+
>+struct MobileConnectionReplySuccessMmi
>+{
>+  nsString serviceCode;
>+  nsString statusMessage;
>+  AdditionalInformation additionalInformation;
>+};
>+
>+struct MobileConnectionReplySuccessCallForwarding
>+{
>+  MozCallForwardingOptions[] results;
>+};
>+
>+struct MobileConnectionReplySuccessCallBarring
>+{
>+  uint16_t program;
>+  bool enabled;
>+  uint16_t serviceClass;
>+};
>+
>+struct MobileConnectionReplySuccessClirStatus
>+{
>+  uint16_t n;
>+  uint16_t m;
>+};
>+
>+// Error
>+struct MobileConnectionReplyError
>+{
>+  nsString message;
>+};
>+
>+struct MobileConnectionReplyErrorMmi
>+{
>+  nsString name;
>+  nsString message;
>+  nsString serviceCode;
>+  AdditionalInformation additionalInformation;
>+};
>+
>+union MobileConnectionReply
>+{
>+  // Success
>+  MobileConnectionReplySuccess;
>+  MobileConnectionReplySuccessString;
>+  MobileConnectionReplySuccessBoolean;
>+  MobileConnectionReplySuccessNetworks;
>+  MobileConnectionReplySuccessMmi;
>+  MobileConnectionReplySuccessCallForwarding;
>+  MobileConnectionReplySuccessCallBarring;
>+  MobileConnectionReplySuccessClirStatus;
>+  // Error
>+  MobileConnectionReplyError;
>+  MobileConnectionReplyErrorMmi;
>+};
>+
>+} // namespace dom
>+} // namespace mozilla
>diff --git a/dom/mobileconnection/src/ipc/PMobileConnectionTypes.ipdlh b/dom/mobileconnection/src/ipc/PMobileConnectionTypes.ipdlh
>new file mode 100644
>--- /dev/null
>+++ b/dom/mobileconnection/src/ipc/PMobileConnectionTypes.ipdlh
>@@ -0,0 +1,24 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set sw=2 ts=8 et ft=cpp : */
>+/* 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/. */
>+
>+using nsMobileConnectionInfo from "mozilla/dom/MobileConnectionIPCSerializer.h";
>+using nsMobileNetworkInfo from "mozilla/dom/MobileConnectionIPCSerializer.h";
>+using struct mozilla::void_t from "ipc/IPCMessageUtils.h";
>+using struct IPC::MozCallForwardingOptions from "mozilla/dom/MobileConnectionIPCSerializer.h";
>+using struct IPC::MozCallBarringOptions from "mozilla/dom/MobileConnectionIPCSerializer.h";
>+
>+namespace mozilla {
>+namespace dom {
>+
>+union AdditionalInformation {
>+  void_t;
>+  uint16_t;
>+  nsString[];
>+  MozCallForwardingOptions[];
>+};
>+
>+} // namespace dom
>+} // namespace mozilla
>diff --git a/dom/mobileconnection/src/moz.build b/dom/mobileconnection/src/moz.build
>--- a/dom/mobileconnection/src/moz.build
>+++ b/dom/mobileconnection/src/moz.build
>@@ -1,16 +1,17 @@
> # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> # vim: set filetype=python:
> # 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/.
> 
> EXPORTS.mozilla.dom += [
>     'DOMMMIError.h',
>+    'ipc/MobileConnectionIPCSerializer.h',
>     'MobileCellInfo.h',
>     'MobileConnection.h',
>     'MobileConnectionArray.h',
>     'MobileConnectionCallback.h',
>     'MobileConnectionInfo.h',
>     'MobileNetworkInfo.h',
> ]
> 
>@@ -19,13 +20,19 @@ SOURCES += [
>     'MobileCellInfo.cpp',
>     'MobileConnection.cpp',
>     'MobileConnectionArray.cpp',
>     'MobileConnectionCallback.cpp',
>     'MobileConnectionInfo.cpp',
>     'MobileNetworkInfo.cpp',
> ]
> 
>+IPDL_SOURCES += [
>+    'ipc/PMobileConnection.ipdl',
>+    'ipc/PMobileConnectionRequest.ipdl',
>+    'ipc/PMobileConnectionTypes.ipdlh',
>+]
>+
> FAIL_ON_WARNINGS = True
> 
> include('/ipc/chromium/chromium-config.mozbuild')
> 
> FINAL_LIBRARY = 'xul'
Attachment #8464549 - Flags: review+ → review?(bugs)
Comment on attachment 8464549 [details] [diff] [review]
Part 3-1: IPDL for MobileConnection, v6

>+    *aResult = new MobileNetworkInfo(shortName,
>+                                     longName,
>+                                     mcc,
>+                                     mnc,
>+                                     state);
>+    // We release this ref after receiver finishes processing.
>+    NS_IF_ADDREF(*aResult);
NS_ADDREF

>+    *aResult = new MobileCellInfo(gsmLac, gsmCellId, cdmaBsId, cdmaBsLat,
>+                                  cdmaBsLong, cdmaSystemId, cdmaNetworkId);
>+    // We release this ref after receiver finishes processing.
>+    NS_IF_ADDREF(*aResult);
ditto



>+    nsIMobileNetworkInfo* networkInfo = nullptr;
>+    nsIMobileCellInfo* cellInfo = nullptr;
Couldn't you use nsCOMPtr here


>+          ReadParam(aMsg, aIter, &networkInfo) &&
>+          ReadParam(aMsg, aIter, &cellInfo))) {
And then getter_Addrefs here.
Not quite sure if that compiles here, but please test.


>+
>+    *aResult = new MobileConnectionInfo(state,
>+                                        connected,
>+                                        emergencyOnly,
>+                                        roaming,
>+                                        networkInfo,
>+                                        type,
>+                                        signalStrength,
>+                                        relSignalStrength,
>+                                        cellInfo);
>+    // We release this ref after receiver finishes processing.
>+    NS_IF_ADDREF(*aResult);
NS_ADDREF



>+    // We already clone the data into MobileConnectionInfo, so release the ref
>+    // of networkInfo and cellInfo here.
>+    NS_IF_RELEASE(networkInfo);
>+    NS_IF_RELEASE(cellInfo);
if nsCOMPtr + getter_AddRefs approach works, these wouldn't be needed.


I still don't quite understand in which cases those objects wouldn't be used in JS.
Could you please add a comment to the ctors which don't call SetIsDOMBinding()
Attachment #8464549 - Flags: review?(bugs) → review+
Comment on attachment 8464626 [details] [diff] [review]
Part 3-2: IPDL implementation for MobileConnection, v6

>+%{C++
>+#define NO_ADDITIONINFORMATION 0
NO_ADDITIONAL_INFORMATION perhaps
>+MobileConnectionChild::UnregisterListener(nsIMobileConnectionListener* aListener)
>+{
>+  if (mListeners.IndexOf(aListener) != NoIndex) {
>+    mListeners.RemoveObject(aListener);
>+  }
You could just call RemoveObject here. IndexOf is extra work.

>+MobileConnectionRequestChild::DoReply(const MobileConnectionReplySuccessNetworks& aReply)
>+{
>+  uint32_t count = aReply.results().Length();
>+  nsTArray<nsCOMPtr<nsIMobileNetworkInfo>> results;
>+  for (uint32_t i = 0; i < count; i++) {
>+    // Use dont_AddRef here because these instances are already AddRef-ed in
>+    // MobileConnectionIPCSerializer.h
>+    nsCOMPtr<nsIMobileNetworkInfo> item = dont_AddRef(aReply.results()[i]);
>+    results.AppendElement(item);
>+  }
>+
>+  return NS_SUCCEEDED(mRequestCallback->NotifyGetNetworksSuccess(count,
>+                                                                 reinterpret_cast<nsIMobileNetworkInfo **>(results.Elements())));
hmm, this doesn't look good.
Couldn't you pass aReply.results() to NotifyGetNetworksSuccess ?

>+  MobileConnectionRequestChild(nsIMobileConnectionCallback *aRequestCallback)
* goes with the type

>+MobileConnectionParent::MobileConnectionParent(uint32_t aClientId)
>+  : mClientId(aClientId)
>+  , mLive(true)
>+{
>+  MOZ_COUNT_CTOR(MobileConnectionParent);
>+
>+  mService = do_GetService(NS_MOBILE_CONNECTION_SERVICE_CONTRACTID);
>+  NS_ASSERTION(mService, "This shouldn't fail!");
>+
>+  mService->RegisterListener(mClientId, this);
The NS_ASSERTION is useless since we just crash if do_GetService fails


>+  // We expect the element type is wstring.
>+  if (type == nsIDataType::VTYPE_WCHAR_STR) {
>+    char16_t **rawArray = reinterpret_cast<char16_t **>(data);
** goes with the type

>+MobileConnectionRequestParent::DoRequest(const SetCallForwardingRequest& aRequest)
>+{
>+  NS_ENSURE_TRUE(mService, false);
>+
>+  AutoSafeJSContext cx;
>+  JS::Rooted<JS::Value> options(cx);
>+  if (!ToJSValue(cx, aRequest.options(), &options)) {
clear the exception?

>+MobileConnectionRequestParent::DoRequest(const GetCallBarringRequest& aRequest)
>+{
>+  NS_ENSURE_TRUE(mService, false);
>+
>+  AutoSafeJSContext cx;
>+  JS::Rooted<JS::Value> options(cx);
>+  if (!ToJSValue(cx, aRequest.options(), &options)) {
ditto


These patches are massive, and it is hard to see the big picture.
Could you upload also a patch which contains all the changes.
Attachment #8464626 - Flags: review?(bugs) → review+
Comment on attachment 8464631 [details] [diff] [review]
Part 3-3: MobileConnectionIPCService for content process, v5


>+NS_IMETHODIMP
>+MobileConnectionIPCService::GetSupportedNetworkTypes(uint32_t aClientId,
>+                                                     nsIVariant** aSupportedTypes)
>+{
>+  if (aClientId >= mClients.Length()) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  nsCOMPtr<nsIVariant> supportedTypes = mClients[aClientId]->GetSupportedNetworkTypes();
>+  NS_IF_ADDREF(*aSupportedTypes = supportedTypes);
>+
>+  return NS_OK;
supportedTypes.forget(aSupportedTypes);



>+MobileConnectionIPCService::GetCallBarring(uint32_t aClientId,
>+                                           JS::Handle<JS::Value> aOptions,
>+                                           nsIMobileConnectionCallback* aRequest)
>+{
>+  AutoSafeJSContext cx;
>+  RootedDictionary<IPC::MozCallBarringOptions> options(cx);
Why Rooted? I thought your IPC* dictionary doesn't have any JS stuff.
Same also elsewhere.

>+nsIMobileConnectionServiceConstructor(nsISupports *aOuter, REFNSIID aIID,
>+                                      void **aResult)
>+{
>+  nsCOMPtr<nsIMobileConnectionService> service;
>+
>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+    service = MobileConnectionIPCService::GetSingleton();
>+  }
>+
>+  if (nullptr == service) {
Should be if (!service)

>+    return NS_ERROR_OUT_OF_MEMORY;
So on parent process if one tries to get this service, there will
be NS_ERROR_OUT_OF_MEMORY error? Doesn't sound right.
Could you please but XRE_GetProcessType() == GeckoProcessType_Content check to MobileConnectionIPCService::GetSingleton();
and then if service is null here, return NS_ERROR_NOT_AVAILABLE or some such


>+  }
>+
>+  return service->QueryInterface(aIID, aResult);
>+}
Attachment #8464631 - Flags: review?(bugs) → review-
Assignee

Comment 140

5 years ago
(In reply to Olli Pettay [:smaug] from comment #138)
> Comment on attachment 8464626 [details] [diff] [review]
> Part 3-2: IPDL implementation for MobileConnection, v6
>
> These patches are massive, and it is hard to see the big picture.
> Could you upload also a patch which contains all the changes.

Sure, I will upload a squashed patch after all review comments are addressed.
Assignee

Comment 141

5 years ago
(In reply to Olli Pettay [:smaug] from comment #137)
> Comment on attachment 8464549 [details] [diff] [review]
> Part 3-1: IPDL for MobileConnection, v6
> 
> >+    nsIMobileNetworkInfo* networkInfo = nullptr;
> >+    nsIMobileCellInfo* cellInfo = nullptr;
> Couldn't you use nsCOMPtr here
> 
> 
> >+          ReadParam(aMsg, aIter, &networkInfo) &&
> >+          ReadParam(aMsg, aIter, &cellInfo))) {
> And then getter_Addrefs here.
> Not quite sure if that compiles here, but please test.
>  
> 
> >+    // We already clone the data into MobileConnectionInfo, so release the ref
> >+    // of networkInfo and cellInfo here.
> >+    NS_IF_RELEASE(networkInfo);
> >+    NS_IF_RELEASE(cellInfo);
> if nsCOMPtr + getter_AddRefs approach works, these wouldn't be needed.

Unfortunately nsCOMPtr + getter_AddRefs doesn't work, I got compiler error as below :(

../../../dist/include/mozilla/dom/MobileConnectionIPCSerializer.h: In static member function 'static bool IPC::ParamTraits<nsIMobileConnectionInfo*>::Read(const IPC::Message*, void**, nsIMobileConnectionInfo**)':
../../../dist/include/mozilla/dom/MobileConnectionIPCSerializer.h:338: error: no matching function for call to 'ReadParam(const IPC::Message*&, void**&, nsGetterAddRefs<nsIMobileNetworkInfo>)'
../../../dist/include/mozilla/dom/MobileConnectionIPCSerializer.h:339: error: no matching function for call to 'ReadParam(const IPC::Message*&, void**&, nsGetterAddRefs<nsIMobileCellInfo>)
Assignee

Comment 142

5 years ago
(In reply to Olli Pettay [:smaug] from comment #138)
> Comment on attachment 8464626 [details] [diff] [review]
> Part 3-2: IPDL implementation for MobileConnection, v6
> 
> >+MobileConnectionRequestChild::DoReply(const MobileConnectionReplySuccessNetworks& aReply)
> >+{
> >+  uint32_t count = aReply.results().Length();
> >+  nsTArray<nsCOMPtr<nsIMobileNetworkInfo>> results;
> >+  for (uint32_t i = 0; i < count; i++) {
> >+    // Use dont_AddRef here because these instances are already AddRef-ed in
> >+    // MobileConnectionIPCSerializer.h
> >+    nsCOMPtr<nsIMobileNetworkInfo> item = dont_AddRef(aReply.results()[i]);
> >+    results.AppendElement(item);
> >+  }
> >+
> >+  return NS_SUCCEEDED(mRequestCallback->NotifyGetNetworksSuccess(count,
> >+                                                                 reinterpret_cast<nsIMobileNetworkInfo **>(results.Elements())));
> hmm, this doesn't look good.
> Couldn't you pass aReply.results() to NotifyGetNetworksSuccess ?

Since the results are AddRef-ed in IPCSerializer, we need to handle the refcnt things here.

And if I pass aReply.results() to NotifyGetNetworksSuccess, I got below error. :(

/data/mercurial/mozilla-central/dom/mobileconnection/src/ipc/MobileConnectionChild.cpp:422: error: no matching function for call to 'nsIMobileConnectionCallback::NotifyGetNetworksSuccess(uint32_t&, const nsTArray<nsIMobileNetworkInfo*>&)'
../../../dist/include/nsIMobileConnectionService.h:266: note: candidates are: virtual nsresult nsIMobileConnectionCallback::NotifyGetNetworksSuccess(uint32_t, nsIMobileNetworkInfo**)
Assignee

Comment 143

5 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #142)
> (In reply to Olli Pettay [:smaug] from comment #138)
> > Comment on attachment 8464626 [details] [diff] [review]
> > Part 3-2: IPDL implementation for MobileConnection, v6
> > 
> > >+MobileConnectionRequestChild::DoReply(const MobileConnectionReplySuccessNetworks& aReply)
> > >+{
> > >+  uint32_t count = aReply.results().Length();
> > >+  nsTArray<nsCOMPtr<nsIMobileNetworkInfo>> results;
> > >+  for (uint32_t i = 0; i < count; i++) {
> > >+    // Use dont_AddRef here because these instances are already AddRef-ed in
> > >+    // MobileConnectionIPCSerializer.h
> > >+    nsCOMPtr<nsIMobileNetworkInfo> item = dont_AddRef(aReply.results()[i]);
> > >+    results.AppendElement(item);
> > >+  }
> > >+
> > >+  return NS_SUCCEEDED(mRequestCallback->NotifyGetNetworksSuccess(count,
> > >+                                                                 reinterpret_cast<nsIMobileNetworkInfo **>(results.Elements())));
> > hmm, this doesn't look good.
> > Couldn't you pass aReply.results() to NotifyGetNetworksSuccess ?
> 
> Since the results are AddRef-ed in IPCSerializer, we need to handle the
> refcnt things here.
> 
> And if I pass aReply.results() to NotifyGetNetworksSuccess, I got below
> error. :(
> 
> /data/mercurial/mozilla-central/dom/mobileconnection/src/ipc/
> MobileConnectionChild.cpp:422: error: no matching function for call to
> 'nsIMobileConnectionCallback::NotifyGetNetworksSuccess(uint32_t&, const
> nsTArray<nsIMobileNetworkInfo*>&)'
> ../../../dist/include/nsIMobileConnectionService.h:266: note: candidates
> are: virtual nsresult
> nsIMobileConnectionCallback::NotifyGetNetworksSuccess(uint32_t,
> nsIMobileNetworkInfo**)

Er, pass aReply.results().Elements() and got below error

/data/mercurial/mozilla-central/dom/mobileconnection/src/ipc/MobileConnectionChild.cpp: In member function 'void mozilla::dom::mobileconnection::MobileConnectionChild::RegisterListener(nsIMobileConnectionListener*)':
/data/mercurial/mozilla-central/dom/mobileconnection/src/ipc/MobileConnectionChild.cpp:86: warning: comparison between signed and unsigned integer expressions
/data/mercurial/mozilla-central/dom/mobileconnection/src/ipc/MobileConnectionChild.cpp: In member function 'bool mozilla::dom::mobileconnection::MobileConnectionRequestChild::DoReply(const mozilla::dom::MobileConnectionReplySuccessNetworks&)':
/data/mercurial/mozilla-central/dom/mobileconnection/src/ipc/MobileConnectionChild.cpp:422: error: invalid conversion from 'nsIMobileNetworkInfo* const*' to 'nsIMobileNetworkInfo**'
/data/mercurial/mozilla-central/dom/mobileconnection/src/ipc/MobileConnectionChild.cpp:422: error:   initializing argument 2 of 'virtual nsresult nsIMobileConnectionCallback::NotifyGetNetworksSuccess(uint32_t, nsIMobileNetworkInfo**)'
Assignee

Comment 144

5 years ago
(In reply to Olli Pettay [:smaug] from comment #139)
> Comment on attachment 8464631 [details] [diff] [review]
> Part 3-3: MobileConnectionIPCService for content process, v5
> 
> 
> >+nsIMobileConnectionServiceConstructor(nsISupports *aOuter, REFNSIID aIID,
> >+                                      void **aResult)
> >+{
> >+  nsCOMPtr<nsIMobileConnectionService> service;
> >+
> >+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> >+    service = MobileConnectionIPCService::GetSingleton();
> >+  }
> >+
> >+  if (nullptr == service) {
> Should be if (!service)
> 
> >+    return NS_ERROR_OUT_OF_MEMORY;
> So on parent process if one tries to get this service, there will
> be NS_ERROR_OUT_OF_MEMORY error? Doesn't sound right.

Yes, should not be NS_ERROR_OUT_OF_MEMORY. Will address this in next version. (NS_ERROR_NOT_AVAILABLE)

> Could you please but XRE_GetProcessType() == GeckoProcessType_Content check
> to MobileConnectionIPCService::GetSingleton();
> and then if service is null here, return NS_ERROR_NOT_AVAILABLE or some such

We need this check in nsLayoutModule, because we have different handling for child process (In part 4-5.c).

if (XRE_GetProcessType() == GeckoProcessType_Content) {
  service = MobileConnectionIPCService::GetSingleton();
} else {
#if defined(MOZ_WIDGET_GONK)
  // service will be MobileConnectionGonkService
#endif
}
 
> 
> 
> >+  }
> >+
> >+  return service->QueryInterface(aIID, aResult);
> >+}
Assignee

Comment 145

5 years ago
Address review comment #135:
- Add MOZ_ASSERT(IsDOMBinding())
Address review comment #137:
- s/NS_IF_ADDREF/NS_ADDREF/
- For nsCOMPtr + getter_AddRefs, please see comment #141
Attachment #8464549 - Attachment is obsolete: true
Attachment #8465355 - Flags: review+
Assignee

Comment 146

5 years ago
Address review comment #138:
- Don't need IndexOf before RemoveObject
- Please see comment #143
- * goes with the type
- Add check for mService
- JS_ClearPendingException()
Attachment #8464626 - Attachment is obsolete: true
Attachment #8465360 - Flags: review+
Assignee

Comment 147

5 years ago
Hi Olli,

Thanks for your review.

I have addressed review comment #139:
- Use forget() instead.
- IPC::* dictionary doesn't need RootedDictionary
- returns NS_ERROR_NOT_AVAILABLE for no service case.
- Please see comment #144.

Could you take a look again?
((I will attach squashed patch later))
Attachment #8464631 - Attachment is obsolete: true
Attachment #8465364 - Flags: review?(bugs)
Assignee

Comment 148

5 years ago
Address review comment #119.
Attachment #8458582 - Attachment is obsolete: true
Assignee

Comment 149

5 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #140)
> (In reply to Olli Pettay [:smaug] from comment #138)
> > Comment on attachment 8464626 [details] [diff] [review]
> > Part 3-2: IPDL implementation for MobileConnection, v6
> >
> > These patches are massive, and it is hard to see the big picture.
> > Could you upload also a patch which contains all the changes.
> 
> Sure, I will upload a squashed patch after all review comments are addressed.

Here is the squashed patch.
Attachment #8465366 - Flags: feedback?(bugs)
(In reply to Edgar Chen [:edgar][:echen] from comment #142)
> (In reply to Olli Pettay [:smaug] from comment #138)
> > Comment on attachment 8464626 [details] [diff] [review]
> > Part 3-2: IPDL implementation for MobileConnection, v6
> > 
> > >+MobileConnectionRequestChild::DoReply(const MobileConnectionReplySuccessNetworks& aReply)
> > >+{
> > >+  uint32_t count = aReply.results().Length();
> > >+  nsTArray<nsCOMPtr<nsIMobileNetworkInfo>> results;
> > >+  for (uint32_t i = 0; i < count; i++) {
> > >+    // Use dont_AddRef here because these instances are already AddRef-ed in
> > >+    // MobileConnectionIPCSerializer.h
> > >+    nsCOMPtr<nsIMobileNetworkInfo> item = dont_AddRef(aReply.results()[i]);
> > >+    results.AppendElement(item);
> > >+  }
> > >+
> > >+  return NS_SUCCEEDED(mRequestCallback->NotifyGetNetworksSuccess(count,
> > >+                                                                 reinterpret_cast<nsIMobileNetworkInfo **>(results.Elements())));
> > hmm, this doesn't look good.
> > Couldn't you pass aReply.results() to NotifyGetNetworksSuccess ?
> 
> Since the results are AddRef-ed in IPCSerializer, we need to handle the
> refcnt things here.
Yes, you need to assign the values to some smart pointer which deals with releasing, and keeps the object
alive while NotifyGetNetworksSuccess is called. So creating nsTArray<nsCOMPtr<nsIMobileNetworkInfo>> results; makes sense. But reinterpret_cast is just evil.


> 
> And if I pass aReply.results() to NotifyGetNetworksSuccess, I got below
> error. :(
> 
> /data/mercurial/mozilla-central/dom/mobileconnection/src/ipc/
> MobileConnectionChild.cpp:422: error: no matching function for call to
> 'nsIMobileConnectionCallback::NotifyGetNetworksSuccess(uint32_t&, const
> nsTArray<nsIMobileNetworkInfo*>&)'
> ../../../dist/include/nsIMobileConnectionService.h:266: note: candidates
> are: virtual nsresult
> nsIMobileConnectionCallback::NotifyGetNetworksSuccess(uint32_t,
> nsIMobileNetworkInfo**)

Couldn't you just pass aReply.results().Elements() then?
Attachment #8465364 - Flags: review?(bugs) → review+
Assignee

Comment 151

5 years ago
(In reply to Olli Pettay [:smaug] from comment #150)
> (In reply to Edgar Chen [:edgar][:echen] from comment #142)
> > (In reply to Olli Pettay [:smaug] from comment #138)
> > 
> > And if I pass aReply.results() to NotifyGetNetworksSuccess, I got below
> > error. :(
> > 
> > /data/mercurial/mozilla-central/dom/mobileconnection/src/ipc/
> > MobileConnectionChild.cpp:422: error: no matching function for call to
> > 'nsIMobileConnectionCallback::NotifyGetNetworksSuccess(uint32_t&, const
> > nsTArray<nsIMobileNetworkInfo*>&)'
> > ../../../dist/include/nsIMobileConnectionService.h:266: note: candidates
> > are: virtual nsresult
> > nsIMobileConnectionCallback::NotifyGetNetworksSuccess(uint32_t,
> > nsIMobileNetworkInfo**)
> 
> Couldn't you just pass aReply.results().Elements() then?

Passing aReply.results().Elements() doesn't work, either :(

/data/mercurial/mozilla-central/dom/mobileconnection/src/ipc/MobileConnectionChild.cpp: In member function 'void mozilla::dom::mobileconnection::MobileConnectionChild::RegisterListener(nsIMobileConnectionListener*)':
/data/mercurial/mozilla-central/dom/mobileconnection/src/ipc/MobileConnectionChild.cpp:86: warning: comparison between signed and unsigned integer expressions
/data/mercurial/mozilla-central/dom/mobileconnection/src/ipc/MobileConnectionChild.cpp: In member function 'bool mozilla::dom::mobileconnection::MobileConnectionRequestChild::DoReply(const mozilla::dom::MobileConnectionReplySuccessNetworks&)':
/data/mercurial/mozilla-central/dom/mobileconnection/src/ipc/MobileConnectionChild.cpp:422: error: invalid conversion from 'nsIMobileNetworkInfo* const*' to 'nsIMobileNetworkInfo**'
/data/mercurial/mozilla-central/dom/mobileconnection/src/ipc/MobileConnectionChild.cpp:422: error:   initializing argument 2 of 'virtual nsresult nsIMobileConnectionCallback::NotifyGetNetworksSuccess(uint32_t, nsIMobileNetworkInfo**)'
but it is almost right. I guess some const_cast is needed to make 
nsIMobileNetworkInfo* const*' to 'nsIMobileNetworkInfo**'.
const_cast is way less evil than reinterpret_cast
Comment on attachment 8465366 [details] [diff] [review]
Squashed patch

Could you still explain ownership model of various objects.
Who owns what and how long and what guarantees all the objects are released
at some point. (Well, services are alive until the end of the process).

If I read the code correctly MobileConnection::Shutdown is currently called
by MobileConnectionArray::DropConnections().
And that is usually called when unlink runs. But what if JS keeps a ref
to a MobileConnection object. It shouldn't be shutdown.
Attachment #8465366 - Flags: feedback?(bugs) → feedback+
Possible changes to ownership model should be done in a followup bug/patch.

Comment 155

5 years ago
Comment on attachment 8464496 [details] [diff] [review]
Part 2-1: Rename MobileConnectionProvider to MobileConnectionService and redesign for IPDL, v7, r=hsinyi

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

as per comment 44 and comment 46, please reconsider the usage of jsval in public interfaces.
Attachment #8464496 - Flags: feedback-

Comment 156

5 years ago
Product team Update: This story needs to target 2.1. b2g-feature flag already set.
(See related discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1000016#c4)
(In reply to pgravel from comment #155)
> Comment on attachment 8464496 [details] [diff] [review]
> Part 2-1: Rename MobileConnectionProvider to MobileConnectionService and
> redesign for IPDL, v7, r=hsinyi
> 
> Review of attachment 8464496 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> as per comment 44 and comment 46, please reconsider the usage of jsval in
> public interfaces.

Hi,

Thanks for raising this again. Edgar and I just talked about this and we are still not clear enough about the problems you are facing. For example, are you talking about the usage of an input parameter or an output return value? What are the public interfaces in your definition, nsIMobileConnectionGonkService.idl counted? We do need more concrete information so that we know how we could improve the situation.

Considering the work we've done so far and the tight schedule, to not block this, I suggest we file another bug to thoroughly discuss difficulties and possible solutions.
Bug 1047196 filed as follow up to comment 44 and comment 46.
See Also: → 1047196
Assignee

Comment 159

5 years ago
(In reply to Olli Pettay [:smaug] from comment #152)
> but it is almost right. I guess some const_cast is needed to make 
> nsIMobileNetworkInfo* const*' to 'nsIMobileNetworkInfo**'.
> const_cast is way less evil than reinterpret_cast

Oh, I see, will address this. Thank you.
Assignee

Comment 160

5 years ago
(In reply to Olli Pettay [:smaug] from comment #153)
> If I read the code correctly MobileConnection::Shutdown is currently called
> by MobileConnectionArray::DropConnections().
> And that is usually called when unlink runs. But what if JS keeps a ref
> to a MobileConnection object. It shouldn't be shutdown.

(In reply to Olli Pettay [:smaug] from comment #154)
> Possible changes to ownership model should be done in a followup bug/patch.

File a follow-up for this, bug 1047246. Thank you
Assignee

Comment 161

5 years ago
Comment on attachment 8465365 [details] [diff] [review]
Part 4-5.c: MobileConnectionGonkService for gonk backend, v4

Address review comment #119:
- s/again/against/
- use Ci.nsIMobileConnectionService.* instead.
- Combine _updateInfo() and _updateConnectionInfo()
- Have a consistent way for state checking
  state === RIL.GECKO_MOBILE_CONNECTION_STATE_REGISTERED
- No return value for changeCallBarringPassword()
- s/need not/no need/

Hi Hsinyi, could you help to review again? Thank you.
Attachment #8465365 - Flags: review?(htsai)
Comment on attachment 8465365 [details] [diff] [review]
Part 4-5.c: MobileConnectionGonkService for gonk backend, v4

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

Awesome! Thank you Edgar for the whole work :D
Attachment #8465365 - Flags: review?(htsai) → review+
Duplicate of this bug: 1046588
Assignee

Comment 164

5 years ago
Comment on attachment 8465365 [details] [diff] [review]
Part 4-5.c: MobileConnectionGonkService for gonk backend, v4

Hi Kyle:

This patch touches moz.build and also package-manifest.in for adding MobileConnectionGonkService. May I have your review for these changes?

Thank you.
Attachment #8465365 - Flags: review?(khuey)
Assignee

Comment 165

5 years ago
Assignee

Comment 166

5 years ago
Comment on attachment 8466080 [details] [diff] [review]
Part 5: Changes for xpcshell test

We combine GECKO_RADIOSTATE_* and GECKO_DETAILED_RADIOSTATE_* in part 4-5.b, some of xpcshell tests also need to be modified. Hi Hsinyi, may I have your review? Thank you.
Attachment #8466080 - Flags: review?(htsai)
Attachment #8466080 - Flags: review?(htsai) → review+
Comment on attachment 8465365 [details] [diff] [review]
Part 4-5.c: MobileConnectionGonkService for gonk backend, v4

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

r=me on the build changes
Attachment #8465365 - Flags: review?(khuey) → review+
QA Whiteboard: [COM=RIL]
Assignee

Comment 168

5 years ago
Try result: https://tbpl.mozilla.org/?tree=Try&rev=24e94e5417bf

Only B2G ICS emulator is green. :(

I am working on fix those error now.
Assignee

Updated

5 years ago
Blocks: 1050140
Assignee

Comment 169

5 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #168)
> Try result: https://tbpl.mozilla.org/?tree=Try&rev=24e94e5417bf
> 
> Only B2G ICS emulator is green. :(
> 
> I am working on fix those error now.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=73536f35602d

I have fixed the build error, it can success to build on all platform now.
But we still have some failure on test cases, have no idea why, still under investigating:
- TEST-UNEXPECTED-FAIL | page-mod-debugger-post/main.testDebugger | Test timed out
- INFO TEST-UNEXPECTED-FAIL | /tests/toolkit/devtools/apps/tests/test_webapps_actor.html | Test timed out.
Assignee

Comment 171

5 years ago
Comment on attachment 8471285 [details] [diff] [review]
Part 4-6: [NetworkManager] Get dataInfo via MobileConnectionService, v1

dataInfo is no longer exposed to rilContext any more, modules who need this information should get it from MobileConnectionService.

Hi Vicamo, could you help to review the changes for NetworkManager? Thank you.
Attachment #8471285 - Flags: review?(vyang)
Attachment #8471285 - Flags: review?(vyang) → review+
Assignee

Comment 174

5 years ago
Comment on attachment 8471293 [details] [diff] [review]
Part 4-7: [NetworkGeolocationProvider] Get dataInfo via MobileConnectionService, v1

voiceInfo is no longer exposed to rilContext any more [1], modules who need this information should get it from MobileConnectionService.

Hi Vicamo, could you help to review the changes for NetworkGeolocationProvider? Thank you.

[1] Please see patch part 4-1.
Attachment #8471293 - Flags: review?(dougt)
Assignee

Updated

5 years ago
Attachment #8471293 - Flags: review?(dougt)
Assignee

Comment 175

5 years ago
Comment on attachment 8471293 [details] [diff] [review]
Part 4-7: [NetworkGeolocationProvider] Get dataInfo via MobileConnectionService, v1

voiceInfo is no longer exposed to rilContext any more [1], modules who need this information should get it from MobileConnectionService.

Hi Doug, could you help to review the changes for NetworkGeolocationProvider? Thank you.

[1] Please see patch part 4-1.
Attachment #8471293 - Flags: review?(dougt)
Assignee

Comment 176

5 years ago
Comment on attachment 8471297 [details] [diff] [review]
Part 4-8: [MobileIdentityManager] Get {voice|data}Info via MobileConnectionService, v1

voiceInfo and dataInfo are no longer exposed to rilContext any more [1], modules who need these information should get them from MobileConnectionService.

Hi Fernando, could you help to review the changes for MobileIdentityManager? Thank you.

[1] Please see patch part 4-1.
Attachment #8471297 - Flags: review?(ferjmoreno)
Comment on attachment 8471297 [details] [diff] [review]
Part 4-8: [MobileIdentityManager] Get {voice|data}Info via MobileConnectionService, v1

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

LGTM
Attachment #8471297 - Flags: review?(ferjmoreno) → review+
Assignee

Comment 178

5 years ago
Comment on attachment 8471293 [details] [diff] [review]
Part 4-7: [NetworkGeolocationProvider] Get dataInfo via MobileConnectionService, v1

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

voiceInfo is no longer exposed to rilContext any more [1], modules who need this information should get it from MobileConnectionService.

Hi Kan-Ru, could you help to review the changes for NetworkGeolocationProvider? Thank you.

[1] Please see patch part 4-1.
Attachment #8471293 - Flags: review?(dougt) → review?(kchen)
Attachment #8471293 - Flags: review?(kchen) → review+
Assignee

Comment 180

5 years ago
Comment on attachment 8472256 [details] [diff] [review]
Part 6: Build MobileConnection DOM/IPC by default, v1

Te eliminate the use of MOZ_B2G_RIL and also fix the build error on non-b2g platform [1], DOM and IPC part s are built by default, only backend part, MobileConnectionGonkService, is guarded by MOZ_WIDGET_GONK and MOZ_B2G_RIL.

Hi Olli, Kyle, may I have your review? Thank you.

[1] Please see comment #168
Attachment #8472256 - Flags: review?(khuey)
Attachment #8472256 - Flags: review?(bugs)
Attachment #8472256 - Flags: review?(bugs) → review+
Comment on attachment 8472256 [details] [diff] [review]
Part 6: Build MobileConnection DOM/IPC by default, v1

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

::: dom/mobileconnection/src/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +
> +SOURCES += [
> +    'DOMMMIError.cpp',

Doesn't seem like we have a reason to move this block around?
Assignee

Comment 182

5 years ago
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #181)
> Comment on attachment 8472256 [details] [diff] [review]
> Part 6: Build MobileConnection DOM/IPC by default, v1
> 
> Review of attachment 8472256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobileconnection/src/moz.build
> @@ +5,5 @@
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> > +
> > +SOURCES += [
> > +    'DOMMMIError.cpp',.,
> 
> Doesn't seem like we have a reason to move this block around?

No, we don't need to move this block, will address this in next version. Thank you, Vicamo.
Assignee

Comment 183

5 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #169)
> (In reply to Edgar Chen [:edgar][:echen] from comment #168)
> > Try result: https://tbpl.mozilla.org/?tree=Try&rev=24e94e5417bf
> > 
> > Only B2G ICS emulator is green. :(
> > 
> > I am working on fix those error now.
> 
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=73536f35602d
> 
> I have fixed the build error, it can success to build on all platform now.
> But we still have some failure on test cases, have no idea why, still under
> investigating:
> - INFO TEST-UNEXPECTED-FAIL |
> /tests/toolkit/devtools/apps/tests/test_webapps_actor.html | Test timed out.

Finally, I found the root cause of the time out in test_webapps_actor.html:

test_webapps_actor.html relies on remote-debugger, so it tries to enable remote-debugger by change settings via SettingService [1] and waits for observer event "debugger-server-started" before starting tests.

In time out case, the settings.js doesn't receive the settings change event and the USBRemoteDebugger won't be activated [2], either. So test_webapps_actor.html gets stuck on waiting "debugger-server-started" event.

After digging into the code, I found we hit a timing problem between SettingsManager [3] and SettingsChangeNotifier [4]. 

The problem is: During SettingsManager init process [5], it registers it's target to SettingsChangeNotifier by sending Settings:RegisterForMessages message, but at that time SettingsChangeNotifier might not yet ready to receive message (doesn't register listener to ppmm yet [6]). So SettingsManager fails to register and is unable to receive any Settings:Changed message from SettingsChangeNotifier.

Note that test_webapps_actor.html now also has chance to get stuck (please see bug 918528), moving mobileconnection to ipdl just makes it 100% reproducible.

[1] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/apps/tests/debugger-protocol-helper.js#21
[2] http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#460
[3] http://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsManager.js
[4] http://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsChangeNotifier.jsm
[5] http://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsManager.js#368
[6] http://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsChangeNotifier.jsm#35
Assignee

Comment 184

5 years ago
Just a quick fix for the timing problem mentioned in comment #183.
((Will file a separated bug for it))
Assignee

Comment 185

5 years ago
Rebase only.
Attachment #8464497 - Attachment is obsolete: true
Attachment #8472884 - Flags: review+
Assignee

Comment 186

5 years ago
Rebase only.
Attachment #8464499 - Attachment is obsolete: true
Attachment #8472887 - Flags: review+
Assignee

Comment 187

5 years ago
Rebase only.
Attachment #8465355 - Attachment is obsolete: true
Attachment #8472889 - Flags: review+
Assignee

Comment 188

5 years ago
- Rebase only.
- Address comment #152: Use const_cast instead.
Attachment #8465360 - Attachment is obsolete: true
Attachment #8472892 - Flags: review+
Assignee

Comment 189

5 years ago
Rebase only
Attachment #8464634 - Attachment is obsolete: true
Attachment #8472895 - Flags: review+
Assignee

Comment 190

5 years ago
- Squash part 4-5.a (Attachment 8449182 [details] [diff]), part 4-5.b (Attachment 8458578 [details] [diff]), part 4-5.c (Attachment 8465365 [details] [diff]) and part 4-5.d (Attachment 8458585 [details] [diff])
- Rebase.
Attachment #8449182 - Attachment is obsolete: true
Attachment #8458578 - Attachment is obsolete: true
Attachment #8458585 - Attachment is obsolete: true
Attachment #8465365 - Attachment is obsolete: true
Attachment #8472897 - Flags: review+
Assignee

Comment 191

5 years ago
Just add r=kchen after r+.
Attachment #8471293 - Attachment is obsolete: true
Attachment #8472903 - Flags: review+
Assignee

Comment 192

5 years ago
Just add r=ferjmoreno after r+.
Attachment #8471297 - Attachment is obsolete: true
Attachment #8472904 - Flags: review+
Assignee

Updated

5 years ago
Depends on: 1053733
Assignee

Comment 193

5 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #183)
> (In reply to Edgar Chen [:edgar][:echen] from comment #169)
> > (In reply to Edgar Chen [:edgar][:echen] from comment #168)
> > > Try result: https://tbpl.mozilla.org/?tree=Try&rev=24e94e5417bf
> > > 
> > > Only B2G ICS emulator is green. :(
> > > 
> > > I am working on fix those error now.
> > 
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=73536f35602d
> > 
> > I have fixed the build error, it can success to build on all platform now.
> > But we still have some failure on test cases, have no idea why, still under
> > investigating:
> > - INFO TEST-UNEXPECTED-FAIL |
> > /tests/toolkit/devtools/apps/tests/test_webapps_actor.html | Test timed out.
> 
> Finally, I found the root cause of the time out in test_webapps_actor.html:
> 
> test_webapps_actor.html relies on remote-debugger, so it tries to enable
> remote-debugger by change settings via SettingService [1] and waits for
> observer event "debugger-server-started" before starting tests.
> 
> In time out case, the settings.js doesn't receive the settings change event
> and the USBRemoteDebugger won't be activated [2], either. So
> test_webapps_actor.html gets stuck on waiting "debugger-server-started"
> event.
> 
> After digging into the code, I found we hit a timing problem between
> SettingsManager [3] and SettingsChangeNotifier [4]. 
> 
> The problem is: During SettingsManager init process [5], it registers it's
> target to SettingsChangeNotifier by sending Settings:RegisterForMessages
> message, but at that time SettingsChangeNotifier might not yet ready to
> receive message (doesn't register listener to ppmm yet [6]). So
> SettingsManager fails to register and is unable to receive any
> Settings:Changed message from SettingsChangeNotifier.
> 
> Note that test_webapps_actor.html now also has chance to get stuck (please
> see bug 918528), moving mobileconnection to ipdl just makes it 100%
> reproducible.
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/apps/tests/
> debugger-protocol-helper.js#21
> [2]
> http://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.
> js#460
> [3]
> http://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsManager.js
> [4]
> http://dxr.mozilla.org/mozilla-central/source/dom/settings/
> SettingsChangeNotifier.jsm
> [5]
> http://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsManager.
> js#368
> [6]
> http://dxr.mozilla.org/mozilla-central/source/dom/settings/
> SettingsChangeNotifier.jsm#35

(In reply to Edgar Chen [:edgar][:echen] from comment #184)
> Created attachment 8472875 [details] [diff] [review]
> ix the time out in test_webapps_actor.html
> 
> Just a quick fix for the timing problem mentioned in comment #183.
> ((Will file a separated bug for it))

Bug 1053733 is filed for this.

Updated

5 years ago
Depends on: 1047196
Assignee

Updated

5 years ago
Duplicate of this bug: 1050140
Assignee

Comment 195

5 years ago
Blocked by bug 1053733 now. (see comment #183)
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Blocks: 1013847
Confirmed with EM/EPM, and this can be landed before FL.
Blocks: 1047196
No longer depends on: 1047196
Assignee

Comment 198

5 years ago
Rebase only.
Attachment #8472887 - Attachment is obsolete: true
Attachment #8479646 - Flags: review+
Assignee

Comment 199

5 years ago
Rebase only.
Attachment #8472889 - Attachment is obsolete: true
Attachment #8479655 - Flags: review+
Assignee

Comment 200

5 years ago
Rebase and fix build error.
Attachment #8472892 - Attachment is obsolete: true
Attachment #8479661 - Flags: review+
Assignee

Comment 201

5 years ago
Rebase and fix build error.
Attachment #8465364 - Attachment is obsolete: true
Attachment #8479663 - Flags: review+
Assignee

Comment 202

5 years ago
Rebase only.
Attachment #8449180 - Attachment is obsolete: true
Attachment #8479664 - Flags: review+
Assignee

Comment 203

5 years ago
Rebase only.
Attachment #8472897 - Attachment is obsolete: true
Attachment #8479667 - Flags: review+
Assignee

Comment 204

5 years ago
Address comment #181.
Attachment #8472256 - Attachment is obsolete: true
Attachment #8479670 - Flags: review+
fwiw, this looks like a pretty large refactoring of a critical api. I think it's a bit to late to land that in 2.1, and I don't see any user facing use case that needs that. 
Even bug 889737 won't be needed in 2.1 since the gaia part is being de-scoped (see https://bugzilla.mozilla.org/show_bug.cgi?id=1031175#c14)
(In reply to Fabrice Desré [:fabrice] from comment #206)
> fwiw, this looks like a pretty large refactoring of a critical api. I think
> it's a bit to late to land that in 2.1, and I don't see any user facing use
> case that needs that. 
> Even bug 889737 won't be needed in 2.1 since the gaia part is being
> de-scoped (see https://bugzilla.mozilla.org/show_bug.cgi?id=1031175#c14)

I don't see any problem of landing a feature before feature landed date. Please don't confuse a devel branch with a stable branch, or no aggressive work can ever be done.
(In reply to Fabrice Desré [:fabrice] from comment #206)
> fwiw, this looks like a pretty large refactoring of a critical api. I think
> it's a bit to late to land that in 2.1, and I don't see any user facing use
> case that needs that. 
> Even bug 889737 won't be needed in 2.1 since the gaia part is being
> de-scoped (see https://bugzilla.mozilla.org/show_bug.cgi?id=1031175#c14)

I agree with you. This is a big patch. But there are 2 benefits to land this patch.
1. Because this is a big patch, that means if we only land these on master, 
devs. need to spend efforts taking care 2 different codes between master and
branch while having the blocking bugs. To land this patch, we can minimize
these efforts.

2. Because there isn't a real user story for user experience, we could focus on
testing gecko side and make sure these changes are stable enough first if they were
found unstable. (Of cause, we have tested these patches. But the test scopes are
different from release tests.)
Then, when we start to develop user experience feature related to these patches
afterwords, we don't need to worry about the stability of gecko.

Indeed, the big patch may create risks for release. But we would like to take
these risks and whole responsibilities for landing these patches. If these patches
aren't well control in branch afterwords, we will back out them as well.   

Sorry for requesting to land these patches in this week. I know this is a tough
decision. But it is 6.5 weeks to FC from now, I think we still have time to stabilize
or backout these patches if we really get into problems on these patches.
If there is no user facing feature that rely on that for 2.1, it makes absolutely no sense to take the risk of having to backout this refactoring on aurora or later on the 2.1 branch. By landing only on master and riding the next train, what do you loose? A few days of coverage?

And indeed, no risky work should land close to a merge date if it's not absolutely needed for the merged version. I also don't care who's taking responsibility, I want a minimal churn on 2.1 to control regressions and stability.
I am kind of agreed with Fabrice actually. But, of course, if developers have high confidence about the quality of these patches, it could be another story. Also, I would like to get product team's feedback if we're going to postpone this to the next version of Firefox OS. Ping Sandip here. Sandip, what do you think? 

And, QAs need to be aware of this change, too, because it would affect their focus and work load on 2.1 testing.
Flags: needinfo?(skamat)
I don't think it is fair to use UI feature to decide if a patch should be landed. Some features are not able to finish in one release. We need an incremental development. This patch is a big step for us for MMI code feature. That's why we add it in 2.1. And then we don't need to have a bigger patch and efforts in 2.2.

Again, if we can not land this, we need to spend efforts maintaining 2 different codes between master and branch. This kind of maintaining efforts are huge for us as well.

We have tested these patches and everything is green on try server, and these patches are ready before FL.
According to current situation, we are confident for quality of this patch. So I would say there is no real reason to forbid us to land this patch. Thanks.
I am totally confused if we aren't allowed to land work on main trunk before FL what is FL supposed to mean?
I think the point fabrice is making is that we're landing a lot of risky stuff already right before FL, so we should avoid landing risky stuff that we don't need so that we don't increase the risk of regressions/etc further.