1058305 - [B2G][RIL] define data apn types in nsIRadioInterface.idl

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[pgravel]

Just like it's done in bug 937485 I would like to request to use the enums for data APN types for the following APIs in nsIRadioInterfaceLayer.idl:

- SetupDataCallByType
- DeactivateDataCallByType
- GetDataCallByType

Comment 1

[pgravel]

Also should be reflected in APN type field in setting entry "ril.data.apnSettings"

Comment 2

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

We don't touch .webidl in this case so changing the bug title.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Hi Phil,

I wonder if we can reuse the data types NETWORK_TYPE_* defined in nsINetworkManager.idl, considering that after Bug 904514, all network connection requests should be acquired through NetworkManager. 
Regarding comment 2, as mentioned in bug 1058308 comment 3, the values defined in ril .idl doesn't help settings API much now, so we think it should remain the same and do the conversion internally, at least until bug 904515.
What do you think?

Comment 4

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #3)
> Hi Phil,
> 
> I wonder if we can reuse the data types NETWORK_TYPE_* defined in
> nsINetworkManager.idl, considering that after Bug 904514, all network
> connection requests should be acquired through NetworkManager. 
> Regarding comment 2, as mentioned in bug 1058308 comment 3, the values
> defined in ril .idl doesn't help settings API much now, so we think it
> should remain the same and do the conversion internally, at least until bug
> 904515.

Sorry, should be bug 904514.

> What do you think?

Comment 5

[pgravel]

That sounds like a good idea.

Comment 6

[Jessica Jong [:jessica] (inactive)]

(In reply to pgravel from comment #5)
> That sounds like a good idea.

We'll move toward that way then, thanks!

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl changes, v1.

Hsinyi, may I have your review on the interface change? Thanks.

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: implementation, v1.

Edgar, may I have your review? Thanks.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: other componenets, v1.

MmsService, GonkGPSGeolocationProvider and NetworkManager specifically.

Edgar, may I have your review? Thanks.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: test changes, v1.

Edgar, here are the changes needed in mnw tests. Thanks.

Comment 11

[Edgar Chen [:edgar]]

Comment on attachment 8525093 [details] [diff] [review]
Part 2: implementation, v1.

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

r=me with below comments addressed. Thank you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3751,5 @@
> +  setupDataCallByType: function(networkType) {
> +    if (!this._isMobileNetworkType(networkType)) {
> +      if (DEBUG) this.debug(networkType + " is not a mobile network type!");
> +      throw Cr.NS_ERROR_INVALID_ARG;
> +    }

Move this check to DataConnectionHandler.setupDataCallByType().
Since we are going to deprecate RadioInterface.setupDataCallByType(), I'd prefer to keep this method just forwarding the request to DataConnectionHandler.

@@ +3762,5 @@
> +  deactivateDataCallByType: function(networkType) {
> +    if (!this._isMobileNetworkType(networkType)) {
> +      if (DEBUG) this.debug(networkType + " is not a mobile network type!");
> +      throw Cr.NS_ERROR_INVALID_ARG;
> +    }

Ditto

@@ +3772,5 @@
> +  getDataCallStateByType: function(networkType) {
> +    if (!this._isMobileNetworkType(networkType)) {
> +      if (DEBUG) this.debug(networkType + " is not a mobile network type!");
> +      throw Cr.NS_ERROR_INVALID_ARG;
> +    }

Ditto

Comment 12

[Edgar Chen [:edgar]]

Comment on attachment 8525099 [details] [diff] [review]
Part 4: test changes, v1.

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

Thank you.

::: dom/system/gonk/tests/marionette/head.js
@@ +14,5 @@
> +const NETWORK_TYPE_MOBILE_SUPL = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL;
> +const NETWORK_TYPE_MOBILE_IMS = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_IMS;
> +const NETWORK_TYPE_MOBILE_DUN = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_DUN;
> +
> +let networkTypes = [

nit: const networkTypes = [ ...

Comment 13

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #11)
> Comment on attachment 8525093 [details] [diff] [review]
> Part 2: implementation, v1.
> 
> Review of attachment 8525093 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with below comments addressed. Thank you.
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +3751,5 @@
> > +  setupDataCallByType: function(networkType) {
> > +    if (!this._isMobileNetworkType(networkType)) {
> > +      if (DEBUG) this.debug(networkType + " is not a mobile network type!");
> > +      throw Cr.NS_ERROR_INVALID_ARG;
> > +    }
> 
> Move this check to DataConnectionHandler.setupDataCallByType().
> Since we are going to deprecate RadioInterface.setupDataCallByType(), I'd
> prefer to keep this method just forwarding the request to
> DataConnectionHandler.

Sounds good! will do.

> 
> @@ +3762,5 @@
> > +  deactivateDataCallByType: function(networkType) {
> > +    if (!this._isMobileNetworkType(networkType)) {
> > +      if (DEBUG) this.debug(networkType + " is not a mobile network type!");
> > +      throw Cr.NS_ERROR_INVALID_ARG;
> > +    }
> 
> Ditto

will do.

> 
> @@ +3772,5 @@
> > +  getDataCallStateByType: function(networkType) {
> > +    if (!this._isMobileNetworkType(networkType)) {
> > +      if (DEBUG) this.debug(networkType + " is not a mobile network type!");
> > +      throw Cr.NS_ERROR_INVALID_ARG;
> > +    }
> 
> Ditto

will do.

Comment 14

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #12)
> Comment on attachment 8525099 [details] [diff] [review]
> Part 4: test changes, v1.
> 
> Review of attachment 8525099 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you.
> 
> ::: dom/system/gonk/tests/marionette/head.js
> @@ +14,5 @@
> > +const NETWORK_TYPE_MOBILE_SUPL = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL;
> > +const NETWORK_TYPE_MOBILE_IMS = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_IMS;
> > +const NETWORK_TYPE_MOBILE_DUN = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_DUN;
> > +
> > +let networkTypes = [
> 
> nit: const networkTypes = [ ...

will do, thanks!

Comment 15

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 1: idl changes. r=hsinyi

Add r=hsinyi.

Comment 16

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 2: implementation. r=echen

- Address review comment 11: move mobile network type check into DataConnectionHandler
- Add r=echen

Comment 17

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 3: other componenets. r=echen

Add r=echen

Comment 18

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 4: tests. r=echen

- Address review comment 12: use const for networkTypes
- Add r=echen

Comment 19

[Jessica Jong [:jessica] (inactive)]

try looks good!

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

Comment 20

[Jessica Jong [:jessica] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/8044b311493f
https://hg.mozilla.org/integration/b2g-inbound/rev/7c32d6aa2be9
https://hg.mozilla.org/integration/b2g-inbound/rev/316905ac2460
https://hg.mozilla.org/integration/b2g-inbound/rev/a573c8e256ea

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8044b311493f
https://hg.mozilla.org/mozilla-central/rev/7c32d6aa2be9
https://hg.mozilla.org/mozilla-central/rev/316905ac2460
https://hg.mozilla.org/mozilla-central/rev/a573c8e256ea

Comment 22

[dattaz]

These commit break ZTE Open C master build. I have debug with a lot of ZTE Open C build with git bisec and build before these commit (a52d89e75d724675fb4f14e3d1ffb5c65d030917) work, and build after these commit (67c6c865fe2b050c0bd77d506b3437d8f907b23a) doesn't work. Last commit at d3d5e369341659702178bfccecddcb7484013661 doesn't work too

Technically ZTE Open C doesn't start, stay on fox picture. 

See boot log here => http://pastebin.fr/37856

Comment 23

[Jessica Jong [:jessica] (inactive)]

(In reply to dattaz from comment #22)
> These commit break ZTE Open C master build. I have debug with a lot of ZTE
> Open C build with git bisec and build before these commit
> (a52d89e75d724675fb4f14e3d1ffb5c65d030917) work, and build after these
> commit (67c6c865fe2b050c0bd77d506b3437d8f907b23a) doesn't work. Last commit
> at d3d5e369341659702178bfccecddcb7484013661 doesn't work too
> 
> Technically ZTE Open C doesn't start, stay on fox picture. 
> 
> See boot log here => http://pastebin.fr/37856

Sorry for the incovenience. From the log, it seems to crash on nsIRadioInterfaceLayer.getRadioInterface(), but I am not sure why since it simple returns the corresponding 'RadioInterface'. Maybe we need to change the uuid of nsIRadioInterfaceLayer as well, can you give it a try? it works fine on Flame though...

I can not find the commit ids you mentioned, are you using 'https://git.mozilla.org/?p=releases/gecko.git;a=shortlog;h=refs/heads/master'?

Comment 24

[dattaz]

I use repo for building, it's http://git.mozilla.org/?p=releases/gecko.git;a=summary

Commit before : https://git.mozilla.org/?p=releases/gecko.git;a=commit;h=a52d89e75d724675fb4f14e3d1ffb5c65d030917
Commit after : https://git.mozilla.org/?p=releases/gecko.git;a=commit;h=67c6c865fe2b050c0bd77d506b3437d8f907b23a

How i can change uuid of nsIRadioInterfaceLayer ?

Comment 25

[Jessica Jong [:jessica] (inactive)]

Attachment: 0001-Bug-1058305-follow-up-update-nsIRadioInterfaceLayer-.patch

dattaz, can you apply this patch (git) and try again? Thanks.

Comment 26

[dattaz]

I have try => http://pastebin.fr/37862
It's doesn't start

Comment 27

[Jessica Jong [:jessica] (inactive)]

Attachment: 0001-Bug-1058305-follow-up-re-update-nsIRadioInterface-uu.patch

It's weird, it works if I change nsIRadioInterface uuid again... might be that the uuid used before collides with something else?

Comment 28

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #27)
> Created attachment 8537044 [details] [diff] [review]
> 0001-Bug-1058305-follow-up-re-update-nsIRadioInterface-uu.patch
> 
> It's weird, it works if I change nsIRadioInterface uuid again... might be
> that the uuid used before collides with something else?

dattaz, can you try this patch? I'll prepare a formal patch if it works.
Thanks for your help!

Comment 29

[dattaz]

I have try. And it's work, with this patch ZTE Open C boot correctly.

LOG : http://pastebin.fr/37884

Comment 30

[Jessica Jong [:jessica] (inactive)]

Attachment: (follow-up) re-update nsIRadioInterface uuid.

Hsinyi, the previous uuid used for nsIRadioInterface seems not to work on Open C, so changing to a new one. May I have your review on this? Thanks.

Comment 31

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

Comment on attachment 8537677 [details] [diff] [review]
(follow-up) re-update nsIRadioInterface uuid.

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

r=me!

Comment 32

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b8707e6cf2f

try looks good!

Comment 33

[Jessica Jong [:jessica] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/5358dd225ca9

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5358dd225ca9