Closed Bug 1002327 Opened 6 years ago Closed 6 years ago

We are trying to get imei by dialing the number *#06# on a device, which has two sim card slots, but only one imei returns

Categories

(Firefox OS Graveyard :: Gaia::Dialer, defect)

defect
Not set

Tracking

(blocking-b2g:1.3T+, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 unaffected, b2g-v1.3 wontfix, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 verified, b2g-v2.1 verified)

RESOLVED FIXED
2.0 S5 (4july)
blocking-b2g 1.3T+
Tracking Status
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- wontfix
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: shiwei.zhang, Assigned: gsvelto)

References

Details

(Keywords: late-l10n, Whiteboard: [sprd309631][sprd318592][sprd324092])

Attachments

(8 files, 8 obsolete files)

46 bytes, text/x-github-pull-request
Details | Review
2.94 KB, patch
Details | Diff | Splinter Review
42.92 KB, patch
etienne
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
gsvelto
: review+
Details | Review
2.35 KB, patch
Details | Diff | Splinter Review
46 bytes, patch
etienne
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
etienne
: review+
Details | Review
46 bytes, text/x-github-pull-request
Details | Review
Steps to reproduce:

I'm from Spreadtrum corporation and we are now developing engmode on version v1.3.

The window.navigator.mozMobileConnection.sendMMI('*#06#') fuction works well on one sim card version. We could get the imei number from the dialer interface or the setting app.

But when we try to use the function to get the imei on two sim card slots device,it returns only one imei.


Actual results:

Only one imei returned

Expected results:

Two imei returned, because there are two sim card slots.

Pls give me a hand .

Tks a lot.
Summary: We trying to get imei by dialing the number *#06# on a device, which has two sim card slots, but only one imei returns → We are trying to get imei by dialing the number *#06# on a device, which has two sim card slots, but only one imei returns
The MMI-related code is not DSDS-aware yet, see the code here:

https://github.com/mozilla-b2g/gaia/blob/b67e0617cf215ec7110a5b3f8a1ecb3c4801ce46/apps/communications/dialer/js/mmi.js#L36

This should be relatively easy to change however provided we know what's the desired behavior. One of the reasons we haven't done this yet is because we wanted to wait for bug 889737 to be fixed; once that was done we wouldn't need MMI-specific code anymore but I'm not sure how far away that work is from completion.
See Also: → 889737
Thanks for your attention,
If convenient,please see further information at the link
http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=304729

and you can check the patch in the attachment if necessary.
(In reply to Shiwei Zhang from comment #2)
> Thanks for your attention,
> If convenient,please see further information at the link
> http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=304729
> 
> and you can check the patch in the attachment if necessary.

Thanks for the link; I've checked the patch and I was actually thinking of a more complete fix that would properly send MMI codes to the current default SIM. I'll try to come up with a patch quickly.
Status: UNCONFIRMED → NEW
Ever confirmed: true
thanks and look forward to your good news .
This unit tests are still largely broken so don't look at them just yet. For the rest what I did was to add a |cardIndex| parameter to the MmiManager functions and then used it to pick the right connection. Since MMI interaction works in sessions where the user can interact I made it so that the MmiManager will refuse starting a new session if there's an old one still going on. This allows me to cache the connection used for the session and reduces the amount of changes required. I've modified the text shown by the UI to also reflect the presence of more than one SIM (if more than one is present that is).
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Attachment #8418126 - Flags: feedback?(etienne)
blocking-b2g: --- → 1.3T?
Duplicate of this bug: 1006988
triage: 1.3T+, both SIMs should support MMI codes. thanks
blocking-b2g: 1.3T? → 1.3T+
Comment on attachment 8418126 [details] [diff] [review]
[PATCH WIP] Make MMI code DSDS-aware

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

::: apps/communications/dialer/js/mmi.js
@@ +336,5 @@
>          }
> +
> +        return;
> +
> +      case 'ussdreceived':

Double checking: are we guaranteed to get the ussdreceived event even if we add the listener a few ticks after the ussd-received system message?

And if we do, I think we still need to make sure to call |MmiManager.init()| in the system message handler otherwise we won't be listening in the case of a cold launch.

We have to keep in mind that on some operators you can get a ussd message *without* sending one first.
Attachment #8418126 - Flags: feedback?(etienne)
(In reply to Etienne Segonzac (:etienne) from comment #8)
> Double checking: are we guaranteed to get the ussdreceived event even if we
> add the listener a few ticks after the ussd-received system message?

I'm not sure, that's a good point in fact.

> And if we do, I think we still need to make sure to call |MmiManager.init()|
> in the system message handler otherwise we won't be listening in the case of
> a cold launch.
> 
> We have to keep in mind that on some operators you can get a ussd message
> *without* sending one first.

I wasn't aware of this, I need to rethink it then and inspect the system message to see if it carries information about the connection it's related to. If it doesn't then I'll have to fix bug 942740 first.
Depends on: 942740
This is a working patch with unit-tests and all, a couple of notes though:

- The ussd-received event is now handled only in the main dialer code, I've removed the ussdreceived listeners from the mobile connections object because they were useless. For this to work properly however you'll need the patch for bug 942740 which adds a |serviceId| field to the ussd-received system message.

- This allows the changes in the MmiManager to be smaller but not really self-contained. IMHO this code is in dire need of refactoring but it won't happen this week.

- The unit-tests have been fixed by moving pieces of the existing mock directly into the test code and removing the old mozMobileConnection mock in favor of the new shared one. The tests aren't pretty - just like they weren't before - but they work and I've added some to cover the new functionality.

- I tested this on a single-SIM master device and I'm now in the process of trying this on my Tarako with the v1.3t branch. Unfortunately I don't have a multi-SIM device on which to try the master build so if anybody can test it there it would be appreciated.
Attachment #8418126 - Attachment is obsolete: true
Attachment #8419469 - Flags: review?(etienne)
This is an adapted patch for v1.3t, it required a few changes compared to the master patch and some are ugly but it seems to work. It also needs a Gecko patch if you want to test it, I'll attach it shortly.
This is the gecko patch required for testing attachment 8419490 [details] [diff] [review] on v1.3t, it's a hacked version of the patch for bug 942740 that should apply cleanly. This will *not* be the final gecko patch, I'm attaching it only for testing.
(just started the review, but the late l10n could be an issue)
Keywords: late-l10n
Comment on attachment 8419469 [details] [diff] [review]
[PATCH] Make the MMI code DSDS-aware

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

Ping me on IRC for an expedited follow up review :)

::: apps/communications/dialer/js/mmi.js
@@ +269,5 @@
>        if (message == null && !sessionEnded) {
>          return;
>        }
>  
> +      var conn = navigator.mozMobileConnections[cardIndex];

wonder if we could default to cardIndex 0 when the cardIndex parameter is undefined (ie. on phones without the gecko patch) to make landing easier.

::: apps/communications/dialer/test/unit/mmi_test.js
@@ +55,5 @@
> +    realMobileConnections = navigator.mozMobileConnections;
> +    navigator.mozMobileConnections = MockNavigatorMozMobileConnections;
> +
> +    /* Replace the default mock connection with our own specialized version
> +     * tailored for this suite of tests. */

this was really brave :)

@@ +438,5 @@
> +        assert.equal(MmiManager._ui._messageReceived, MMI_MSG);
> +        assert.isTrue(MmiManager._ui._sessionEnded);
> +        done();
> +      }, TINY_TIMEOUT);
> +    });

I _think_ we're not covering sending a MMI on the second SIM, and since cardIndexForConnection default to 0, I think we should :)

::: apps/communications/dialer/test/unit/mock_mozMobileConnection.js
@@ -1,1 @@
> -'use strict';

I think removing this might cause the telephony_helper_test.js to fail...
Attachment #8419469 - Flags: review?(etienne)
I've addressed all the review comments and I'll try to attach a v1.3t version of this patch.
Attachment #8419469 - Attachment is obsolete: true
Attachment #8419490 - Attachment is obsolete: true
Attachment #8420122 - Flags: review?(etienne)
Comment on attachment 8420122 [details] [diff] [review]
[PATCH v2] Make the MMI code DSDS-aware

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

all good!
Thank you for championing through this one with only one good hand :)
Attachment #8420122 - Flags: review?(etienne) → review+
Thanks for the review, pushed to gaia/master 7fe1f921349c8bee9b2514972b3882ee4ae1b221, the green Travis run is here: https://travis-ci.org/mozilla-b2g/gaia/builds/24802301

I'll need some help for the uplift because with only one good hand I'm having some serious trouble in keeping up with the coding effort. Attachment 8419490 [details] [diff] is missing the changes I did on the v2 patch which are small and shouldn't be too hard to integrate. The changes make it possible to land the patch before the gecko one in bug 942740 and for the rest affect only the unit tests. Any help with this will be greatly appreciated.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
I can help out. I used the interdiff here to create a 1.3t PR. 

https://bugzilla.mozilla.org/attachment.cgi?oldid=8419469&action=interdiff&newid=8420122&headers=1

Gabriele, let me know if this is correct, or if I should make changes. I hope your hand gets better soon!
Attachment #8420620 - Flags: review?(gsvelto)
Comment on attachment 8420620 [details] [review]
[PULL REQUEST gaia/v1.3t] V2 - Make the MMI code DSDS-aware

Excellent, thanks Michael!

I've verified that this works correctly on my Tarako; the only minor issue is that due to how Gecko responds to MMI messages on v1.3t the SIM id won't be shown in the response header. We could work around this but I don't think it's worth the fuss. The more relevant case of unsolicited USSD messages will have the SIM id once bug 942740 is uplifted.
Attachment #8420620 - Flags: review?(gsvelto) → review+
Merged to gaia/v1.3t cf039fa2749a2e0f25357e4c8ca97170ad670073
Cherry-picked patch for v1.4, this required a few changes though not as much as the one for v1.3t.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: MMI codes cannot be sent on the second SIM
[Testing completed]: Tested on a device and covered the new functionality with unit-tests
[Risk to taking this patch] (and alternatives if risky): While no regressions were found testing the modified MMI code there is the possibility that some corner-case I couldn't test broke though it should be pretty slim as the patch modifies as little logic as possible
[String changes made]: Added the 'mmi-notification-title-with-sim' string to apps/communications/dialer/locales/dialer.en-US.properties
Attachment #8420803 - Flags: approval-gaia-v1.4?(release-mgmt)
Whiteboard: [sprd309631]
Comment on attachment 8420803 [details] [diff] [review]
[PATCH gaia/v1.4] Make the MMI code DSDS-aware

Taking for 1.4
Attachment #8420803 - Flags: approval-gaia-v1.4?(release-mgmt) → approval-gaia-v1.4+
I'm adding bug 990003 as a dependency because we need to uplift that bug before we can uplift this one.
Depends on: 990003
Whiteboard: [sprd309631] → [sprd309631][1.4-approval-needed][sprd318592]
(In reply to Gabriele Svelto [:gsvelto] from comment #24)
> I'm adding bug 990003 as a dependency because we need to uplift that bug
> before we can uplift this one.

Mind if you can elaborate more about the dependency with 990003?  I see this bug is already landed in master (2.0) without 990003. So I wonder why we cannot do the same for v1.4. Thanks!
Flags: needinfo?(gsvelto)
(In reply to Ivan Tsay (:ITsay) from comment #25)
> Mind if you can elaborate more about the dependency with 990003?  I see this
> bug is already landed in master (2.0) without 990003. So I wonder why we
> cannot do the same for v1.4. Thanks!

Bug 990003 landed on master three weeks before this bug, see this:

https://github.com/mozilla-b2g/gaia/commit/7afce8a6e965a45f38988227828ccdfb315394f3

And this:

https://github.com/mozilla-b2g/gaia/commit/7fe1f921349c8bee9b2514972b3882ee4ae1b221
Flags: needinfo?(gsvelto)
Thanks. I am asking because I did not see the status-b2g-v2.0=fixed in bug 990003.
I'm afraid the issue is unresolved, still only one imei return in my experiments.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch two_imeis_patchSplinter Review
Use this patch could completely solve the problem , but need a review.
(In reply to Ivan Tsay (:ITsay) from comment #27)
> Thanks. I am asking because I did not see the status-b2g-v2.0=fixed in bug
> 990003.

https://bugzilla.mozilla.org/show_bug.cgi?id=990003#c62 is marked RESOLVED FIXED, which was dated 2014-04-15.  that means it landed in the master/m-c branch, which was 2.0 at the time.    There's no need to add the "status-b2g-v2.0=fixed" flag since it landed before the merge.
I'm a bit confused: are we taking string changes for 1.4 at this point (comment 22)? 
I was triaging bugs with late-l10n, but that's already for 2.0 at this point :-\
(In reply to Shiwei Zhang from comment #28)
> I'm afraid the issue is unresolved, still only one imei return in my
> experiments.

Yes, my change here didn't completely fix the issue but let's keep this bug closed as we've already landed changes here. We're tracking the follow-up work in bug 1019370, I'll CC you there.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
This bug has v1.4 uplift approval, but bug 990003 hasn't. We should uplift that one first then right? We want this patch in 1.4 for Dolphin release.
Flags: needinfo?(gsvelto)
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #33)
> This bug has v1.4 uplift approval, but bug 990003 hasn't. We should uplift
> that one first then right? We want this patch in 1.4 for Dolphin release.

Yes, bug 990003 has so many other benefits besides this dependency that I think it's a must have for 1.4.
Flags: needinfo?(gsvelto)
Comment on attachment 8420803 [details] [diff] [review]
[PATCH gaia/v1.4] Make the MMI code DSDS-aware

Removing the approval here since the dependency is not going to be taken into 1.4.
Attachment #8420803 - Flags: approval-gaia-v1.4+
Hi Etienne

I think this patch "Make the MMI code DSDS-aware" has a bug in it.
When ffos receive a ussd menu, there will be no response if the menu is clicked.
I think the "eventlistener" removed is the cause.

If i add the "eventlistener", it will work normal.
Can this patch be granted? or some other code should be modified?

Thanks.
Flags: needinfo?(etienne)
Whiteboard: [sprd309631][1.4-approval-needed][sprd318592] → [sprd309631][1.4-approval-needed][sprd318592][sprd324092]
Comment on attachment 8441254 [details] [diff] [review]
add_eventlistener_of_mmi_reply.patch

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

Thanks, can you put it in the LazyLoader callback in the init method?
We want to add it only once, currently it'll be added each time init is called so we'll have duplicates.
Attachment #8441254 - Flags: feedback+
Hi Etienne

Your kindly reminder is right, Thanks.
I have modified this patch, please help to check it.

Thanks a lot.
Attachment #8441254 - Attachment is obsolete: true
should we file a new bug for comment 36 or reopen this bug? it seems we are reviewing the new patch here.
Comment on attachment 8441782 [details] [diff] [review]
add_eventlistener_of_mmi_reply_2.patch

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

Please use the review flag to make sure I get to the review quickly :)
This looks good!
Attachment #8441782 - Flags: review+
(In reply to pcheng from comment #39)
> should we file a new bug for comment 36 or reopen this bug? it seems we are
> reviewing the new patch here.

We can land this as "Follow up to bug 1002327", but if opening a new bug makes the uplift/branch tracking easier feel free to do so.
Flags: needinfo?(etienne)
(In reply to Etienne Segonzac (:etienne) from comment #40)
> Please use the review flag to make sure I get to the review quickly :)

Ok, thanks. I will pay attention to the next.

By the way, Is there anything I need to do next? 
Do I need to commit a pull request?
Thanks.
Flags: needinfo?(etienne)
(In reply to wei.gao from comment #42)
> (In reply to Etienne Segonzac (:etienne) from comment #40)
> > Please use the review flag to make sure I get to the review quickly :)
> 
> Ok, thanks. I will pay attention to the next.
> 
> By the way, Is there anything I need to do next? 
> Do I need to commit a pull request?
> Thanks.

Yes please open a pull request and add it as an attachment here with review? :etienne
Flags: needinfo?(etienne)
Hi Etienne

This is a pull request for this bug on v1.3t
Please help to review.

Thanks.
Attachment #8441782 - Attachment is obsolete: true
Attachment #8443440 - Flags: review?(etienne)
Comment on attachment 8443440 [details] [diff] [review]
(v1.3t) Add EventListener for mmi reply

Please open a pull request against gaia/master where it needs to land first, we'll then uplift it to 1.3t.
Attachment #8443440 - Flags: review?(etienne)
Blocks: 1019783
Hi Etienne

This is a pull request for this bug on gaia/master.
Can this patch be granted? Please help to review.

Thanks.
Attachment #8445673 - Flags: review?(etienne)
Comment on attachment 8443440 [details] [diff] [review]
(v1.3t) Add EventListener for mmi reply

>https://github.com/mozilla-b2g/gaia/pull/20802
Attachment #8443440 - Attachment description: add eventlistener for mmi reply → (v1.3t) Add EventListener for mmi reply
Attachment #8443440 - Attachment is patch: true
Attachment #8443440 - Attachment mime type: text/x-github-pull-request → text/plain
(In reply to wei.gao from comment #46)
> Created attachment 8445673 [details] [review]
> (master) Add EventListener for mmi reply.

There was a failure on Travis for this PR but it looked like an intermittent so I've retriggered the job.
Comment on attachment 8445673 [details] [review]
(master) Add EventListener for mmi reply.

Thanks!
Attachment #8445673 - Flags: review?(etienne) → review+
(In reply to Gabriele Svelto [:gsvelto] from comment #48)
> (In reply to wei.gao from comment #46)
> > Created attachment 8445673 [details] [review]
> > (master) Add EventListener for mmi reply.
> 
> There was a failure on Travis for this PR but it looked like an intermittent
> so I've retriggered the job.

Hi Gabriele

I don't know why this happen, my other pull request also has some different failure, what could I do for this?
Thanks.
(In reply to wei.gao from comment #50)
> I don't know why this happen, my other pull request also has some different
> failure, what could I do for this?

It's an intermittent issue so re-triggering the Travis job usually fixes the problem. In the case of your master PR the Travis run has now turned green so you can merge it. I've re-triggered the job for your v1.3t PR too, let's see if that also goes well.
(In reply to Gabriele Svelto [:gsvelto] from comment #51)
> It's an intermittent issue so re-triggering the Travis job usually fixes the
> problem. In the case of your master PR the Travis run has now turned green
> so you can merge it. I've re-triggered the job for your v1.3t PR too, let's
> see if that also goes well.

Hi Gabriele

The Travis run of v1.3t has also now turned green. Thanks.
By the way, can I retrigger it by myself? My another pull request also has this issue. I don't know how to retrigger it.
https://github.com/mozilla-b2g/gaia/pull/20953

I also want to know, after the Travis run turning green and review+ is tagged, is the patch merged into automatic?

Thanks a great.
(In reply to wei.gao from comment #52)
> By the way, can I retrigger it by myself? My another pull request also has
> this issue. I don't know how to retrigger it.

Yes, you need to be logged in with Travis, then go into the job page (e.g. https://travis-ci.org/mozilla-b2g/gaia/jobs/28377129 in this case), click on the failed job and then click on the "Restart" icon in the top right corner, next to the job tab.
(In reply to Gabriele Svelto [:gsvelto] from comment #53)
> Yes, you need to be logged in with Travis, then go into the job page (e.g.
> https://travis-ci.org/mozilla-b2g/gaia/jobs/28377129 in this case), click on
> the failed job and then click on the "Restart" icon in the top right corner,
> next to the job tab.

Hi Gabriele
Thanks for your kindly help. I will try as you say.
Comment on attachment 8443440 [details] [diff] [review]
(v1.3t) Add EventListener for mmi reply

Hi Etienne

Can you help to review v1.3t patch now?
Thanks a lot.
Attachment #8443440 - Flags: review?(etienne)
Attachment #8443440 - Flags: review?(etienne) → review+
(In reply to Etienne Segonzac (:etienne) from comment #57)
> And on 1.3t:
> https://github.com/mozilla-b2g/gaia/commit/
> 925100f6f7b8dbe158d5921d01404ea823f4e64e

Hi Etienne
Thanks for your kindly help.
Have a good day.
Blocks: 1019370
Blocks: 1016885
Comment on attachment 8420803 [details] [diff] [review]
[PATCH gaia/v1.4] Make the MMI code DSDS-aware

Re-requesting approval now that bug 990003 has landed; see comment 22 for details. As an additional note this blocks bug 1016885 and the latter affects the dolphin so I'm going to request approval for that one too.
Attachment #8420803 - Flags: approval-gaia-v1.4?(release-mgmt)
This bug is marked at late-l10n and there is a string change as Gabriele identified. However, this patch landed on 1.3T a while ago. Is there an l10n impact to landing this fix on 1.4?
Flags: needinfo?(l10n)
Flags: needinfo?(gsvelto)
Flags: needinfo?(francesco.lodolo)
(In reply to Lawrence Mandel [:lmandel] from comment #60)
> This bug is marked at late-l10n and there is a string change as Gabriele
> identified. However, this patch landed on 1.3T a while ago. Is there an l10n
> impact to landing this fix on 1.4?

Yes, the patch adds the 'mmi-notification-title-with-sim = ({{sim}}) {{title}}' string for localizing the header of MMI messages.
Flags: needinfo?(gsvelto)
(In reply to Lawrence Mandel [:lmandel] from comment #60)
> This bug is marked at late-l10n and there is a string change as Gabriele
> identified. However, this patch landed on 1.3T a while ago. Is there an l10n
> impact to landing this fix on 1.4?

1.3T is a different beast in terms of localization, but it has impact on both 1.4 and 2.0, so we should try to find a string-free approach.

Given that the string is "({{sim}}) {{title}}" (no localizable elements beside order and parenthesis), I'd be fine with this being identical for all locales. That means either provide the string in JS without a string ID exposed in .properties files, or use the approach of bug 990537 comment 69
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(l10n)
(In reply to Francesco Lodolo [:flod] from comment #62)
> Given that the string is "({{sim}}) {{title}}" (no localizable elements
> beside order and parenthesis), I'd be fine with this being identical for all
> locales. That means either provide the string in JS without a string ID
> exposed in .properties files, or use the approach of bug 990537 comment 69

This already landed on v1.3t with the string change, so for the v1.4 uplift I should go for a string-free approach instead?
(In reply to Gabriele Svelto [:gsvelto] from comment #63)
> (In reply to Francesco Lodolo [:flod] from comment #62)
> > Given that the string is "({{sim}}) {{title}}" (no localizable elements
> > beside order and parenthesis), I'd be fine with this being identical for all
> > locales. That means either provide the string in JS without a string ID
> > exposed in .properties files, or use the approach of bug 990537 comment 69
> 
> This already landed on v1.3t with the string change, so for the v1.4 uplift
> I should go for a string-free approach instead?

Yes. We manage the localization for 1.4 and 2.0 (so we don't break string freeze), that's not the case for 1.3T.
Comment on attachment 8420803 [details] [diff] [review]
[PATCH gaia/v1.4] Make the MMI code DSDS-aware

As per comment 63 and comment 64, we need to go with a string free approach. 1.4-
Attachment #8420803 - Flags: approval-gaia-v1.4?(release-mgmt) → approval-gaia-v1.4-
Cherry-picked PR for v1.4, this includes attachment 8445673 [details] [review] and doesn't contain string changes.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: MMI codes cannot be sent on the second SIM
[Testing completed]: Tested on a device and covered the new functionality with unit-tests
[Risk to taking this patch] (and alternatives if risky): The only regression found was already addressed in the follow up present in this patch.
[String changes made]: None
Attachment #8420803 - Attachment is obsolete: true
Attachment #8451275 - Flags: approval-gaia-v1.4?(release-mgmt)
Comment on attachment 8451275 [details] [review]
[PULL REQUEST] v1.4 uplift

Thank you for reworking with a string free approach. 1.4+
Attachment #8451275 - Flags: approval-gaia-v1.4?(release-mgmt) → approval-gaia-v1.4+
Thanks for the approval, merged to gaia/v1.4 f2c118767aa26981386570e5d0bed95bab593de5

https://github.com/mozilla-b2g/gaia/commit/f2c118767aa26981386570e5d0bed95bab593de5
This still needs to land on v2.0 doesn't it? I'm pretty confused by the history of this bug, though. Seems we should have filed a new bug for file-up work instead of piling things into one place.
Flags: needinfo?(gsvelto)
Whiteboard: [sprd309631][1.4-approval-needed][sprd318592][sprd324092] → [sprd309631][sprd318592][sprd324092]
Target Milestone: --- → 2.0 S5 (4july)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #69)
> This still needs to land on v2.0 doesn't it? I'm pretty confused by the
> history of this bug, though. Seems we should have filed a new bug for
> file-up work instead of piling things into one place.

Yes, we still need to land this on 2.0. Do we need explicit approval for that?
Flags: needinfo?(gsvelto)
(In reply to Gabriele Svelto [:gsvelto] from comment #70)
> Yes, we still need to land this on 2.0. Do we need explicit approval for
> that?

Yes since 1.3T+ doesn't grant it.
Flags: needinfo?(gsvelto)
Duplicate of this bug: 1035743
Attached file [PULL REQUEST] v2.0 uplift (obsolete) —
See comment 66 for more info. For v2.0 the existing patches apply cleanly and do not require modifications.
Attachment #8452361 - Flags: approval-gaia-v2.0?(release-mgmt)
Flags: needinfo?(gsvelto)
Attachment #8452361 - Flags: approval-gaia-v2.0?(release-mgmt) → approval-gaia-v2.0+
(In reply to Gabriele Svelto [:gsvelto] from comment #73)
> Created attachment 8452361 [details] [review]
> [PULL REQUEST] v2.0 uplift
> 
> See comment 66 for more info. For v2.0 the existing patches apply cleanly
> and do not require modifications.
The 2.0 PR that you attached seems like is for bug 1016885 and not this bug. Is that intentional?
Flags: needinfo?(gsvelto)
(In reply to Gabriele Svelto [:gsvelto] from comment #70)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #69)
> > This still needs to land on v2.0 doesn't it? I'm pretty confused by the
> > history of this bug, though. Seems we should have filed a new bug for
> > file-up work instead of piling things into one place.
> 
> Yes, we still need to land this on 2.0. Do we need explicit approval for
> that?

This bug is already available in 2.0 as the merge to master happened before the 2.0 branch.
(In reply to Anshul from comment #74)
> The 2.0 PR that you attached seems like is for bug 1016885 and not this bug.
> Is that intentional?

No, it was confusion on my part, this belongs to bug 1016885. Let's call it uplift fatigue, I've spent the past two weeks landing stuff on four branches at the same time :-/

(In reply to Anshul from comment #75)
> This bug is already available in 2.0 as the merge to master happened before
> the 2.0 branch.

The original patch did but the follow up in attachment 8445673 [details] [review] didn't so I'm going to ask approval for that and clean up the other attachments / flags.
Flags: needinfo?(gsvelto)
Attachment #8452361 - Attachment is obsolete: true
Attachment #8452361 - Flags: approval-gaia-v2.0+
Comment on attachment 8445673 [details] [review]
(master) Add EventListener for mmi reply.

This follow-up patch arrived to late so only the first part in attachment 8420122 [details] [diff] [review] landed on v2.0. This is needed as the original patch broke the case were an USSD message had some interactive content.
Attachment #8445673 - Flags: approval-gaia-v2.0?(release-mgmt)
Blocks: 1036386
(In reply to Gabriele Svelto [:gsvelto] from comment #77)
> Comment on attachment 8445673 [details] [review]
> (master) Add EventListener for mmi reply.
> 
> This follow-up patch arrived to late so only the first part in attachment
> 8420122 [details] [diff] [review] landed on v2.0. This is needed as the
> original patch broke the case were an USSD message had some interactive
> content.

wait, this attachment(attachment 8420122 [details] [diff] [review] ) looks like its adding new strings, is that right ?
(In reply to Francesco Lodolo [:flod] from comment #79)
> That already landed on 2.0, or at least I see the string there
> https://github.com/mozilla-b2g/gaia/blob/v2.0/apps/communications/dialer/
> locales/dialer.en-US.properties#L54

Yes, the first part (attachment 8420122 [details] [diff] [review]) had already landed before v2.0 was branched of so it's already present in v2.0. Only the second part (attachment 8445673 [details] [review]) needs to be uplifted and it's a one line change w/o any new strings in it.
Attachment #8445673 - Flags: approval-gaia-v2.0?(release-mgmt) → approval-gaia-v2.0+
Thanks for the approval and thanks to everybody who contributed to this bug, we're done here finally.

Landed on gaia/v2.0 84f59d7b26851e583b9de5a7ba4c56b7427abfea

https://github.com/mozilla-b2g/gaia/commit/84f59d7b26851e583b9de5a7ba4c56b7427abfea
Looks good on today's flame builds.
flame v2.0 aurora
Gaia      8a8622d58af2c24ef8df1c7fc7aa9cda50ac0085
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/05c9d0a949bc
BuildID   20140717160202
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
B1TC00011220

flame v2.1 central
Gecko     https://hg.mozilla.org/mozilla-central/rev/f77a9f825427
BuildID   20140717160201
Version   33.0a1
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
B1TC00011220
Attached file pull request two IMEIs (obsolete) —
This issue is NOT fixed without these modifications on v1.3t, please give a review.
Attachment #8459443 - Flags: review?(jmcf)
Attachment #8459443 - Flags: review?(jmcf) → review?(etienne)
(In reply to Shiwei Zhang from comment #83)
> This issue is NOT fixed without these modifications on v1.3t, please give a
> review.

The issue was actually fixed in bug 1019783, the patch there uses promises though so it's not suitable for v1.3t and truly enough the bug that was originally described in this issue is not fixed there yet. Etienne do you think we could take a V1.3T-specific patch here or should I adapt the one in bug 1019783? That bug was a 1.4+ but I think we could consider it 1.3T+ because this problem was originally reported here and this bug is indeed a 1.3T+ blocker.
(In reply to Gabriele Svelto [:gsvelto] from comment #84)
> (In reply to Shiwei Zhang from comment #83)
> > This issue is NOT fixed without these modifications on v1.3t, please give a
> > review.
> 
> The issue was actually fixed in bug 1019783, the patch there uses promises
> though so it's not suitable for v1.3t and truly enough the bug that was
> originally described in this issue is not fixed there yet. Etienne do you
> think we could take a V1.3T-specific patch here or should I adapt the one in
> bug 1019783? That bug was a 1.4+ but I think we could consider it 1.3T+
> because this problem was originally reported here and this bug is indeed a
> 1.3T+ blocker.

Making a 1.3+ version of bug 1019783 and uplifiting it is the ideal solution imo.
Attachment #8459443 - Flags: review?(etienne)
(In reply to Shiwei Zhang from comment #83)
> This issue is NOT fixed without these modifications on v1.3t, please give a
> review.

I fixed this by uplifting bug 1019783, see bug 1019783 comment 37. Please verify is this is now completely fixed on v1.3t.
Flags: needinfo?(shiwei.zhang)
Attachment #8459443 - Attachment is obsolete: true
(In reply to Gabriele Svelto [:gsvelto] from comment #86)
> (In reply to Shiwei Zhang from comment #83)
> > This issue is NOT fixed without these modifications on v1.3t, please give a
> > review.
> 
> I fixed this by uplifting bug 1019783, see bug 1019783 comment 37. Please
> verify is this is now completely fixed on v1.3t.

Thank you Gabriele, I will verify it as soon as possible.
Flags: needinfo?(shiwei.zhang)
Hi Gabriele:
   There is a problem about imei in FFOS1.4,I am confused about it,hope you can give me a hand,thank you.
1) when you reopen the telephone which have no sim,you dailer *#06#,it will return a error,Generic Failure.after this,it work well.but after you restart the telephone,it will appear again。

2) if the telephone have two sim,this issue will not happen.

3) I take some logs,from the log,It all because the first time you dailer *#06#,the result.serviceCode is null.then it will give a error.

4) related code:
var request = navigator.mozMobileConnections[cardIndex].sendMMI('*#06#');
request.onsuccess = function mm_onGetImeiSuccess(event) {
var result = event.target.result;
if ((result === null) || (result.serviceCode !== 'scImei') ||
            (result.statusMessage === null)) {
          reject(new Error('Could not retrieve the IMEI code for SIM' +
                           cardIndex));

5) log information:
app://communications.gaiamobile.org/dialer/js/mmi.js:609 in mm_onGetImeiSuccess: yetta result=={"statusMessage":"352751018441570"}

        }

thank you for your help.
Flags: needinfo?(gsvelto)
This sounds like a RIL problem: the first request fails - possibly because the RIL is not fully initialized yet? - and then the second one succeeds. You should probably extract the RIL logs to verify what's going on but this is not a dialer problem. If it persists it would be better to open a 1.4 specific bug for this including the information on which device you used for testing and exact gecko and gaia versions.
Flags: needinfo?(gsvelto)
(In reply to Gabriele Svelto [:gsvelto] from comment #89)
> This sounds like a RIL problem: the first request fails - possibly because
> the RIL is not fully initialized yet? - and then the second one succeeds.
> You should probably extract the RIL logs to verify what's going on but this
> is not a dialer problem. If it persists it would be better to open a 1.4
> specific bug for this including the information on which device you used for
> testing and exact gecko and gaia versions.
Hi Gabriele:
  Thank you for your advice,I will check out the RIL.

1) after you send MMI request,the result should return two IMEI,I take a log from the result.
08-01 08:18:34.980 E/GeckoConsole(  613):{"serviceCode":"scImei","statusMessage":"352273017386340"}
08-01 08:18:34.980 E/GeckoConsole(  613):{"statusMessage":"352751018441570"}
the second simlot can not get the serviceCode,can you tell me where to get the serviceCode and it used for what?

Thanks.
(In reply to ting.yang from comment #90)
> 1) after you send MMI request,the result should return two IMEI,I take a log
> from the result.
> 08-01 08:18:34.980 E/GeckoConsole( 
> 613):{"serviceCode":"scImei","statusMessage":"352273017386340"}

This is the correct message sent from here:

http://hg.mozilla.org/mozilla-central/file/f41a267983c1/dom/system/gonk/ril_worker.js#l2640

> 08-01 08:18:34.980 E/GeckoConsole(  613):{"statusMessage":"352751018441570"}
> the second simlot can not get the serviceCode,can you tell me where to get
> the serviceCode and it used for what?

|serviceCode| identifies the type of message we received, I just checked and there's one place in the code where we don't add it to the message, see here:

http://hg.mozilla.org/mozilla-central/file/f41a267983c1/dom/system/gonk/ril_worker.js#l6016

I'll file a bug about this and provide a fix ASAP. In the meantime you can fix the above by just adding this line there:

options.mmiServiceCode = MMI_KS_SC_IMEI;

Try it out and see if it works.
Hi Gabriele,
   Please see here
   http://hg.mozilla.org/mozilla-central/file/f41a267983c1/dom/system/gonk/ril_worker.js#l6748
   Maybe change the '&&' to '||' is fine.
   If there is no SIM card on device, GECKO_RADIOSTATE_READY will never happen, right?
(In reply to Gabriele Svelto [:gsvelto] from comment #91)
> (In reply to ting.yang from comment #90)
> > 1) after you send MMI request,the result should return two IMEI,I take a log
> > from the result.
> > 08-01 08:18:34.980 E/GeckoConsole( 
> > 613):{"serviceCode":"scImei","statusMessage":"352273017386340"}
> 
> This is the correct message sent from here:
> 
> http://hg.mozilla.org/mozilla-central/file/f41a267983c1/dom/system/gonk/
> ril_worker.js#l2640
> 
> > 08-01 08:18:34.980 E/GeckoConsole(  613):{"statusMessage":"352751018441570"}
> > the second simlot can not get the serviceCode,can you tell me where to get
> > the serviceCode and it used for what?
> 
> |serviceCode| identifies the type of message we received, I just checked and
> there's one place in the code where we don't add it to the message, see here:
> 
> http://hg.mozilla.org/mozilla-central/file/f41a267983c1/dom/system/gonk/
> ril_worker.js#l6016
> 
> I'll file a bug about this and provide a fix ASAP. In the meantime you can
> fix the above by just adding this line there:
> 
> options.mmiServiceCode = MMI_KS_SC_IMEI;
> 
> Try it out and see if it works.

Hi Gabriele, 

1) I have Modified the code as you suggested,it works.

2) I have filed a bug about this issue.please see
   https://bugzilla.mozilla.org/show_bug.cgi?id=1050067

Thanks.
You need to log in before you can comment on or make changes to this bug.