961571 - B2G RIL: support ims apn type

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

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?

Comment 1

[Jessica Jong [:jessica] (inactive)]

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.

Comment 2

[Ken Chang[:kenkai]]

Blocking 1.4 LTE feature.

Comment 3

[Ken Chang[:kenkai]]

This is a 1.4+ bug.

Comment 4

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

remove 1.4 flag as this is a feature work. let's use metabug (b2g-LTE-1.4) to keep tracking.

Comment 5

[Ken Chang[:kenkai]]

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.

Comment 6

[[:fabrice] Fabrice Desré]

Jessica, do you have everything you need to make progress here?

Comment 7

[Kai-Chih Hu [:khu]]

(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!!

Comment 8

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 9

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

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.

Comment 10

[Ken Chang[:kenkai]]

Comments has been added.

Comment 11

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: add ims apn type in nsINetworkInterface.

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: handle ims apn type in RILNetworkInterface.

Comment 13

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: handle ims apn type in NetworkManager.

Comment 14

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: add test case for ims.

Comment 15

[Jessica Jong [:jessica] (inactive)]

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.

Comment 16

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

(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 17

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

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.

Comment 18

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

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.

Comment 19

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

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

Comment 20

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

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 ;)

Comment 21

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: add test case for ims, v2.

So sorry for this, uploading the right patch. Will verify it again in the morning.

Comment 22

[Ken Chang[:kenkai]]

After discussing with Jessica and Edgar, edgar will help following things for this bug.

Comment 23

[Ken Chang[:kenkai]]

(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.

Comment 24

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL changes for adding ims apn type, v2

Address review comment #17.

Comment 25

[Edgar Chen [:edgar]]

Attachment: Part 3: Handle ims apn type in NetworkManager, v2

Address review comment #19.
- s/exculde/exclude/
- Set excludeIms in NetworkInterfaceListService.js

Comment 26

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

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!

Comment 27

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

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.

Comment 28

[Edgar Chen [:edgar]]

Attachment: Part 4: Add test case for data connection, v3

Comment 29

[Edgar Chen [:edgar]]

Attachment: Part 4: Add test case for data connection, v4

Remove redundant empty line.

Comment 30

[Edgar Chen [:edgar]]

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.

Comment 31

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL changes for adding ims apn type, v3, r=hsinyi

Rebase for bug 976959

Comment 32

[Edgar Chen [:edgar]]

Attachment: Part 2: Handle ims apn type in RILNetworkInterface, v2, r=hsinyi

Rebase for bug 976959

Comment 33

[Edgar Chen [:edgar]]

Attachment: Part 3: Handle ims apn type in NetworkManager, v3, r=hsinyi

Rebase for bug 976959.

Comment 34

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

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.

Comment 35

[Edgar Chen [:edgar]]

Attachment: Part 3: Handle ims apn type in NetworkManager, v3, r=hsinyi

Correct hg user info.

Comment 36

[Edgar Chen [:edgar]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=c1a9339bb1d6

Comment 37

[Edgar Chen [:edgar]]

Attachment: Part 4: Add test case for data connection, v5, r=hsinyi

Address review comment #34.
- Handling promise rejection

Comment 38

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #36)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=c1a9339bb1d6

All green \o/

https://hg.mozilla.org/integration/b2g-inbound/rev/1f4d436f213a
https://hg.mozilla.org/integration/b2g-inbound/rev/d57fbc7bdd37
https://hg.mozilla.org/integration/b2g-inbound/rev/cb047a99c13c
https://hg.mozilla.org/integration/b2g-inbound/rev/e7eda63c0ee7

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f4d436f213a
https://hg.mozilla.org/mozilla-central/rev/d57fbc7bdd37
https://hg.mozilla.org/mozilla-central/rev/cb047a99c13c
https://hg.mozilla.org/mozilla-central/rev/e7eda63c0ee7