(inactive after 6/18) Alive Kuo [:alive] Assignee
	Description • 11 years ago

Bug 880588 doesn't fix all the problems of inline activity's visibility.

1. Activity opener should be setVisible(false) when activity is opened(activityopen event).
However we may have OOM problem about this, see https://bugzilla.mozilla.org/show_bug.cgi?id=880588#c7
Briefly, if an app is having inline activity, we should lessen the OOM_ADJ of the app, to make it not easy to be killed by OOM killer even when it's in background.

2. If multiple inline activities are opened, the middleware activities visibility is not set to hidden.
This is because bug 846850 and gecko fix bug 853759.
Briefly, requested activity shouldn't close itself when visibility is hidden. Gecko should do that for them since app itself won't be notified when it's crashed/killed (I think).

	Justin Lebar (not reading bugmail)
	Comment 1 • 11 years ago

This is a stupid question, but can you give me an example of how to trigger an inline activity?

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 2 • 11 years ago

example (1): Long press on homescreen -> wallpaper
example (2): Type any number in dialer -> click + -> open new contact activity -> click + on photo -> open another pick camera photo activity

	Leo
	Comment 3 • 11 years ago

Steps to repro an issue:

1. Start playing a video.
2. Receive image/audio by Bluetooth from other device.
3. Open the received file.
4. Observe the video is still playing, when the visibility has been moved away.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 4 • 11 years ago

Fix (1) would fix comment 3.
However I still concern the memory management malfunction.

	Justin Lebar (not reading bugmail)
	Comment 5 • 11 years ago

(In reply to Alive Kuo [:alive] from comment #4)
> Fix (1) would fix comment 3.
> However I still concern the memory management malfunction.

I would r- a patch which did only (1) because of precisely the problem you described.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 6 • 11 years ago

(In reply to Justin Lebar [:jlebar] from comment #5)
> (In reply to Alive Kuo [:alive] from comment #4)
> > Fix (1) would fix comment 3.
> > However I still concern the memory management malfunction.
> 
> I would r- a patch which did only (1) because of precisely the problem you
> described.

No idea how to move forward now.
I think we still need Fabrice or someone to feedback?

BTW before that, how do you think give gaia more API about memory management?
Is it OK for gaia system to touch (read and write) OOM-ADJ directly?

Until now I only know background frame has hight OOM ADJ than foreground, but I don't the accurate meaning of OOM ADJ value 0-6.

	Justin Lebar (not reading bugmail)
	Comment 7 • 11 years ago

> Is it OK for gaia system to touch (read and write) OOM-ADJ directly?

No; we should handle this in Gecko, as we currently do.  Gaia just needs to give Gecko enough information so it can make the right decision.

We had a similar problem with the lock screen.  In this situation, the problem was that we were setVisible(false)'ing the topmost frame when you turned the phone off.  But that causes us to discard all images in that frame.

There's no reason to discard the images in the frame that's going to be shown when you unlock the screen, but the only information Gecko got was "this frame is not visible", and so it couldn't make a good decision.

I use the past tense above, but that bug may not be fixed; I forget what we ended up doing.

Anyway, this is similar.

I think the only question here is, do we have Gaia explicitly tell Gecko "this frame opened an inline activity", or do we rely on Gecko to keep track of this?  Does Gecko have enough information to know when a frame opened an inline activity and that activity is in the foreground?

Either way, once Gecko knows that a frame opened an inline activity, it can set the oom_adj appropriately.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 8 • 11 years ago

(In reply to Justin Lebar [:jlebar] from comment #7)
> > Is it OK for gaia system to touch (read and write) OOM-ADJ directly?
> 
> I think the only question here is, do we have Gaia explicitly tell Gecko
> "this frame opened an inline activity", or do we rely on Gecko to keep track
> of this?  Does Gecko have enough information to know when a frame opened an
> inline activity and that activity is in the foreground?

NOPE. This is also my question. We even don't know who is the activity opener but just give a guess that the current app is the one who opens it because we know that inline activity is triggered by user action.

 543   openAppForSystemMessage: function shell_openAppForSystemMessage(msg) {
 544     let origin = Services.io.newURI(msg.manifest, null, null).prePath;
 545     this.sendChromeEvent({
 546       type: 'open-app',
 547       url: msg.uri,
 548       manifestURL: msg.manifest,
 549       isActivity: (msg.type == 'activity'),
 550       target: msg.target,
 551       expectingSystemMessage: true
 552     });
 553   },

The event detail is insufficient, indeed, in the case of comment because the real one to invoke activity is in the system app(bluetooth transfer), not the current active app.

I also desire and acquire the information if we want to manage activity correctly, not only on setting visibility.

	[:fabrice] Fabrice Desré
	Comment 9 • 11 years ago

Gecko gives a hint to gaia to let it know that the activity must be inlined. Then gaia does whatever it needs and don't send anything back to gecko explicitly, but we have a hook point where gecko get a handle to the window that has been opened for the activity. Here we should be able to know if it's actually an inline activity or not.

	[:fabrice] Fabrice Desré
	Comment 10 • 11 years ago

So, while true my last comment was not really relevant to what we need here. Discussing with Alive on irc, we want to:
- change the oom_adj value for the caller of an activity.
- restore its previous value when the activity is done.

And gaia could use the manifest url and page url of the caller.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 11 • 11 years ago

(In reply to Fabrice Desré [:fabrice] from comment #10)
> So, while true my last comment was not really relevant to what we need here.
> Discussing with Alive on irc, we want to:
> - change the oom_adj value for the caller of an activity.
> - restore its previous value when the activity is done.
> 
> And gaia could use the manifest url and page url of the caller.

YES.

Gaia part work would be: correctly set the visibility of activity caller app, per information given by gecko, when activity is started/terminated.

Gecko part: besides the stuff :fabrice mentioned, we need to consider an activity "callee" may also open another activity so its OOM score also need to be adjusted. I think you guys know more detail than me.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 12 • 11 years ago

Let's track the gecko part fix in bug 892371.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 13 • 11 years ago

WIP without gecko tell me who is the inline activity caller.

https://github.com/alivedise/gaia/commit/1679ed57c7ebae7b295635b58a784f21f99a3e42

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 14 • 11 years ago

:fabrice, please help to give some advise..
I think not every system message would have a caller,
but I don't want to add 'type=activity' check in system-message-internal..don't know if there's better way without affecting other system message format.

	[:fabrice] Fabrice Desré
	Comment 15 • 11 years ago

Comment on attachment 775593 [details] [diff] [review]
Proposed gecko fix about appending activity caller info in system message

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

I don't like very much the idea of adding activity-specific information there in system messages, but I don't have yet a really better idea.
One minor enhancement would be instead of adding "manifestURL" and "pageURL" to sendMessage() to just pass an opaque "extra" object. This would carry less activity-specfic semantics.

If you can make this change, I would not object until I come with a something nicer....

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 16 • 11 years ago

I know it's dirty ;) Pollute the middleware |SystemMessageInternal| does make me unhappy, too.
But I think we need to carry additional info without changing SystemMessageInternal, and this kind of requirement would getting more in the future.

  _openAppPage: function _openAppPage(aPage, aMessage) {
    // We don't need to send the full object to observers.
    let page = { uri: aPage.uri,
                 manifest: aPage.manifest,
                 type: aPage.type,
                 target: aMessage.target };
    debug("Asking to open " + JSON.stringify(page));
    Services.obs.notifyObservers(this, "system-messages-open-app", JSON.stringify(page));
  },

Why do we reorganize the message into page object here instead of sending page + message directly?
Does your 'extra' live in the page object as well?

	[:fabrice] Fabrice Desré
	Comment 17 • 11 years ago

(In reply to Alive Kuo [:alive] from comment #16)

> Why do we reorganize the message into page object here instead of sending
> page + message directly?

As the comment says, we don't want to send the full page and message objects, so we build a new one.

> Does your 'extra' live in the page object as well?

yes.

But it may be even cleaner to add the "extra" data as an optional parameter to https://mxr.mozilla.org/mozilla-central/source/dom/messages/interfaces/nsISystemMessagesInternal.idl#22 and broadcastMessage(). At least that would make this clear for all callers.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 18 • 11 years ago

(In reply to Fabrice Desré [:fabrice] from comment #17)
> 
> But it may be even cleaner to add the "extra" data as an optional parameter
> to
> https://mxr.mozilla.org/mozilla-central/source/dom/messages/interfaces/
> nsISystemMessagesInternal.idl#22 and broadcastMessage(). At least that would
> make this clear for all callers.

This looks clear to go.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 19 • 11 years ago

Though I do think this change is not very trivial..
The optional parameter is only meaningful to js caller but not c++ caller.
There c++ caller needs to assign an undefined value to the function.

Note: bluetooth has two functions in different files using broadcastMessage, they seem to be able to be merged into one.

	Gina Yeh [:gyeh] [:ginayeh]
	Comment 20 • 11 years ago

Comment on attachment 776982 [details] [diff] [review]
Patch v2: Add optional parameter for sendMessage/broadcastMessage

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

Looks great for Bluetooth part. I think we should also use BroadcastSystemMessage() in BluetoothService.cpp (as we did in other files). Let me open a follow-up for this. Thanks for your effort.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 21 • 11 years ago

See last review request +
* Change UUID

	[:fabrice] Fabrice Desré
	Comment 22 • 11 years ago

Comment on attachment 777033 [details] [diff] [review]
Patch v2: Add optional parameter for sendMessage/broadcastMessage

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

r=me with nits addressed and pending Gene review.

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +17,5 @@
>     * @param type        The type of the message to be sent.
>     * @param message     The message payload.
>     * @param pageURI     The URI of the page that will be opened.
>     * @param manifestURI The webapp's manifest URI.
> +   * @param extra       Extra information

nit: "Extra opaque information that will be passed around in the observer notification to open the page".

@@ +26,5 @@
>     * Allow any internal user to broadcast a message of a given type.
>     * The application that registers the message will be launched.
>     * @param type        The type of the message to be sent.
>     * @param message     The message payload.
> +   * @param extra       Extra information

Nit: same here.

	Gene Lian (I already quit Mozilla)
	Comment 23 • 11 years ago

Comment on attachment 777033 [details] [diff] [review]
Patch v2: Add optional parameter for sendMessage/broadcastMessage

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

Giving review- because one line is obviously wrong...

::: dom/messages/SystemMessageInternal.js
@@ +172,1 @@
>                                           aManifestURI.spec);

This is wrong! |aExtra| should be the last argument...

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 24 • 11 years ago

Thank you guys for review.

Gene, please help to check this again. I'd test manually nothing bad happens.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 25 • 11 years ago

In system app, extra info got when it's activity request:

E/GeckoConsole(  108): Content JS LOG at app://system.gaiamobile.org/js/window_manager.js:1325 in WindowManager</<: [alive] {"type":"open-app","url":"app://wallpaper.gaiamobile.org/pick.html","manifestURL":"app://wallpaper.gaiamobile.org/manifest.webapp","isActivity":true,"target":{"filters":{"type":["image/*","image/jpeg"],"width":320,"height":480},"disposition":"inline","returnValue":true,"href":"app://wallpaper.gaiamobile.org/pick.html"},"expectingSystemMessage":true,"extra":{"manifestURL":"app://homescreen.gaiamobile.org/manifest.webapp","pageURL":"app://homescreen.gaiamobile.org/index.html#root"}}

No extra info if it's not activity:

E/GeckoConsole(  108): Content JS LOG at app://system.gaiamobile.org/js/window_manager.js:1325 in WindowManager</<: [alive] {"type":"open-app","url":"app://clock.gaiamobile.org/index.html","manifestURL":"app://clock.gaiamobile.org/manifest.webapp","isActivity":false,"expectingSystemMessage":true}

	Gene Lian (I already quit Mozilla)
	Comment 26 • 11 years ago

Comment on attachment 777660 [details] [diff] [review]
Patch v3: fix the argument issue

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

Looks good to me. Btw, you need to make b2g18-specific patch since we had some differences in the SystemMessageInternal.js and shell.js.

::: dom/messages/SystemMessageInternal.js
@@ +161,5 @@
>      // clean it up from the pending message queue when apps receive it.
>      let messageID = gUUIDGenerator.generateUUID().toString();
>  
>      debug("Sending " + aType + " " + JSON.stringify(aMessage) +
> +      " for " + aPageURI.spec + " @ " + aManifestURI.spec + '; extra=' + JSON.stringify(aExtra));

nit: s/=/:/.

We prefer that within this file although I don't have strong opinion.

@@ +182,5 @@
>      if (page) {
>        // Queue this message in the corresponding pages.
>        this._queueMessage(page, aMessage, messageID);
>  
>          if (result === MSG_SENT_FAILURE_APP_NOT_RUNNING) {

Since you're here, could you please align this if-block? Thanks!

@@ +195,5 @@
>      // so that we can know the correct pages registered to be broadcasted.
>      if (!this._webappsRegistryReady) {
>        this._bufferedSysMsgs.push({ how: "broadcast",
>                                     type: aType,
> +                                   extra: aExtra,

Put this property after |msg| to align the order of arguments.

@@ +204,5 @@
>      // Give this message an ID so that we can identify the message and
>      // clean it up from the pending message queue when apps receive it.
>      let messageID = gUUIDGenerator.generateUUID().toString();
>  
>      debug("Broadcasting " + aType + " " + JSON.stringify(aMessage));

Add debug message for |aExtra| just like what you did for sendMessage(...).

@@ +580,5 @@
>                                   { type: aType,
>                                     msg: aMessage,
>                                     manifest: aManifestURI,
>                                     uri: aPageURI,
> +                                   msgID: aMessageID});

Don't need this change. Please recover it.

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +17,5 @@
>     * @param type        The type of the message to be sent.
>     * @param message     The message payload.
>     * @param pageURI     The URI of the page that will be opened.
>     * @param manifestURI The webapp's manifest URI.
> +   * @param extra       Extra opaque information that will be passed around in the observer

nit: add an period at the end of this sentence.

@@ +27,5 @@
>     * Allow any internal user to broadcast a message of a given type.
>     * The application that registers the message will be launched.
>     * @param type        The type of the message to be sent.
>     * @param message     The message payload.
> +   * @param extra       Extra opaque information that will be passed around in the observer

Ditto.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 27 • 11 years ago

I am told by wayne they may want to leo? this one so let's wait..
I will make another b2g18 patch if needed.

The gaia part fix depends on 892371, but this gecko patch is safe to go without other change.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 28 • 11 years ago

(Update bug commit info)

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 30 • 11 years ago

Uh..I think we also need to know 'which' activity is done in 'activity-done' event.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 31 • 11 years ago

Nominate leo? since the dependent 892371 isn't an easy one to go.

	Wayne Chang [:wchang]
	Comment 32 • 11 years ago

Removing leo+ and milestone as no longer blocker by leo.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 33 • 11 years ago

checkin needed for mc

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 11 years ago

https://hg.mozilla.org/projects/birch/rev/4ee3d4c78ae0

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/4ee3d4c78ae0