1063304 - B2G RIL: eliminate 'clientId' argument usage in nsIMobileConnectionService interface

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

+++ This bug was initially created as a clone of Bug #843452 +++

Currently most methods in nsIMobileConnectionService have following signature:

  void doFoo(in unsigned long serviceId, ...);

I'd like to align these to DOM WebIDL interface by creating an interface, nsIMobileConnection, for each entity.

Comment 1

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

Attachment: part 1/3: rename MobileConnectionGonkService.js to MobileConnectionService.js

Sync naming with other IPDL-based RIL components -- SMS & Telephony.

1) For JS chrome service, it's named "FooService.js" without "Gonk" prefix,
2) GONK_FOOSERVICE_CONTRACTID & GONK_FOOSERVICE_CID, "@mozilla.org/foo/gonkfooservice;1"
3) nsIGonkMobileConnectionService for interface name.

Comment 2

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

Attachment: part 2/3: export header files in right namespace

1) export only DOM components in mozilla::dom namespace, and move utility classes into mozilla::dom::mobileconnection,

2) the preprocessor guard should reflect the namespace of the content. For example, use mozilla_dom_mobileconnection_MobileConnectionCallback_h for mozilla::dom::mobileconnection::MobileConnectionCallback

3) we don't really have to export MobileConnectionCallback because it's only referenced in dom/mobileconnection only.

Comment 3

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

Attachment: part 3.a/3: add nsIMobileConnection

Let nsIMobileConnectionService owns several instances of nsIMobileConnection and move all service ID depending methods, attributes to nsIMobileConnection.

This patch is only about the interface changes.

Comment 4

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

Attachment: part 3.b/3: DOM/IPC implementation

1) Let MobileConnectionChild implement nsIMobileConnection and MobileConnectionIPCService owns several MobileConnectionChild instances as before.

2) mozilla::dom::MobileConnection owns a reference to a nsIMobileConnection instance, rather than a nsIMobileConnectionService instance.

3) instantiate a MobileConnectionArray owned mozilla::dom::MobileConnection instance only when referenced rather than instantiate all of them at once.

4) To hide more implementation details, don't read "ril.numRadioInterfaces" in ctor of MobileConnectionArray. Instead, use newly added nsIMobileConnectionService::GetLength() call.

5) change the GetSupportedNetworkTypes prototype to

  void getSupportedNetworkTypes([array, size_is(length)] out wstring types,
                                [retval] out unsigned long length);

This way we may avoid nsIVariant usage and therefore usage of JS API. For the necessity for that wstring type, see bug 878533 comment 53 to bug 878533 comment 57.

6) owning a nsIMobileConnection reference in MobileConnectionRequestParent rather a nsIMobileConnectionService ref.

Comment 5

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

Attachment: part 3.c/3: accommodate other components

Comment 6

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

Attachment: part 3.d/3: Gonk JS service fixes

1) define MobileConnectionProvider.QueryInterface,

2) add an nsIMobileConnectionInfo implementing class MobileConnectionInfo to replace the plain js object used for voiceInfo and dataInfo.

3) rename 'voiceInfo' to 'voice', 'dataInfo' to 'data', and 'networkSelectMode' to 'networkSelectionMode' as defined in the IDL.

3) remove some duplicated lines originally added for accessing correct provider.

Comment 7

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

Attachment: part 3.b/3: DOM/IPC implementation

Complete nsIMobileConnection::GetSupportedNetworkTypes().

Comment 8

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

Marionette tests passed. Try all: https://tbpl.mozilla.org/?tree=Try&rev=1775c25a2ea1

Comment 9

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

Attachment: part 3.b/3: DOM/IPC implementation

Fix OOP process crash & build failure in platforms.

Comment 10

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

Try again: https://tbpl.mozilla.org/?tree=Try&rev=3bf277eb0c95

Comment 11

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

Attachment: part 3.b/3: DOM/IPC implementation

Comment 12

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

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

Comment 13

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

Attachment: part 2/3: export header files in right namespace

Move IPC classes into mozilla::dom::mobileconnection as well.

Comment 14

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

Bug 833229 needs a way to retrieve number of mobileconnection instances correctly. I don't want any other component to access "ril.numRadioInterfaces" but mobileconnection itself. All others should call to some interface method of nsIMobileConnectionService, and it is nsIMobileConnectionService::GetLength with the patches here.

Bug 1038606 has to implement nsIMobileConnectionService for simulator, so I'd like to implement the one that's not going to change a lot any more.

Comment 15

[Edgar Chen [:edgar]]

Comment on attachment 8485666 [details] [diff] [review]
part 2/3: export header files in right namespace

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

r=me with below comments addressed.

Thank you.

::: dom/mobileconnection/MobileConnectionCallback.cpp
@@ +321,5 @@
>  }
> +
> +} // namespace mobileconnection
> +} // name space dom
> +} // name space mozilla

nit: s/name space/namespace/

::: dom/mobileconnection/MobileConnectionCallback.h
@@ +77,2 @@
>  } // name space dom
>  } // name space mozilla

Please help to correct above two comments as well
s/name space/namespace/
Thank you.

Comment 16

[Edgar Chen [:edgar]]

Comment on attachment 8485390 [details] [diff] [review]
part 3.a/3: add nsIMobileConnection

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

Looks good to me, but I would like to invite Hsinyi for the review.
Thank you.

Comment 17

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

Comment on attachment 8485390 [details] [diff] [review]
part 3.a/3: add nsIMobileConnection

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

::: dom/mobileconnection/interfaces/nsIMobileConnectionService.idl
@@ +205,3 @@
>  interface nsIMobileConnectionService : nsISupports
>  {
> +  readonly attribute unsigned long length;

I like the idea of exposing the length information here, but nsIMobileConnectionsSevice.length looks ambiguous to me. Do you think "numItems" clearer?

Comment 18

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

(In reply to Hsin-Yi Tsai (OOO Sep. 12) [:hsinyi] from comment #17)
> Comment on attachment 8485390 [details] [diff] [review]
> part 3.a/3: add nsIMobileConnection
> 
> Review of attachment 8485390 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobileconnection/interfaces/nsIMobileConnectionService.idl
> @@ +205,3 @@
> >  interface nsIMobileConnectionService : nsISupports
> >  {
> > +  readonly attribute unsigned long length;
> 
> I like the idea of exposing the length information here, but
> nsIMobileConnectionsSevice.length looks ambiguous to me. Do you think
> "numItems" clearer?

Will do.

Comment 19

[Edgar Chen [:edgar]]

Comment on attachment 8485395 [details] [diff] [review]
part 3.c/3: accommodate other components

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

::: dom/phonenumberutils/PhoneNumberUtils.jsm
@@ +54,5 @@
>      let clientId = 0;
>  
>      // Get network mcc
> +    let connection = mobileConnection.getItemByServiceId(clientId);
> +    let voice = connection.voice;

let voice = connection && connection.voice;

::: dom/system/gonk/NetworkManager.js
@@ +859,5 @@
>    setupDunConnection: function() {
>      this.dunRetryTimer.cancel();
> +    let connection =
> +      gMobileConnectionService.getItemByServiceId(this._dataDefaultServiceId);
> +    let data = connection.data;

let data = connection && connection.data;

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +585,5 @@
>      },
>  
>      _isValidStateForSetRadioEnabled: function(clientId) {
> +      let radioState =
> +        gMobileConnectionService.getItemByServiceId(clientId).radioState;

Please check |connection| like other components do.

let connection = gMobileConnectionService.getItemByServiceId(clientId);
let radioState = connection && connection.radioState;

here and elsewhere

::: services/mobileid/MobileIdentityManager.jsm
@@ +128,5 @@
>        }
>  
> +      let connection = mobileConnectionService.getItemByServiceId(i);
> +      let voice = connection.voice;
> +      let data = connection.data;

and here

Comment 20

[Edgar Chen [:edgar]]

Comment on attachment 8485396 [details] [diff] [review]
part 3.d/3: Gonk JS service fixes

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

Thank you. :)

::: dom/mobileconnection/gonk/MobileConnectionService.js
@@ +510,5 @@
>      this.radioState = aRadioState;
>      this.deliverListenerEvent("notifyRadioStateChanged");
>    },
>  
> +  getSupportedNetworkTypes: function(types) {

nit: s/types/aTypes/

@@ +1008,5 @@
>  
>    /**
>     * nsIMobileConnectionService interface.
>     */
> +  get length() {

Please remember to address comment #17. :)

Comment 21

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

Comment on attachment 8485390 [details] [diff] [review]
part 3.a/3: add nsIMobileConnection

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

Waiting for a new version based on comment 18 :)

Comment 22

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

Attachment: part 1/3: rename MobileConnectionGonkService.js to MobileConnectionService.js : v2

Update r=echen only.

Comment 23

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

Attachment: part 2/3: export header files in right namespace : v2

1) rebase after bug 994190.

2) address review comment 15 and update commit summary.

Comment 24

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

Attachment: part 3.a/3: add nsIMobileConnection : v2

Address comment 17.

Comment 25

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

Attachment: part 3.b/3: DOM/IPC implementation : v2

Address comment 15 and comment 17.

Comment 26

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

Attachment: part 3.c/3: accommodate other components : v2

Address review comment 19.

Comment 27

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

Attachment: part 3.d/3: Gonk JS service fixes : v2

Address comment 17 and comment 20.

Comment 28

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

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

Comment 29

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

Comment on attachment 8489268 [details] [diff] [review]
part 3.c/3: accommodate other components : v2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ -598,5 @@
>  
>      _handleMessage: function(message) {
>        if (DEBUG) debug("RadioControl: handleMessage: " + JSON.stringify(message));
>        let clientId = message.clientId || 0;
> -      let radioInterface = _ril.getRadioInterface(clientId);

Never used in this function. Removed.

@@ -1488,5 @@
>      }
>  
> -    if (gRadioEnabledController.isDeactivatingDataCalls() ||
> -        radioState == RIL.GECKO_RADIOSTATE_ENABLING ||
> -        radioState == RIL.GECKO_RADIOSTATE_DISABLING) {

The radioState is always GECKO_RADIOSTATE_ENABLED here.

Comment 30

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

Comment on attachment 8489265 [details] [diff] [review]
part 3.a/3: add nsIMobileConnection : v2

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

Thank you~

Comment 31

[Edgar Chen [:edgar]]

Comment on attachment 8489268 [details] [diff] [review]
part 3.c/3: accommodate other components : v2

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

Theoretically, we need other module peer's review, but the changes here are trivial, so r=me.
Thank you. :)

Comment 32

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

Flash Gecko & Gaia only into Flame with v123 Gonk ok. Try all before push: https://tbpl.mozilla.org/?tree=Try&rev=c8d6811ae990

Comment 33

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

https://hg.mozilla.org/integration/b2g-inbound/rev/5d8e56deff5a
https://hg.mozilla.org/integration/b2g-inbound/rev/eed2f616bc3e
https://hg.mozilla.org/integration/b2g-inbound/rev/32f152567996
https://hg.mozilla.org/integration/b2g-inbound/rev/11f7b3142762
https://hg.mozilla.org/integration/b2g-inbound/rev/9ff2889f236a
https://hg.mozilla.org/integration/b2g-inbound/rev/251ec0478b72

Comment 34

[Ed Morley [:emorley]]

Please can you land a followup to bump UUIDs? This needed a clobber due to their omission. Thanks :-)

Comment 35

[Ed Morley [:emorley]]

Also please can the reviewers take extra care to watch out for UUID bumps that might be required :-)

Comment 36

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

https://hg.mozilla.org/mozilla-central/rev/5d8e56deff5a
https://hg.mozilla.org/mozilla-central/rev/eed2f616bc3e
https://hg.mozilla.org/mozilla-central/rev/32f152567996
https://hg.mozilla.org/mozilla-central/rev/11f7b3142762
https://hg.mozilla.org/mozilla-central/rev/9ff2889f236a
https://hg.mozilla.org/mozilla-central/rev/251ec0478b72

Comment 37

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

Attachment: bug-1063304-follow-up-uuid.patch

Hi, any way we may know in advance that tbpl has to be explicitly clobbered?

Comment 38

[Ed Morley [:emorley]]

Comment on attachment 8490565 [details] [diff] [review]
bug-1063304-follow-up-uuid.patch

Whenever an IDL interface is changed the UUIDs of that interface and often others will need updating. See the script here for helping figure out which:
http://blog.mozilla.org/sfink/2011/03/02/updating-uuids/

I'm not a peer of this code, so will leave the review to someone else :-)

Comment 39

[Carsten Book [:Tomcat]]

backed this out since this might have caused ongoing frequent cppunit tests like https://tbpl.mozilla.org/php/getParsedLog.php?id=48267235&tree=B2g-Inbound

also i guess the uuid change still have to land

where still is mentioned 15:30:26     INFO -  JavaScript strict warning: jar:file:///system/b2g/omni.ja!/components/MobileConnectionService.js, line 340: ReferenceError: reference to undefined property aDestInfo[key]
15:30:26     INFO -  JavaScript strict warning: , line 0: ReferenceError: reference to undefined property "QueryInterface"
15:30:26     INFO -  [217] WARNING: Trying to SendCommand() without a SLC: file ../../../gecko/dom/bluetooth/bluez/BluetoothHfpManager.cpp, line 1279
15:30:26     INFO -  [217] WARNING: Trying to SendCommand() without a SLC: file ../../../gecko/dom/bluetooth/bluez/BluetoothHfpManager.cpp, line 1279
15:30:27     INFO -  [217] WARNING: NS_ENSURE_TRUE(iccInfo) failed: file ../../../gecko/dom/bluetooth/bluez/BluetoothHfpManager.cpp, line 688
15:30:34     INFO -  JavaScript strict warning: , line 0: ReferenceError: reference to undefined property "QueryInterface"

Comment 40

[Edgar Chen [:edgar]]

Comment on attachment 8490565 [details] [diff] [review]
bug-1063304-follow-up-uuid.patch

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

Thank you.

Comment 41

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

Attachment: part 1/3: rename MobileConnectionGonkService.js to MobileConnectionService.js : v3

Comment 42

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

Attachment: part 2/3: export header files in right namespace : v3

Rebase only.

Comment 43

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

Attachment: part 3.d/3: Gonk JS service fixes : v3

Accommodate to changes in part 1/3.

Comment 44

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

Attachment: part 3.e/3: ensure 'network', 'cell' are created with correct types

'cell', 'network' of nsIMobileConnection might be created with incorrect types.

Don't know how to run cppunit locally. Try: https://tbpl.mozilla.org/?tree=Try&rev=57f66c99c75b

Comment 45

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

https://hg.mozilla.org/integration/b2g-inbound/rev/96e435cffa55
https://hg.mozilla.org/integration/b2g-inbound/rev/e27e309897fd
https://hg.mozilla.org/integration/b2g-inbound/rev/299482975a2f
https://hg.mozilla.org/integration/b2g-inbound/rev/09012bde1827
https://hg.mozilla.org/integration/b2g-inbound/rev/b4c4679975dc
https://hg.mozilla.org/integration/b2g-inbound/rev/203a12787dec
https://hg.mozilla.org/integration/b2g-inbound/rev/381d918e3702

Comment 46

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

Checking build failure for b2g-desktop on Mac. The same thing built in https://tbpl.mozilla.org/php/getParsedLog.php?id=48547115&tree=B2g-Inbound , but failed in https://tbpl.mozilla.org/php/getParsedLog.php?id=48548656&tree=B2g-Inbound

Comment 47

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

Builds locally and on tbpl (https://tbpl.mozilla.org/?tree=B2g-Inbound&jobname=b2g_b2g-inbound_macosx64_gecko%20build)

Comment 48

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

Clobber: https://hg.mozilla.org/integration/b2g-inbound/rev/bb9ae71d8533

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96e435cffa55
https://hg.mozilla.org/mozilla-central/rev/e27e309897fd
https://hg.mozilla.org/mozilla-central/rev/299482975a2f
https://hg.mozilla.org/mozilla-central/rev/09012bde1827
https://hg.mozilla.org/mozilla-central/rev/b4c4679975dc
https://hg.mozilla.org/mozilla-central/rev/203a12787dec
https://hg.mozilla.org/mozilla-central/rev/381d918e3702
https://hg.mozilla.org/mozilla-central/rev/bb9ae71d8533