Closed Bug 814840 Opened 12 years ago Closed 11 years ago

Remove workaround about 'hold-home' during FTU

Categories

(Firefox OS Graveyard :: Gaia::First Time Experience, defect, P1)

Product:
Firefox OS Graveyard
Component:
Gaia::First Time Experience
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(blocking-b2g:leo+, blocking-basecamp:-, b2g18+ fixed, b2g-v1.1hd fixed)

Status:
VERIFIED FIXED
Project Flags:
blocking-b2g leo+
blocking-basecamp -
Tracking Flags:
Tracking Status
b2g18 + fixed
b2g-v1.1hd --- fixed

People

(Reporter: borjasalguero, Assigned: alive)

References

Details

Attachments

(3 files)

https://github.com/mozilla-b2g/gaia/pull/8435
86 bytes, text/html
timdream
: review+
Details
cuttherope_application.zip
3.72 MB, application/octet-stream
Details
cuttherope_sound_issue_repro_video
9.11 MB, application/octet-stream
alive
: feedback+
Details 
As we agree through IRC, It would be necessary to 'refactor' this workaround in order to align the code with 'System App' structure. It's a refactor, so once the patch would be ready the code should be align with the rules of 'System App' and all the functionality should be working:
- Timing during FTU should be right
- Hold home during FTU should not be working
blocking-basecamp: --- → ?
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Assignee: nobody → fernando.campo
blocking-basecamp: ? → +
Priority: -- → P1
Thanks for filing the bug and work on this.
Assignee: fernando.campo → nobody
(In reply to Borja Salguero [:borjasalguero] from comment #0)
> As we agree through IRC, It would be necessary to 'refactor' this workaround
> in order to align the code with 'System App' structure. It's a refactor, so
> once the patch would be ready the code should be align with the rules of
> 'System App' and all the functionality should be working:
> - Timing during FTU should be right
> - Hold home during FTU should not be working

What is the downside of letting the workaround in for v1?
Flags: needinfo?(fbsc)
I'm renominating this bug and let's take a blocking decision on it while it is clear what are the side effect of not removing the workaround.
blocking-basecamp: + → ?
Flags: needinfo?(timdream+bugs)
Currently the workaround is working as expected. However, as a 'workaround', someday should be removed in order to preserve the 'structure' that [:timdream] mention in his mail about 'System'.
On the other hand it's true that we have a lot of bb+ to focus in, so I would consider it as a polish issue (bb-).
Flags: needinfo?(fbsc)
Sounds good. As much as I would like to remove workaround it does not make sense to block the release for that.
Flags: needinfo?(timdream+bugs)
Flags: needinfo?(francisco.jordano)
Borjasalguero and Vivien,

I am no code sanity police; bb- is fine.

Just remember that if there are more unforeseen (blocking+!) bug found in the current workaround FTU launching code, it might be saner to implement this bug for v1. I hope that doesn't happen :)
Summary: [FTU] [Follow-up] Remove workaround about 'hold-home' during FTU → Remove workaround about 'hold-home' during FTU
blocking-basecamp: ? → -
Alive, are you interesting taking this non-v1, master-only, non-trivial bug?

Vivien, I would really like to fix this, even if that means diverge master/v1-train.
Flags: needinfo?(alive)
Flags: needinfo?(21)
Assignee: nobody → alive
Being glad to remove work around,
Diverge between master and v1-train is unavoidable IMO.
Flags: needinfo?(alive)
Blocks: 845251
WIP patch v1:

https://github.com/alivedise/gaia/tree/bugzilla/814840/remove-ftu-workaround
https://github.com/alivedise/gaia/commit/cb9312ddb21badc551040f807cccd4e7d734d8af

* Done
1. Move all ftu variable/function out of WM.
2. Fix related function call in system app.

* Todo
1. Sanity lockscreen unlock function
2. Variable rename under ftu launcher
3. Utilize app window object
4. Unit test
5. Fix ftu close issue
Diverging is fine too imo. It would make our life harder for merging other patches but this does not seems a god strategy to be blocked by old code dirtyness.
Flags: needinfo?(21)
Status update:
Nearly done, but I am blocked by quick screen timeout when FTU running :/
This bug is blocking a tef+ bug 845251 so nominating it for tef+ too
blocking-b2g: --- → tef?
When verifying this bug please check STR from bug 832895 too.
Keywords: qawanted
Looks like we need the patch, please mark as needinfo :nhirata so that I'm notified to check it as soon as the patch lands.
Flags: needinfo?(nhirata.bugzilla)
(In reply to Maria Angeles Oteo:oteo from comment #12)
> This bug is blocking a tef+ bug 845251 so nominating it for tef+ too

Bug 845251 is not tef+, do you link the wrong bug #?
Bug 832895 is terrible because it's power consuming forever and the screen won't go off anyway before FTU ends.
Flags: needinfo?(oteo)
Patch v1.

No unit test at this point.
I prefer to create one in another bug because it seems somewhat complex.
Attachment #721054 - Flags: review?(timdream)
Comment on attachment 721054 [details]
https://github.com/mozilla-b2g/gaia/pull/8435

r=me, more manual tests is needed (discussed offline) to ensure this will not break anything.
Attachment #721054 - Flags: review?(timdream) → review+
(In reply to Alive Kuo [:alive] (Life is a struggle.) from comment #16)
> (In reply to Maria Angeles Oteo:oteo from comment #12)
> > This bug is blocking a tef+ bug 845251 so nominating it for tef+ too
> 
> Bug 845251 is not tef+, do you link the wrong bug #?

You are right, Alive, I made a mistake with the number ;)

The tef+ bug is Bug 837558 that according to its comments it seems that bug 814840 turns out to be its root cause.
Flags: needinfo?(oteo)
Great. Sounds like we need more testing!

(In reply to Maria Angeles Oteo:oteo from comment #19)
> (In reply to Alive Kuo [:alive] (Life is a struggle.) from comment #16)
> > (In reply to Maria Angeles Oteo:oteo from comment #12)
> > > This bug is blocking a tef+ bug 845251 so nominating it for tef+ too
> > 
> > Bug 845251 is not tef+, do you link the wrong bug #?
> 
> You are right, Alive, I made a mistake with the number ;)
> 
> The tef+ bug is Bug 837558 that according to its comments it seems that bug
> 814840 turns out to be its root cause.
sorry. the code is not landing yet....
Flags: needinfo?(nhirata.bugzilla)
1. make NOFTU=1 doesn't work for me even on master.
2. QA would help on gaia uitest stuff about Window Manager change. (Talked with atsai)

I will keep testing and then merge to master tomorrow if nothing bad happens.
QA Contact: nhirata.bugzilla
https://github.com/mozilla-b2g/gaia/commit/b4db4ccac38811b1c1992cba7781049cc90ce1f6  master
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to Alive Kuo [:alive] (Life is a struggle.) from comment #23)
> https://github.com/mozilla-b2g/gaia/commit/
> b4db4ccac38811b1c1992cba7781049cc90ce1f6  master

Thanks for the effort!!
Flags: needinfo?(nhirata.bugzilla)
Blocks: 837558
Blocks: 849200
This patch create a regression in FTU when PIN CODE it's required. Please fix the following bug https://bugzilla.mozilla.org/show_bug.cgi?id=849200
While testing this bug, I found : https://bugzilla.mozilla.org/show_bug.cgi?id=849443
Also to note that the PUK has the same type of issue as bug 849200, I see the PUK appear in 2 different screens in the FTU if I don't bother to complete it.

thanks Borja for pointing out the SIM lock/Pin Code.
Flags: needinfo?(nhirata.bugzilla)
Marking as verified:

"mozilla-central" revision="cb432984d5ce"
"integration/gaia-central" revision="df56729d46b7"

gecko.git revision="0be8fda9a032227e46bb011fda43aa589c632033"
gaia.git revision="c7ac21c48e3718a178340a56b17a0508ae903aba"
 
2013-03-08-03-10-58_MC
Status: RESOLVED → VERIFIED
blocking-b2g: tef? → ---
We need to decide this is tef? because the patch to bug 837558(tef+) depends on this bug.
blocking-b2g: --- → tef?
This patch is pretty big so I'm definitely very concerned with a possible regression  if we pick this up for v1.0.1.  

Does comment 26 of bug 837558 not point to a way that we can pull in that fix without this bug landing on v1.0.1?
(In reply to Michael Vines [:m1] [:evilmachines] from comment #29)
> This patch is pretty big so I'm definitely very concerned with a possible
> regression  if we pick this up for v1.0.1.  
> 
> Does comment 26 of bug 837558 not point to a way that we can pull in that
> fix without this bug landing on v1.0.1?

OK I see. I am fine ;) I just want to know why tef? is set to --- without any reason.
blocking-b2g: tef? → ---
Yes, I'm sorry for neglecting to add a comment and thanks for challenging.  It's so easy to clear a flag by accident!
Great! and NVM, thanks for clarification.
Can we consider uplifting this patch to v1-train? This patch will fix Bug 864620 issue. I have set leo? flag to this bug. 
Should the status of this bug also need to change? (It's verified fixed now.)
blocking-b2g: --- → leo?
(In reply to leo.bugzilla.gaia from comment #34)
> Can we consider uplifting this patch to v1-train? This patch will fix Bug
> 864620 issue. I have set leo? flag to this bug. 
> Should the status of this bug also need to change? (It's verified fixed now.)

No, uplifting process/leo? triage/bug status are going independently.
(In reply to leo.bugzilla.gaia from comment #34)
> Can we consider uplifting this patch to v1-train? This patch will fix Bug
> 864620 issue. I have set leo? flag to this bug. 
> Should the status of this bug also need to change? (It's verified fixed now.)

We can ask the engineers to nominate for approval with a risk/reward evaluation, but this is not a blocker.
blocking-b2g: leo? → -
tracking-b2g18: --- → +
Blocks: 870708
No longer blocks: 870708
FYI, the original FTU launching code was implemented in bug 803101.
We have seem many, many regression on v1-train for the last few months and it's worthy to re-think whether or not we should take this patch to v1-train.

Alive told me yesterday even now, the likelihood of uplifting this bug and break things is less than more potential regressions on v1-train. We would also have to backout many of the hacks put in place in v1-train that supposedly to replace this fix.

So triage, should we do it? I believe Alive have more to add on risk/reward evaluation.
blocking-b2g: - → leo?
Flags: needinfo?(alive)
I believe the risk(bugs regressed after uplifting) is less then keep regression grow up, because during these three months all FTU relevant bugs on v1-train doesn't occur to master.

All bugs I could find now:
Bug 854842 - blank screen when use customize power on animation on v1-train & v1.0.1 
Bug 837558 - [FTU] Sim Card Toolkits may interfere with FTU process
Bug 875646 - [Buri][FTU] boot-up cannot complete to enter home screen 
Bug 831697 - White screen after receive a call in FTE
(The following two are regressions caused by a workaround patch of bug 831697)
Bug 877062 - [v1-train][FTU] You can kill FTU by long press the home key again
Bug 876340 - [Sound] Vibrate option is not available during/after FTU
Flags: needinfo?(alive)
(In reply to Alive Kuo [:alive] from comment #39)

Let's have this uplifted to v1-train!
Thanks
blocking-b2g: leo? → leo+
I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 b4db4ccac38811b1c1992cba7781049cc90ce1f6
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(alive)
v1.1.0hd: 84b027e1deff1015a69611b3731b190731cdb391
v1.1.0hd: 950f81700da59c06e6cd205e415fc9457936f567
status-b2g-v1.1hd: --- → fixed
Hi Alive, 

I have seen the following commit in v1-train.

   * Revert "Bug 854842 - Check FTU running before visibility change"

   This reverts commit ee55b12d8bf8fcfc49f271e7f4a1086f47eeea96.
   Because we are uplifting bug 814840 to v1-train.


And somehow this revert causes the following issue.

1. Launch CutTheRope game and observe it plays sounds.
2. Press Power button to screen-off.
3. The sound still plays.        <== issue.
4. Press power button and unlock the lockscreen. Now the sounds stops. <== weird.

And If I put the following reverted code back in, the issue goes away.

the https://github.com/mozilla-b2g/gaia/pull/9038/files 

Should we put the reverted code back in?
Flags: needinfo?(alive)
It's behavior from bug 828283, not by reverting 854842.
(I remember we had discussed 828283 somewhere)
Flags: needinfo?(alive)
(In reply to Alive Kuo [:alive] from comment #45)
> It's behavior from bug 828283, not by reverting 854842.
> (I remember we had discussed 828283 somewhere)

How about #4 below? The sound stops when lockscreen unlocks. Should I create a new bug for this?


1. Launch CutTheRope game and observe it plays sounds.
2. Press Power button to screen-off.
3. The sound still plays.        <== issue.
4. Press power button and unlock the lockscreen. Now the sounds stops. <== weird.

Thanks
Flags: needinfo?(alive)
Hi,
I am unlucky to find cut-the-rope app so I just add an audio tag to test app,
and I cannot reproduce what you mentioned. The sounds still plays after lockscreen unlocks.

Any further info?
Flags: needinfo?(alive)
(In reply to Alive Kuo [:alive] from comment #47)

Let me upload the cuttherope application.zip and a repro video. Let me know if this helps.

I tried imdb.com with browser app and it doesn't have this issue. (A movie trailer video still plays/makes sounds on lockscreen after screen-off/on)


By the way, my college already created a new bug for #3. You can close that one if the #3 behavior is expected. And let me know if #4 is an issue, I will create a new bug.
(In reply to hanj.kim25 from comment #48)
> Created attachment 773737 [details]
> cuttherope_application.zip
> 
> (In reply to Alive Kuo [:alive] from comment #47)
> 
> Let me upload the cuttherope application.zip and a repro video. Let me know
> if this helps.
> 
> I tried imdb.com with browser app and it doesn't have this issue. (A movie
> trailer video still plays/makes sounds on lockscreen after screen-off/on)
> 
> 
> By the way, my college already created a new bug for #3. You can close that
> one if the #3 behavior is expected. And let me know if #4 is an issue, I
> will create a new bug.

I haven't tried your attachment but please do that because this thread is spamming some people. thanks.
(In reply to Alive Kuo [:alive] from comment #47)

Here's the repro video. Please listen to the sound of the video.

00:19 Sound stops at lockscreen.
00:27 Sound resume once page is moved. (as per https://bugzilla.mozilla.org/show_bug.cgi?id=891715#c2)

The new bug that my college created is 891715.
Attachment #773738 - Flags: feedback?(alive)
Comment on attachment 773738 [details]
cuttherope_sound_issue_repro_video

I think this is a cut-the-rope specific issue since I am using the commit: 46ed9a47c75c6f8e9be8114ac7a819cc1efdc280 which is just before this bug uplifted to v1-train, it still happens(unlock causing the sound stops). I still don't know why but please don't discuss that on this bug.
Attachment #773738 - Flags: feedback?(alive) → feedback+
v1.1.0hd: 2ec2c37dddf39e9ac54a56f8ae5c400d28fd4310
v1.1.0hd: 22d7b30bc88a6407e5b5cf0b1ea2ccaa394c6320
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: