Closed
Bug 1016885
Opened 11 years ago
Closed 10 years ago
[Tarako][Dolphin][LMK][Youtube][USSD] Pop-up USSD message causes Youtube being killed at background due to LMK while using Youtube app
Categories
(Firefox OS Graveyard :: Gaia::Dialer, defect)
Tracking
(blocking-b2g:1.3T+, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 verified, b2g-v2.1 verified)
People
(Reporter: bli, Assigned: gsvelto)
References
Details
(Whiteboard: [partner-blocker][sprd317445][planned-sprint])
Attachments
(10 files, 2 obsolete files)
500.92 KB,
application/zip
|
Details | |
16.46 KB,
patch
|
etienne
:
review+
|
Details | Diff | Splinter Review |
46 bytes,
text/x-github-pull-request
|
Details | Review | |
10.76 KB,
patch
|
etienne
:
review+
|
Details | Diff | Splinter Review |
46 bytes,
text/x-github-pull-request
|
Details | Review | |
46 bytes,
text/x-github-pull-request
|
etienne
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
lmandel
:
approval-gaia-v1.4+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
bajaj
:
approval-gaia-v2.0+
|
Details | Review |
659 bytes,
patch
|
Details | Diff | Splinter Review | |
2.19 MB,
video/3gpp
|
Details |
Steps to reproduce:
-----------------------------------------
1. Install Youtube app from Market Place.
2. Launch Youtube and play a video
3. USSD message pops up while playing the video
Actual result:
-----------------------------------------
YouTube is pushed to background and gets killed due to LMK
Reporter | ||
Updated•11 years ago
|
status-b2g-v1.3T:
--- → affected
Whiteboard: [sprd298584 ]
Reporter | ||
Updated•11 years ago
|
Whiteboard: [sprd298584 ]
Comment 1•11 years ago
|
||
it's hard to view YouTube video using prepaid SIM, since the USSD msg comes actively and reports how much mins / data left to use. nom to 1.3? is a blocker.
blocking-b2g: --- → 1.3?
Whiteboard: [sprd298584 ]
Updated•11 years ago
|
Whiteboard: [sprd298584 ]
Assignee | ||
Comment 2•11 years ago
|
||
I'm preparing a fix for bug 989713 that might help with this too.
Updated•11 years ago
|
blocking-b2g: 1.3? → 1.3T?
Comment 3•11 years ago
|
||
(In reply to Marvin Khoo [:Marvin_Khoo] from comment #1)
> it's hard to view YouTube video using prepaid SIM, since the USSD msg comes
> actively and reports how much mins / data left to use. nom to 1.3? is a
> blocker.
I'm assuming you meant to nominate this for Tarako.
Updated•11 years ago
|
blocking-b2g: 1.3T? → 1.3T+
Comment 4•11 years ago
|
||
Bing, can you confirm if this is a recent regression or have we had this issue forever on the youtube app ?
Flags: needinfo?(bli)
Comment 5•11 years ago
|
||
Thanks Jason, yes, is 1.3T?.. my bad. :(
also, after tapping the X on upper left and close the USSD dialog, the next UI we see is dialer pad. i think we shouldn't show dialer pad unless the USSD command is typed from the pad.
may i know what's the behavior or UI display in US / China if user receives "remaining minutes / data" USSD info when using pre-paid SIM?
Reporter | ||
Comment 6•11 years ago
|
||
(In reply to bhavana bajaj [:bajaj] from comment #4)
> Bing, can you confirm if this is a recent regression or have we had this
> issue forever on the youtube app ?
I'm truly sorry that we are unable to reproduce it at our site, need partner to confirm this.
Hi Marvin and James, could you please help with this?
Flags: needinfo?(mkhoo)
Flags: needinfo?(james.zhang)
Flags: needinfo?(bli)
Updated•11 years ago
|
Whiteboard: [partner-blocker]
Updated•11 years ago
|
Flags: needinfo?(james.zhang)
Whiteboard: [partner-blocker] → [partner-blocker][sprd317445]
Comment 7•11 years ago
|
||
Youtube killed by LMK when USSD app popup.
You can also try other app popup case.
Comment 8•11 years ago
|
||
per discussion with partner, partner will see if they can workaround this
ni? James
Flags: needinfo?(james.zhang)
Comment 9•11 years ago
|
||
Loop Siiaceon.
Assignee: nobody → siiaceon.cao
Flags: needinfo?(siiaceon.cao)
Flags: needinfo?(james.zhang)
Flags: needinfo?(arvin.zhang)
Updated•11 years ago
|
Component: General → Gaia
Comment 10•11 years ago
|
||
attached a flow for better understanding. You may found this issue when using whatever tools to send device an USSD while playback a YouTube content.
Flags: needinfo?(mkhoo)
Comment 11•11 years ago
|
||
try to group ussd, and need check if can make ussd not prompt but only one tone(ussd notify unhide while exit video). (Many handset use this solution for such as MMS/SMS case)
loop arvin also.
Flags: needinfo?(siiaceon.cao)
Comment 12•11 years ago
|
||
Youtobe need background while ussd coming. So just make USSD group also can make Youtobe crash . So solution is
Need moz cooperator change the design, which need seperate the dialer and USSD. At least cannot launch dailer when recieved notify USSD (ussd which no need dailer)
anyone else solution?
Flags: needinfo?(james.zhang)
Comment 14•11 years ago
|
||
just make USSD group also can make Youtobe crash except group Youtobe(not a good solution).
need seperate the dialer and USSD by mozilla designed (no need dailer when ussd just notify).
agree ?
Flags: needinfo?(james.zhang)
Comment 16•11 years ago
|
||
But I don't think change design is a good idea at this time.
Comment 17•11 years ago
|
||
dailer/USSD owner to aprove, pls
change designed or not launch dailer in this case?
Updated•11 years ago
|
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 18•11 years ago
|
||
I've done some investigation on this and the solution looks more like a design change / window management issue rather than an LMK problem, let me describe why: I've tried running both the YouTube app and the dialer at the same time and both work fine. I can switch between the two w/o either one getting killed and more importantly I can simulate a USSD interaction w/o the YouTube app getting killed. For example this sequence works fine:
- Launch the YouTube app
- Start playing a video
- Go back to the homescreen
- Launch the dialer
- Type an MMI code and send it (such as *#30#)
- Wait for the USSD panel to show up and close it
- Go back to the homescreen
- Go back to the YouTube app
Once we're back in the YouTube app it's still running fine.
Now solving this is not trivial however as two different changes are required:
- We need to close the dialer after the USSD interaction has finished and the user has closed the USSD screen. This shouldn't be really hard if we add some tracking to the app to distinguish between solicited and unsolicited USSD messages.
- We need a way to bring back the YouTube app to the foreground when the dialer is closed. This should behave somewhat similarly to what we do when we close the callscreen and the former background app comes back into the foreground.
Flags: needinfo?(gsvelto)
Comment 19•11 years ago
|
||
Thanks Gabriele for your comment 18
What do you think of the risk and estimate of effort on this bug? this will help to evaluate whether this bug should stay as 1.3T+. Thanks
Updated•11 years ago
|
Flags: needinfo?(gsvelto)
Updated•11 years ago
|
Flags: needinfo?(arvin.zhang)
Assignee | ||
Comment 20•11 years ago
|
||
(In reply to Joe Cheng [:jcheng] from comment #19)
> What do you think of the risk and estimate of effort on this bug? this will
> help to evaluate whether this bug should stay as 1.3T+. Thanks
I've studied the code a bit more and the required change is to move the USSD UI into an attention screen and change the related code accordingly. This has both risks and drawbacks:
- The affected code is relatively isolated so the change is unlikely to affect the rest of the application however the change is fairly radical so it might cause breakage the existing MMI/USSD functionality.
- Additionally moving the USSD display into an attention screen changes it's behavior. Currently when we display an USSD message we do it in the context of the dialer app. Tapping the home button when viewing an USSD message will send the dialer to the background and bringing it back will still display the current USSD message. In an attention screen this will work differently: tapping the home button will shrink the attention screen and show a reduced version of it in the top bar and the only way to dismiss it will be to explicitly close it using the close button. Would this be acceptable from a UX POV? What should appear in the minimized attention screen then? What happens if a new USSD message arrives while the attention screen is shrunk? All this probably requires some additional UX.
- Last but not least the upside is that we don't need to move the USSD code anywhere else (like in the system app). It can still live in the dialer and this shouldn't cause problems with a foreground app such as YouTube.
Flags: needinfo?(gsvelto)
Updated•11 years ago
|
Assignee: nobody → anthony
Target Milestone: --- → 2.0 S4 (20june)
Comment 21•11 years ago
|
||
Carrie, please review the proposed UX change for USSD. Thank you very much.
Flags: needinfo?(mtsai)
Flags: needinfo?(cawang)
Comment 22•11 years ago
|
||
Hi Gabriele,
I don't really understand the "Shrunk UI" part. Is it like something from Dialer that if users tap Home while they are in a call and they can tap the bar again to get back to the call screen (in this case, that will be USSD page)?
However, I'd prefer to keep this USSD info in Notification. In this way, users tap Home and they can still access the page whenever they need (by dragging down the notification tray and tapping it to access the page), but if they tap "x" to close the page, it will be cleared from notification tray. Is this doable? Thanks!
Flags: needinfo?(cawang) → needinfo?(gsvelto)
Comment 24•10 years ago
|
||
We may be killing the Youtube app because we request a high-priority wake lock when we receive a MMI. This was introduced to fix bug 999356. I need to try some code to verify this. If I do verify it, I'm not sure how we can fix it though.
For the UI part, moving the code outside of the Dialer app or moving it to an attention screen will introduce a lot of regressions in behaviour. What about this solution: If the dialer is not the front app, we show a notification. In the dialer, we store all the mmi received and when a user taps a notification, we display the ussd window for that ussd. That may help us solve bug 861754 at the same time.
Flags: needinfo?(anthony)
Comment 25•10 years ago
|
||
I'd go with the "notification" proposal as what Rik mentioned here.
Comment 26•10 years ago
|
||
Our Indian QA sain dolphin also has 30% reproduce rate.
status-b2g-v1.4:
--- → affected
Summary: [Tarako][LMK][Youtube][USSD] Pop-up USSD message causes Youtube being killed at background due to LMK while using Youtube app → [Tarako][Dolphin][LMK][Youtube][USSD] Pop-up USSD message causes Youtube being killed at background due to LMK while using Youtube app
Comment 27•10 years ago
|
||
1025457 related issue
Comment 28•10 years ago
|
||
I am wondering that is this issue reproducible on other devices as well?
Assignee | ||
Comment 29•10 years ago
|
||
(In reply to Anthony Ricaud (:rik) from comment #24)
> We may be killing the Youtube app because we request a high-priority wake
> lock when we receive a MMI. This was introduced to fix bug 999356. I need to
> try some code to verify this. If I do verify it, I'm not sure how we can fix
> it though.
Some clearing on this: we're not killing the YouTube app, we're just sending it in the background as the USSD display is part of the dialer so once that is closed you'll find yourself in the dialer instead of the YouTube app.
> For the UI part, moving the code outside of the Dialer app or moving it to
> an attention screen will introduce a lot of regressions in behaviour. What
> about this solution: If the dialer is not the front app, we show a
> notification. In the dialer, we store all the mmi received and when a user
> taps a notification, we display the ussd window for that ussd. That may help
> us solve bug 861754 at the same time.
+1
The MMI code is already tracking the state of a single MMI request and expanding it to handle more should be doable w/o too many changes.
Flags: needinfo?(gsvelto)
Comment 30•10 years ago
|
||
While working on this, I'm sometimes getting this error:
[JavaScript Error: "requestWindow is undefined" {file: "jar:file:///system/b2g/omni.ja!/components/RILContentHelper.js" line: 2105}]
Hsin-Yi: I'm not yet sure if it's a problem but could you take a look in case we need this fixed for the solution to work?
Here is the patch I use to receive USSD messages while developing. Pressing volume down sends a USSD request.
diff --git a/apps/system/js/hardware_buttons.js b/apps/system/js/hardware_buttons.js
index 9c77db3..c96de57 100644
--- a/apps/system/js/hardware_buttons.js
+++ b/apps/system/js/hardware_buttons.js
@@ -493,6 +493,9 @@
case 'volume-down-button-release':
if (this.direction === 'volume-down-button-press') {
if (!this.repeating) {
+ console.log('XXX: MMI sent');
+ navigator.mozMobileConnections[0].sendMMI('#123#');
+
this.hardwareButtons.publish('volumedown');
}
this.hardwareButtons.setState('base', type);
Flags: needinfo?(htsai)
Updated•10 years ago
|
Flags: needinfo?(janjongboom)
Comment 32•10 years ago
|
||
I am looking at this. Will keep you posted!
Comment 33•10 years ago
|
||
Here is my current state: https://github.com/Rik/gaia/tree/youtube-ussd-1.3t-1016885
Although we'd like to land this on master, I had to work on 1.3t because of bug 1028039. The current patch works but it is not ready for review yet. I'll need someone else to continue the work while I am out (unassigning myself to reflect that).
Updated•10 years ago
|
Assignee: anthony → nobody
Comment 34•10 years ago
|
||
Thanks for the sharp eye, :rik. File a gecko bug 1028062.
Flags: needinfo?(htsai)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Updated•10 years ago
|
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Comment 35•10 years ago
|
||
Bug 1028062 doesn't block this issue IMO. It is caught when Rik is working on this. Bug 1028082 makes firing request error for mobileConnection.cancelMMI().
Assignee | ||
Comment 36•10 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #35)
> Bug 1028062 doesn't block this issue IMO. It is caught when Rik is working
> on this. Bug 1028082 makes firing request error for
> mobileConnection.cancelMMI().
Agreed.
Updated•10 years ago
|
Flags: needinfo?(janjongboom)
Updated•10 years ago
|
Whiteboard: [partner-blocker][sprd317445] → [partner-blocker][sprd317445][planned-sprint]
Assignee | ||
Comment 37•10 years ago
|
||
Here's a more complete version of Anthony's patch now that we've got the document.hidden issue fixed. The only major change is that I've converted both the handleMMIReceived() and sendNotification() functions to use promises so that we can properly release the high-priority wakelock when we're finished. I've given this patch only light testing and I'm not 100% happy with it yet so I'm asking for feedback first. BTW it also lacks tests so this is barely above a WIP.
Attachment #8432888 -
Attachment is obsolete: true
Attachment #8447745 -
Flags: feedback?(etienne)
Comment 38•10 years ago
|
||
Hi Gabriele,
Thank you for supporting here, can you suggest a comfortable ETA for this and the risk assessment on this change?
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 39•10 years ago
|
||
(In reply to Wayne Chang [:wchang] from comment #38)
> Thank you for supporting here, can you suggest a comfortable ETA for this
> and the risk assessment on this change?
A comfortable ETA including reviews and a couple of extra iterations would be sometimes later this week like Thursday or Friday at the latest. The risk for this change should be modest as it affects only unsolicited USSD messages making them pop-up notifications when the dialer is not visible. The associated logic received only minor changes and only for the unsollicited case.
Flags: needinfo?(gsvelto)
Comment 40•10 years ago
|
||
Comment on attachment 8447745 [details] [diff] [review]
[PATCH] Send a notification instead of displaying the dialer when receiving an unsolicited USSD notification
Review of attachment 8447745 [details] [diff] [review]:
-----------------------------------------------------------------
Let me know if my comments are unclear, had a hard time formulating them.
::: apps/communications/dialer/js/dialer.js
@@ +413,5 @@
> }
>
> MmiManager.handleMMIReceived(evt.message, evt.sessionEnded,
> + evt.serviceId)
> + .then(releaseWakeLock);
I'm afraid in the cases where we're doing a getSelf.launch(), this might release the wake lock _before_ the visibility change. (since handleMMIReceived resolves outside of the getSelf block)
::: apps/communications/dialer/js/mmi.js
@@ +356,5 @@
> + };
> + var notification =
> + new Notification('USSD', {body: message, icon: iconURL});
> + notification.addEventListener('click', clickCB);
> + resolve();
Since we're resolving as soon as the notification is sent (which is a good thing), we're releasing the wake lock and the dialer could get killed.
Should we add support for those notifications in dialer.js/handleNotification? (called via system message when the app gets killed taking the clickCB with it).
Attachment #8447745 -
Flags: feedback?(etienne)
Assignee | ||
Comment 41•10 years ago
|
||
(In reply to Etienne Segonzac (:etienne) from comment #40)
> I'm afraid in the cases where we're doing a getSelf.launch(), this might
> release the wake lock _before_ the visibility change. (since
> handleMMIReceived resolves outside of the getSelf block)
Good point. We should release the wakelock immediately only after having posted the notification. I'm splitting that case of handleMMIReceived() to make the code cleaner so I'll use that to handle the unlocking properly too.
> Since we're resolving as soon as the notification is sent (which is a good
> thing), we're releasing the wake lock and the dialer could get killed.
> Should we add support for those notifications in
> dialer.js/handleNotification? (called via system message when the app gets
> killed taking the clickCB with it).
Yes, I forgot about that. That's definitely needed otherwise we might lose some messages and that would be especially likely so on the Tarako.
Assignee | ||
Comment 42•10 years ago
|
||
This is a fully working patch for gaia/master with the issues you had highlighted addressed plus a little test coverage. A few notes:
- I'm passing information using the iconURL field of a notification; the tag would be better but we need to uplift this to the v1.3t branch and we don't have tags there so I didn't want to make my life more complicated than it already is :-p
- On the topic of notifications, the dialer is a bit of a mess in this regard still abusing iconURL to distinguish between the origin of notifications and closing all notifications when one is tapped. I've made it play nice with my changes but we should open a nice follow-up bug to clean up the mess.
- I've had to modify the fake origin of certain mock messages from 'app://communications.gaiamobile.org' to 'http://communications.gaiamobile.org:8080' because the test failed otherwise.
Attachment #8447745 -
Attachment is obsolete: true
Attachment #8448690 -
Flags: review?(etienne)
Assignee | ||
Comment 43•10 years ago
|
||
Comment 44•10 years ago
|
||
Comment on attachment 8448690 [details] [diff] [review]
[PATCH] Send a notification instead of displaying the dialer when receiving an unsolicited USSD notification
Review of attachment 8448690 [details] [diff] [review]:
-----------------------------------------------------------------
(In reply to Gabriele Svelto [:gsvelto] from comment #42)
> Created attachment 8448690 [details] [diff] [review]
> [PATCH] Send a notification instead of displaying the dialer when receiving
> an unsolicited USSD notification
>
> This is a fully working patch for gaia/master with the issues you had
> highlighted addressed plus a little test coverage. A few notes:
>
> - I'm passing information using the iconURL field of a notification; the tag
> would be better but we need to uplift this to the v1.3t branch and we don't
> have tags there so I didn't want to make my life more complicated than it
> already is :-p
Make sense, let's make sure we have a follow up filed (and referenced in the code).
>
> - On the topic of notifications, the dialer is a bit of a mess in this
> regard still abusing iconURL to distinguish between the origin of
> notifications and closing all notifications when one is tapped. I've made it
> play nice with my changes but we should open a nice follow-up bug to clean
> up the mess.
The cost of being a pioneer :)
>
> - I've had to modify the fake origin of certain mock messages from
> 'app://communications.gaiamobile.org' to
> 'http://communications.gaiamobile.org:8080' because the test failed
> otherwise.
Ok. let's keep an eye open to make sure this doesn't fail on some test infrastructure.
Attachment #8448690 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 45•10 years ago
|
||
Thanks for the quick review!
(In reply to Etienne Segonzac (:etienne) from comment #44)
> Make sense, let's make sure we have a follow up filed (and referenced in the
> code).
I've filed bug 1033254 and referenced it in the code; let's get this code cleaned up ASAP.
> Ok. let's keep an eye open to make sure this doesn't fail on some test
> infrastructure.
My first Travis run contained some changes to the fake app origin but did fail, now I've reverted those changes again and the tests fail locally. I'll be waiting to see what happens on Travis; annoying :-|
Assignee | ||
Comment 46•10 years ago
|
||
This is the Travis run: https://travis-ci.org/mozilla-b2g/gaia/builds/28946591
It's red but the failing tests are completely unrelated so I'll just go ahead and push it.
Assignee | ||
Comment 47•10 years ago
|
||
Pushed to gaia/master ddbf0e11b494fd9665545141485d0b70cab6d514
https://github.com/mozilla-b2g/gaia/commit/ddbf0e11b494fd9665545141485d0b70cab6d514
I'll post the v1.3t patch shortly.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → fixed
Resolution: --- → FIXED
Assignee | ||
Comment 48•10 years ago
|
||
While testing the v1.3t patch on the Tarako I noticed that a mistake had slipped in my original patch that makes the changes work only when one SIM is used. When multiple SIMs are used localization fails and the notification is not delivered when receiving an unsolicited message so I'm reopening the bug. Fix pending.
Comment 49•10 years ago
|
||
Hi Gabriel,
When you have a patch good to go, please also request for 1.4 approval... partner is also encountering the same problem on Dolphin.
Assignee | ||
Comment 50•10 years ago
|
||
This patch initializes the MmiManager before sending a notification, not doing so made notifications fail on multi-SIM devices as the message couldn't be localized properly. The change is conceptually small but the patch is large because I've added test coverage for this specific case. BTW I should really file a follow up to clean up the MMI unit-tests as they're in dire shape (and a bunch of them are disabled to make things worse).
Attachment #8450069 -
Flags: review?(etienne)
Assignee | ||
Comment 51•10 years ago
|
||
Comment 52•10 years ago
|
||
Comment on attachment 8450069 [details] [diff] [review]
[PATCH] Always initialize the MmiManager before sending a notification
Review of attachment 8450069 [details] [diff] [review]:
-----------------------------------------------------------------
Good catch!
Attachment #8450069 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 53•10 years ago
|
||
Pushed to gaia/master c664153cd702ae1db12a485e539bae88b9bdcdea
https://github.com/mozilla-b2g/gaia/commit/c664153cd702ae1db12a485e539bae88b9bdcdea
The Travis run was partially red but the failures were unrelated:
https://travis-ci.org/mozilla-b2g/gaia/builds/29041867
I'll be uplifting the patches right away.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 54•10 years ago
|
||
Uplifting this patch has been an uphill battle against a race in the LazyL10n which I finally managed to fix. I'm asking for review again before landing because the patches had to undergo some important changes, namely:
- New style notifications have been replaced with old style ones, this had been planned ahead and was thus relatively painless.
- The LazyL10n object was suffering from the following race: initialization would go through two asynchronous steps, one to wait for the 'localized' event and one to lazy load further code. If another caller would invoke LazyL10n.get() in between the two calls its callback would be lost as it would never receive the localized event nor would it be able to proceed further. I've modified the code to mark the beginning and end of the initialization procedure and allow only one caller to go through it. Successive callers arriving in the middle of the initialization phase will have their callbacks delayed until the end the object is fully initialized by stacking them into an array.
Besides doing some testing of the scenarios at hand I also tested the other users of LazyL10n to ensure this change didn't break anything but I want to stress that this hasn't been a simple, straightforward uplift and thus we have to proceed with some caution before merging.
Attachment #8450584 -
Flags: review?(etienne)
Assignee | ||
Comment 55•10 years ago
|
||
I've updated the PR because the new unit-tests were failing; hopefully they should be fixed now.
Assignee | ||
Comment 56•10 years ago
|
||
Good news, Travis is green for the uplift too: https://travis-ci.org/mozilla-b2g/gaia/builds/29123015
Comment 57•10 years ago
|
||
Comment on attachment 8450584 [details] [review]
[PULL REQUEST] v1.3t uplift
r=me with the test added (see github).
Stupid question but better safe than sorry, we do have Promise support in the gecko we're releasing with 1.3t right?
Attachment #8450584 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 58•10 years ago
|
||
(In reply to Etienne Segonzac (:etienne) from comment #57)
> r=me with the test added (see github).
Good point, I'm going to add that.
> Stupid question but better safe than sorry, we do have Promise support in
> the gecko we're releasing with 1.3t right?
Mmm... now that you mention it the documentation says we've got promises enabled by default in Gecko 29 and hidden behind a flag starting with Gecko 25. v1.3t is Gecko 28 so it's a matter of figuring out if they've been enabled or not. I'll double-check, worst case I'll turn that specific instance into a callback (ugly but functional).
Assignee | ||
Comment 59•10 years ago
|
||
I've updated the PR once more:
- Now we use a callback instead of a promise to release the wakelock after having sent the notification, I checked that it's released correctly.
- I've adjusted the tests to cope with the callback.
- I've slightly modified the LazyL10n code: I push the queued up callbacks and then shift them so they're called in order of arrival. Before I popped them out of the array causing them to be called in inverse order of arrival. I think this is cleaner and less "surprising".
- There's an additional test for the LazyL10n code that verifies that all the queued callbacks are invoked properly.
If Travis turns green I'm ready to merge: https://travis-ci.org/mozilla-b2g/gaia/builds/29150394
Comment 60•10 years ago
|
||
Please also merge to v1.4. Dolphin has the same issue.
Assignee | ||
Comment 61•10 years ago
|
||
(In reply to James Zhang (Spreadtrum) from comment #60)
> Please also merge to v1.4. Dolphin has the same issue.
I will as soon as I get approval for this and its dependency.
Assignee | ||
Comment 62•10 years ago
|
||
Uplifted to gaia/v1.3t 50578b579c378018bf19e966a8cda5b0bcd04a1d
https://github.com/mozilla-b2g/gaia/commit/50578b579c378018bf19e966a8cda5b0bcd04a1d
Comment 63•10 years ago
|
||
Hi Gabriele,
Can you also merge this patch to v1.4? We met this issue on dolphin and 1.4 is dolphin branch now.
Comment 64•10 years ago
|
||
Gabriele,
Is the 1.3t patch good for 1.4 or does it need rebasing?
Thanks
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 65•10 years ago
|
||
(In reply to Wayne Chang [:wchang] from comment #64)
> Is the 1.3t patch good for 1.4 or does it need rebasing?
Some rebasing is needed, I'm almost done with. Will post the PR for approval soon.
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 66•10 years ago
|
||
Here's a PR for the v1.4 branch. It required some light modifications, mostly to the unit-tests. I'm not asking for approval just yet because I haven't had the time to test it on a device.
Assignee | ||
Comment 67•10 years ago
|
||
Comment on attachment 8452163 [details] [review]
[PULL REQUEST] v1.4 uplift
[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: Unsolicited USSD messages will bring the dialer application to the front hiding whatever the user was doing in the meantime (e.g. watching a video on YouTube). According to our partners this is quite detrimental in the target markets where unsolicited USSD messages are sent to inform the user of its remaining credit / data cap.
[Testing completed]: Tested on a Flame & covered with unit-tests
[Risk to taking this patch] (and alternatives if risky): USSD/MMI code may be adversely affected. A minor issue that cropped up during testing is bug 1034279.
[String changes made]: None
Attachment #8452163 -
Flags: approval-gaia-v1.4?(release-mgmt)
Comment 68•10 years ago
|
||
Comment on attachment 8452163 [details] [review]
[PULL REQUEST] v1.4 uplift
Approved for 1.4.
Attachment #8452163 -
Flags: approval-gaia-v1.4?(release-mgmt) → approval-gaia-v1.4+
Assignee | ||
Comment 69•10 years ago
|
||
Thanks for the approval, landed attachment 8452163 [details] [review] on gaia/v1.4 b0e9b4bdb39c5eb93a6783a34624ffc84f62b126
https://github.com/mozilla-b2g/gaia/commit/b0e9b4bdb39c5eb93a6783a34624ffc84f62b126
Now let's move on to v2.0.
Assignee | ||
Comment 70•10 years ago
|
||
See comment 67, the patches here are unmodified and can be cherry-picked cleanly from the master branch.
Attachment #8453028 -
Flags: approval-gaia-v2.0?
Assignee | ||
Updated•10 years ago
|
Attachment #8453028 -
Flags: approval-gaia-v2.0? → approval-gaia-v2.0?(release-mgmt)
Updated•10 years ago
|
Attachment #8453028 -
Flags: approval-gaia-v2.0?(release-mgmt) → approval-gaia-v2.0+
Assignee | ||
Comment 72•10 years ago
|
||
Uplifted to gaia/v2.0 1bd6e8957ccf310b2f75ba5695b058a2e284df3a
https://github.com/mozilla-b2g/gaia/commit/1bd6e8957ccf310b2f75ba5695b058a2e284df3a
Comment 73•10 years ago
|
||
Hi Gabriele,
In v1.4, there's a little problem occurred that it won't show title info(sim index and operator name) on USSD message received. I filed a new bug to track this, let's check it in Bug 1051841.
Thanks.
Comment 74•10 years ago
|
||
Dear Gabriele
Our QA in India found a new bug. When he clicked STK menu which meaned sending *121# in settings, there was neither ussd page nor ussd notification shows.
After I checked the log, I found the ussd had been received, but the ussd page was on background.
I think that is because "sessionEnded" flag, After clicking STK menu to send *121#, "sessionEnded" flag in received ussd message is false. So there is no ussd notification.
https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/communications/dialer/js/dialer.js#L299
Comment 75•10 years ago
|
||
I think this patch can resolve this issue.
Could you give some advice?
Thanks.
Flags: needinfo?(gsvelto)
Comment 76•10 years ago
|
||
(In reply to Wei Gao (Spreadtrum) from comment #75)
> Created attachment 8484789 [details] [diff] [review]
> ussd_sessionEnd.patch
>
> I think this patch can resolve this issue.
> Could you give some advice?
> Thanks.
Maybe we can also launch('dialer') instead of showing notification in "MmiManager.handleMMIReceived" to resolve this issue.
https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/communications/dialer/js/dialer.js#L307
Assignee | ||
Comment 77•10 years ago
|
||
(In reply to Wei Gao (Spreadtrum) from comment #74)
> Our QA in India found a new bug. When he clicked STK menu which meaned
> sending *121# in settings, there was neither ussd page nor ussd notification
> shows.
> After I checked the log, I found the ussd had been received, but the ussd
> page was on background.
>
> I think that is because "sessionEnded" flag, After clicking STK menu to send
> *121#, "sessionEnded" flag in received ussd message is false. So there is no
> ussd notification.
Yes, but that was done on purpose to solve the issue in this bug. We need to find a better way. Let's file another bug for this.
Flags: needinfo?(gsvelto)
Comment 78•10 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #77)
> Yes, but that was done on purpose to solve the issue in this bug. We need to
> find a better way. Let's file another bug for this.
Ok, thanks, I will file a new bug.
Comment 80•10 years ago
|
||
This issue has been successfully verified on Flame 2.1, 2.0
See attachment:verify.3gp
Reproducing rate: 0/5
Flame 2.1 new versions:
Gaia-Rev dbaf3e31c9ba9c3436e074381744f2971e15c7bf
Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ebce587d2194
Build-ID 20141203001205
Version 34.0
Flame 2.0 new versions:
Gaia-Rev 8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/29222e215db8
Build-ID 20141203000201
Version 32.0
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•