843452 - B2G RIL: use ipdl as IPC in MozMobileConnection

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

To deprecate RILContentHelper, we need to use IPDL to replace current architecture of MozMobileConnection.

Comment 1

[Edgar Chen [:edgar]]

Working branch: https://github.com/EdgarChen/mozilla-central/tree/bug_843452

Comment 2

[Edgar Chen [:edgar]]

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?

Comment 3

[Edgar Chen [:edgar]]

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

Comment 4

[Edgar Chen [:edgar]]

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

Comment 5

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

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)

Comment 6

[Edgar Chen [:edgar]]

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

Comment 7

[Edgar Chen [:edgar]]

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

Comment 8

[Anshul]

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?

Comment 9

[Edgar Chen [:edgar]]

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

Comment 10

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 1-1: WEBIDL changes for MobileConnection IPDL, v1

Comment 12

[Edgar Chen [:edgar]]

Attachment: Part 1-2: Mobile*Info implements both webidl and idl interfaces, v1

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 2-1: MobileConnectionProvider interface changes for IPDL, v1

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 2-2: Bluetooth changes for MobileConnectionProvider interface, v1

Comment 15

[Edgar Chen [:edgar]]

Attachment: Part 2-3: MobileConnection DOM changes for MobileConnectionProvider interface, v1

Comment 16

[Edgar Chen [:edgar]]

Attachment: Part 2-3: MobileConnection DOM changes for MobileConnectionProvider interface, v2

Comment 17

[Edgar Chen [:edgar]]

Attachment: Part 2-1: MobileConnectionProvider interface changes for IPDL, v2

Comment 18

[Edgar Chen [:edgar]]

Attachment: Part 3-1: IPDL for MobileConnection, v1

Comment 19

[Edgar Chen [:edgar]]

Attachment: Part 2-1: MobileConnectionProvider interface changes for IPDL, v3

Comment 20

[Edgar Chen [:edgar]]

Attachment: Part 2-3: MobileConnection DOM changes for MobileConnectionProvider interface, v3

Comment 21

[Edgar Chen [:edgar]]

Attachment: Part 3-1: IPDL for MobileConnection, v2

Comment 22

[Edgar Chen [:edgar]]

Attachment: Part 3-2: IPDL implementation for MobileConnection, v1

Comment 23

[Edgar Chen [:edgar]]

Attachment: Part 3-2: IPDL implementation for MobileConnection, v2

Comment 24

[Edgar Chen [:edgar]]

Attachment: Part 4: MobileConnectionIPCProvider for content process, v1

Comment 25

[Edgar Chen [:edgar]]

Attachment: Part 5-1: Interface changes for gonk backend, v1

Comment 26

[Edgar Chen [:edgar]]

Attachment: Part 5-2: [GPS] Get voiceInfo via MobileConnectionProvider, v1

Comment 27

[Edgar Chen [:edgar]]

Attachment: Part 5-3: [MMS] Get voiceInfo via MobileConnectionProvider, v1

Comment 28

[Edgar Chen [:edgar]]

Attachment: Part 5-4: PhoneNumberUtils changes for MobileConnection ipdl, v1

Comment 29

[Edgar Chen [:edgar]]

Attachment: Part 5-5.a: Refactor otaStatus updating, v1

Comment 30

[Edgar Chen [:edgar]]

Attachment: Part 5-5.b: Combine GECKO_RADIOSTATE_* and GECKO_DETAILED_RADIOSTATE_*, v1

Comment 32

[Edgar Chen [:edgar]]

Attachment: Part 4: MobileConnectionIPCProvider for content process, v2

Comment 33

[Edgar Chen [:edgar]]

Attachment: Part 5-5.c: MobileConnectionGonkProvider for gonk backend, v1

Comment 34

[Edgar Chen [:edgar]]

Attachment: Part 5-5.d: gRadioEnabledController changes, v1

Comment 35

[Edgar Chen [:edgar]]

Attachment: Part 1-1: WEBIDL changes for MobileConnection IPDL, v2

Comment 36

[Edgar Chen [:edgar]]

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.

Comment 37

[Edgar Chen [:edgar]]

Attachment: Part 1-2: Mobile*Info implements both webidl and idl interfaces, v2

Comment 38

[Edgar Chen [:edgar]]

Attachment: Part 1-2: Mobile*Info implements both webidl and idl interfaces, v3

Comment 39

[Edgar Chen [:edgar]]

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.

Comment 40

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

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?

Comment 41

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

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 42

[Edgar Chen [:edgar]]

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

Comment 43

[Edgar Chen [:edgar]]

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

[pgravel]

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.

Comment 45

[Edgar Chen [:edgar]]

(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

[pgravel]

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.

Comment 47

[Edgar Chen [:edgar]]

Attachment: Part 1-1: WEBIDL changes for MobileConnection IPDL, v3, r=hsinyi,smaug

1). Rebase
2). Address review comment #40 and comment #41,
    - Keep the old name: MozMMIResult

Comment 48

[Edgar Chen [:edgar]]

Attachment: Part 2-1.a: Rename MobileConnectionProvider to MobileConnectionService, v1

Comment 49

[Edgar Chen [:edgar]]

Attachment: Part 2-1.b: MobileConnectionService interface changes for IPDL, v4

Comment 50

[Edgar Chen [:edgar]]

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.

Comment 51

[Edgar Chen [:edgar]]

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.

Comment 52

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

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.

Comment 53

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

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

Comment 54

[Edgar Chen [:edgar]]

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

Comment 55

[Edgar Chen [:edgar]]

Attachment: Part 2-1.b: MobileConnectionService interface changes for IPDL, v5

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/

Comment 56

[Edgar Chen [:edgar]]

Attachment: Part 2-2: Bluetooth changes for MobileConnectionService interface, v2

Comment 57

[Edgar Chen [:edgar]]

Attachment: Part 2-3: MobileConnection DOM changes for MobileConnectionService interface, v4

Comment 58

[Edgar Chen [:edgar]]

Attachment: Part 3-1: IPDL for MobileConnection, v3

Comment 59

[Edgar Chen [:edgar]]

Attachment: Part 3-2: IPDL implementation for MobileConnection, v3

Comment 60

[Edgar Chen [:edgar]]

Attachment: Part 3-3: MobileConnectionIPCService for content process, v3

Comment 61

[Edgar Chen [:edgar]]

Attachment: Part 4-1: Interface changes for gonk backend, v2

Comment 62

[Edgar Chen [:edgar]]

Attachment: Part 4-2: [GPS] Get voiceInfo via MobileConnectionService, v2

Comment 63

[Edgar Chen [:edgar]]

Attachment: Part 4-3: [MMS] Get voiceInfo via MobileConnectionService, v2

Comment 64

[Edgar Chen [:edgar]]

Attachment: Part 4-4: PhoneNumberUtils changes for MobileConnection ipdl, v2

Comment 65

[Edgar Chen [:edgar]]

Attachment: Part 4-5.a: Refactor otaStatus updating, v2

Comment 66

[Edgar Chen [:edgar]]

Attachment: Part 4-5.b: Combine GECKO_RADIOSTATE_* and GECKO_DETAILED_RADIOSTATE_*, v2

Comment 67

[Edgar Chen [:edgar]]

Attachment: Part 4-5.c: MobileConnectionGonkService for gonk backend, v2

Comment 68

[Edgar Chen [:edgar]]

Attachment: Part 4-5.d: gRadioEnabledController changes, v2

Comment 69

[Edgar Chen [:edgar]]

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

Comment 70

[Kai-Chih Hu [:khu]]

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?

Comment 71

[Edgar Chen [:edgar]]

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

Comment 72

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

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

Comment 73

[Edgar Chen [:edgar]]

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

Comment 74

[Edgar Chen [:edgar]]

(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

[Anshul]

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.

Comment 76

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

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.

Comment 77

[Edgar Chen [:edgar]]

Attachment: Part 2-1.b: MobileConnectionService interface changes for IPDL, v6

Comment 78

[Edgar Chen [:edgar]]

Attachment: Part 2-3: MobileConnection DOM changes for MobileConnectionService interface, v5

Comment 79

[Edgar Chen [:edgar]]

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.

Comment 80

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

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.

Comment 81

[Edgar Chen [:edgar]]

Attachment: Part 2-1.a: Rename MobileConnectionProvider to MobileConnectionService, v2, r=hsinyi

Rebase

Comment 82

[Edgar Chen [:edgar]]

Attachment: Part 2-2: Bluetooth changes for MobileConnectionService interface, v3

Rebase

Comment 83

[Edgar Chen [:edgar]]

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.

Comment 84

[Edgar Chen [:edgar]]

Attachment: Part 2-3: MobileConnection DOM changes for MobileConnectionService interface, v6

Rebase

Comment 85

[Edgar Chen [:edgar]]

Attachment: Part 2-3: MobileConnection DOM changes for MobileConnectionService interface, v7

Use AutoJSAPI instead.

Comment 86

[Edgar Chen [:edgar]]

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.

Comment 87

[Edgar Chen [:edgar]]

Attachment: Part 3-1: IPDL for MobileConnection, v4

1). Address comment #72
    - no ns prefix, but put it IPC namespace.
2). Address comment #74
    - Combine all synced protocol into one.

Comment 88

[Edgar Chen [:edgar]]

Attachment: Part 3-2: IPDL implementation for MobileConnection, v4

Comment 89

[Edgar Chen [:edgar]]

Attachment: Part 3-3: MobileConnectionIPCService for content process, v4

Comment 90

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

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.

Comment 91

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

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.

Comment 92

[Edgar Chen [:edgar]]

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.

Comment 93

[Edgar Chen [:edgar]]

Attachment: Part 2-3: MobileConnection DOM changes for MobileConnectionService interface, v8

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.

Comment 94

[Edgar Chen [:edgar]]

Attachment: Part 3-1: IPDL for MobileConnection, v5

Just revise some comments.

Comment 95

[Edgar Chen [:edgar]]

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.

Comment 96

[Edgar Chen [:edgar]]

Attachment: Part 3-2: IPDL implementation for MobileConnection, v5

Changes since v4,
- exception handling
- don't need RootedDictionary if no JS stuff
- Add some comments about child/parent actor

Comment 97

[Edgar Chen [:edgar]]

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

Here is the implementation of PMobileConnection.

Comment 98

[Edgar Chen [:edgar]]

Attachment: Part 4-1: Interface changes for gonk backend, v3

Just rebase

Comment 99

[Edgar Chen [:edgar]]

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.

Comment 100

[Edgar Chen [:edgar]]

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

Comment 101

[Edgar Chen [:edgar]]

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.

Comment 102

[Edgar Chen [:edgar]]

Attachment: Part 4-4: PhoneNumberUtils changes for MobileConnection ipdl, v3

Rebase only

Comment 103

[Kan-Ru Chen [:kanru] (UTC+9)]

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

Comment 104

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

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?

Comment 105

[Edgar Chen [:edgar]]

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

Comment 106

[Edgar Chen [:edgar]]

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

Comment 107

[Edgar Chen [:edgar]]

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

Comment 108

[Edgar Chen [:edgar]]

Attachment: Part 4-5.b: Combine GECKO_RADIOSTATE_* and GECKO_DETAILED_RADIOSTATE_*, v3

Rebase only

Comment 109

[Edgar Chen [:edgar]]

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))

Comment 110

[Edgar Chen [:edgar]]

Attachment: Part 4-5.c: MobileConnectionGonkService for gonk backend, v3

Comment 111

[Edgar Chen [:edgar]]

Attachment: Part 4-5.d: gRadioEnabledController changes, v3

Comment 112

[Edgar Chen [:edgar]]

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

Comment 113

[Edgar Chen [:edgar]]

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.

Comment 114

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

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?

Comment 115

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

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

Comment 116

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

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() ?

Comment 117

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

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

Comment 118

[Edgar Chen [:edgar]]

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

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

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?

Comment 120

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

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

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

Thank you!

Comment 121

[Edgar Chen [:edgar]]

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.

Comment 122

[Edgar Chen [:edgar]]

(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

Comment 123

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

In that case Rooting isn't indeed needed, right.

Comment 124

[Edgar Chen [:edgar]]

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

Comment 125

[Edgar Chen [:edgar]]

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

Comment 126

[Edgar Chen [:edgar]]

Attachment: Part 2-1: Rename MobileConnectionProvider to MobileConnectionService and redesign for IPDL, v7, r=hsinyi

- Squash part 2-1.a (Attachment 8456046 [details] [diff]) and part 2-1.b (Attachment 8455231 [details] [diff])
- Rebase

Comment 127

[Edgar Chen [:edgar]]

Attachment: Part 2-2: Bluetooth changes for MobileConnectionService interface, v4, r=echou

Rebase

Comment 128

[Edgar Chen [:edgar]]

Attachment: Part 2-3: MobileConnection DOM changes for MobileConnectionService interface, v9, r=smaug

Address review comment #115:
- Use EmptyString() instead.

Comment 129

[Edgar Chen [:edgar]]

Attachment: Part 3-1: IPDL for MobileConnection, v6

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.

Comment 130

[Edgar Chen [:edgar]]

Attachment: Part 3-2: IPDL implementation for MobileConnection, v6

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.

Comment 131

[Edgar Chen [:edgar]]

Attachment: Part 3-3: MobileConnectionIPCService for content process, v5

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.

Comment 132

[Edgar Chen [:edgar]]

Attachment: Part 4-1: Interface changes for gonk backend, v4, r=hsinyi

Comment 133

[Edgar Chen [:edgar]]

Attachment: Part 4-2: [GPS] Get voiceInfo via MobileConnectionService, v3, r=kchen

Address review comment #103:
- Sort #incldue in alphabetical order
- Address nit

Comment 134

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

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);

Comment 135

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

Er, MOZ_ASSERT(IsDOMBinding());
Though, I can't recall now whether we even call that method if IsDOMBinding() returns false.

Comment 136

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

Comment on attachment 8464549 [details] [diff] [review]
Part 3-1: IPDL for MobileConnection, v6

># HG changeset patch
># User Edgar Chen <[e-mail]>
># 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'

Comment 137

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

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

Comment 138

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

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.

Comment 139

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

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);
>+}

Comment 140

[Edgar Chen [:edgar]]

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

Comment 141

[Edgar Chen [:edgar]]

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

Comment 142

[Edgar Chen [:edgar]]

(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**)

Comment 143

[Edgar Chen [:edgar]]

(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**)'

Comment 144

[Edgar Chen [:edgar]]

(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);
> >+}

Comment 145

[Edgar Chen [:edgar]]

Attachment: Part 3-1: IPDL for MobileConnection, v7, r=smaug

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

Comment 146

[Edgar Chen [:edgar]]

Attachment: Part 3-2: IPDL implementation for MobileConnection, v7, r=smaug

Address review comment #138:
- Don't need IndexOf before RemoveObject
- Please see comment #143
- * goes with the type
- Add check for mService
- JS_ClearPendingException()

Comment 147

[Edgar Chen [:edgar]]

Attachment: Part 3-3: MobileConnectionIPCService for content process, v6

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))

Comment 148

[Edgar Chen [:edgar]]

Attachment: Part 4-5.c: MobileConnectionGonkService for gonk backend, v4

Address review comment #119.

Comment 149

[Edgar Chen [:edgar]]

Attachment: Squashed patch

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

Comment 150

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

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

Comment 151

[Edgar Chen [:edgar]]

(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**)'

Comment 152

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

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 153

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

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.

Comment 154

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

Possible changes to ownership model should be done in a followup bug/patch.

Comment 155

[pgravel]

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.

Comment 156

[Sandip Kamat]

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)

Comment 157

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

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

Comment 158

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

Bug 1047196 filed as follow up to comment 44 and comment 46.

Comment 159

[Edgar Chen [:edgar]]

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

Comment 160

[Edgar Chen [:edgar]]

(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

Comment 161

[Edgar Chen [:edgar]]

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.

Comment 162

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

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

Comment 164

[Edgar Chen [:edgar]]

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.

Comment 165

[Edgar Chen [:edgar]]

Attachment: Part 5: Changes for xpcshell test

Comment 166

[Edgar Chen [:edgar]]

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.

Comment 167

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 168

[Edgar Chen [:edgar]]

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

Only B2G ICS emulator is green. :(

I am working on fix those error now.

Comment 169

[Edgar Chen [:edgar]]

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

Comment 170

[Edgar Chen [:edgar]]

Attachment: Part 4-6: [NetworkManager] Get dataInfo via MobileConnectionService, v1

Comment 171

[Edgar Chen [:edgar]]

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.

Comment 172

[Edgar Chen [:edgar]]

Attachment: Part 4-7: [NetworkGeolocationProvider] Get dataInfo via MobileConnectionService, v1

Comment 173

[Edgar Chen [:edgar]]

Attachment: Part 4-8: [MobileIdentityManager] Get {voice|data}Info via MobileConnectionService, v1

Comment 174

[Edgar Chen [:edgar]]

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.

Comment 175

[Edgar Chen [:edgar]]

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.

Comment 176

[Edgar Chen [:edgar]]

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.

Comment 177

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 178

[Edgar Chen [:edgar]]

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.

Comment 179

[Edgar Chen [:edgar]]

Attachment: Part 6: Build MobileConnection DOM/IPC by default, v1

Comment 180

[Edgar Chen [:edgar]]

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

Comment 181

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

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?

Comment 182

[Edgar Chen [:edgar]]

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

Comment 183

[Edgar Chen [:edgar]]

(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

Comment 184

[Edgar Chen [:edgar]]

Attachment: 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))

Comment 185

[Edgar Chen [:edgar]]

Attachment: Part 2-2: Bluetooth changes for MobileConnectionService interface, v5, r=echou

Rebase only.

Comment 186

[Edgar Chen [:edgar]]

Attachment: Part 2-3: MobileConnection DOM changes for MobileConnectionService interface, v10, r=smaug

Rebase only.

Comment 187

[Edgar Chen [:edgar]]

Attachment: Part 3-1: IPDL for MobileConnection, v8, r=smaug

Rebase only.

Comment 188

[Edgar Chen [:edgar]]

Attachment: Part 3-2: IPDL implementation for MobileConnection, v8, r=smaug

- Rebase only.
- Address comment #152: Use const_cast instead.

Comment 189

[Edgar Chen [:edgar]]

Attachment: Part 4-2: [GPS] Get voiceInfo via MobileConnectionService, v4, r=kchen

Rebase only

Comment 190

[Edgar Chen [:edgar]]

Attachment: Part 4-5: MobileConnectionGonkService for gonk backend, v5, r=hsinyi,khuey

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

Comment 191

[Edgar Chen [:edgar]]

Attachment: Part 4-7: [NetworkGeolocationProvider] Get dataInfo via MobileConnectionService, v1, r=kchen

Just add r=kchen after r+.

Comment 192

[Edgar Chen [:edgar]]

Attachment: Part 4-8: [MobileIdentityManager] Get {voice|data}Info via MobileConnectionService, v1, r=ferjmoreno

Just add r=ferjmoreno after r+.

Comment 193

[Edgar Chen [:edgar]]

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

Comment 195

[Edgar Chen [:edgar]]

Blocked by bug 1053733 now. (see comment #183)

Comment 196

[Kai-Chih Hu [:khu]]

Confirmed with EM/EPM, and this can be landed before FL.

Comment 197

[Edgar Chen [:edgar]]

Attachment: Part 2-2: Bluetooth changes for MobileConnectionService interface, v6, r=echou

Rebase only.

Comment 198

[Edgar Chen [:edgar]]

Attachment: Part 2-3: MobileConnection DOM changes for MobileConnectionService interface, v11, r=smaug

Rebase only.

Comment 199

[Edgar Chen [:edgar]]

Attachment: Part 3-1: IPDL for MobileConnection, v9, r=smaug

Rebase only.

Comment 200

[Edgar Chen [:edgar]]

Attachment: Part 3-2: IPDL implementation for MobileConnection, v9, r=smaug

Rebase and fix build error.

Comment 201

[Edgar Chen [:edgar]]

Attachment: Part 3-3: MobileConnectionIPCService for content process, v7, r=smaug

Rebase and fix build error.

Comment 202

[Edgar Chen [:edgar]]

Attachment: Part 4-3: [MMS] Get voiceInfo via MobileConnectionService, v3, r=vicamo

Rebase only.

Comment 203

[Edgar Chen [:edgar]]

Attachment: Part 4-5: MobileConnectionGonkService for gonk backend, v6, r=hsinyi,khuey

Rebase only.

Comment 204

[Edgar Chen [:edgar]]

Attachment: Part 6: Build MobileConnection DOM/IPC by default, v2, r=smaug,khuey

Address comment #181.

Comment 205

[Edgar Chen [:edgar]]

Full try: https://tbpl.mozilla.org/?tree=Try&rev=ec63a52368d3

Comment 206

[[:fabrice] Fabrice Desré]

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)

Comment 207

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

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

Comment 208

[Ken Chang[:kenkai]]

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

Comment 209

[[:fabrice] Fabrice Desré]

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.

Comment 210

[Kai-Chih Hu [:khu]]

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.

Comment 211

[Ken Chang[:kenkai]]

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.

Comment 212

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

I am totally confused if we aren't allowed to land work on main trunk before FL what is FL supposed to mean?

Comment 213

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 214

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/pushloghtml?changeset=e72a0297d488

Comment 215

[[:fabrice] Fabrice Desré]

(In reply to Edgar Chen [:edgar][:echen] from comment #214)
> https://hg.mozilla.org/integration/b2g-inbound/
> pushloghtml?changeset=e72a0297d488

Please back this out.

Comment 216

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out https://hg.mozilla.org/integration/b2g-inbound/rev/de824c1ec349

Comment 217

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

(In reply to Fabrice Desré [:fabrice] from comment #215)
> (In reply to Edgar Chen [:edgar][:echen] from comment #214)
> > https://hg.mozilla.org/integration/b2g-inbound/
> > pushloghtml?changeset=e72a0297d488
> 
> Please back this out.

Fine. I think this will definitely mean something in the future.

Comment 218

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

Hey, everybody wants a stable release. That's why we have been working so hard and closely with other team on testing and verifying this huge patch to ensure everything is perfect on try and on devices. As the sheriff process started, the sheriff's decision theoretically should be be taken. I don't wanna argue that. However, my question is still not answered – if we aren't allowed to land work on main trunk before FL, what is FL supposed to mean? Since the first day of v2.1, “FL” is the only milestone of feature landing, that is the only information what teams rely on for planing and proceeding. It's really sad that the sheriff policy suddenly happened this week, and isn't well-communicated with teams and developers in advance. If we believe it benefits that sheriff process starts earlier before FL, it's fine. But I also believe, to help teams better plan their committed features for the next release, improvement in the communication and the whole management thing is needed, such as 1) Clarify milestones of each stage and clarify when the sheriff process will start before feature commitment, and 2) Clarify the acceptance criteria of acquiring the sheriff approval: what kind of work can land or not.

Comment 219

[[:fabrice] Fabrice Desré]

Hsin-Yi,
It was conveyed in a driver's meeting that we wanted to avoid large, potentially risky landings in the last week before FL, especially if they are not blocking. If the gaia parts that initially were blocked on this bug had stayed as a 2.1 feature, things would have been different.

Please understand that this is not a judgement on the work that has been done there. No one denies that datelines were reached, no one claims this patch breaks the world.

Comment 220

[Kai-Chih Hu [:khu]]

Next version, let's have better and clear definition about this: when should we stop landing, and have sheriff to be involved in the decision making.

Comment 221

[Edgar Chen [:edgar]]

Attachment: Part 1-1: WEBIDL changes for MobileConnection IPDL, v4, r=hsinyi,smaug

Rebase only

Comment 222

[Edgar Chen [:edgar]]

Attachment: Part 1-2: Mobile*Info implements both webidl and idl interfaces, v4, r=smaug

Rebase only

Comment 223

[Edgar Chen [:edgar]]

Attachment: Part 2-3: MobileConnection DOM changes for MobileConnectionService interface, v12, r=smaug

Rebase only

Comment 224

[Edgar Chen [:edgar]]

Attachment: Part 3-1: IPDL for MobileConnection, v10, r=smaug

Rebase only.

Comment 225

[Edgar Chen [:edgar]]

Attachment: Part 3-2: IPDL implementation for MobileConnection, v10, r=smaug

Rebase only.

Comment 226

[Edgar Chen [:edgar]]

Attachment: Part 3-3: MobileConnectionIPCService for content process, v8, r=smaug

Rebase only.

Comment 227

[Edgar Chen [:edgar]]

Attachment: Part 4-3: [MMS] Get voiceInfo via MobileConnectionService, v4, r=vicamo

Rebase only.

Comment 228

[Edgar Chen [:edgar]]

Attachment: Part 4-5: MobileConnectionGonkService for gonk backend, v7, r=hsinyi,khuey

Rebase only.

Comment 229

[Edgar Chen [:edgar]]

Attachment: Part 5: Changes for xpcshell test, v2, r=hsinyi

Rebase

Comment 230

[Edgar Chen [:edgar]]

Attachment: Part 6: Build MobileConnection DOM/IPC by default, v3, r=smaug,khuey

Rebase only.

Comment 231

[Edgar Chen [:edgar]]

Try server again:
https://tbpl.mozilla.org/?tree=Try&rev=f7086dbbc796
https://tbpl.mozilla.org/?tree=Try&rev=29e9f8c6ff13

Comment 232

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/pushloghtml?changeset=cfd71a5a178e

Comment 233

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5cecf9d2750
https://hg.mozilla.org/mozilla-central/rev/7e867b1da3c7
https://hg.mozilla.org/mozilla-central/rev/196f37f1b35e
https://hg.mozilla.org/mozilla-central/rev/3468cd2bbf59
https://hg.mozilla.org/mozilla-central/rev/c6896f655e86
https://hg.mozilla.org/mozilla-central/rev/df5f41ac3cb9
https://hg.mozilla.org/mozilla-central/rev/2202544e8831
https://hg.mozilla.org/mozilla-central/rev/d390304bad9d
https://hg.mozilla.org/mozilla-central/rev/3b97cf01b88a
https://hg.mozilla.org/mozilla-central/rev/40b544b38924
https://hg.mozilla.org/mozilla-central/rev/580e30e552d4
https://hg.mozilla.org/mozilla-central/rev/7804996432d6
https://hg.mozilla.org/mozilla-central/rev/9dbd60f38f65
https://hg.mozilla.org/mozilla-central/rev/328c344675c5
https://hg.mozilla.org/mozilla-central/rev/c081a875e554
https://hg.mozilla.org/mozilla-central/rev/f956aff0bd99
https://hg.mozilla.org/mozilla-central/rev/79222e32c3a1
https://hg.mozilla.org/mozilla-central/rev/cfd71a5a178e

Comment 234

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

Congrats, Edgar!!! And thank you so much for the great effort. \o/

Comment 235

[Sandip Kamat]

Great job, thx guys. clearing NI.

Comment 237

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

setting feature-b2g:2.2+ for already landed.