Closed
Bug 947198
Opened 11 years ago
Closed 10 years ago
[DSDS][dolphin][OMA CP] Support for DSDS
Categories
(Firefox OS Graveyard :: Gaia::Wappush, defect, P1)
Tracking
(blocking-b2g:-, b2g-v1.4 affected, b2g-v2.0 affected, b2g-v2.1 verified)
RESOLVED
FIXED
blocking-b2g | - |
People
(Reporter: wmathanaraj, Assigned: Jinghua.Xing)
References
Details
(Whiteboard: [2.0-flame-test-run-1][sprd320677])
Attachments
(4 files, 5 obsolete files)
When implementing a OMA CP application on top of DSDS support I need a way to differentiate from which SIM the OMA CP message was received from and to which SIM the message applies to.
Updated•11 years ago
|
Blocks: 1.4-comms-committed
Updated•11 years ago
|
Blocks: b2g-dsds-1.4
Updated•11 years ago
|
Summary: [DSDS] [OMA CP] Support for DSDS → [DSDS][OMA CP] Support for DSDS
Comment 1•11 years ago
|
||
Hi Ken, Per the discussion, we'll need to include the "which SIM gets the message" information. Please help to file engineering bugs. This shall also apply to bug 947195 and bug 947192.
Flags: needinfo?(kchang)
Comment 2•11 years ago
|
||
(In reply to Wesley Huang [:wesley_huang] from comment #1) > Hi Ken, > Per the discussion, we'll need to include the "which SIM gets the message" > information. > Please help to file engineering bugs. > This shall also apply to bug 947195 and bug 947192. Have created bug 951999 for this.
Depends on: 951999
Flags: needinfo?(kchang)
Updated•11 years ago
|
Assignee: nobody → nhsieh
Comment 3•11 years ago
|
||
Since client provisioning messages are handled by the wappush app I think this should be moved to the Gaia::WapPush component. Do we already have a UX flow for displaying from which SIM a message belongs?
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Comment 4•11 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #3) > Since client provisioning messages are handled by the wappush app I think > this should be moved to the Gaia::WapPush component. Do we already have a UX > flow for displaying from which SIM a message belongs? I think Ayman is going to take care of it, at least we had a chat a few days ago about the OMA CP message dance and how it is supposed to work for multi ICC card devices.
Comment 5•11 years ago
|
||
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4) > (In reply to Gabriele Svelto [:gsvelto] from comment #3) > > Since client provisioning messages are handled by the wappush app I think > > this should be moved to the Gaia::WapPush component. Do we already have a UX > > flow for displaying from which SIM a message belongs? > > I think Ayman is going to take care of it, at least we had a chat a few days > ago about the OMA CP message dance and how it is supposed to work for multi > ICC card devices. ni to Neo Hsieh who will be the one providing UX flows for covering dsds OMA CP scenario. Thanks!
Flags: needinfo?(nhsieh)
Comment 6•11 years ago
|
||
Hi Joe, About OMA CP UX spec document. According to workweek discussion. That will be handled by TEF side. Can you help to verify that ? Thank you.
Flags: needinfo?(nhsieh)
Comment 7•11 years ago
|
||
(In reply to Neo Hsieh from comment #6) > Hi Joe, About OMA CP UX spec document. According to workweek discussion. > That will be handled by TEF side. > Can you help to verify that ? Thank you. Hi Neo, ni to Ayman who will provide dsds OMA CP UX spec. Sorry for the misunderstanding. Thanks!
Flags: needinfo?(aymanmaat)
Comment 8•11 years ago
|
||
Release note new wireframes - none updated wireframes - none deleted wireframes - none new flows - none updated flows OMA CP : USERPIN - ‘3. PIN not entered’ and ‘3. PIN entered’ PIN indication moved and annotation appropriately - ‘1. Lock screen notification’ annotation updated OMA CP : NETWPIN - ‘1. Lock screen notification’ annotation updated deleted flows - none
Flags: needinfo?(aymanmaat)
Updated•11 years ago
|
Assignee: nhsieh → nobody
Updated•11 years ago
|
Assignee: nobody → josea.olivera
Updated•11 years ago
|
No longer blocks: 1.4-comms-committed
Updated•11 years ago
|
No longer blocks: b2g-dsds-1.4
Reporter | ||
Comment 9•11 years ago
|
||
to be considered for partner contribution or future work - added to backlog.
blocking-b2g: 1.4+ → backlog
Updated•10 years ago
|
status-b2g-v2.0:
--- → affected
Whiteboard: [2.0-flame-test-run-1]
Updated•10 years ago
|
Summary: [DSDS][OMA CP] Support for DSDS → [DSDS][dolphin][OMA CP] Support for DSDS
Whiteboard: [2.0-flame-test-run-1] → [2.0-flame-test-run-1][sprd320677]
Updated•10 years ago
|
status-b2g-v1.4:
--- → affected
Assignee | ||
Comment 12•10 years ago
|
||
The message object the wappush app received contains a serviceId property, and the value of this property figured out whether the SIM 1 or SIM 2 received this OTA message. To fix the problem which we cannot configure the OMA CP message right in DSDS devices, my solution is using the value of the serviceId property to obtain the mcc and mnc of the right sim card which received the message, and then add the apns parsed from the message to the apn list of the corresponding operator.
Attachment #8449258 -
Flags: review?(josea.olivera)
Updated•10 years ago
|
Assignee: josea.olivera → Jinghua.Xing
Component: Gaia::Settings → Gaia::Wappush
Comment 13•10 years ago
|
||
Comment on attachment 8449258 [details] [diff] [review] Apply the OTA message to the SIM card which received this message Review of attachment 8449258 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to jinghua.xing from comment #12) > Created attachment 8449258 [details] [diff] [review] > Apply the OTA message to the SIM card which received this message First of all thanks so much for taking care of it. I'm glad to see your contribution here. > The message object the wappush app received contains a serviceId property, > and the value of this property figured out whether the SIM 1 or SIM 2 > received this OTA message. Yes, you're right. > To fix the problem which we cannot configure the OMA CP message right in > DSDS devices, my solution is using the value of the serviceId property to > obtain the mcc and mnc of the right sim card which received the message, and > then add the apns parsed from the message to the apn list of the > corresponding operator. That's the plan I also had for this. We would need to follow the UX specs attached in this bug and provide some test cases for the changes being added here. P.S. I'm not a peer of this app but I'll be happy to provide you some feedback if needed.
Attachment #8449258 -
Flags: review?(josea.olivera) → feedback+
Comment 14•10 years ago
|
||
Tim, do you know who is module owner for wappush? Can't find it in the module list.
Flags: needinfo?(timdream)
Comment 15•10 years ago
|
||
Usually that means you should |git count -- apps/wappush| :) I think Gabriele Svelto and Comms team people knows this app the best.
Flags: needinfo?(timdream)
Comment 16•10 years ago
|
||
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #14) > Tim, do you know who is module owner for wappush? Can't find it in the > module list. I'm the de-facto owner as I was the one who put the app together in the first place. However for the content provisioning messages part which was mostly written by :jaoo himself I usually defer reviews to him as he knows the code best. If we're blocked on the review here however I can do that if :jaoo doesn't have enough time right now.
Status: NEW → ASSIGNED
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8449258 [details] [diff] [review] Apply the OTA message to the SIM card which received this message Review of attachment 8449258 [details] [diff] [review]: ----------------------------------------------------------------- Thank for Gabriele Svelto's reply for my patch. Hi :jaoo : Would you like to help us to review this patch? Thank you.
Attachment #8449258 -
Flags: review?(josea.olivera)
Comment 18•10 years ago
|
||
Comment on attachment 8449258 [details] [diff] [review] Apply the OTA message to the SIM card which received this message Review of attachment 8449258 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Gabriele Svelto [:gsvelto] from comment #16) > If we're blocked on the review here however I can do that if :jaoo doesn't > have enough time right now. I'll be happy to review this. (In reply to jinghua.xing from comment #17) > Comment on attachment 8449258 [details] [diff] [review] > Apply the OTA message to the SIM card which received this message > > Review of attachment 8449258 [details] [diff] [review]: > ----------------------------------------------------------------- > Would you like to help us to review this patch? Sure, as I commented at comment 13 the patch LGMT but we need to follow the UX specs attached in this bug and provide some test cases.
Attachment #8449258 -
Flags: review?(josea.olivera)
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #18) > Sure, as I commented at comment 13 the patch LGMT but we need to follow the > UX specs attached in this bug and provide some test cases. Hi :jaoo: I am sorry for have misunderstood your comment before. I have revise my patch to make it follow the UX specs attached in this bug, please review it. But I have no idea of how to provide the test cases, can you help me or guide me to write the test cases? Thank you.
Attachment #8452993 -
Flags: review?(josea.olivera)
Assignee | ||
Comment 20•10 years ago
|
||
Hi :jaoo: I have revise my patch to make it follow the UX specs attached in this bug, please review it and try to add some testcases. But I have no idea of how to run the test cases, can you help me to review this patch? Thank you.
Attachment #8449258 -
Attachment is obsolete: true
Attachment #8452993 -
Attachment is obsolete: true
Attachment #8452993 -
Flags: review?(josea.olivera)
Attachment #8455124 -
Flags: review?(josea.olivera)
Comment 21•10 years ago
|
||
(In reply to jinghua.xing from comment #20) > Created attachment 8455124 [details] [diff] [review] > patch for add apns to the right sim card and some testcases > I have revise my patch to make it follow the UX specs attached in this bug, > please review it and try to add some testcases. Thanks, I'll have a look at it and let you know. > But I have no idea of how to > run the test cases, can you help me to review this patch? Read about unit testing at [1] please. [1] https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests
Comment 22•10 years ago
|
||
Hey jinghua.xing, the patch no longer applies cleanly on Gaia master branch. Could you please rebase it and even open a pull request for this patch please? Thanks!
Flags: needinfo?(Jinghua.Xing)
Comment 23•10 years ago
|
||
Comment on attachment 8455124 [details] [diff] [review] patch for add apns to the right sim card and some testcases Please, request review again when you are done please.
Attachment #8455124 -
Flags: review?(josea.olivera)
Assignee | ||
Comment 24•10 years ago
|
||
Hi,:jaoo
Attachment #8458550 -
Flags: review?(josea.olivera)
Flags: needinfo?(Jinghua.Xing)
Assignee | ||
Comment 25•10 years ago
|
||
Hi :jaoo Thank you for your advice. I tried to rebase it on the master branch. I am sorry for have not open a pull request, but there are two problems block me. 1.When I read the code on the master branch,I find it already contains the code to show which card received the message on the notification,as follows (L212): > var title = message.sender; > > if (navigator.mozIccManager && > navigator.mozIccManager.iccIds.length > 1) { > var simName = _('sim', { id: +message.serviceId + 1 }); > > title = _( > 'dsds-notification-title-with-sim', > { sim: simName, title: title } > ); > } > > var options = { > ... > }; > > var notification = new Notification(title, options); But when I tried this method on 1.4 branch, I find "navigator.mozIccManager" is always undefined, even if there are two sim card in the devices. 2. The test case "Add APN to SIM2: Pannon MMS type" in store_test.js is always run to be error. The error information is: > 1) [wappush-test/unit/store_test.js] "after each" hook: > Uncaught Error: Error: expected [ { carrier: 'Movistar MMS', > apn: 'telefonica.es', > user: 'telefonica', > password: 'telefonica', > _id: '9de53e1b-c55b-4762-ae13-edb0ac790c9d', > types: [ 'mms' ] } ] to have a length of 2 but got 1 (app://wappush.gaiamobile.org/common/test/helper.js?time=1405673810923:33) > at onerror (app://wappush.gaiamobile.org/common/vendor/mocha/mocha.js:4959:7) Can you help me to review this patch and give me some advice to modify my code? Thank you.
Flags: needinfo?(josea.olivera)
Comment 26•10 years ago
|
||
Comment on attachment 8458550 [details] [diff] [review] configure_apn_to_right_sim_card_master.patch Review of attachment 8458550 [details] [diff] [review]: ----------------------------------------------------------------- Looking really go. I'll review and test the rest tomorrow. ::: apps/wappush/js/cp_screen_helper.js @@ +76,4 @@ > > /** All APNs list */ > var apns = null; > + nit: whitespace @@ +76,5 @@ > > /** All APNs list */ > var apns = null; > + > + var cardId = 0; We already used `iccCardIndex` in store.js to index the ICC card the message came from so I'll prefer keeping `iccCardIndex` instead `cardId`. Do not forget to add a comment about the meaning of this new variable please. ::: apps/wappush/js/store.js @@ +29,4 @@ > * useful for unit testing). This function doesn't > * any parameter. > */ > + function sp_getMccMncCodes(cardId, callback) { I'll prefer keeping `iccCardIndex` instead `cardId` here as well. Please add a comment as the one included for the callback parameter in the function description. @@ +42,3 @@ > // We must add support for multi ICC card devices to the OMA CP logic. > // In the meantime we assume the ICC card the WAP push app is working with > // is the first one. Delete the comment above as you are actually adding the support we mention here. @@ +76,4 @@ > * useful for unit testing). This function doesn't > * accetp any parameter. > */ > + function sp_provision(parameters, cardId, callback) { Please add a comment as the one included for the callback parameter in the function description. @@ +118,3 @@ > > // XXX: Bug 947198 > // We must add support for multi ICC card devices to the OMA CP logic. Delete the comment above as you are actually adding the support we mention here.
Updated•10 years ago
|
Flags: needinfo?(josea.olivera)
Assignee | ||
Comment 27•10 years ago
|
||
Hi :jaoo : Thank you for your kindly advice :) I have made some changes according to your advice. Please help me review my patch. The two questions mentioned in comment 25 are still there, can you help me to check them? If there are something wrong in my patch, please let me know immediately. Thank you
Attachment #8455124 -
Attachment is obsolete: true
Attachment #8458550 -
Attachment is obsolete: true
Attachment #8458550 -
Flags: review?(josea.olivera)
Attachment #8460172 -
Flags: review?(josea.olivera)
Comment 28•10 years ago
|
||
Comment on attachment 8460172 [details] [diff] [review] config_wappush_message_supported_DSDS.patch Review of attachment 8460172 [details] [diff] [review]: ----------------------------------------------------------------- Well, I see a couple of issues here. First, you always find `navigator.mozIccManager` undefined because the app is lacking of the permission to use the ICC manager API. Add the '"mobileconnection":{}' permission to the manifest. I really don't know how the existing use of the ICC manager is working in the app (I have not seen it working to be honest). Second, the 'ril.data.cp.apns' setting must be an array from now on as the 'ril.data.apnSettings' setting is. The OMA CP APNs must be saved in the corresponding index of that array. I mean, save the OMA CP APNs for the first ICC card in the first position of the 'ril.data.cp.apns' array and so on. As you are becoming the 'ril.data.cp.apns' setting to an array you must update the logic of the usage of that settings in the setting app. If you need further information or help please ping me on IRC. ::: apps/wappush/js/store.js @@ +70,4 @@ > * Stores the APNs into the settings database. > * > * @param {Array} parameters The array containing the APNs. > + * @param {Number} iccCardIndex The number means which card receive this nit: trailing whitespace
Attachment #8460172 -
Flags: review?(josea.olivera)
Assignee | ||
Comment 29•10 years ago
|
||
Hi jaoo : I am sorry about I cannot access to the IRC. Can I write email to you if I have more questions ? I think the position where the OMA CP APNs be saved in 'ril.data.cp.apns' setting is due to the mcc and mnc of operator which the card received the message belongs to. The 'ril.data.cp.apns' setting is not an array but an object which stores all operators' apns. My work is just save the OMA CP APNs to the right position of this object. How do you see about this ? Thank you.
Flags: needinfo?(josea.olivera)
Comment 30•10 years ago
|
||
Hi José and Jinghua, do you have any further progress on this ? Could you please kindly update ? Thanks a lot !
Flags: needinfo?(Jinghua.Xing)
Assignee | ||
Comment 31•10 years ago
|
||
jaoo: I have open a pull request for this issue. Can you help me to review it? Thank you.
Attachment #8463934 -
Flags: review?(josea.olivera)
Flags: needinfo?(Jinghua.Xing)
Comment 33•10 years ago
|
||
Hi Gabriele, Since Jose is on PTO would you be comfortable to help on the review?
Flags: needinfo?(gsvelto)
Comment 34•10 years ago
|
||
(In reply to Bruce Huang [:bhuang] <bhuang@mozilla.com> from comment #33) > Hi Gabriele, > Since Jose is on PTO would you be comfortable to help on the review? Sure, I can do this.
Flags: needinfo?(gsvelto)
Comment 35•10 years ago
|
||
1.4 is not taking new features. Patch will be reviewed for master
blocking-b2g: 1.4? → -
Comment 36•10 years ago
|
||
Comment on attachment 8463934 [details]
pull request for this issue
Redirecting the review, I will do it ASAP.
Attachment #8463934 -
Flags: review?(josea.olivera) → review?(gsvelto)
Comment 37•10 years ago
|
||
Comment on attachment 8463934 [details]
pull request for this issue
Sorry for the delay; I have also been on PTO for a few weeks and couldn't finish the review before. The patch logic is mostly sound but the unit-tests require some serious overhauling. I've put extensive comments on the GitHub PR but if you need more information you can reach me at my e-mail address.
Attachment #8463934 -
Flags: review?(gsvelto) → review-
Flags: needinfo?(josea.olivera)
Assignee | ||
Comment 38•10 years ago
|
||
Comment on attachment 8463934 [details]
pull request for this issue
Hi Gabriele:
Thank you for your review for my pull request. I have change the code according to your advices. Can you help me to review my modification? If there are still mistakes in my code, please let me know without hesitation.
Thank you.
Attachment #8463934 -
Flags: review- → review?(gsvelto)
Comment 39•10 years ago
|
||
Comment on attachment 8463934 [details]
pull request for this issue
LGTM, feel free to land on a green Try.
Attachment #8463934 -
Flags: review?(gsvelto) → review+
Comment 40•10 years ago
|
||
Gabriele, Can you review if this patch will be directly applicable to 1.4 too? A customer is looking at 1.4 deployment of DSDS.
Flags: needinfo?(gsvelto)
Comment 41•10 years ago
|
||
(In reply to rdandu from comment #40) > Gabriele, Can you review if this patch will be directly applicable to 1.4 > too? A customer is looking at 1.4 deployment of DSDS. Sure, I'll do and since the author hasn't landed this yet I'll also land it myself.
Flags: needinfo?(gsvelto)
Comment 42•10 years ago
|
||
Landed on gaia/master 05176f64f15efd92c654d0eed801991ab2199bf8 https://github.com/mozilla-b2g/gaia/commit/05176f64f15efd92c654d0eed801991ab2199bf8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 43•10 years ago
|
||
The patch here can be cherry picked cleanly to v2.0 but not to v1.4. It depends on bug 1015875 and bug 1023338 and neither of those is 1.4+ so getting approval for that won't be trivial. Could you handle the approval requests? I'm terribly busy right now but once you we have approval I can handle all the uplifting to v1.4 myself.
Flags: needinfo?(rdandu)
Comment 44•10 years ago
|
||
Reverted for Linter failures. https://github.com/mozilla-b2g/gaia/commit/feb85187bd77862a48a35970b11a7b21c27b19b7 https://tbpl.mozilla.org/php/getParsedLog.php?id=46096639&tree=B2g-Inbound
Assignee | ||
Comment 45•10 years ago
|
||
I feel so sorry for the linter failures caused by my careless. I have modified the code for this issue and opened a new pull request for it. Can you help me to review it again and land it if it is right? Thank you.
Attachment #8463934 -
Attachment is obsolete: true
Attachment #8474333 -
Flags: review?(gsvelto)
Comment 46•10 years ago
|
||
Comment on attachment 8474333 [details] pull request for this issue (In reply to jinghua.xing from comment #45) > I feel so sorry for the linter failures caused by my careless. I am to blame since I reviewed the patch and forgot to run the linter and I must have missed it in the try run. Anyway there's some intermittent issues still happening so I'll be waiting until Try runs green before merging this: https://tbpl.mozilla.org/?rev=2cd9b61a371fa1bf8968b2c7cbaed6134a502851&tree=Gaia-Try
Attachment #8474333 -
Flags: review?(gsvelto) → review+
Comment 47•10 years ago
|
||
Re-landed, this time it should be fine, sorry for the inconvenience. https://github.com/mozilla-b2g/gaia/commit/b33b4d9558e0b9eabbfda7be23435e2b38fd40bf
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
QA Whiteboard: [COM=RIL]
Assignee | ||
Comment 48•10 years ago
|
||
Hi gsvelto: As the change is landed on the master branch, can you help me uplift it to the v1.4 branch? This change is related to bug1023338、bug1015875 and bug947192. And when I looking the bug1023338, I find that there is no gaia/shared/js/uuid.js file in v1.4 branch.Please handle these at the same time. Thank you.
Flags: needinfo?(gsvelto)
Comment 49•10 years ago
|
||
(In reply to jinghua.xing from comment #48) > As the change is landed on the master branch, can you help me uplift it to > the v1.4 branch? Yes, I've asked :rdandu if he can handle the approval process since this requires uplifting more bugs too, see comment 43. > This change is related to bug1023338、bug1015875 and bug947192. And when I > looking the bug1023338, I find that there is no gaia/shared/js/uuid.js file > in v1.4 branch.Please handle these at the same time. Yes, as I've already mentioned this requires uplifting bug 1015875 and bug 1023338 too. I'm not sure if bug 947192 is a dependency too but it might. All of this makes uplifting this patch quite complicated so late in the v1.4 branch.
Flags: needinfo?(gsvelto)
Updated•10 years ago
|
Comment 50•10 years ago
|
||
We're not pulling into 1.4, due to added risk due to complication of uplifting on 1.4, and customer not mandating this.
Flags: needinfo?(rdandu)
Comment 51•10 years ago
|
||
Checked on Flame, Message received, APN installed for SIM2 and device can make data call using that APn. Gaia-Rev 55ce3612bd8a8665139d6b85114ce59993a3fa0a Gecko-Rev https://hg.mozilla.org/releases/mozilla-aurora/rev/8811060bf3fe Build-ID 20141008160203 Version 34.0a2 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141008.191730 FW-Date Wed Oct 8 19:17:41 EDT 2014 Bootloader L1TC00011840
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•