850555 - B2G RIL: Change data call setting architecture.

Status: RESOLVED DUPLICATE of bug 837488

(Core :: DOM: Core & HTML, defect)

Description

[Ken Chang[:kenkai]]

In order to provide all data call information for RIL at the same time, we have to change the data call setting architecture.
Following is the new data call setting format,
{"0": {apnSetting0: {apn: "Internet", 
                     user: "", 
                     passwd: "", 
                     httpProxyHost: "", 
                     httpProxyPort: "", 
                     type: ["default"],
                     },
       apnSetting1: {apn: "emome", 
                     user: "", 
                     passwd: "", 
                     httpProxyHost: "", 
                     httpProxyPort: "", 
                     type: ["mms","supl"],
                     }
       }
}

Comment 1

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

(In reply to Ken Chang from comment #0)
> In order to provide all data call information for RIL at the same time, we
> have to change the data call setting architecture.
> Following is the new data call setting format,
> {"0": {apnSetting0: {apn: "Internet", 
>                      user: "", 
>                      passwd: "", 
>                      httpProxyHost: "", 
>                      httpProxyPort: "", 
>                      type: ["default"],
>                      },
>        apnSetting1: {apn: "emome", 
>                      user: "", 
>                      passwd: "", 
>                      httpProxyHost: "", 
>                      httpProxyPort: "", 
>                      type: ["mms","supl"],
>                      }
>        }
> }

I would like to have |types: []| that makes more sense to me.

Comment 2

[Ken Chang[:kenkai]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> I would like to have |types: []| that makes more sense to me.
Yes, it makes more sense.

Comment 3

[Ken Chang[:kenkai]]

Attachment: Change the apn data setting format.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 725301 [details] [diff] [review]
Change the apn data setting format.

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

The style this time is much better.

And you could just ask one person for r?,
you don't have to send r? twice unless it's a huge part.
But you could send feedback? to me for making sure your code fits in style.

I'll leave the review part to Hsinyi since she has discussed the detail with you before.
Also the bug title seems not so right,
I think you didn't change whole 'architecture'.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1148,5 @@
> +      // into the data call setting of data.
> +      if (apnSetting.types.indexOf("default") != -1) {
> +        for (let apnItem in apnSetting) {
> +          this.dataCallSettings[apnItem] = apnSetting[apnItem];
> +        }

Seems
this.dataCallSettings = apnSetting;
is enough?

Same as below.

@@ +1986,5 @@
> +        if (aName == "ril.data.apnSettings") {
> +          // aResult["0"] means that we want to get the apn settings for sim1.
> +          // This design is a forward compatibility for multi-sim feature.
> +          // See bug 832808.
> +          this.updateApnSettings(aResult["0"]);

Forward compatibility ?

I would suggest you do this in Bug 832808.

Comment 5

[Ken Chang[:kenkai]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #4)
> Comment on attachment 725301 [details] [diff] [review]
> Change the apn data setting format.
> 
> Review of attachment 725301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The style this time is much better.
> 
> And you could just ask one person for r?,
> you don't have to send r? twice unless it's a huge part.
> But you could send feedback? to me for making sure your code fits in style.
Thanks for your suggestion.

> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1148,5 @@
> > +      // into the data call setting of data.
> > +      if (apnSetting.types.indexOf("default") != -1) {
> > +        for (let apnItem in apnSetting) {
> > +          this.dataCallSettings[apnItem] = apnSetting[apnItem];
> > +        }
> 
> Seems
> this.dataCallSettings = apnSetting;
> is enough?
There are a little difference between dataCallSettings and apnSetting.
dataCallSettings has 2 more flags, enable and roaming_enable. 
For dataCallSettingsSUPL and dataCallSettingsMMS, we should be able to use this kind 
of method. But it will causes 2 kind of copy styles for 3 different data call types, 
I am not sure if it is bad for readability.

> Same as below.
> 
> @@ +1986,5 @@
> > +        if (aName == "ril.data.apnSettings") {
> > +          // aResult["0"] means that we want to get the apn settings for sim1.
> > +          // This design is a forward compatibility for multi-sim feature.
> > +          // See bug 832808.
> > +          this.updateApnSettings(aResult["0"]);
> 
> Forward compatibility ?
> 
> I would suggest you do this in Bug 832808.
The reason to do this in this bug is to avoid that Gaia have to change apn settings 2 times.

Comment 6

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

Comment on attachment 725301 [details] [diff] [review]
Change the apn data setting format.

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

I like small patches!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1139,5 @@
>    },
>  
> +  /**
> +   * This function updates the data call setting from inputting apn
> +   * settings.

Thanks for the comment here, it looks no more information is offered than the function name since the name itself is clear enough. I think we can just let it go.

@@ +1144,5 @@
> +   */
> +  updateApnSettings: function updateApnSettings(allApnSettings) {
> +    for each (let apnSetting in allApnSettings) {
> +      // If this apn setting supports default type, copy this apn setting
> +      // into the data call setting of data.

nit: Again, worth to think about if the comment is helpful.

@@ +1980,5 @@
>        case "ril.data.enabled":
>          this._oldRilDataEnabledState = this.dataCallSettings["enabled"];
>          // Fall through!
>        case "ril.data.roaming_enabled":
> +      case "ril.data.apnSettings":

Right now the handlers for "ril.data.roaming_enabled" and "ril.data.apnSettings" are not that similar. Please separate the two cases, not falling through. So we can also save an additional check for aName.

@@ +1986,5 @@
> +        if (aName == "ril.data.apnSettings") {
> +          // aResult["0"] means that we want to get the apn settings for sim1.
> +          // This design is a forward compatibility for multi-sim feature.
> +          // See bug 832808.
> +          this.updateApnSettings(aResult["0"]);

Let's have a const defining the number of sim cards. Then use the constant to retrieve settings for each sim. Remove your original comment, please, but add //TODO Support multi-SIM, bug 799023.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Ken Chang from comment #5)
> There are a little difference between dataCallSettings and apnSetting.
> dataCallSettings has 2 more flags, enable and roaming_enable. 
> For dataCallSettingsSUPL and dataCallSettingsMMS, we should be able to use
> this kind 
> of method. But it will causes 2 kind of copy styles for 3 different data
> call types, 
> I am not sure if it is bad for readability.
> 
Oh, I am okay with this.

> > 
> > Forward compatibility ?
> > 
> > I would suggest you do this in Bug 832808.
> The reason to do this in this bug is to avoid that Gaia have to change apn
> settings 2 times.

If you file two bugs, then did it seperately.
That's why we file 'seperate' bugs.

Comment 8

[Ken Chang[:kenkai]]

Attachment: Change the apn data setting format.

Comment 9

[Ken Chang[:kenkai]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #7)
> > > I would suggest you do this in Bug 832808.
> > The reason to do this in this bug is to avoid that Gaia have to change apn
> > settings 2 times.
> 
> If you file two bugs, then did it seperately.
> That's why we file 'seperate' bugs.
I had re-edited the title of the Bug 832808 for making it clearly.

Comment 10

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

Comment on attachment 725981 [details] [diff] [review]
Change the apn data setting format.

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

Looks great, thanks! r=me but we shouldn't merge this until we have correspoding revision in gaia.

Comment 11

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

Comment on attachment 725981 [details] [diff] [review]
Change the apn data setting format.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1138,5 @@
>      }
>    },
>  
> +  updateApnSettings: function updateApnSettings(allApnSettings) {
> +    // TODO. Support multi-SIM, bug 799023.

nit: replace the period after TODO with a colon.

Still r+, so don't need to request for review :)

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Ken Chang from comment #0)
> In order to provide all data call information for RIL at the same time, we
> have to change the data call setting architecture.
> Following is the new data call setting format,
> {"0": {apnSetting0: {apn: "Internet", 

Does that "apnSetting0" really helpful? Why can't we have a two dimention array instead?

Comment 13

[Ken Chang[:kenkai]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #12)
> Does that "apnSetting0" really helpful? Why can't we have a two dimention
> array instead?
"apnSetting?" is just for readability. And yes, it is okay to use 2 dimension array. 
Here is the new format.
[
  [ 
    {
       apn: "internet", 
       user: "", 
       passwd: "", 
       httpProxyHost: "", 
       httpProxyPort: "", 
       types: ["default"],
    },
    {
       apn: "emome", 
       user: "", 
       passwd: "", 
       httpProxyHost: "", 
       httpProxyPort: "", 
       types: ["mms","supl"],
    }
  ],
  [ 
    {
       apn: "internet", 
       user: "", 
       passwd: "", 
       httpProxyHost: "", 
       httpProxyPort: "", 
       types: ["dun"],
    },
    {
       apn: "fetne01", 
       user: "", 
       passwd: "", 
       httpProxyHost: "", 
       httpProxyPort: "", 
       types: ["mms","supl","default"],
    }
  ]
]

Comment 14

[Ken Chang[:kenkai]]

Attachment: The new apn data setting format - 2 dimensions

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 726060 [details] [diff] [review]
The new apn data setting format - 2 dimensions

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

An explicit r- because there seems to be something wrong on Bugzilla.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1142,5 @@
> +    // TODO. Support multi-SIM, bug 799023.
> +    let simNumber = 1;
> +    for (let simId = 0; simId < simNumber; simId++) {
> +      let thisSimApnSettings = allApnSettings[simId];
> +      for (let apnIndex = 0; thisSimApnSettings[apnIndex]; apnIndex++) {

Considering a settings value [[...], null, [...]], the for-loop breaks unexpectedly.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 726060 [details] [diff] [review]
The new apn data setting format - 2 dimensions

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

Sorry, seems that's for per SIM apn settings, so nullity for ending is reasonable.

Comment 17

[Ken Chang[:kenkai]]

Attachment: The new apn data setting format - 2 dimensions. r = hsinyi; r = vyang

Comment 18

[Leo]

Dear Mozilla engineer,

When is this patch merged into Master git?
Bug 866003 and Bug 868934 are waiting for this patch is landed first.
Let me know the schedule please.

Thanks,
Jinho Lee.

Comment 19

[Edgar Chen [:edgar]]

If bug 866003 and bug 868934 are nominated as leo+, we can landed them first without changing data call architecture. So I think this bug need not be nominated as leo+, thanks.

Comment 20

[David Scravaglieri [:scravag]]

Minused as per comment 19 agreed with Leo team.

Comment 21

[Ken Chang[:kenkai]]

This patch has been included in the patch of bug 837488.