B2G RIL: support ims apn type

RESOLVED FIXED in 1.4 S2 (28feb)

Status

Firefox OS
RIL
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jessica, Assigned: edgar)

Tracking

(Blocks: 2 bugs)

unspecified
1.4 S2 (28feb)
x86_64
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

(Whiteboard: [FT:RIL])

Attachments

(4 attachments, 10 obsolete attachments)

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
(Reporter)

Description

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

Comment 1

4 years ago
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

4 years ago
Blocking 1.4 LTE feature.
Assignee: nobody → jjong
Blocks: 960206, 959978

Comment 3

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

Updated

4 years ago
Blocks: 972737

Comment 5

4 years ago
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)
No longer blocks: 960206
Jessica, do you have everything you need to make progress here?
Flags: needinfo?(jjong)

Comment 7

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

Comment 8

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

Comment 10

4 years ago
Comments has been added.
Flags: needinfo?(kchang)
(Reporter)

Comment 11

4 years ago
Created attachment 8381966 [details] [diff] [review]
Part 1: add ims apn type in nsINetworkInterface.
Attachment #8381966 - Flags: review?(htsai)
(Reporter)

Comment 12

4 years ago
Created attachment 8381968 [details] [diff] [review]
Part 2: handle ims apn type in RILNetworkInterface.
Attachment #8381968 - Flags: review?(htsai)
(Reporter)

Comment 13

4 years ago
Created attachment 8381969 [details] [diff] [review]
Part 3: handle ims apn type in NetworkManager.
Attachment #8381969 - Flags: review?(htsai)
(Reporter)

Comment 14

4 years ago
Created attachment 8381970 [details] [diff] [review]
Part 4: add test case for ims.
Attachment #8381970 - Flags: review?(htsai)
(Reporter)

Comment 15

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

Comment 21

4 years ago
Created attachment 8382232 [details] [diff] [review]
Part 4: add test case for ims, v2.

So sorry for this, uploading the right patch. Will verify it again in the morning.
Attachment #8381970 - Attachment is obsolete: true

Comment 22

4 years ago
After discussing with Jessica and Edgar, edgar will help following things for this bug.
Assignee: jjong → echen

Comment 23

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

Comment 24

4 years ago
Created attachment 8382785 [details] [diff] [review]
Part 1: IDL changes for adding ims apn type, v2

Address review comment #17.
Attachment #8381966 - Attachment is obsolete: true
(Assignee)

Comment 25

4 years ago
Created attachment 8382797 [details] [diff] [review]
Part 3: Handle ims apn type in NetworkManager, v2

Address review comment #19.
- s/exculde/exclude/
- Set excludeIms in NetworkInterfaceListService.js
Attachment #8381969 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8382785 - Flags: review?(htsai)
(Assignee)

Updated

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

Comment 28

4 years ago
Created attachment 8382925 [details] [diff] [review]
Part 4: Add test case for data connection, v3
Attachment #8382232 - Attachment is obsolete: true
(Assignee)

Comment 29

4 years ago
Created attachment 8382926 [details] [diff] [review]
Part 4: Add test case for data connection, v4

Remove redundant empty line.
Attachment #8382925 - Attachment is obsolete: true
(Assignee)

Comment 30

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

Comment 31

4 years ago
Created attachment 8382952 [details] [diff] [review]
Part 1: IDL changes for adding ims apn type, v3, r=hsinyi

Rebase for bug 976959
Attachment #8382785 - Attachment is obsolete: true
Attachment #8382952 - Flags: review+
(Assignee)

Updated

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

Comment 32

4 years ago
Created attachment 8382953 [details] [diff] [review]
Part 2: Handle ims apn type in RILNetworkInterface, v2, r=hsinyi

Rebase for bug 976959
Attachment #8381968 - Attachment is obsolete: true
Attachment #8382953 - Flags: review+
(Assignee)

Comment 33

4 years ago
Created attachment 8382954 [details] [diff] [review]
Part 3: Handle ims apn type in NetworkManager, v3, r=hsinyi

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+
(Assignee)

Comment 35

4 years ago
Created attachment 8382970 [details] [diff] [review]
Part 3: Handle ims apn type in NetworkManager, v3, r=hsinyi

Correct hg user info.
Attachment #8382954 - Attachment is obsolete: true
Attachment #8382970 - Flags: review+
(Assignee)

Comment 37

4 years ago
Created attachment 8383059 [details] [diff] [review]
Part 4: Add test case for data connection, v5, r=hsinyi

Address review comment #34.
- Handling promise rejection
Attachment #8382926 - Attachment is obsolete: true
Attachment #8383059 - Flags: review+
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.