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

RESOLVED FIXED in 2.1 S5 (26sep)

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vicamo, Assigned: vicamo)

Tracking

unspecified
2.1 S5 (26sep)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 14 obsolete attachments)

24.82 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
76.48 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
30.40 KB, patch
edgar
: review+
Details | Diff | Splinter Review
16.82 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
27.22 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
28.88 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
5.52 KB, patch
Details | Diff | Splinter Review
+++ 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.
Blocks: 1047196
Assignee

Comment 1

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

Comment 2

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

Comment 3

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

Comment 4

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

Comment 6

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

Comment 7

5 years ago
Complete nsIMobileConnection::GetSupportedNetworkTypes().
Attachment #8485394 - Attachment is obsolete: true
Assignee

Comment 8

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

Comment 9

5 years ago
Fix OOP process crash & build failure in platforms.
Attachment #8485412 - Attachment is obsolete: true
Assignee

Comment 11

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

Comment 13

5 years ago
Move IPC classes into mozilla::dom::mobileconnection as well.
Attachment #8485389 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8485388 - Flags: review?(echen)
Assignee

Comment 14

5 years ago
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.
Blocks: 833229, 1038606
Assignee

Updated

5 years ago
Attachment #8485666 - Flags: review?(echen)
Assignee

Updated

5 years ago
Attachment #8485390 - Flags: superreview?(echen)
Assignee

Updated

5 years ago
Attachment #8485469 - Flags: review?(echen)
Assignee

Updated

5 years ago
Attachment #8485395 - Flags: review?(echen)
Assignee

Updated

5 years ago
Attachment #8485396 - Flags: review?(echen)
Attachment #8485388 - Flags: review?(echen) → review+
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.
Attachment #8485666 - Flags: review?(echen) → review+
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.
Attachment #8485390 - Flags: review?(htsai)
Attachment #8485390 - Flags: superreview?(echen) → review+
Attachment #8485469 - Flags: review?(echen) → review+
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?
Assignee

Comment 18

5 years ago
(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 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
Attachment #8485395 - Flags: review?(echen)
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. :)
Attachment #8485396 - Flags: review?(echen) → review+
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 :)
Attachment #8485390 - Flags: review?(htsai)
Assignee

Comment 22

5 years ago
Update r=echen only.
Attachment #8485388 - Attachment is obsolete: true
Attachment #8489258 - Flags: review+
Assignee

Comment 23

5 years ago
1) rebase after bug 994190.

2) address review comment 15 and update commit summary.
Attachment #8485666 - Attachment is obsolete: true
Attachment #8489259 - Flags: review+
Assignee

Comment 24

5 years ago
Address comment 17.
Attachment #8485390 - Attachment is obsolete: true
Attachment #8489265 - Flags: review?(htsai)
Assignee

Comment 25

5 years ago
Address comment 15 and comment 17.
Attachment #8485469 - Attachment is obsolete: true
Attachment #8489266 - Flags: review+
Assignee

Comment 26

5 years ago
Address review comment 19.
Attachment #8485395 - Attachment is obsolete: true
Attachment #8489268 - Flags: review?(echen)
Assignee

Comment 27

5 years ago
Address comment 17 and comment 20.
Attachment #8485396 - Attachment is obsolete: true
Attachment #8489270 - Flags: review+
Assignee

Comment 29

5 years ago
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 on attachment 8489265 [details] [diff] [review]
part 3.a/3: add nsIMobileConnection : v2

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

Thank you~
Attachment #8489265 - Flags: review?(htsai) → review+
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. :)
Attachment #8489268 - Flags: review?(echen) → review+
Assignee

Comment 32

5 years ago
Flash Gecko & Gaia only into Flame with v123 Gonk ok. Try all before push: https://tbpl.mozilla.org/?tree=Try&rev=c8d6811ae990
Please can you land a followup to bump UUIDs? This needed a clobber due to their omission. Thanks :-)
Flags: needinfo?(vyang)
Also please can the reviewers take extra care to watch out for UUID bumps that might be required :-)
Assignee

Comment 37

5 years ago
Hi, any way we may know in advance that tbpl has to be explicitly clobbered?
Attachment #8490565 - Flags: review?(emorley)
Flags: needinfo?(vyang)
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 :-)
Attachment #8490565 - Flags: review?(emorley)
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"
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Updated

5 years ago
Attachment #8490565 - Flags: review?(echen)
Comment on attachment 8490565 [details] [diff] [review]
bug-1063304-follow-up-uuid.patch

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

Thank you.
Attachment #8490565 - Flags: review?(echen) → review+
Assignee

Comment 42

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

Comment 43

5 years ago
Accommodate to changes in part 1/3.
Attachment #8489270 - Attachment is obsolete: true
Attachment #8492619 - Flags: review+
Assignee

Comment 44

5 years ago
'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
You need to log in before you can comment on or make changes to this bug.