799039 - Switching back to the web activity caller from System when the activity is complete.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

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

Since Gaia system app is currently responsible of switching from the caller app to the handler app, it should also be responsible of switching from the handler app back to caller app. The fix depend on Gecko fix in bug 798445 to fire an 'activity-done' chromeEvent. With that, the require Gaia changes would be

1. when activity too place, save the "origin" of the currently displayed app (which is the caller)
2. in activities.js, handle 'activity-done' chromeEvent, and ask window_manager.js to launch the app originally displayed
3. in window_manager.js, bring the caller app to foreground, and properly close the handling app (by remove the frame, for example for inline activity frame)
4. For all the apps in Gaia that is currently using activity, remove the |reopenApp|, |reopenSelf| function, or the |window.close()| call once window_manager.js in System can handle the app switching.

I have a WIP patch do everything in except the last one. For the last one I have SMS app modification done for testing but I have not going through all apps. 

We can certainly deal the last item in another bug as the 1-3 fix will not break apps that have not implemented 4.

Comment 1

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

Attachment: Patch v1

Besides doing what mentioned in comment 0, this patch clean up window_manager.js a bit to remove some redundant public APIs and update the comments.

Comment 2

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

Attachment: Remove reopenSelf() from SMS app

Comment 3

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

Attachment: Patch v2

An error was spotted after uploading the patch :-/

Comment 4

[Mounir Lamouri (:mounir)]

Note that the System app should also be able to stop an activity if fireSuccess/fireError isn't sent while a return value is expected. IOW, the system app should be able to stop an activity and send an error event on its behalf to handle time outs.

Also, how does Gaia currently handle app switching when an activity is running?

Comment 5

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

(In reply to Mounir Lamouri (:mounir) from comment #4)
> Note that the System app should also be able to stop an activity if
> fireSuccess/fireError isn't sent while a return value is expected. IOW, the
> system app should be able to stop an activity and send an error event on its
> behalf to handle time outs.
> 

Hum, we won't do that currently. You should probably just file another bug for that.

> Also, how does Gaia currently handle app switching when an activity is
> running?

Yes, Gaia will receive an 'open-app' chromeEvent with |isActvitiy| true. App will be launched and switch to accordingly.

Comment 6

[[:fabrice] Fabrice Desré]

Tim, when you say that you sve the "origin", is it really just the origin? This will not work once we'll allow multiple apps per origin, so new code should really not do that.

Comment 7

[David Flanagan [:djf]]

Comment on attachment 669059 [details] [diff] [review]
Patch v2

Review of attachment 669059 [details] [diff] [review]:
-----------------------------------------------------------------

Its been a long time since I've looked at window_manager.js, so I don't understand everything here.  Many of my comments are just questions or things for you to consider.

The only issue that gets you a r- is the bug in wrapper.js.

::: apps/system/js/activities.js
@@ +20,5 @@
>  
> +      case 'activity-done':
> +        this.reopenActivityCaller(detail);
> +        break;
> +    }

isn't there also an activity-error case?  Or did Fabrice thake that out?

::: apps/system/js/window_manager.js
@@ +93,5 @@
>  
>    // Make the specified app the displayed app.
>    // Public function.  Pass null to make the homescreen visible
>    function launch(origin) {
> +    // If the origin is indeed valid we make that app as the displayed app.

Thanks for cleaning up the documentation and tightening up the API here.  Since you're doing that, I'd propose that you change the name of this method.  When launch() was originally written, it would start the app if it was not already running, or at least that was the intent, I think.  And that is what the word 'launch' implies to me.  Since this method now just displays an already running app, consider renaming it to display() or show().

@@ +102,5 @@
>  
> +    // ... if not, we have no choice but to show the homescreen.
> +    // We cannot launch/relaunch a given app based on the "origin" because
> +    // we would need the manifest URL and the specific entry point.
> +    setDisplayedApp(homescreen);

Issue a warning if origin is not null and not homescreen?  That way there will be some record if the methods is used incorrectly.

Also, I'm nervous about the fact that we sometimes use null to mean the homescreen and sometimes use the homescreen variable.
Look at lines 708 and 715 (before your patch is applied).  Two if statements right next to each other.  One is testing for null and one is testing for homescreen.  That seems like a bug.  (And I know it is not directly relevent to your code here, but it might be best for you to fix it here.)

@@ -106,5 @@
>  
> -    // launch() can be called from outside the card switcher
> -    // hiding it if needed
> -    if (CardsView.cardSwitcherIsShown())
> -      CardsView.hideCardSwitcher();

I don't know how the CardsView is shown and hidden. Are you confident that you will never need to show hideCardSwitcher() here?

::: apps/system/js/wrapper.js
@@ +6,5 @@
>      dump(' -+- Launcher -+-: ' + str + '\n');
>    }
>  
>    function currentAppFrame() {
> +    return WindowManager.getAppFrame(WindowManager.getRunningApps());

Does this line actually work?  getRunningApps() sounds like it returns an array, and getAppFrame() sounds like it expects a single app.

I'm guessing you meant getDisplayedApp()

Comment 8

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

(In reply to Fabrice Desré [:fabrice] from comment #6)
> Tim, when you say that you sve the "origin", is it really just the origin?
> This will not work once we'll allow multiple apps per origin, so new code
> should really not do that.

That's not really the origin. Consider "origin" is just a internal identifier window manager uses for app frames until bug 796629 is resolved.

(In reply to David Flanagan [:djf] from comment #7)
> Comment on attachment 669059 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 669059 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Its been a long time since I've looked at window_manager.js, so I don't
> understand everything here.  Many of my comments are just questions or
> things for you to consider.
> 
> The only issue that gets you a r- is the bug in wrapper.js.

Yeah that's a bug. Patch coming. Having no luck on Patch v1 or v2 :'(

> 
> ::: apps/system/js/activities.js
> @@ +20,5 @@
> >  
> > +      case 'activity-done':
> > +        this.reopenActivityCaller(detail);
> > +        break;
> > +    }
> 
> isn't there also an activity-error case?  Or did Fabrice thake that out?

Fabrice was thinking about 'activity-success' and 'activity-error' in his previous patch, now two events are merged into one with a |success| attribute in |detail|.

> 
> ::: apps/system/js/window_manager.js
> @@ +93,5 @@
> >  
> >    // Make the specified app the displayed app.
> >    // Public function.  Pass null to make the homescreen visible
> >    function launch(origin) {
> > +    // If the origin is indeed valid we make that app as the displayed app.
> 
> Thanks for cleaning up the documentation and tightening up the API here. 
> Since you're doing that, I'd propose that you change the name of this
> method.  When launch() was originally written, it would start the app if it
> was not already running, or at least that was the intent, I think.  And that
> is what the word 'launch' implies to me.  Since this method now just
> displays an already running app, consider renaming it to display() or show().

That's not what launch() do, at least right before this patch. There is also a broken start(), being removed in this patch, that actually start running the apps.

> 
> @@ +102,5 @@
> >  
> > +    // ... if not, we have no choice but to show the homescreen.
> > +    // We cannot launch/relaunch a given app based on the "origin" because
> > +    // we would need the manifest URL and the specific entry point.
> > +    setDisplayedApp(homescreen);
> 
> Issue a warning if origin is not null and not homescreen?  That way there
> will be some record if the methods is used incorrectly.
> 

Yeah will do.

> Also, I'm nervous about the fact that we sometimes use null to mean the
> homescreen and sometimes use the homescreen variable.

We would just have to tackle this later when there is an opportunity to do a bigger clean up.

> Look at lines 708 and 715 (before your patch is applied).  Two if statements
> right next to each other.  One is testing for null and one is testing for
> homescreen.  That seems like a bug.  (And I know it is not directly relevent
> to your code here, but it might be best for you to fix it here.)

Line 715 is not a bug, it's just not properly documented.

|null| now refer to the state where phone has just booted and homescreen is not launched. I can remove the redundant logic and update the comments.

> 
> @@ -106,5 @@
> >  
> > -    // launch() can be called from outside the card switcher
> > -    // hiding it if needed
> > -    if (CardsView.cardSwitcherIsShown())
> > -      CardsView.hideCardSwitcher();
> 
> I don't know how the CardsView is shown and hidden. Are you confident that
> you will never need to show hideCardSwitcher() here?

No, coz cards_view.js now handles 'appwillopen' event and close itself (part of clean up)

> 
> ::: apps/system/js/wrapper.js
> @@ +6,5 @@
> >      dump(' -+- Launcher -+-: ' + str + '\n');
> >    }
> >  
> >    function currentAppFrame() {
> > +    return WindowManager.getAppFrame(WindowManager.getRunningApps());
> 
> Does this line actually work?  getRunningApps() sounds like it returns an
> array, and getAppFrame() sounds like it expects a single app.
> 
> I'm guessing you meant getDisplayedApp()

It's a bug :'(

Comment 9

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

Attachment: Patch v3

Comment 10

[Dietrich Ayala (:dietrich)]

Blocking for incomplete user experience - given the target market of first-time smartphone users it's important to close the loop in activities experience.

Comment 11

[David Flanagan [:djf]]

Tim,

I never saw your review request for this in my inbox.  I've only just learned about the "my requests" link in bugzilla...  Sorry this has taken me so long.

The code looks good to me.  Because I delayed the review so long, though, it no longer applies cleanly, so I can't test your patch.

r=me, assuming you have tested it yourself.

Comment 12

[David Flanagan [:djf]]

Comment on attachment 669397 [details] [diff] [review]
Patch v3

Review of attachment 669397 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 13

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

https://github.com/mozilla-b2g/gaia/commit/1efdcdd7341ed65016049582fad88c95867aac6e

This is safe to land before the gecko fix so I merged this first.