801520 - Gaia part of bug 798445: Remove |activity.onsuccess = reopenApp| callbacks.

Status: VERIFIED FIXED

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

When both bug 798445 and bug 799039 landz, apps, doesn't need to, and should not attempt to, bring themselves to the foreground since all switching is done entirely by the System app.

The patch of this bug should do what bug 799039 comment 0 item 4 says.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Will work on this.

Comment 2

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

Tim I move you from QA contact to Assigned. I believe that was your first intent.

Comment 3

[David Flanagan [:djf]]

Reassigning this to myself since I'm doing it anyway as part of my work in 796729

Comment 4

[David Flanagan [:djf]]

I've attached a patch to bug 796729 that gets rid of most (all?) of the reopen calls.  See https://github.com/mozilla-b2g/gaia/pull/6231

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

David, would you update the patch please?

Comment 6

[David Flanagan [:djf]]

Created attachment 687163 [details]
alive's pull request

I'm reassigning this bug to Alive and attaching a link to his pull request, which he did for bug 816878.  Also granting review+ for that pull request, with the conditions below:

See the comments in the pull request.  There are a few console.log() statements to remove, and one test_app that needs the same fix applied.

The code looks good to me, but please add a comment describing how you tested the affected activities before you land this patch.

Comment 8

[David Flanagan [:djf]]

Also, when you update your pull request, remember to change the commit message to reference this bug instead of 816878.

Thanks!

Comment 9

[David Flanagan [:djf]]

Hmm. I thought I reassigned this. Re-assining to Alive now.

Comment 10

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

(In reply to David Flanagan [:djf] from comment #6)
> The code looks good to me, but please add a comment describing how you
> tested the affected activities before you land this patch.

The test is easy to state.
Now any activity frame doesn't block home button, therefore we could try these 
1. Open dialer app, type some number, click add contact button
2. Open sms app, click + button, click contact icon to invoke contact app
3. Open e-mail app, click new mail, click + to pick a contact
4. Open UItest=>InlineActivity=>Click Test
5. Open UItest=>Contact test=>Click pick/new a contact

When the activity frame is up, click home button.
Homescreen should then shows and the app requests the activity should not be reopened.

Comment 11

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

Github is down now. I will land it after github resumes.

Thank you David Flanagan, I thought these workaround should be removed long time ago but I got some trouble in another bug and finally recognized that it's because reopenApp isn't removed :( So I filed that patch, I don't want others to waste their time on investigating who is launching the app...app itself.

Comment 12

[David Flanagan [:djf]]

Alive: just to be clear... Did you actually run the tests you describe above? In addition to testing that the Home button works as expected, please also verify that the activities still work correctly if you don't push the Home button!

Thanks for fixing this. I didn't realize that there were any possible side effects to the reopen code, or I would have tried harder to get my own patch landed. Sorry!

Comment 13

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

(In reply to David Flanagan [:djf] from comment #12)
> Alive: just to be clear... Did you actually run the tests you describe
> above? In addition to testing that the Home button works as expected, please
> also verify that the activities still work correctly if you don't push the
> Home button!
> 

Oh, I forgot to describe this part!

This afternoon, in order to make sure all things are well done,
I first time used the facebook import utility to increase my contacts. Because I didn't have any.
I think all these apps are related to contact picking/creating so this is the fastest way to do real test.
And I use the email to pick one of my ex-colleague who exposes his email on facebook. But sign, this doesn't work because another bug about facebook import. So I created one contact with an email to test. I created with my name and my mozilla mail account. It works because the gmail did send the mail to my mozilla mailbox.

I did the same thing to sms app to send a message to my own phone, and I did get the message.

The remaining apps are not related to data passing so I don't have something to do, just click around to see if something bad happened.

BTW, I only have 100+ contacts in my facebook account, but after I imported, the contact app got very very bad performance....

> Thanks for fixing this. I didn't realize that there were any possible side
> effects to the reopen code, or I would have tried harder to get my own patch
> landed. Sorry!

It's not your fault ;)

Comment 14

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

Additionally, the modification in system app is to remove outdated codes. Confirmed with timdream.
I also tested the ones in ui-test app just now.

Github seems recovered but I cannot press the merge button even it's green..sigh, I bet there's still something wrong in github so I had better not to merge before it goes stable.

Comment 15

[Chris Lee [:clee]]

What's the user impact here?

Comment 16

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

(In reply to Chris Lee [:clee] from comment #15)
> What's the user impact here?

Wrong application window popping up unsuspectingly when you press the home button.

Comment 17

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

https://github.com/alivedise/gaia/commit/756181bd1bfbce1b86e56a96cbb7b0ec1ce66a4b

Comment 18

[Deepa Subramanian]

Verified on Unagi device Build ID: 20130114073222 v 1.0.0-Prerelease