Closed Bug 961571 Opened 6 years ago Closed 6 years ago

B2G RIL: support ims apn type

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
1.4 S2 (28feb)
tracking-b2g backlog

People

(Reporter: jessica, Assigned: edgar)

References

Details

(Whiteboard: [FT:RIL])

Attachments

(4 files, 10 obsolete files)

2.83 KB, patch
edgar
: review+
Details | Diff | Splinter Review
2.00 KB, patch
edgar
: review+
Details | Diff | Splinter Review
9.04 KB, patch
edgar
: review+
Details | Diff | Splinter Review
8.20 KB, patch
edgar
: review+
Details | Diff | Splinter Review
I am not sure if changes are needed in gecko side, since we support setting up different types of APN through 'setupDataCallByType(apntype)'. But I guess changes are needed somewhere in SMS to trigger this data call?
Just talked to Bevis, and it depends on the IMS architecture whether SMS needs to trigger this data call or not. But we will still support this APN type for flexibility and for following Android's design.
Blocking 1.4 LTE feature.
Assignee: nobody → jjong
This is a 1.4+ bug.
blocking-b2g: --- → 1.4+
remove 1.4 flag as this is a feature work. let's use metabug (b2g-LTE-1.4) to keep tracking.
blocking-b2g: 1.4+ → backlog
Whiteboard: [FT:RIL]
Target Milestone: --- → 1.4 S3 (14mar)
Blocks: 972737
The target milestone is too late. I have changed it to 2/28. If it is impossible to finish it before 2/28, please let me know.
Target Milestone: 1.4 S3 (14mar) → 1.4 S2 (28feb)
Jessica, do you have everything you need to make progress here?
Flags: needinfo?(jjong)
(In reply to Ken Chang[:ken] from comment #5)
> The target milestone is too late. I have changed it to 2/28. If it is
> impossible to finish it before 2/28, please let me know.

Why don't you guys have answer about this yet?! 
Ken and Wesley, please help on this bug ASAP!!
Flags: needinfo?(whuang)
Flags: needinfo?(kchang)
(In reply to Kevin Hu [:khu] from comment #7)
> (In reply to Ken Chang[:ken] from comment #5)
> > The target milestone is too late. I have changed it to 2/28. If it is
> > impossible to finish it before 2/28, please let me know.
> 
> Why don't you guys have answer about this yet?! 
> Ken and Wesley, please help on this bug ASAP!!

I will upload patches for this bug today.
Flags: needinfo?(jjong)
Bug 960865 and this one were added to tracking by us.
Dev is working on this, but I think we can update progress more frequently to reflect the status.
Flags: needinfo?(whuang)
Comments has been added.
Flags: needinfo?(kchang)
Attached patch Part 4: add test case for ims. (obsolete) — Splinter Review
Attachment #8381970 - Flags: review?(htsai)
Hsinyi,

Some code are duplicated from Bug 960865, but I guess this will land first?
One of the bugs will need to be rebased on another.

Thank you.
(In reply to Jessica Jong [:jjong] [:jessica] from comment #15)
> Hsinyi,
> 
> Some code are duplicated from Bug 960865, but I guess this will land first?
> One of the bugs will need to be rebased on another.
> 
> Thank you.

Got it. Thanks for the explanation. I am looking at it! Stay tuned ;)
Comment on attachment 8381966 [details] [diff] [review]
Part 1: add ims apn type in nsINetworkInterface.

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

We also need to modify nsINetworkInterfaceListService. 
Please add "const long LIST_NOT_INCLUDE_IMS_INTERFACES" there. 
r- for this.
Attachment #8381966 - Flags: review?(htsai) → review-
Comment on attachment 8381968 [details] [diff] [review]
Part 2: handle ims apn type in RILNetworkInterface.

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

This part looks good.
Attachment #8381968 - Flags: review?(htsai) → review+
Comment on attachment 8381969 [details] [diff] [review]
Part 3: handle ims apn type in NetworkManager.

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

Something missed in NetworkInterfaceListService.js.

::: dom/system/gonk/NetworkManager.js
@@ +289,5 @@
>        case "NetworkInterfaceList:ListInterface": {
>  #ifdef MOZ_B2G_RIL
>          let excludeMms = aMsg.json.exculdeMms;
>          let excludeSupl = aMsg.json.exculdeSupl;
> +        let exculdeIms = aMsg.json.excludeIms;

1) s/exculdeIms/excludeIms/

2) aMsg is coming from NetworkInterfaceListService.js. We'd make sure "excludeIms" carries right information.

So, add below in NetworkInterfaceListService.js [1]:

|excludeIms: (aConditions &
              Ci.nsINetworkInterfaceListService.
              LIST_NOT_INCLUDE_IMS_INTERFACES) != 0|

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkInterfaceListService.js?from=NetworkInterfaceListService.js&case=true#31
Attachment #8381969 - Flags: review?(htsai) → review-
Comment on attachment 8381970 [details] [diff] [review]
Part 4: add test case for ims.

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

Wrong patch, I guess ;)
Attachment #8381970 - Flags: review?(htsai)
So sorry for this, uploading the right patch. Will verify it again in the morning.
Attachment #8381970 - Attachment is obsolete: true
After discussing with Jessica and Edgar, edgar will help following things for this bug.
Assignee: jjong → echen
(In reply to Ken Chang[:ken] from comment #22)
> After discussing with Jessica and Edgar, edgar will help following things
> for this bug.
typo, follow-up.
Address review comment #17.
Attachment #8381966 - Attachment is obsolete: true
Address review comment #19.
- s/exculde/exclude/
- Set excludeIms in NetworkInterfaceListService.js
Attachment #8381969 - Attachment is obsolete: true
Attachment #8382785 - Flags: review?(htsai)
Attachment #8382797 - Flags: review?(htsai)
Comment on attachment 8382785 [details] [diff] [review]
Part 1: IDL changes for adding ims apn type, v2

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

::: dom/system/gonk/nsINetworkInterfaceListService.idl
@@ +24,5 @@
>  interface nsINetworkInterfaceListService : nsISupports
>  {
> +  const long LIST_NOT_INCLUDE_MMS_INTERFACES  = (1 << 0);
> +  const long LIST_NOT_INCLUDE_SUPL_INTERFACES = (1 << 1);
> +  const long LIST_NOT_INCLUDE_IMS_INTERFACES  = (1 << 2);

Nice catch!
Attachment #8382785 - Flags: review?(htsai) → review+
Comment on attachment 8382797 [details] [diff] [review]
Part 3: Handle ims apn type in NetworkManager, v2

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

Looks good.
Attachment #8382797 - Flags: review?(htsai) → review+
Attachment #8382232 - Attachment is obsolete: true
Remove redundant empty line.
Attachment #8382925 - Attachment is obsolete: true
Comment on attachment 8382926 [details] [diff] [review]
Part 4: Add test case for data connection, v4

Summaries of this patch, 
1). Add some test cases for both default and non-default data connection.
2). The modification of test_mobile_set_radio.js is to restore the data setting, otherwise the |testInitialState| of test_data_connection.js will fail.

Hi Hsinyi, could you help to review this patch? thank you.
Attachment #8382926 - Flags: review?(htsai)
Rebase for bug 976959
Attachment #8382785 - Attachment is obsolete: true
Attachment #8382952 - Flags: review+
Attachment #8382952 - Attachment description: Part 1: IDL changes for adding ims apn type, v3 → Part 1: IDL changes for adding ims apn type, v3, r=hsinyi
Rebase for bug 976959
Attachment #8381968 - Attachment is obsolete: true
Attachment #8382953 - Flags: review+
Rebase for bug 976959.
Attachment #8382797 - Attachment is obsolete: true
Attachment #8382954 - Flags: review+
Comment on attachment 8382926 [details] [diff] [review]
Part 4: Add test case for data connection, v4

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

We might have promise rejected but the handlers are missing. r=me with the comment addressed.

::: dom/mobileconnection/tests/marionette/test_mobile_set_radio.js
@@ +148,5 @@
>        is(connection.data.connected, false);
>      })
> +    .then(setRadioEnabled.bind(null, true, "enabling", "enabled"))
> +    // Disable data
> +    .then(setSetting.bind(null, DATA_KEY, false));

Thanks for this.

::: dom/system/gonk/tests/marionette/test_data_connection.js
@@ +120,5 @@
> +  return Promise.resolve()
> +    .then(() => getSetting(DATA_KEY))
> +    .then(value => {
> +      is(value, false, "Data must be off");
> +    })

Add one line here for handling promise rejection:

.then(null, error => {
  ok(false, "promise rejected during test");
})

@@ +134,5 @@
> +    .then(() => setSetting(DATA_KEY, true))
> +    .then(() => waitNetworkConnected(Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE))
> +    // Disable data
> +    .then(() => setSetting(DATA_KEY, false))
> +    .then(() => waitNetworkDisconnected(Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE))

ditto.

@@ +170,5 @@
> +    .then(() => doTestNonDefaultDataConnection("mms"))
> +    .then(() => doTestNonDefaultDataConnection("supl"))
> +    .then(() => doTestNonDefaultDataConnection("ims"))
> +    // Restore APN settings
> +    .then(() => setSetting(APN_KEY, currentApn))

ditto.
Attachment #8382926 - Flags: review?(htsai) → review+
Correct hg user info.
Attachment #8382954 - Attachment is obsolete: true
Attachment #8382970 - Flags: review+
Address review comment #34.
- Handling promise rejection
Attachment #8382926 - Attachment is obsolete: true
Attachment #8383059 - Flags: review+
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.