Shiwei Zhang Reporter
	Description • 10 years ago

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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 1 • 10 years ago

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.

	Shiwei Zhang Reporter
	Comment 2 • 10 years ago

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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 3 • 10 years ago

(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.

	Shiwei Zhang Reporter
	Comment 4 • 10 years ago

thanks and look forward to your good news .

	Gabriele Svelto [:gsvelto] Assignee
	Comment 5 • 10 years ago

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).

	Joe Cheng [:jcheng] (please needinfo)
	Comment 7 • 10 years ago

triage: 1.3T+, both SIMs should support MMI codes. thanks

	Etienne Segonzac (:etienne)
	Comment 8 • 10 years ago

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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 9 • 10 years ago

(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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 10 • 10 years ago

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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 11 • 10 years ago

Here's the PR, the Travis run is here: https://travis-ci.org/mozilla-b2g/gaia/builds/24719590

	Gabriele Svelto [:gsvelto] Assignee
	Comment 12 • 10 years ago

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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 13 • 10 years ago

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.

	Etienne Segonzac (:etienne)
	Comment 14 • 10 years ago

(just started the review, but the late l10n could be an issue)

	Etienne Segonzac (:etienne)
	Comment 15 • 10 years ago

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...

	Gabriele Svelto [:gsvelto] Assignee
	Comment 16 • 10 years ago

I've addressed all the review comments and I'll try to attach a v1.3t version of this patch.

	Etienne Segonzac (:etienne)
	Comment 17 • 10 years ago

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 :)

	Gabriele Svelto [:gsvelto] Assignee
	Comment 18 • 10 years ago

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.

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 19 • 10 years ago

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!

	Gabriele Svelto [:gsvelto] Assignee
	Comment 20 • 10 years ago

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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 21 • 10 years ago

Merged to gaia/v1.3t cf039fa2749a2e0f25357e4c8ca97170ad670073

	Gabriele Svelto [:gsvelto] Assignee
	Comment 22 • 10 years ago

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

	Preeti Raghunath(:Preeti)
	Comment 23 • 10 years ago

Comment on attachment 8420803 [details] [diff] [review]
[PATCH gaia/v1.4] Make the MMI code DSDS-aware

Taking for 1.4

	Gabriele Svelto [:gsvelto] Assignee
	Comment 24 • 10 years ago

I'm adding bug 990003 as a dependency because we need to uplift that bug before we can uplift this one.

	Ivan Tsay (:ITsay)
	Comment 25 • 10 years ago

(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!

	Gabriele Svelto [:gsvelto] Assignee
	Comment 26 • 10 years ago

(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

	Ivan Tsay (:ITsay)
	Comment 27 • 10 years ago

Thanks. I am asking because I did not see the status-b2g-v2.0=fixed in bug 990003.

	Shiwei Zhang Reporter
	Comment 28 • 10 years ago

I'm afraid the issue is unresolved, still only one imei return in my experiments.

	Shiwei Zhang Reporter
	Comment 29 • 10 years ago

Use this patch could completely solve the problem , but need a review.

	Tony Chung [:tchung]
	Comment 30 • 10 years ago

(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.

	Francesco Lodolo [:flod]
	Comment 31 • 10 years ago

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 :-\

	Gabriele Svelto [:gsvelto] Assignee
	Comment 32 • 10 years ago

(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.

	Jan Jongboom [:janjongboom] (inactive)
	Comment 33 • 10 years ago

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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 34 • 10 years ago

(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.

	Jason Smith [:jsmith]
	Comment 35 • 10 years ago

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.

	Wei Gao (Spreadtrum)
	Comment 36 • 10 years ago

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.

	Etienne Segonzac (:etienne)
	Comment 37 • 10 years ago

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.

	Wei Gao (Spreadtrum)
	Comment 38 • 10 years ago

Hi Etienne

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

Thanks a lot.

	Peipei Cheng (needinfo if you need my action)
	Comment 39 • 10 years ago

should we file a new bug for comment 36 or reopen this bug? it seems we are reviewing the new patch here.

	Etienne Segonzac (:etienne)
	Comment 40 • 10 years ago

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!

	Etienne Segonzac (:etienne)
	Comment 41 • 10 years ago

(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.

	Wei Gao (Spreadtrum)
	Comment 42 • 10 years ago

(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.

	Etienne Segonzac (:etienne)
	Comment 43 • 10 years ago

(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

	Wei Gao (Spreadtrum)
	Comment 44 • 10 years ago

Hi Etienne

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

Thanks.

	Etienne Segonzac (:etienne)
	Comment 45 • 10 years ago

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.

	Wei Gao (Spreadtrum)
	Comment 46 • 10 years ago

Hi Etienne

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

Thanks.

	Wei Gao (Spreadtrum)
	Comment 47 • 10 years ago

Comment on attachment 8443440 [details] [diff] [review]
(v1.3t) Add EventListener for mmi reply

>https://github.com/mozilla-b2g/gaia/pull/20802

	Gabriele Svelto [:gsvelto] Assignee
	Comment 48 • 10 years ago

(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.

	Etienne Segonzac (:etienne)
	Comment 49 • 10 years ago

Comment on attachment 8445673 [details] [review]
(master) Add EventListener for mmi reply.

Thanks!

	Wei Gao (Spreadtrum)
	Comment 50 • 10 years ago

(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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 51 • 10 years ago

(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.

	Wei Gao (Spreadtrum)
	Comment 52 • 10 years ago

(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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 53 • 10 years ago

(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.

	Wei Gao (Spreadtrum)
	Comment 54 • 10 years ago

(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.

	Wei Gao (Spreadtrum)
	Comment 55 • 10 years ago

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.

	Etienne Segonzac (:etienne)
	Comment 56 • 10 years ago

Landed on master:
https://github.com/mozilla-b2g/gaia/commit/8b742fde1be1226931eb198997994f4297375acd

	Etienne Segonzac (:etienne)
	Comment 57 • 10 years ago

And on 1.3t: https://github.com/mozilla-b2g/gaia/commit/925100f6f7b8dbe158d5921d01404ea823f4e64e

	Wei Gao (Spreadtrum)
	Comment 58 • 10 years ago

(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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 59 • 10 years ago

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.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 60 • 10 years ago

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?

	Gabriele Svelto [:gsvelto] Assignee
	Comment 61 • 10 years ago

(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.

	Francesco Lodolo [:flod]
	Comment 62 • 10 years ago

(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

	Gabriele Svelto [:gsvelto] Assignee
	Comment 63 • 10 years ago

(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?

	Francesco Lodolo [:flod]
	Comment 64 • 10 years ago

(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.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 65 • 10 years ago

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-

	Gabriele Svelto [:gsvelto] Assignee
	Comment 66 • 10 years ago

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 67 • 10 years ago

Comment on attachment 8451275 [details] [review]
[PULL REQUEST] v1.4 uplift

Thank you for reworking with a string free approach. 1.4+

	Gabriele Svelto [:gsvelto] Assignee
	Comment 68 • 10 years ago

Thanks for the approval, merged to gaia/v1.4 f2c118767aa26981386570e5d0bed95bab593de5

https://github.com/mozilla-b2g/gaia/commit/f2c118767aa26981386570e5d0bed95bab593de5

	Ryan VanderMeulen [:RyanVM]
	Comment 69 • 10 years ago

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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 70 • 10 years ago

(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?

	Ryan VanderMeulen [:RyanVM]
	Comment 71 • 10 years ago

(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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 73 • 10 years ago

See comment 66 for more info. For v2.0 the existing patches apply cleanly and do not require modifications.

	Anshul
	Comment 74 • 10 years ago

(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?

	Anshul
	Comment 75 • 10 years ago

(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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 76 • 10 years ago

(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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 77 • 10 years ago

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.

	bhavana bajaj [:bajaj]
	Comment 78 • 10 years ago

(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 ?

	Francesco Lodolo [:flod]
	Comment 79 • 10 years ago

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

	Gabriele Svelto [:gsvelto] Assignee
	Comment 80 • 10 years ago

(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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 81 • 10 years ago

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

	Eric Chang [:ericcc] [:echang]
	Comment 82 • 10 years ago

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

	Shiwei Zhang Reporter
	Comment 83 • 10 years ago

This issue is NOT fixed without these modifications on v1.3t, please give a review.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 84 • 10 years ago

(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.

	Etienne Segonzac (:etienne)
	Comment 85 • 10 years ago

(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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 86 • 10 years ago

(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.

	Shiwei Zhang Reporter
	Comment 87 • 10 years ago

(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.

	ting.yang
	Comment 88 • 10 years ago

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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 89 • 10 years ago

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.

	ting.yang
	Comment 90 • 10 years ago

(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.

	Gabriele Svelto [:gsvelto] Assignee
	Comment 91 • 10 years ago

(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.

	Shiwei Zhang Reporter
	Comment 92 • 10 years ago

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?

	ting.yang
	Comment 93 • 10 years ago

(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.