Closed
Bug 842252
Opened 11 years ago
Closed 11 years ago
[B2G][Setting] Change data call setting architecture in Gaia.
Categories
(Firefox OS Graveyard :: Gaia, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: kchang, Assigned: johnhu)
References
Details
(Whiteboard: [TAIPEI_FND_TRACKING])
Attachments
(1 file)
For getting all necessary data of data call, RIL is changing the data call setting architecture. Gaia also have to do corresponding modifications. We add a new key, "ril.data.apnSetting",to contain all information making data call need. Here is the example for data format, lock.set("ril.data.apnSettings", {apnSetting0: {apn: "Internet", user: "", passwd: "", httpProxyHost: "192.168.0.1", httpProxyPort: "8080", types: "data" }, apnSetting1: {apn: "emome", user: "emome", passwd: "emome", httpProxyHost: "192.168.0.1", httpProxyPort: "8080", types: "mms,supl", } }, null);
Updated•11 years ago
|
Reporter | ||
Comment 1•11 years ago
|
||
We need to change the data format for reusing this design in multi-SIM fuction. {"0": {apnSetting0: {apn: "Internet", user: "", passwd: "", httpProxyHost: "", httpProxyPort: "", type: ["default"], }, apnSetting1: {apn: "emome", user: "", passwd: "", httpProxyHost: "", httpProxyPort: "", type: ["mms","supl"], }}}
Reporter | ||
Comment 2•11 years ago
|
||
After discussing with Vicamo, the following is the new format. [ // It is for sim1. [ { apn: "internet", user: "", passwd: "", httpProxyHost: "", httpProxyPort: "", types: ["default"], }, { apn: "emome", user: "", passwd: "", httpProxyHost: "", httpProxyPort: "", types: ["mms","supl"], } ], // It is for sim2. [ { apn: "internet", user: "", passwd: "", httpProxyHost: "", httpProxyPort: "", types: ["dun"], }, { apn: "fetne01", user: "", passwd: "", httpProxyHost: "", httpProxyPort: "", types: ["mms","supl","default"], } ] ]
Updated•11 years ago
|
Whiteboard: [TAIPEI_FND_TRACKING]
Comment 3•11 years ago
|
||
Arthur, Could you take this bug or mentoring John to work on it?
Assignee: schung → arthur.chen
Flags: needinfo?(arthur.chen)
Comment 4•11 years ago
|
||
I'm not familiar with this part. I'll do some surveys to see if it is okay for John to work on it.
Flags: needinfo?(arthur.chen)
Comment 5•11 years ago
|
||
Just a waring here. If we are changing the data call settings architecture we must change the way of getting the APN settings from the `apn.json` database through the operator variant code (see [1]). [1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/operator_variant/operator_variant.js
Assignee | ||
Updated•11 years ago
|
Assignee: arthur.chen → johu
Assignee | ||
Comment 6•11 years ago
|
||
1. add new APN Settings and keep old settings 2. make settings app save setting in both new and old version. old version is for backward compatible.
Attachment #749777 -
Flags: review?(arthur.chen)
Comment 7•11 years ago
|
||
Comment on attachment 749777 [details]
pull request for 842252
Fernando, please help review the changes made on FTU.
Jose, please help review operator_variants.js.
Thanks!
Attachment #749777 -
Flags: review?(josea.olivera)
Attachment #749777 -
Flags: review?(fernando.campo)
Comment 8•11 years ago
|
||
Comment on attachment 749777 [details]
pull request for 842252
Good patch, John! Thanks! r=me.
Attachment #749777 -
Flags: review?(arthur.chen) → review+
Comment 9•11 years ago
|
||
Comment on attachment 749777 [details]
pull request for 842252
See comments in github please. I'd like to receive the review request again once the review comments get addressed. Thanks!
Attachment #749777 -
Flags: review?(josea.olivera)
Assignee | ||
Comment 10•11 years ago
|
||
Comment on attachment 749777 [details]
pull request for 842252
jaoo,
I had change the implementation on operator_variant.js. Please take a look at.
Attachment #749777 -
Flags: review?(josea.olivera)
Comment 11•11 years ago
|
||
Comment on attachment 749777 [details]
pull request for 842252
See comments in github please. I'd like to receive the review request again once the review comments get addressed. Thanks!
Attachment #749777 -
Flags: review?(josea.olivera)
Assignee | ||
Comment 12•11 years ago
|
||
Jose, To replying your first comment: 1. why just first simcard: This is because the UX and underlying API only supports one simcard currently. That's the reason we only put APN settings for first simcard. We will open another bug for changing single simcard to multiple simcard. But the APN settings architecture will still be the same. 2. why not mmsc, mmsproxy, mmsport For the view of ril, they shares the same settings if APN name is the same, and mmsc, mmsproxy, mmsport is only for MMS service, not for ril layer.
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(josea.olivera)
Comment 13•11 years ago
|
||
Comment on attachment 749777 [details]
pull request for 842252
Sorry this one fell off my list.
Changes on FTU look fine, double thanks for remembering about the tests :D
Attachment #749777 -
Flags: review?(fernando.campo) → review+
Assignee | ||
Comment 14•11 years ago
|
||
Ken, I need your opinions on pull request discuss of https://github.com/mozilla-b2g/gaia/pull/9784 Thanks.
Flags: needinfo?(kchang)
Comment 15•11 years ago
|
||
Hey guys, let's discuss a bit about what to do in Gaia right now about this bug. The new APN settings architecture (I really like Ken's idea about how that architecture should be) will be needed once we have multi-SIM devices and the RIL plumbing bits get ready. Moreover (AFAIK) we don't have yet the corresponding UX design to deal with the multi-SIM device support. I guess you guys want to land bug 850555 ASAP but IHMO we should try to avoid -as much as possible- changes in Gaia that are not currently needed. At the moment the old APN settings architecture, as I have seen in the patch in this bug, coexists with the new one and IHMO we should avoid that as well. If you guys really want to move forward with this bug and land this bug that's fine I just wanted to expose my thoughts here. Jonh, thanks for the patch. I did quickly a WIP patch [1] for this bug to see if something much simplier was possible. It might need some improvements/corrections. Feel free to take a look and use here what you need. [1] https://github.com/jaoo/gaia/commit/6141dd3dd33918bf7d46670ba42dd42883764674
Flags: needinfo?(josea.olivera)
Comment 16•11 years ago
|
||
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #15) > IHMO we should try to avoid -as much as possible- changes in > Gaia that are not currently needed. At the moment the old APN settings > architecture, as I have seen in the patch in this bug, coexists with the new > one and IHMO we should avoid that as well. I second that. And to be completely honest, I’m a bit surprised that the module owners haven’t been involved in the discussion for such a big change. The current APN architecture has proven its efficiency (we had a lot of issues with the former one…) and I’d be relieved if we could keep it as long as possible — e.g. until we get the full picture of the multi-SIM use case. Can we please have a closer look at Jaoo’s approach? He’s familiar with the code base, he has a very good view of the whole APN picture (both back-end and front-end) and he’s very aware of what we need to be able to fetch all the data from the existing databases (Gnome + Android + custom data).
Reporter | ||
Comment 17•11 years ago
|
||
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #15) (In reply to Fabien Cazenave [:kaze] from comment #16) Hi, thanks for your opinions. This architecture isn't only for multi-sim(bug 850555) but also for bug 837488 (shared APN settings). We had found some coexistence problems in shared APN settings(For example, data and MMS use the same APN. Can we send MMS when data option isn't enable?) in old design and done a lot changes in data call architecture of RIL(bug 837488) to solve those problems. However, the new apn settings is not ready in Gaia now, we land those patches. That is why we are eager to land this patch. Although the UX isn't ready now, we still can be based on current UX design to have those fixes. Moreover, if we keep waiting for UX being ready, we will shorten the development and verification time. And you can also check the apn.json, the new architecture of apnSettings is very similar with apn.json. I think this change are in the right direction. Indeed, we should have a good communication with module owner.(I didn't know the the owner of Gaia data call setting before, but I know now.) And I hope that it isn't too late to communicate it now.
Flags: needinfo?(kchang)
Reporter | ||
Comment 18•11 years ago
|
||
(In reply to Ken Chang from comment #17) Oops! Correct sentence. > those problems. However, the new apn settings is not ready in Gaia now, we > land those patches. That is why we are eager to land this patch. Although those problems. However, the new apn settings is not ready in Gaia now, we cannot land those patches. That is why we are eager to land this patch. Although
Assignee | ||
Comment 19•11 years ago
|
||
Jose, IMHO, I had put some comments on your pull request. Most of them are the reason I use different approach. Thanks for your patch.
Assignee | ||
Comment 20•11 years ago
|
||
Jaoo, I had modified a new pull request based on your opinions. Please help me check if there is anything improper.
Assignee | ||
Comment 21•11 years ago
|
||
Ken, Is it possible to change the name of apn seting fields to be similar to apn.json file?? like: { apn: "internet", user: "", password: "", proxy: "", port: "", types: ["default"], } That will make things simpler
Flags: needinfo?(kchang)
Reporter | ||
Comment 22•11 years ago
|
||
(In reply to John Hu [:johnhu] from comment #21) > Ken, > > Is it possible to change the name of apn seting fields to be similar to > apn.json file?? like: > > { > apn: "internet", > user: "", > password: "", > proxy: "", > port: "", > types: ["default"], > } > > That will make things simpler Yes, you can do that.
Flags: needinfo?(kchang)
Assignee | ||
Comment 23•11 years ago
|
||
Sorry, another change is "types" to type. { apn: "internet", user: "", password: "", proxy: "", port: "", type: ["default"], }
Comment 24•11 years ago
|
||
John, I left some comments in the PR review. A couple of things First one, do not override the APN settings in the fields when the user enters the subpanel please, leave -do not change- the settings in the fields and the preselected APN in the subpanel. Second one, you must continue handling the settings under the `ril.mms.{mmsc, mmsproxy, mmsport}` keys. If the user choose a different APN for MMS in the Setting app you must update the values under the `ril.mms.{mmsc, mmsproxy, mmsport}` keys. If you don't do that the mmsc, mmsproxy, mmsport settings retrieved by the MMS plumbings would be the ones set by the operator variant logic on boot. You are only updating the values for the `ril.{data,mms,supl}.{carrier,apn,user,passwd,httpProxyHost,httpProxyPort}` keys. The overall approach would be to save under the `ril.data.apnSettings` key the APN settings for the one that the user selects in the setting app according to the new APN settings architecture and let the RIL uses those settings to set up the data call. I've applied both patches (bugs 850555 and 842252) It seems that It doesn't work correctly. The value of `ril.data.apnSettings` sometimes's wrong because It is not correctly updated in the setting app. Btw, are you planning guys to uplift bug 850555 and this one to v1-train gaia branch?
Assignee | ||
Comment 25•11 years ago
|
||
Jaoo, I had read your comments. Thanks for that. I already change the patch according to your comments: 1. add the handling of mmsc, mmsproxy, mmsport, and carrier, 2. not remove the override code on subpanel. BTW, if you applied the patch after 2013-05-23 04:00:31 PDT, I had changed the setting name according your comments. So, you may find "apnSetting.types is undefined" in console and nothing works. I will sync with Ken to have the correct path for both.
Comment 26•11 years ago
|
||
(In reply to John Hu [:johnhu] from comment #23) > Sorry, another change is "types" to type. > > > { > apn: "internet", > user: "", > password: "", > proxy: "", > port: "", > type: ["default"], > } Note, in backend implementation, we already use 'types.' I prefer |types| to |type| since it can be plural. But if 'types' indeed brings some difficult in gaia implementation, then please kindly make sure the consistency.
Assignee | ||
Comment 27•11 years ago
|
||
We want to use this kind of settings just because apn.json use the same field name. If you guys prefer types instead of type, I can make the change from type to types (I had made the change to this pull request). The type field also need to be processed for preventing duplication type in the same APN settings, so it may not be difficult to us.
Reporter | ||
Comment 28•11 years ago
|
||
(In reply to John Hu [:johnhu] from comment #27) > We want to use this kind of settings just because apn.json use the same > field name. If you guys prefer types instead of type, I can make the change > from type to types (I had made the change to this pull request). The type > field also need to be processed for preventing duplication type in the same > APN settings, so it may not be difficult to us. Please use the |types|. thanks
Assignee | ||
Comment 29•11 years ago
|
||
Comment on attachment 749777 [details]
pull request for 842252
Jose,
I had changed the PR with your suggestion code. Thanks for that. Please review it again.
Attachment #749777 -
Flags: review?(josea.olivera)
Comment 30•11 years ago
|
||
Triage - leo-'ing as this does not seem to be a required change for 1.1's feature set. Leo, can you renominate with reasons if you believe this is required?
blocking-b2g: leo? → ---
Comment 31•11 years ago
|
||
(In reply to John Hu [:johnhu] from comment #29) > Comment on attachment 749777 [details] > pull request for 842252 > > Jose, > > I had changed the PR with your suggestion code. Thanks for that. Please > review it again. Jonh, Ken, see my comments on github please. We are almost there.
Comment 32•11 years ago
|
||
As jaoo pinged me on github to raise a suggestion, let’s discuss it here. Quick sum-up: we currently have one APN set per connection type (data, mms, supl), which is not optimal in the (frequent) cases where the same APN is used for several connection types -- see bug 837488. A new data architecture is proposed to solve this (bug 850555): the point is to merge APN sets that share the same apn/username/password. Now the question is, where should we merge these APNs? This bug thinks of merging APNs in Gaia; but could we merge this APNs in Gecko instead? It would allow us to keep Gaia and the settings DB unchanged: no Gaia/Gecko interdependency, and we won’t break the 3G during the transition. (FTR, that’s what Android does) IMHO, doing the merge in Gaia without changing the UI does not make much sense: we’d still keep the user-facing complexity of having separate APNs, and for developers the Settings app is easier to deal with when the UI reflects the setting names closely. Long story short: • if the Settings *UI* can be simplified by merging these APNs sets on the Gaia side, I think it’s worth waiting for some UX wireframes (including the multi-SIM case) before modifying Gaia; • if we can’t wait for UX wireframes, I’d prefer to merge the APNs on the Gecko side. Vicamo, what do you think? (pinging UX for the wireframes…)
Flags: needinfo?(fdjabri)
Comment 33•11 years ago
|
||
Handing the need info over to Neo in Taipei as he is now the UX owner for this area.
Flags: needinfo?(fdjabri) → needinfo?(nhsieh)
Comment 34•11 years ago
|
||
Hi Everyone, As far as I know, There are 7 types APN setting in the world. Refer to iPhone and Android APN settings, I saw they merge APN settings and change them in a flexible way. Means If the operator has separated APN to MMS and Internet, System will show 2 items. And If the operator has merged APNS, System will show 1 items. I think at v1.1 we don't need to change UI or UX right now. Since Ken will merge them from Gecko layer. I think that is a good start. I will discuss this question with more UX designers for future version.
Flags: needinfo?(nhsieh)
Assignee | ||
Comment 35•11 years ago
|
||
Comment on attachment 749777 [details]
pull request for 842252
Please review this PR. Thanks.
Comment 36•11 years ago
|
||
Comment on attachment 749777 [details]
pull request for 842252
Thanks John, please see my comments in github. Some changes are still needed, nothing difficult. Thanks.
Attachment #749777 -
Flags: review?(josea.olivera) → review-
Assignee | ||
Comment 37•11 years ago
|
||
Jose, I had changed them in this PR. And some of these changes may not be needed, like type and types. I also replied your comment in this PR. Let us discuss them before next review. Thanks.
Comment 38•11 years ago
|
||
(In reply to John Hu [:johnhu] from comment #37) > Jose, > > I had changed them in this PR. And some of these changes may not be needed, > like type and types. I also replied your comment in this PR. Let us discuss > them before next review. Thanks. John, see the discussion on github. You were right about all my comments. I'm wondering if it could be possible to update bug 850555 to check whether everything is right before giving the formal r+ flags here. So ni? to Ken.
Flags: needinfo?(kchang)
Assignee | ||
Comment 39•11 years ago
|
||
Jose, Sure, it's a nice step to do it. I will love to wait your feedback after applying both patches.
Comment 40•11 years ago
|
||
Ideally, changing the data call setting should get us rid of the kind of hack we did in bug 882095.
Reporter | ||
Comment 41•11 years ago
|
||
This bug blocks a leo+ bug, bug 837488.
Reporter | ||
Comment 42•11 years ago
|
||
Jose, the 837488 is r+ and I had verified the John's patch.
Comment 43•11 years ago
|
||
(In reply to Ken Chang from comment #42) > Jose, the 837488 is r+ and I had verified the John's patch. I'll test John's patch on top of 837488 applied in birch. I'll keep you updated. Thanks.
Comment 44•11 years ago
|
||
John, the patch does not apply on top of latest gaia master branch. I'll rebase it for my test.
Comment 45•11 years ago
|
||
(In reply to Ken Chang from comment #42) > Jose, the 837488 is r+ and I had verified the John's patch. Yep, Seems to work correctly. Let's wait John to rebase the patch and ask for review at me again. I'll be happy to give r=me. Thanks for your work in bug 837488.
Assignee | ||
Comment 46•11 years ago
|
||
Jose, I had rebase and update the patch, please review it. I found there is a conflict with bug 882059. It says those codes need to be refined after this bug is landed. I had read the bug 882059. Do I need to do it in this patch, or we should open another bug for it??
Comment 47•11 years ago
|
||
(In reply to John Hu [:johnhu] from comment #46) > I found there is a conflict with bug 882059. It says those codes need to be > refined after this bug is landed. I had read the bug 882059. Do I need to do > it in this patch, or we should open another bug for it?? Please, file another bug for a follow-up for bug 882059. Thanks!
Assignee | ||
Comment 48•11 years ago
|
||
Bug 887659 is create for a follow-up for bug 882095.
Assignee | ||
Comment 49•11 years ago
|
||
Comment on attachment 749777 [details]
pull request for 842252
Change the flag to r?
Attachment #749777 -
Flags: review- → review?(josea.olivera)
Comment 50•11 years ago
|
||
Comment on attachment 749777 [details] pull request for 842252 r=me with the comments in the PR. I guess this could land right now even without waiting bug 837488 to land. John, thanks for your work and my apologizes for the long discussion and so many changes in the code.
Attachment #749777 -
Flags: review?(josea.olivera) → review+
Assignee | ||
Comment 51•11 years ago
|
||
Thanks, Jose. never mind it. This process makes me understand APN more. landed to master: https://github.com/mozilla-b2g/gaia/commit/ecc78307f2d2dc51dd207af0534cac26347999be
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 52•11 years ago
|
||
The bug 837488 isn't r+ bug, so this bug is also not r+ bug. I remove the leo?.
blocking-b2g: leo? → ---
Reporter | ||
Comment 53•11 years ago
|
||
(In reply to Ken Chang from comment #52) > The bug 837488 isn't r+ bug, so this bug is also not r+ bug. I remove the > leo?. Oops! Typo, what I mean is, "the bug 837488 isn't leo+ bug, so this bug, 842252, is also not leo+ bug." The reason of this bugs is nominated as leo+ is because the bug 837488 is a leo+ bug. But the bug 837488 isn't leo+ bug, so this bug, 842252, is also not leo+ bug. And this bug is a improvement for V1.2 or later version, I don't think we should include it now. That is why I remove leo+ from this bug.
Comment 54•11 years ago
|
||
(In reply to Ken Chang from comment #53) > (In reply to Ken Chang from comment #52) > Oops! Typo, what I mean is, "the bug 837488 isn't leo+ bug, so this bug, > 842252, is also not leo+ bug." Ken, John, do you think we should back out the patch landed for this bug in Gaia until bug 837488 lands?
blocking-b2g: --- → leo?
Flags: needinfo?(kchang)
Flags: needinfo?(johu)
Updated•11 years ago
|
blocking-b2g: leo? → ---
Reporter | ||
Comment 55•11 years ago
|
||
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #54) > Ken, John, do you think we should back out the patch landed for this bug in > Gaia until bug 837488 lands? If Gaia still send the old apn settings to RIL, we won't back out the patch.
Flags: needinfo?(kchang)
Assignee | ||
Comment 56•11 years ago
|
||
If we have any side effects on this bug, we should do it. I don't know if we have it or not...
Flags: needinfo?(johu)
Reporter | ||
Comment 57•11 years ago
|
||
The apnSettings = [[]] when I used this patch to do the test.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 58•11 years ago
|
||
Sorry about this case. I had tested with known simcards, like CHT, TWM, etc. This case only occurs when simcard has unknown carrier. We use apn.json file to build default settings. If carrier is not known by apn.json, the apnSettings will be [[]]. Because this patch is landed, I will create another patch for it.
Reporter | ||
Comment 59•11 years ago
|
||
According to comment 58, close this bug.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 60•11 years ago
|
||
Follow up bug of Comment 57 and Comment 58: https://bugzilla.mozilla.org/show_bug.cgi?id=890862.
You need to log in
before you can comment on or make changes to this bug.
Description
•