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

RESOLVED FIXED in 2.2 S2 (19dec)

Status

Firefox OS
RIL
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: pgravel, Assigned: jessica)

Tracking

unspecified
2.2 S2 (19dec)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2+, tracking-b2g:backlog)

Details

(Whiteboard: [priority1])

Attachments

(5 attachments, 6 obsolete attachments)

1.56 KB, patch
jessica
: review+
Details | Diff | Splinter Review
13.27 KB, patch
jessica
: review+
Details | Diff | Splinter Review
5.20 KB, patch
jessica
: review+
Details | Diff | Splinter Review
9.39 KB, patch
jessica
: review+
Details | Diff | Splinter Review
890 bytes, patch
hsinyi
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
Also should be reflected in APN type field in setting entry "ril.data.apnSettings"
(Reporter)

Updated

4 years ago
Blocks: 1058308

Updated

4 years ago
Blocks: 1047203
No longer blocks: 1058308
No longer depends on: 1052852
We don't touch .webidl in this case so changing the bug title.
Blocks: 959978
Summary: Use enums for data apn types types in webidl and idl → [B2G][RIL] define data apn typs in nsIRadioInterface.idl

Updated

4 years ago
Summary: [B2G][RIL] define data apn typs in nsIRadioInterface.idl → [B2G][RIL] define data apn types in nsIRadioInterface.idl

Updated

4 years ago
Assignee: nobody → htsai

Updated

4 years ago
Assignee: htsai → nobody
(Assignee)

Comment 3

4 years ago
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?
Flags: needinfo?(pgravel)
(Assignee)

Comment 4

4 years ago
(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?
(Reporter)

Comment 5

4 years ago
That sounds like a good idea.
Flags: needinfo?(pgravel)
(Assignee)

Updated

4 years ago
Assignee: nobody → jjong
(Assignee)

Comment 6

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

We'll move toward that way then, thanks!
(Assignee)

Comment 7

4 years ago
Created attachment 8524483 [details] [diff] [review]
Part 1: idl changes, v1.

Hsinyi, may I have your review on the interface change? Thanks.
Attachment #8524483 - Flags: review?(htsai)
(Assignee)

Comment 8

4 years ago
Created attachment 8525093 [details] [diff] [review]
Part 2: implementation, v1.

Edgar, may I have your review? Thanks.
Attachment #8525093 - Flags: review?(echen)
(Assignee)

Comment 9

4 years ago
Created attachment 8525095 [details] [diff] [review]
Part 3: other componenets, v1.

MmsService, GonkGPSGeolocationProvider and NetworkManager specifically.

Edgar, may I have your review? Thanks.
Attachment #8525095 - Flags: review?(echen)
(Assignee)

Comment 10

4 years ago
Created attachment 8525099 [details] [diff] [review]
Part 4: test changes, v1.

Edgar, here are the changes needed in mnw tests. Thanks.
Attachment #8525099 - Flags: review?(echen)

Updated

4 years ago
Attachment #8524483 - Flags: review?(htsai) → review+
blocking-b2g: --- → backlog
feature-b2g: --- → 2.2+
Whiteboard: [priority1]
(Assignee)

Updated

4 years ago
Target Milestone: --- → 2.2 S2 (19dec)

Comment 11

4 years ago
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
Attachment #8525093 - Flags: review?(echen) → review+

Updated

4 years ago
Attachment #8525095 - Flags: review?(echen) → review+

Comment 12

4 years ago
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 = [ ...
Attachment #8525099 - Flags: review?(echen) → review+
(Assignee)

Comment 13

4 years ago
(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.
(Assignee)

Comment 14

4 years ago
(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!
(Assignee)

Comment 15

4 years ago
Created attachment 8527548 [details] [diff] [review]
[final] Part 1: idl changes. r=hsinyi

Add r=hsinyi.
Attachment #8524483 - Attachment is obsolete: true
Attachment #8527548 - Flags: review+
(Assignee)

Comment 16

4 years ago
Created attachment 8527553 [details] [diff] [review]
[final] Part 2: implementation. r=echen

- Address review comment 11: move mobile network type check into DataConnectionHandler
- Add r=echen
Attachment #8525093 - Attachment is obsolete: true
Attachment #8527553 - Flags: review+
(Assignee)

Comment 17

4 years ago
Created attachment 8527556 [details] [diff] [review]
[final] Part 3: other componenets. r=echen

Add r=echen
Attachment #8525095 - Attachment is obsolete: true
Attachment #8527556 - Flags: review+
(Assignee)

Comment 18

4 years ago
Created attachment 8527557 [details] [diff] [review]
[final] Part 4: tests. r=echen

- Address review comment 12: use const for networkTypes
- Add r=echen
Attachment #8525099 - Attachment is obsolete: true
Attachment #8527557 - Flags: review+

Comment 22

4 years ago
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
Flags: needinfo?(jjong)
(Assignee)

Comment 23

4 years ago
(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'?
Flags: needinfo?(jjong)
(Assignee)

Comment 25

4 years ago
Created attachment 8536349 [details] [diff] [review]
0001-Bug-1058305-follow-up-update-nsIRadioInterfaceLayer-.patch

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

Comment 26

4 years ago
I have try => http://pastebin.fr/37862
It's doesn't start
(Assignee)

Comment 27

4 years ago
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?
Attachment #8536349 - Attachment is obsolete: true
(Assignee)

Comment 28

4 years ago
(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!
Flags: needinfo?(taz)

Comment 29

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

LOG : http://pastebin.fr/37884
Flags: needinfo?(taz)
(Assignee)

Comment 30

4 years ago
Created attachment 8537677 [details] [diff] [review]
(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.
Attachment #8537044 - Attachment is obsolete: true
Attachment #8537677 - Flags: review?(htsai)
Comment on attachment 8537677 [details] [diff] [review]
(follow-up) re-update nsIRadioInterface uuid.

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

r=me!
Attachment #8537677 - Flags: review?(htsai) → review+
(Assignee)

Comment 33

4 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/5358dd225ca9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/5358dd225ca9
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.