Closed Bug 1058305 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
2.2 S2 (19dec)
feature-b2g 2.2+
tracking-b2g backlog

People

(Reporter: pgravel, Assigned: jessica)

References

Details

(Whiteboard: [priority1])

Attachments

(5 files, 6 obsolete files)

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
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
Also should be reflected in APN type field in setting entry "ril.data.apnSettings"
Blocks: 1058308
Blocks: 1047203
No longer blocks: 1058308
No longer depends on: 1052852
We don't touch .webidl in this case so changing the bug title.
Summary: Use enums for data apn types types in webidl and idl → [B2G][RIL] define data apn typs in nsIRadioInterface.idl
Summary: [B2G][RIL] define data apn typs in nsIRadioInterface.idl → [B2G][RIL] define data apn types in nsIRadioInterface.idl
Assignee: nobody → htsai
Assignee: htsai → nobody
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)
(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?
That sounds like a good idea.
Flags: needinfo?(pgravel)
Assignee: nobody → jjong
(In reply to pgravel from comment #5)
> That sounds like a good idea.

We'll move toward that way then, thanks!
Attached patch Part 1: idl changes, v1. (obsolete) — Splinter Review
Hsinyi, may I have your review on the interface change? Thanks.
Attachment #8524483 - Flags: review?(htsai)
Attached patch Part 2: implementation, v1. (obsolete) — Splinter Review
Edgar, may I have your review? Thanks.
Attachment #8525093 - Flags: review?(echen)
Attached patch Part 3: other componenets, v1. (obsolete) — Splinter Review
MmsService, GonkGPSGeolocationProvider and NetworkManager specifically.

Edgar, may I have your review? Thanks.
Attachment #8525095 - Flags: review?(echen)
Attached patch Part 4: test changes, v1. (obsolete) — Splinter Review
Edgar, here are the changes needed in mnw tests. Thanks.
Attachment #8525099 - Flags: review?(echen)
Attachment #8524483 - Flags: review?(htsai) → review+
blocking-b2g: --- → backlog
feature-b2g: --- → 2.2+
Whiteboard: [priority1]
Target Milestone: --- → 2.2 S2 (19dec)
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+
Attachment #8525095 - Flags: review?(echen) → review+
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+
(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.
(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!
Add r=hsinyi.
Attachment #8524483 - Attachment is obsolete: true
Attachment #8527548 - Flags: review+
- Address review comment 11: move mobile network type check into DataConnectionHandler
- Add r=echen
Attachment #8525093 - Attachment is obsolete: true
Attachment #8527553 - Flags: review+
Add r=echen
Attachment #8525095 - Attachment is obsolete: true
Attachment #8527556 - Flags: review+
- Address review comment 12: use const for networkTypes
- Add r=echen
Attachment #8525099 - Attachment is obsolete: true
Attachment #8527557 - Flags: review+
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)
(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)
dattaz, can you apply this patch (git) and try again? Thanks.
I have try => http://pastebin.fr/37862
It's doesn't start
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
(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)
I have try. And it's work, with this patch ZTE Open C boot correctly.

LOG : http://pastebin.fr/37884
Flags: needinfo?(taz)
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+
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
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.