990003 - [Dolphin][Tarako][Perf][Dialer] It takes a long time for the call screen shows up

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Performance, defect, P1)

Description

[Bingqing Li]

Environment:
------------------------------------------------
Gaia 14ef4fcdf9199f04f7678755c917dc77f51e13ba
Gecko https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/b574a7967338
BuildID 20140329004002
Version 28.1
ro.build.version.incremental=70
ro.build.date=Fri Mar 28 06:17:40 CST 2014


Steps to reproduce:
----------------------------
1. Goto Dailer
2. Dail a number then tap on the green button on the bottom
3. Wait for the call screen showing up.


Actual result:
------------------------------
The average time for call screen showing up is 3.8s


Additional info:
-----------------------------
[android2.2.2] The average time is 2.2s

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

oops, I misread.

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

Etienne, do you mind looking at this? thanks

Comment 4

[Etienne Segonzac (:etienne)]

Our best bet here would be the wip mentioned here: https://bugzilla.mozilla.org/show_bug.cgi?id=987022#c26

But it's a big/risky patch.

Comment 5

[Joe Cheng [:jcheng] (please needinfo)]

we seem to be mixing things up in bug 987022
Etienne, do you think it makes sense to provide your patch https://bugzilla.mozilla.org/show_bug.cgi?id=987022#c26 in this bug instead and we track your patch using this bug?
Thanks

Comment 6

[Etienne Segonzac (:etienne)]

(In reply to Joe Cheng [:jcheng] from comment #5)
> we seem to be mixing things up in bug 987022
> Etienne, do you think it makes sense to provide your patch
> https://bugzilla.mozilla.org/show_bug.cgi?id=987022#c26 in this bug instead
> and we track your patch using this bug?
> Thanks

Yes.
But what's the deadline? And can we do a specific QA round for this patch if we move forward (all call screen related scenarios, including bluetooth and headset support)?

Right now it's hard to say if this approach is realistic at all for 1.3t...

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

we have until end of April to stabilize the tarako build but we don't want to wait until the end for major changes. what do you think the level of effort is on this bug? approximately how many working days would be needed? thanks

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

Etienne, this bug seem to be a dup of Bug 983476 
do you have a preference on which bug you want to work on and dup one of it to the other?
Do you mind taking the bug? thanks

Comment 11

[Etienne Segonzac (:etienne)]

Attachment: 1.3T POC

Comment 12

[Etienne Segonzac (:etienne)]

Hi again Fabrice, I think we need some arbitrage re: bug 987022 comment 40. Getting mixed messages :)

Can you confirm we won't take this risky patch for 1.3t?
Do we want this patch ready "in case of emergency"?
Do we need it rock solid by the end of the week?

FWIW I'm more than happy to just continue this work as an investigation for future versions :)

Comment 14

[Etienne Segonzac (:etienne)]

Attachment: Demo of the current progress

Comment 15

[Etienne Segonzac (:etienne)]

Code for the demo

https://github.com/etiennesegonzac/gaia/tree/instacallscreen
https://github.com/etiennesegonzac/gaia/tree/instacallscreen-1.3t

TODO:
* cleanup the style/ (js already done)
* cleanup the system app changes
* add an IAC for the bluetooth/headset commands
* rebase on top of bug 992948 and bug 993018

Comment 16

[[:fabrice] Fabrice Desré]

(In reply to Etienne Segonzac (:etienne) from comment #12)
> Hi again Fabrice, I think we need some arbitrage re: bug 987022 comment 40.
> Getting mixed messages :)
> 
> Can you confirm we won't take this risky patch for 1.3t?

I confirm it's risky, but I want it.

> Do we want this patch ready "in case of emergency"?

That's already an emergency.

> Do we need it rock solid by the end of the week?

As solid as possible, yes!

Comment 17

[Etienne Segonzac (:etienne)]

> TODO:
> * add an IAC for the bluetooth/headset commands
> * cleanup the system app changes

DONE:
> * cleanup the style/ (js already done)
> * rebase on top of bug 992948 and bug 993018

Comment 18

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Please test outbound as well as inbound calls for the screen to come up.  If inbound calls shows a delay, please reopen bug 983476

Comment 20

[Etienne Segonzac (:etienne)]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #18)
> Please test outbound as well as inbound calls for the screen to come up.  If
> inbound calls shows a delay, please reopen bug 983476

It's all smooth.
We even tried inbound call while the phone is playing a video, all good :)

Comment 21

[Etienne Segonzac (:etienne)]

> DONE:
> > * cleanup the style/ (js already done)
> > * rebase on top of bug 992948 and bug 993018
> > * add an IAC for the bluetooth/headset commands (just listening to the system messages)
> > * cleanup the system app changes
> > * add loads of tests

Ok, I'm just waiting for bug 992948 to land (travis...) before I rebase and send the pull request.
Probably tomorrow morning Paris time.

In the meantime, the patch is now complete and ready to enjoy :)
https://github.com/etiennesegonzac/gaia/compare/instacallscreen

Naoki, can we do a QA spin on this branch? (it's marster + bug 992948 + the patch for this bug)

Comment 22

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I couldn't cleanly apply bug 992948; having said that I built my own gecko 1.3T from that bug and then took Etienne's gaia branch.

The speed of the response of the ui for incoming and outgoing phone calls are tremendously increased.
Outgoing calls seem to have "Withheld number" for some reason though?  I think I've also noticed a few other oddities.

Gaia      6b7abd089aa3ac7e6e98db9cdba1751e43bd3017
Gecko     2474baf7cdb31b1986e59c5d1f3fbd8fb6288fc3
BuildID   20140410135103
Version   28.1
ro.build.version.incremental=eng.cltbld.20140410.040335
ro.build.date=Thu Apr 10 04:03:42 EDT 2014

Comment 23

[Etienne Segonzac (:etienne)]

Attachment: Documentation

Just adding quick documentation diagram to make reviews easier.

Comment 24

[Etienne Segonzac (:etienne)]

Attachment: Gaia PR

Here's the pull request against master.
r? Rik for the dialer/callscreen parts
r? Alive for the system app part
r? Vivien for the small build system add

Comment 25

[Etienne Segonzac (:etienne)]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #22)
> I couldn't cleanly apply bug 992948; having said that I built my own gecko
> 1.3T from that bug and then took Etienne's gaia branch.

Thanks for taking a look so quickly!
I'll make a proper 1.3t rebase later today, so we can test/discuss from the exact same version.

Comment 26

[Etienne Segonzac (:etienne)]

Hey Shawn, I think you showed me a Bluetooth certification test suite during a work week.
Can we run it against this PR to make sure we don't regress on any test?
(Already tested the classic Bluetooth use cases manually, but I think we cached a few race conditions with this suite :))

Comment 27

[Etienne Segonzac (:etienne)]

Zac, I updated the call_screen region for the on-device python test, is there anything else missing?

Comment 28

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Etienne Segonzac (:etienne) from comment #26)
> Hey Shawn, I think you showed me a Bluetooth certification test suite during
> a work week.
> Can we run it against this PR to make sure we don't regress on any test?
> (Already tested the classic Bluetooth use cases manually, but I think we
> cached a few race conditions with this suite :))

Hey Etienne,
Thanks for raising up the topic. Shall I wait for r+ first and perform the test? Or you wish to test before r+?

Comment 29

[Etienne Segonzac (:etienne)]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #28)
> Hey Etienne,
> Thanks for raising up the topic. Shall I wait for r+ first and perform the
> test? Or you wish to test before r+?

There shouldn't be too much code churn on this part, if it works for you I think Monday afternoon Taipei time is a good timing, will be online if there's any issue.

Comment 30

[Zac C (:zac)]

Comment on attachment 8405198 [details] [review]
Gaia PR

Bebe or AndreiH, can you reset-gaia this locally onto your phone and run all of the call tests? Cheers.

Comment 31

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8405198 [details] [review]
Gaia PR

The build part seems good to me.

Comment 32

[[:AndreiH]]

Comment on attachment 8405198 [details] [review]
Gaia PR

I tested this without --restart command, tests fail at launch because Bug 990580.
5/6 tests passed: https://pastebin.mozilla.org/4784066

But that can be easily fixed in our tests, changing the self.wait_for_element_not_present(*self._call_screen_locator) to displayed as it should be: https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/apps/phone/regions/call_screen.py#L48

Comment 33

[Zac C (:zac)]

Ettienne, comment #32 is saying that the call screen (HTML) is not being removed from the DOM whereas previously it was.
Is this intentional? If so, just change line #48 there to `wait_for_element_not_displayed` as AndreiH suggested.

Comment 34

[Etienne Segonzac (:etienne)]

(In reply to Zac C (:zac) from comment #33)
> Ettienne, comment #32 is saying that the call screen (HTML) is not being
> removed from the DOM whereas previously it was.
> Is this intentional? 

Yes we need to keep it in the DOM.

> If so, just change line #48 there to
> `wait_for_element_not_displayed` as AndreiH suggested.

Done!

Thanks!

Comment 35

[Etienne Segonzac (:etienne)]

Comment on attachment 8405198 [details] [review]
Gaia PR

Just rebased against the new build system changes.
Yuren I think you should have a look.

We're not super proud of the shared/style/dialer folder, but it's the best compromise we found.

Comment 36

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Looks like it failed on travis ( https://travis-ci.org/mozilla-b2g/gaia/builds/22770418 ) for marionette:
2 failing
1) improve b2g "before each" hook:
Error: timeout of 60000ms exceeded
at null.<anonymous> (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:165:14)
at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

2) improve b2g "after each" hook:
Error: Not connected. To write data you must call connect first.
at TcpSync.send (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/drivers/tcp-sync.js:99:15)
at Object.send (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:458:36)
at Object.Client._sendCommand (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:504:19)
at Object.closeDriver (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:752:14)
at Object.executeHook (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:360:20)
at Object.Client.runHook (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:378:7)
at Object.destroySession [as deleteSession] (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:762:12)
at Context.<anonymous> (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-js-runner/lib/runtime/hostmanager.js:102:14)
at Hook.Runnable.run (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:194:15)
at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:257:10)
at Object._onImmediate (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:274:5)
at processImmediate [as _immediateCallback] (timers.js:330:15)

Comment 37

[Etienne Segonzac (:etienne)]

Attachment: 1.3t version for QA

Here is a branch with this patch and its 2 dependencies rebased on 1.3t.

Comment 38

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8405198 [details] [review]
Gaia PR

Don't want to be picky on attention screen change and I cannot figure out any drawback if we make callscreen in process. (Is the callscreen in process?)
BTW how do communicate with dialer since you cannot postMessage to window.parent?
Also there's a mozbrowserresize event to let callscreen to show its caller if my memory serves me right.
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/attention_screen.js#L67
Another question: how do you recover callscreen when you launch dialer or we are going to discard this feature?


And you owe me AttentionWindow review and help to move the transition. :)

Comment 39

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: 2014-04-11-09-29-16.png

I just took the branch again and put it on the gecko.

The dialer UI comes up pretty consistently as almost as soon as the ring starts happening which is awesome.

There's one oddity that I noticed and I'm not sure if it's me or not.  When dialing out, the number (using a contact name or just calling using a number) will show as withheld.  Incoming calls seem fine.

Comment 40

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Comment on attachment 8405453 [details]
1.3t version for QA

See attachment in regards to "Withheld number" when dialing out.
Otherwise this looks good from an user perspective.

Comment 41

[Etienne Segonzac (:etienne)]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #39)
> There's one oddity that I noticed and I'm not sure if it's me or not.  When
> dialing out, the number (using a contact name or just calling using a
> number) will show as withheld.  Incoming calls seem fine.

Interesting.
I can't reproduce it though, which gecko are you using?

Comment 42

[Etienne Segonzac (:etienne)]

(In reply to Alive Kuo [:alive][NEEDINFO!][God bless Taiwan.] from comment #38)
> Comment on attachment 8405198 [details] [review]
> Gaia PR
> 
> Don't want to be picky on attention screen change and I cannot figure out
> any drawback if we make callscreen in process. (Is the callscreen in
> process?)
> BTW how do communicate with dialer since you cannot postMessage to
> window.parent?
> Also there's a mozbrowserresize event to let callscreen to show its caller
> if my memory serves me right.
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> attention_screen.js#L67

Just repeating the discussion on IRC, those were handled by the dependency bugs.

> Another question: how do you recover callscreen when you launch dialer or we
> are going to discard this feature?

We're indeed losing the fact that when a call is ongoing opening the dialer won't expand the call screen from the status bar (but placing a new call will).

Comment 43

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

(In reply to Etienne Segonzac (:etienne) from comment #41)
> (In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from
> comment #39)
> > There's one oddity that I noticed and I'm not sure if it's me or not.  When
> > dialing out, the number (using a contact name or just calling using a
> > number) will show as withheld.  Incoming calls seem fine.
> 
> Interesting.
> I can't reproduce it though, which gecko are you using?

1.3t Gecko
Version   28.1
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/257dd37da601

Comment 44

[Etienne Segonzac (:etienne)]

Comment on attachment 8405198 [details] [review]
Gaia PR

Flagging Shawn for feedback so we don't loose track of the pending bluetooth testing.

Comment 45

[Etienne Segonzac (:etienne)]

I was able to reproduce the bug from Comment 39 with the latest PVT build.

It does not reproduce with this patch and a gecko from last week.
It does not reproduce without this patch on the latest gecko build.

But, did some debugging and call.number is "" (empty string) for outgoing calls :/
Tried checking again when the call is connected, still "".
The state of the call is good though, and goes from "dialing" to "connected"

Pretty weird, and works perfectly for incoming calls, any idea Hsin-Yi?

Comment 46

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Etienne Segonzac (:etienne) from comment #45)
> I was able to reproduce the bug from Comment 39 with the latest PVT build.
> 
> It does not reproduce with this patch and a gecko from last week.
> It does not reproduce without this patch on the latest gecko build.
> 
> But, did some debugging and call.number is "" (empty string) for outgoing
> calls :/
> Tried checking again when the call is connected, still "".
> The state of the call is good though, and goes from "dialing" to "connected"
> 
> Pretty weird, and works perfectly for incoming calls, any idea Hsin-Yi?

Do you have any logs, including RIL messages? 

I have a problem with flashing gaia :( Will check it tomorrow!

Comment 47

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I'm having trouble to test bluetooth handsfree profile. Now, b2g process will be killed by low memory shrink after making outgoing call with bluetooth headset.

Comment 48

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #47)
> I'm having trouble to test bluetooth handsfree profile. Now, b2g process
> will be killed by low memory shrink after making outgoing call with
> bluetooth headset.
See bug 994411.

Comment 49

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Keep in? flag.

Comment 50

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Can I test dialer patch on branch 1.3 using Buri? I cannot test on Tarako currently due to b2g process will be killed due to out-of-memory. Since we only need to check the bluetooth logic part.

Comment 51

[Etienne Segonzac (:etienne)]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #50)
> Can I test dialer patch on branch 1.3 using Buri? I cannot test on Tarako
> currently due to b2g process will be killed due to out-of-memory. Since we
> only need to check the bluetooth logic part.

Yes I did my some bluetooth testing on a buri 1.3 + the gaia from the PR, it worked fine for telephony stuffs.

Comment 52

[Etienne Segonzac (:etienne)]

Try run (I hope):
https://tbpl.mozilla.org/?tree=Try&rev=13547900b946

Comment 53

[Etienne Segonzac (:etienne)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #46)
> Do you have any logs, including RIL messages? 

Sadly no, I only flash my own gaia on tarakos.

> 
> I have a problem with flashing gaia :( Will check it tomorrow!

Thanks !

Comment 54

[Etienne Segonzac (:etienne)]

Attachment: Gaia PR against 1.3t

Comment 55

[James Zhang (Spreadtrum)]

We need land this patch today.

Comment 56

[[:fabrice] Fabrice Desré]

That will land once Rik has given r+

Comment 57

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8405198 [details] [review]
Gaia PR

I have tested:
* Bluetooth handsfree 3-way calling
* Bluetooth memory dialing / last number dialing

Comment 58

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Etienne Segonzac (:etienne) from comment #45)
> I was able to reproduce the bug from Comment 39 with the latest PVT build.
> 
> It does not reproduce with this patch and a gecko from last week.
> It does not reproduce without this patch on the latest gecko build.
> 
> But, did some debugging and call.number is "" (empty string) for outgoing
> calls :/
> Tried checking again when the call is connected, still "".
> The state of the call is good though, and goes from "dialing" to "connected"
> 
> Pretty weird, and works perfectly for incoming calls, any idea Hsin-Yi?

There's a miss when we provided a 1.3t patch for bug 990467. I filed a followup bug 996421 to address the issue.

Comment 59

[Etienne Segonzac (:etienne)]

(In reply to Fabrice Desré [:fabrice] from comment #56)
> That will land once Rik has given r+

And be uplifted only once bug 996421 lands on 1.3t.

Comment 60

[Anthony Ricaud (:rik)]

Comment on attachment 8405198 [details] [review]
Gaia PR

We did a pair review yesterday and Étienne addressed all my comments, so good to go.
One "regression" from this patch is that when the last call hangs up, during the sliding transition, we see the phone background instead of the app. But I'm fine with it and this will be fixed in a follow up (probably the AttentionWindow refactoring).

Comment 61

[Anthony Ricaud (:rik)]

Attachment: With patch applied

This is a representation of our users with this patch applied.

Comment 62

[Etienne Segonzac (:etienne)]

\o/

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

The PR again 1.3t is up to date, but if we land it before bug 996421 we'll trigger the bug mentioned in Comment 39.

Thanks all, this was a fun one :)

Comment 63

[James Zhang (Spreadtrum)]

Please land it on v1.3t.

Comment 64

[[:fabrice] Fabrice Desré]

I merged, even if we don't yet have the fix for bug 996421 landed:
https://github.com/mozilla-b2g/gaia/commit/8141796c477d17be6ebca44309e6cb82d3f6670a

Comment 65

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Finally, I can build a image with slog disabled. Without this or other patches to improve performance, the call screen can be displayed within 2 ringbacks when playing music. Do we need to re-consider if we want to move callscreen into b2g process?

Comment 69

[Steven Yang [:styang]]

(In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #65)
> Finally, I can build a image with slog disabled. Without this or other
> patches to improve performance, the call screen can be displayed within 2
> ringbacks when playing music. Do we need to re-consider if we want to move
> callscreen into b2g process?

James, could you please explain why slog is needed even in user build? thanks!

Comment 70

[[:fabrice] Fabrice Desré]

(In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #65)
> Finally, I can build a image with slog disabled. Without this or other
> patches to improve performance, the call screen can be displayed within 2
> ringbacks when playing music. Do we need to re-consider if we want to move
> callscreen into b2g process?

Let's not do further changes. We need to stabilize now.

Comment 71

[James Zhang (Spreadtrum)]

(In reply to Steven Yang [:styang] from comment #69)
> (In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #65)
> > Finally, I can build a image with slog disabled. Without this or other
> > patches to improve performance, the call screen can be displayed within 2
> > ringbacks when playing music. Do we need to re-consider if we want to move
> > callscreen into b2g process?
> 
> James, could you please explain why slog is needed even in user build?
> thanks!

OEM will open slog by engineer mode.

Comment 72

[James Zhang (Spreadtrum)]

And we will disable slog in user build.

Comment 73

[Mike Lien[:mlien]]

verified and fixed with today's daily build
screen show up time is 2.15 seconds

Gaia      3f1d8332d127f1d6bc0fdbb08146ce1deb0efbc0
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/5da76152c2bd
BuildID   20140421164001
Version   28.1

Comment 75

[Mike Lee [:mlee]]

Preeti, do we want this fix for 1.4?

Comment 76

[Etienne Segonzac (:etienne)]

(In reply to Mike Lee [:mlee] from comment #75)
> Preeti, do we want this fix for 1.4?

Not knowing the answer to this is already causing issues with uplifts...
We need to know ASAP.

Comment 77

[Preeti Raghunath(:Preeti)]

(In reply to Mike Lee [:mlee] from comment #75)
> Preeti, do we want this fix for 1.4?

Lets take it in 1.4 as well. Sorry for the delay.

Comment 78

[James Zhang (Spreadtrum)]

(In reply to Preeti Raghunath(:Preeti) from comment #77)
> (In reply to Mike Lee [:mlee] from comment #75)
> > Preeti, do we want this fix for 1.4?
> 
> Lets take it in 1.4 as well. Sorry for the delay.

Yes, we have kicked off v1.4 dolphin and shark project now.
Please cherry-pick to v1.4 branch.

Comment 80

[James Zhang (Spreadtrum)]

When can we land it on v1.4?

Comment 81

[Ivan Tsay (:ITsay)]

Comment on attachment 8405198 [details] [review]
Gaia PR

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 82

[Etienne Segonzac (:etienne)]

(In reply to James Zhang from comment #80)
> When can we land it on v1.4?

Yes just let me know if there's a conflict while uplifting

Comment 83

[Preeti Raghunath(:Preeti)]

Ivan

Please have these responses filled in.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 84

[Ivan Tsay (:ITsay)]

Hi Preeti, 

Here is the update, please help to proceed approval process. 

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): N/A
[User impact] if declined: Partner blocker on Dolphin
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Mid
[String changes made]: N/A

Comment 85

[Jason Smith [:jsmith]]

Comment on attachment 8405198 [details] [review]
Gaia PR

This is way too risky to take into 1.4 at this point. There's no strong compelling case to consider this for 1.4 anyways, as this was specifically needed for Tarako.

Comment 86

[Etienne Segonzac (:etienne)]

(In reply to Jason Smith [:jsmith] from comment #85)
> Comment on attachment 8405198 [details] [review]
> Gaia PR
> 
> This is way too risky to take into 1.4 at this point. There's no strong
> compelling case to consider this for 1.4 anyways, as this was specifically
> needed for Tarako.

Just FTR, not uplifting this also means that every dialer related uplift to 1.4 will probably conflict (since we now have 2 separated apps).
Which is fine, but we better not change our opinion once again after that since we're going to make this *very* hard to uplift.

I raised this issue at the end for April (Comment 76) so having relman and QA still arguing now is troublesome.

Comment 87

[Jason Smith [:jsmith]]

(In reply to Etienne Segonzac (:etienne) from comment #86)
> (In reply to Jason Smith [:jsmith] from comment #85)
> > Comment on attachment 8405198 [details] [review]
> > Gaia PR
> > 
> > This is way too risky to take into 1.4 at this point. There's no strong
> > compelling case to consider this for 1.4 anyways, as this was specifically
> > needed for Tarako.
> 
> Just FTR, not uplifting this also means that every dialer related uplift to
> 1.4 will probably conflict (since we now have 2 separated apps).
> Which is fine, but we better not change our opinion once again after that
> since we're going to make this *very* hard to uplift.
> 
> I raised this issue at the end for April (Comment 76) so having relman and
> QA still arguing now is troublesome.

Lawrence is going to discuss this with Preeti. From my perspective, I don't think we should take this. This had a lot of blocking regression fallouts, which is something we can't take into 1.4 at this point. This is also technically a feature request for 1.4.

Comment 88

[Jason Smith [:jsmith]]

I said this once, I'll say again. Way too risky for 1.4, this is not being taken to 1.4 at this time. There's no compelling reason to do so.

Comment 89

[Ivan Tsay (:ITsay)]

(In reply to Jason Smith [:jsmith] from comment #88)
> I said this once, I'll say again. Way too risky for 1.4, this is not being
> taken to 1.4 at this time. There's no compelling reason to do so.

Hi Jason,

If this bug is not uplifted to v1.4, the impact to partner is that:
1. incoming screen appears about 2s after ringtone starts when there is no background app. If there is music on background, the delay is about 3s.
2. calling screen appears about 2.4s after user tap on dial button.

So, What if I can find the QA to verify the patch with v1.4 on Flame locally and see if it will cause any smoke test regression before requesting the approval again? what do you think?

Comment 90

[Jason Smith [:jsmith]]

I don't think local verification is enough here - even with thorough testing, a patch like this can blocking fallouts in areas we don't expect to fail. We also aren't doing heavy testing on 1.4 anymore, so this we really can take patches at this point that are low risk.

What I'd like to know here is the following:

1. How does the performance impact of Dolphin compare to Buri & Flame on 1.4?
2. Are we missing performance targets for Dolphin right now for the call screen? If so, how far off are we from those targets?

Comment 91

[Lawrence Mandel [:lmandel] (use needinfo)]

I agree with Jason in terms of the risk of this rather large patch. Jason is speaking from experience in landing this on 1.3T. There really needs to be an extremely strong need to take a bug like this on 1.4 at this point and I have not yet seen it in the comments. (We need to clarify the scope of our commitment for Dolphin.) I also didn't see much in the way of a risk assessment or explanation of the testing that has been competed in the uplift request in comment 84. I think Etienne made a good point about code divergence in comment 86. We owe it to everyone to come to a decision on this bug ASAP.

1. I would like to see answers to the questions in comment 90. 
2. I would also like to better understand the user impact from the numbers in comment 89. Do these delays make the phone unusable? ie. Can the delay on the incoming call cause a user to miss an incoming call?
3. Do we have alternative options to improve the times in comment 90 that require smaller and less risky changesets?

Comment 92

[Etienne Segonzac (:etienne)]

(In reply to Lawrence Mandel [:lmandel] from comment #91)
> 3. Do we have alternative options to improve the times in comment 90 that
> require smaller and less risky changesets?

On the gaia side not really, we pretty much tried everything else before coming to this solution.

Comment 93

[Ivan Tsay (:ITsay)]

Attachment: Delay_test_Dolphin+Flame.tiff

Performance test for call screen delay on Dolphin and Flame

Comment 94

[Ivan Tsay (:ITsay)]

(In reply to Lawrence Mandel [:lmandel] from comment #91)
> I agree with Jason in terms of the risk of this rather large patch. Jason is
> speaking from experience in landing this on 1.3T. There really needs to be
> an extremely strong need to take a bug like this on 1.4 at this point and I
> have not yet seen it in the comments. (We need to clarify the scope of our
> commitment for Dolphin.) I also didn't see much in the way of a risk
> assessment or explanation of the testing that has been competed in the
> uplift request in comment 84. I think Etienne made a good point about code
> divergence in comment 86. We owe it to everyone to come to a decision on
> this bug ASAP.
> 
> 1. I would like to see answers to the questions in comment 90. 

Here the test result on Dolphin and Flame. We do see a significant delay on Dolphin if more apps are in the background. The more memory is consumed, the longer the delay. Flame has less delay because it has 1G memory. The issue does significant impact the phone that is going to be shipped form our partner.

Dolphin: Delay from 2 seconds (no background app) up to 7 seconds (with background app Camera+Gallery+Music)
Flame: Delay from 1 second (no background app) up to 3 seconds (with background app Camera+Gallery+Music)

I attach the test result in the attachment for your reference.


> 2. I would also like to better understand the user impact from the numbers
> in comment 89. Do these delays make the phone unusable? ie. Can the delay on
> the incoming call cause a user to miss an incoming call?

Since the call screen for the user to pick up the phone is delayed, it is possible the users will miss the call because there is no way for them to pick up the call during the delay. For the worse case, the delay for 7 seconds is way to long that the phone should has been ringing for a while and the caller may hand up the phone. 

> 3. Do we have alternative options to improve the times in comment 90 that
> require smaller and less risky changesets?

Already answered by Eitan in comment 92.

Since we already land this bug in 2.0 an 1.3T, I think the risk should be manageable by my suggestion in comment 89. I can find the QA to verify the patch with v1.4 on Flame locally and see if it will cause any smoke test regression before uplifting the patch.

Comment 95

[Ivan Tsay (:ITsay)]

Attachment: Call_delay_Flame_256.tiff

Limit Flame to memory in 256MB. The delay of the call screen is also significant longer if there are background apps.

Comment 96

[Ivan Tsay (:ITsay)]

add ni? for answer the questions in comment 91

Comment 97

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Etienne Segonzac (:etienne) from comment #92)
> (In reply to Lawrence Mandel [:lmandel] from comment #91)
> > 3. Do we have alternative options to improve the times in comment 90 that
> > require smaller and less risky changesets?
> 
> On the gaia side not really, we pretty much tried everything else before
> coming to this solution.

Seems that this is the only option we have if we want to fix the issue in 1.4 then. Thanks.

(In reply to Ivan Tsay (:ITsay) from comment #94)
> Here the test result on Dolphin and Flame. We do see a significant delay on
> Dolphin if more apps are in the background. The more memory is consumed, the
> longer the delay. Flame has less delay because it has 1G memory. The issue
> does significant impact the phone that is going to be shipped form our
> partner.
> 
> Dolphin: Delay from 2 seconds (no background app) up to 7 seconds (with
> background app Camera+Gallery+Music)
> Flame: Delay from 1 second (no background app) up to 3 seconds (with
> background app Camera+Gallery+Music)
> 
> I attach the test result in the attachment for your reference.

Thank you for posting the results. They are helpful for this discussion.
 
> > 2. I would also like to better understand the user impact from the numbers
> > in comment 89. Do these delays make the phone unusable? ie. Can the delay on
> > the incoming call cause a user to miss an incoming call?
> 
> Since the call screen for the user to pick up the phone is delayed, it is
> possible the users will miss the call because there is no way for them to
> pick up the call during the delay. For the worse case, the delay for 7
> seconds is way to long that the phone should has been ringing for a while
> and the caller may hand up the phone. 

Agreed. If the network has established a connection and has initiated 'ringing' for the caller, a 7 second delay may well be enough time for them to disconnect before the phone rings.

Jason - Can you comment on this user impact of this issue from a QA perspective? (Not whether or not we should take a fix just how this issue compares to others that QA has been willing to accept in prior releases.)
  
> Since we already land this bug in 2.0 an 1.3T, I think the risk should be
> manageable by my suggestion in comment 89. I can find the QA to verify the
> patch with v1.4 on Flame locally and see if it will cause any smoke test
> regression before uplifting the patch.

I agree that we should have some sense of what is likely to break/regress based on our prior experience. However, a smoke test is not a thorough test. We are still opening up the branch to regressions by taking a large patch.

Jason - Can you itemize the specific regressions introduced when this patch landed on 1.3T and 2.0 so that we can have a sense of the scope of the risk based on our prior landings? 

I think the bigger question is whether this bug is in scope for the work that we agreed to take on the 1.4 branch at this point in the schedule (I don't know that supporting a 256MB device was previously a requirement of 1.4) and, if our partner is going to take this patch on their branch if we don't take it on 1.4, do we want to have more visibility and control over the regressions or leave the resolution of these issues to our partner.

Comment 98

[Jason Smith [:jsmith]]

(In reply to Lawrence Mandel [:lmandel] from comment #97)
> > > 2. I would also like to better understand the user impact from the numbers
> > > in comment 89. Do these delays make the phone unusable? ie. Can the delay on
> > > the incoming call cause a user to miss an incoming call?
> > 
> > Since the call screen for the user to pick up the phone is delayed, it is
> > possible the users will miss the call because there is no way for them to
> > pick up the call during the delay. For the worse case, the delay for 7
> > seconds is way to long that the phone should has been ringing for a while
> > and the caller may hand up the phone. 
> 
> Agreed. If the network has established a connection and has initiated
> 'ringing' for the caller, a 7 second delay may well be enough time for them
> to disconnect before the phone rings.
> 
> Jason - Can you comment on this user impact of this issue from a QA
> perspective? (Not whether or not we should take a fix just how this issue
> compares to others that QA has been willing to accept in prior releases.)

7 seconds is pretty bad. Although the question I have is this:

What happens on 1.3 Buri right now?

If we're worse than Buri, then that means we're shipping with lower quality than past releases that have already shipped. If we're the same, then that means at least one partner has been okay with shipping with this problem.

Ivan - Can you get comparable data here on Buri 1.4?

>   
> > Since we already land this bug in 2.0 an 1.3T, I think the risk should be
> > manageable by my suggestion in comment 89. I can find the QA to verify the
> > patch with v1.4 on Flame locally and see if it will cause any smoke test
> > regression before uplifting the patch.
> 
> I agree that we should have some sense of what is likely to break/regress
> based on our prior experience. However, a smoke test is not a thorough test.
> We are still opening up the branch to regressions by taking a large patch.
> 
> Jason - Can you itemize the specific regressions introduced when this patch
> landed on 1.3T and 2.0 so that we can have a sense of the scope of the risk
> based on our prior landings? 

The two known regressions are:

* bug 997924
* bug 1000047

Comment 99

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #98)
> (In reply to Lawrence Mandel [:lmandel] from comment #97)
> > > > 2. I would also like to better understand the user impact from the numbers
> > > > in comment 89. Do these delays make the phone unusable? ie. Can the delay on
> > > > the incoming call cause a user to miss an incoming call?
> > > 
> > > Since the call screen for the user to pick up the phone is delayed, it is
> > > possible the users will miss the call because there is no way for them to
> > > pick up the call during the delay. For the worse case, the delay for 7
> > > seconds is way to long that the phone should has been ringing for a while
> > > and the caller may hand up the phone. 
> > 
> > Agreed. If the network has established a connection and has initiated
> > 'ringing' for the caller, a 7 second delay may well be enough time for them
> > to disconnect before the phone rings.
> > 
> > Jason - Can you comment on this user impact of this issue from a QA
> > perspective? (Not whether or not we should take a fix just how this issue
> > compares to others that QA has been willing to accept in prior releases.)
> 
> 7 seconds is pretty bad. Although the question I have is this:
> 
> What happens on 1.3 Buri right now?

This should say "1.4 Buri"

Comment 100

[Ivan Tsay (:ITsay)]

Hi Pei-pei,

Please help to test/check the this issue on Buri v1.4? Thanks!

Comment 101

[Peipei Cheng (needinfo if you need my action)]

Buri v1.4 has similar problem.

1. No Background App - incoming call delay 2s
2. Music background - incoming call delay 3s
3. Gallery + Music - incoming call delay 5s+, up to 10s sometimes, Music will get killed after call.
   Marketplace - incoming call delay 5s+, up to 10s sometimes.

In test 3, frontend apps and Homescreen will be killed first (due to low memory) and then call screen was launched. The delay can be up to 10s if there is not much memory left on device.

Comment 102

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8405198 [details] [review]
Gaia PR

We've been requested by Spreadtrum to have this uplifted for Dolphin 1.4 release. Does that change anything?

Comment 103

[Jan Jongboom [:janjongboom] (inactive)]

Plus a depending bug has 1.4 uplift approval but this patch doesn't have it... (bug 1002327)

Comment 104

[Jason Smith [:jsmith]]

Comment on attachment 8405198 [details] [review]
Gaia PR

Not really. From Mozilla's perspective, we will only consider performance improvements for 1.4 that demonstrate that Dolphin is worse than Buri for 1.4. The above testing shows that Buri & Dolphin are on par for performance, which is something we've shipped with historically. On that regard, we will not be taking this into 1.4.

Comment 105

[Jan Jongboom [:janjongboom] (inactive)]

Cross-comment from bug 1002327, Gabriele doesn't agree:

(In reply to Gabriele Svelto [:gsvelto] from comment #34)
> (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.

Comment 106

[James Zhang (Spreadtrum)]

Dolphin 1.4 has real production, I think dolphin 1.4 should be better than tarako v1.3t. Nobody can accept that FirefoxOS 1.4 with CotexA7@1.2G and 256MB RAM is worse than FirefoxOS 1.3 with CotexA5@1G and 128MB RAM.

Comment 107

[Jason Smith [:jsmith]]

I'm not interested in continuing this argument. We've already been through this discussion multiple times & are in jeopardy of damaging one of our partner relationships if we take this.

Comment 108

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #107)
> I'm not interested in continuing this argument. We've already been through
> this discussion multiple times & are in jeopardy of damaging one of our
> partner relationships if we take this.

I think the wording here might be coming across a bit too strong. Let me try reword this & clarify a bit more.

I think there's agreement that we want Dolphin to have solid performance for the call screen in the 1.4 time frame. However, we have to be conscious of the risk vs. reward in triage decisions based on the current time of the release to know if we can safely take a patch into the mainline release and maintain the stability guidelines that match the active timing of the release. At this point in the release, we're post 12 weeks in stability, which places a requirement for the release as a whole that we must keep the influx of instability low since we are at the point where Mozilla is trying to close out the current release. I understand that there's late breaking partner needs that might arrive for a release at this point though, so my suggestion is that SPRD pull the patch here into their own custom gaia branch that they can use for 1.4 development. I think this strategy will satisfy SPRD's needs (e.g. comment 106) and Mozilla's needs for shutting down a release in a post 12 weeks of stabilization.

Comment 109

[Wayne Chang [:wchang]]

Hi Etienne,

Can you let us know if there is a 1.4 compatible patch ready and if so, please set request for 1.4 approval?

This is right now blocking partner testing and we'd like to see this move forward.
Thanks.

Comment 110

[Etienne Segonzac (:etienne)]

(In reply to Wayne Chang [:wchang] from comment #109)
> Hi Etienne,
> 
> Can you let us know if there is a 1.4 compatible patch ready and if so,
> please set request for 1.4 approval?
> 

Gabriele will make a 1.4 version today.
This would have been so much easier to uplift back in May...

Comment 111

[Gabriele Svelto [:gsvelto]]

FYI I'm prepping a v1.4 version of both this bug and bug 992948 which is a dependency. I'm doing this as I need to uplift bug 1002327. That being said since it's still unclear to me if this is approved for v1.4 or not I won't land right away. Worst case if we cannot uplift the dependencies I'll make a v1.4-tailored version of the patch in bug 1002327 though I'm not very fond of the idea.

Comment 112

[Anshul]

Please make sure that this patch only goes on 1.4 dolphin branch.

Comment 113

[Preeti Raghunath(:Preeti)]

Anshul

This isn't going in 1.4

Comment 114

[Thomas Ho [:tho]]

The 1.4 branch is the Dolphin branch now.

Comment 115

[Ivan Tsay (:ITsay)]

(In reply to Gabriele Svelto [:gsvelto] from comment #111)
> FYI I'm prepping a v1.4 version of both this bug and bug 992948 which is a
> dependency. I'm doing this as I need to uplift bug 1002327. That being said
> since it's still unclear to me if this is approved for v1.4 or not I won't
> land right away. Worst case if we cannot uplift the dependencies I'll make a
> v1.4-tailored version of the patch in bug 1002327 though I'm not very fond
> of the idea.

Hi  Gabriele,

Thank you for preparing the 1.4 compatible patch. Once the patch is ready, please set the flag "approval-gaia-v1.4?" for requesting the uplift.

Comment 116

[Gabriele Svelto [:gsvelto]]

Attachment: WIP cherry-picked patch for v1.4

This is the first iteration of the cherry picked patch for v1.4. I had to fix a few conflicts which were all related to bugs that did not affect the callscreen/dialer directly so instead of uplifting them too I fixed the conflicts manually. I'm trying to keep the uplifts to a minimum in order to reduce our testing footprint. BTW this hasn't been tested yet except for the unit-tests which were run locally and seem to pass.

You can find this patch and its dependencies on this branch which can be used for testing the entire uplift:

https://github.com/gabrielesvelto/gaia/tree/callscreen-uplift

Comment 117

[Ivan Tsay (:ITsay)]

HI William,

Mind if you can help to test patch provide by Gabriele in comment 116 with v1.4. Thanks!

Comment 118

[William Hsu [:whsu]]

Sure!
But, I cannot provide you the result today because I still need to rebuild the Dolphin build.
The actions I will finish is as following.
 - Reproduce this bug on Dolphin or Buri
 - Test the WIP patch
The deadline is 6/30 (UTC+8).
Sorry for any inconvenience.
Thanks!


* Note: Set up Dolphin build need one day effort.
(...keep needinfo whsu.)

Comment 119

[Gabriele Svelto [:gsvelto]]

Attachment: Cherry-picked patch for v1.4

I've cleaned up the patch and updated the PR mentioned in comment 116. Now all linter tests pass and I've also fixed a unit-test related to the TonePlayer. I'm now proceeding with actual device tests on my Hamachi. I'll ask for approval as soon as I'm confident this works as expected.

Comment 120

[Gabriele Svelto [:gsvelto]]

Unfortunately the Try-Gaia runs are failing due to harness issues, I suspect this is because we're using the v1.4 branch and not master:

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=25c37fa041ac0638184ca52c4e963d2cfd590ead

This is not very helpful unfortunately.

Comment 121

[Gabriele Svelto [:gsvelto]]

Tests on the device are not very promising at this stage. The callscreen opens up correctly both while placing and receiving calls and also works properly during the call however once the call ends the attention screen containing it doesn't seem to go away properly. Maybe I'm missing some system / window manager / attention screen patches, will investigate further.

Comment 122

[William Hsu [:whsu]]

There seemed to have something wrong on the patch.
Please needinfo "[e-mail]" while the patch is ready to test.
Thanks Gabriele!

Comment 123

[Gabriele Svelto [:gsvelto]]

I've been a bit stuck today as I couldn't get a working v1.4 build on my hamachi so I'm asking Etienne to have a look; being the original author of the patch maybe he can get past the roadblock I found.

To sum it up, bug 992948 and bug 993018 have been uplifted already and with attachment 8447073 [details] [diff] [review] applied I can get the callscreen to show up correctly both when receiving calls and when making ones. I've tested also some additional functionality such as the keypad, DTMF tones and friends and everything seems to work fine. However once the callscreen gets closed it slides back correctly but then the homescreen remains hidden. One can see only the background. I've looked at the dependencies and I'm not sure if I've missed something or not. I was also wondering if we might have needed to uplift some of the follow-ups such as bug 999478 and bug 997924.

Comment 124

[Gabriele Svelto [:gsvelto]]

I've had a quick conversation on IRC with Etienne about this and he suggested a fix for the issue I've been experiencing. There's more to be done here though:

- Etienne mentioned that all of the blocking bugs should be uplifted, AFAIK we haven't done all of them already so that will take time.

- I haven't had a working v1.4 build since yesterday morning which effectively bars me from making any progress here.

- It's still not clear to me if we also have to take some of the follow ups to this bug or not (such as bug 997924).

I'll report ASAP here but all in all I sincerely doubt I'll be able to wrap all this stuff up by today.

Comment 125

[Etienne Segonzac (:etienne)]

Update:
I just opened a pull request [1] uplifting this patch and the needed dependencies all at once.
I tested it on a hamachi, worked well.

While the CI is running on the pull request I'll do some more manual testing on the dolphin.

[1] https://github.com/mozilla-b2g/gaia/pull/21282

Comment 126

[Etienne Segonzac (:etienne)]

Yep, this pull request is the one :)

Now it's a matter of getting a try/travis build to run on it.

Comment 127

[Etienne Segonzac (:etienne)]

Try build started (thanks to :jhford!): https://tbpl.mozilla.org/?rev=2e6f44bf3158d0385486aedff9b33835d2b3213d&tree=Gaia-Try

Comment 128

[Etienne Segonzac (:etienne)]

I'm off for the day, feel free to merge if needed, try is still running but looks like the only issues are intermittents.

If somebody merges before I get to it tomorrow morning, please update the tracking flag on all the bugs in the pull request.

Comment 129

[Etienne Segonzac (:etienne)]

Attachment: PR with dependencies against 1.4

Comment 130

[Ivan Tsay (:ITsay)]

Hi William,

Please help to test 1.4 with the PR from comment 129. Thanks.

Comment 131

[William Hsu [:whsu]]

(In reply to Ivan Tsay (:ITsay) from comment #130)
> Hi William,
> 
> Please help to test 1.4 with the PR from comment 129. Thanks.

Roger that! I will post the result later. :)

(Keep NI? "whsu")

Comment 132

[Etienne Segonzac (:etienne)]

Try is looking good (except the known unit test nightmare, but travis was good for those).

I'll wait for the QA round before merging, just tell me when to press the green button :)

Comment 133

[William Hsu [:whsu]]

Hi, everyone,

I have verified this patch on master(Flame) and v1.3t(Tarako).
Here is the symptom I saw.

* Result:
 - Flame(256MB version): Sometimes, lag happens on dialer while you are answering a phone call and there have others apps are running in background.
   I think this is because we are not yet merged OOM/LMK related patches on master branch.
 - Tarako: Cannot reproduce this bug.

* Build information:
 @ Flame:
   - Gaia      7a32f4ce7f922ed174946cfe322f3d6df40f18ea
   - Gecko     https://hg.mozilla.org/mozilla-central/rev/06e9a27a6fcc
   - BuildID   20140703160203
   - Version   33.0a1
 @ Tarako:
   - Gaia      64716f3f6dd79c40ae79ccaf96ebd51645694ce3
   - Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/0c6e9a70115e
   - BuildID   20140703164010
   - Version   28.1


* Conclusion: We can merge this patch on v1.4 then fine tune dialer because I think we still need some OOM/LMK related patches to optimize Dolphin device.

Comment 134

[Wayne Chang [:wchang]]

(In reply to Etienne Segonzac (:etienne) from comment #132)
> Try is looking good (except the known unit test nightmare, but travis was
> good for those).
> 
> I'll wait for the QA round before merging, just tell me when to press the
> green button :)

Etienne, with Williams' comment can you please merge this today?
Thakns

Comment 135

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/38250fcbca0e3ae1ced0b4449a9c31a3d428d65e

Comment 136

[Ivan Tsay (:ITsay)]

Comment on attachment 8449618 [details] [review]
PR with dependencies against 1.4

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug found from partner test
[User impact] if declined: User may not be able to pick up the call since the callscreen will be delayed.
[Testing completed]: Yes in 1.3T, 2.0 official test, and 1.4 local test by QA
[Risk to taking this patch] (and alternatives if risky): Low, since it is rebased and tested by QA
[String changes made]: N/A

Comment 137

[William Hsu [:whsu]]

Thanks, Ivan and Wayne!
Here is the build information we tested today.
It has merged Etienne's patch. FYI.

* Build information:
 - Gaia      b92011e9e7259f41c67b97df43568cadffa65a2f
 - Gecko     https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4c05c3fb440d
 - BuildID   20140703000230
 - Version   30.0

Comment 138

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8449618 [details] [review]
PR with dependencies against 1.4

This patch is known to break l10 in the lockscreen app. (See bug 1032502.) The solution being proposed in bug 1032502 is a build time hack. We should resolve this issue before landing this patch and causing a regression on 1.4.

Comment 139

[Etienne Segonzac (:etienne)]

(In reply to Lawrence Mandel [:lmandel] from comment #138)
> Comment on attachment 8449618 [details] [review]
> PR with dependencies against 1.4
> 
> This patch is known to break l10 in the lockscreen app. (See bug 1032502.)
> The solution being proposed in bug 1032502 is a build time hack. We should
> resolve this issue before landing this patch and causing a regression on 1.4.

Should I back this out right away?
(it landed on 1.4 this morning with the right dependencies)

Who's the ultimate decision maker here?

I'm sorry about the mess, but look at the bug history (which does not include all the email back an forth) we're maybe 6 "out of process over-ruling of the previous decision" in already.
We've been flushing engineering resources down the toilet and everybody's patience is draining very quickly...

Comment 140

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Etienne Segonzac (:etienne) from comment #139)
> (In reply to Lawrence Mandel [:lmandel] from comment #138)
> Should I back this out right away?
> (it landed on 1.4 this morning with the right dependencies)
Given that this has already landed and is going to have to land on 1.4, let's pull :drs into this bug (he's been fixing things in bug 1032502) to see if he has a preference for how to proceed.

> Who's the ultimate decision maker here?
Kevin Hu is the release owner and ultimate decision maker for 1.4.

> I'm sorry about the mess, but look at the bug history (which does not
> include all the email back an forth) we're maybe 6 "out of process
> over-ruling of the previous decision" in already.
> We've been flushing engineering resources down the toilet and everybody's
> patience is draining very quickly...
Completely understandable. I want to put this one to bed as well.

Comment 141

[Doug Sherk (:drs) (inactive)]

Sorry Etienne, I should have mentioned this at our standup, but I thought I would be ready to land my fixes as soon as this landed on v1.4. I think it's best if we leave this as-is instead of backing it out and I'll get bug 1032502 and bug 1030546 landed as well ASAP.

Comment 142

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8449618 [details] [review]
PR with dependencies against 1.4

Change to 1.4+ based on drs response in comment 141. To be clear though, bug 1032502 needs to land in order for this to stick on 1.4.

Comment 143

[Kai-Chih Hu [:khu]]

(In reply to Doug Sherk (:drs) from comment #141)
> I think it's
> best if we leave this as-is instead of backing it out 
+1 to :drs's comment 141 here.