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

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

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?

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

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

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)

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

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

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?

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

Put this bug into backlog.

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

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

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

(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

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.

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

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

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

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

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

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

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

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

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.

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

Rebase

Rebase

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.

Rebase

Use AutoJSAPI instead.

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.

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

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

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.

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.

Just revise some comments.

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.

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

Comment on attachment 8458514 [details] [diff] [review] Part 3-2: IPDL implementation for MobileConnection, v5 Here is the implementation of PMobileConnection.

Just rebase

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

Rebase only

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

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

Rebase only

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

(In reply to Olli Pettay [:smaug] from comment #114) > Comment on attachment 8456797 [details] [diff] [review] > Part 3-3: MobileConnectionIPCService for content process, v4 > > >+MobileConnectionIPCService::GetDataConnectionInfo(uint32_t aClientId, > >+ nsIMobileConnectionInfo** aInfo) > >+{ > >+ if (aClientId >= mClients.Length()) { > >+ return NS_ERROR_FAILURE; > >+ } > >+ > >+ nsCOMPtr<nsIMobileConnectionInfo> info = mClients[aClientId]->GetDataInfo(); > >+ NS_IF_ADDREF(*aInfo = info); > forget(), not NS_IF_ADDREF > > > >+private: > >+ nsTArray<MobileConnectionChild*> mClients; > I don't understand the lifetime management of mClients. > What guarantees the array doesn't ever contain pointers to deleted objects? Ah, thanks for pointing this out. MobileConnectionIPCService may not aware the MobileConnectionChild was deleted in ContentChild::DeallocPMobileConnectionChild. And the array will contain pointers to deleted objects after that. We could have two different way to solve this: 1. Make MobileConnectionChild reference-counted and let MobileConnectionIPCService hold a reference. 2. Notify MobileConnectionIPCService the child is deleted in MobileConnectionChild::ActorDestroy(). Thank you

Comment on attachment 8458582 [details] [diff] [review] Part 4-5.c: MobileConnectionGonkService for gonk backend, v3 Review of attachment 8458582 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for being late :( Overall structure looks good, thank you! Please remember to invite build peer for the review! ::: dom/mobileconnection/src/gonk/MobileConnectionGonkService.js @@ +192,5 @@ > + return supportedNetworkTypes; > + }, > + > + /** > + * Helper for guarding us again invalid reason values for call forwarding. nit:s/again/against/ @@ +196,5 @@ > + * Helper for guarding us again invalid reason values for call forwarding. > + */ > + _isValidCallForwardingReason: function(aReason) { > + switch (aReason) { > + case RIL.CALL_FORWARD_REASON_UNCONDITIONAL: As the requests are coming from nsIMobileConnectionService, let's use Ci.nsIMobileConnectionService.* though the values should be the same as RIL.* now. @@ +209,5 @@ > + } > + }, > + > + /** > + * Helper for guarding us again invalid action values for call forwarding. ditto. @@ +213,5 @@ > + * Helper for guarding us again invalid action values for call forwarding. > + */ > + _isValidCallForwardingAction: function(aAction) { > + switch (aAction) { > + case RIL.CALL_FORWARD_ACTION_DISABLE: ditto. @@ +228,5 @@ > + * Helper for guarding us against invalid program values for call barring. > + */ > + _isValidCallBarringProgram: function(aProgram) { > + switch (aProgram) { > + case RIL.CALL_BARRING_PROGRAM_ALL_OUTGOING: ditto. @@ +262,5 @@ > + * Helper for guarding us against invalid mode for clir. > + */ > + _isValidClirMode: function(aMode) { > + switch (aMode) { > + case RIL.CLIR_DEFAULT: ditto. @@ +321,5 @@ > + let isUpdated = false; > + for (let key in aSrcInfo) { > + if ((key !== "cell") && (aDestInfo[key] !== aSrcInfo[key])) { > + isUpdated = true; > + aDestInfo[key] = aSrcInfo[key]; This is kinda duplicate of _updateInfo. Can we rewrite as: let isUpdated = falsefor (let key in aScrInfo) { if (key === "cell") { if (aSrcInfo.cell && !aDestInfo.cell) { aDestInfo.cell = new MobileCellInfo(); } } isUpdated = this._updateInfo(aDestInfo[key], aSrcInfo[key]); } @@ +468,5 @@ > + }, > + > + updateSignalInfo: function(aNewInfo, aBatch = false) { > + // If the voice is not registered, need not to update signal information. > + if (this.voiceInfo.state === RIL.GECKO_MOBILE_CONNECTION_STATE_REGISTERED) { I know it's simply copied from the current code, but sometime we check voiceInfo.network (e.g. line 460) and sometimes check voiceInfo.state. Wondering if we do want to have separate conditions? Shouldn't they be align? @@ +474,5 @@ > + this.deliverListenerEvent("notifyVoiceChanged"); > + } > + } > + > + // If the data is not registered, need not to update signal information. nit: s/need not/no need/ @@ +829,5 @@ > + // Checking valid PIN for supplementary services. See TS.22.004 clause 5.2. > + if (aOptions.pin == null || !aOptions.pin.match(/^\d{4}$/) || > + aOptions.newPin == null || !aOptions.newPin.match(/^\d{4}$/)) { > + this._dispatchNotifyError(aCallback, "InvalidPassword"); > + return false; Why return false? I thought the return value of the API is void. ::: dom/mobileconnection/src/moz.build @@ +44,5 @@ > +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk': > + EXTRA_COMPONENTS += [ > + 'gonk/MobileConnectionGonkService.js', > + 'gonk/MobileConnectionGonkService.manifest', > + ] Don't we need to modify dom/mobileconnection/interfaces/moz.build, too?

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

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.

(In reply to Olli Pettay [:smaug] from comment #117) > Comment on attachment 8458514 [details] [diff] [review] > Part 3-2: IPDL implementation for MobileConnection, v5 > > >+ // IPC::MozCallForwardingOptions[] > >+ nsTArray<IPC::MozCallForwardingOptions> infos; > >+ for (uint32_t i = 0; i < length; i++) { > >+ RootedDictionary<IPC::MozCallForwardingOptions> info(cx); > >+ if (!JS_GetElement(cx, object, i, &value) || !info.Init(cx, value)) { > >+ return NS_ERROR_TYPE_ERR; > >+ } > >+ > >+ infos.AppendElement(info); > What keeps 'infos' alive? Only info is rooted. Ah, MozCallForwardingOptions contains no JS stuff (only string and short) [1], so we don't need to use RootedDictionary here actually. Am I right? Thank you. [1] http://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozMobileConnection.webidl?from=MozMobileConnection.webidl&case=true#574

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

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

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