896945 - If SMS app is open when clicking 'send sms' icon in contacts app, the last thread open is displayed in sms app instead of 'compose new msg' screen.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Roy Collings]

User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.71 Safari/537.36

Steps to reproduce:

Using b2g build: unagi-ICS.eng.v1-train.Rel0.4.Sprint12.B-165.Gecko-1fc50aa.Gaia-06d56e0

1. Open sms app and start a thread with contact A.
2. Leave sms app open, and open contacts app.
3. For contact B, click the 'send sms' icon.



Actual results:

The currently open thread for contact A is displayed.


Expected results:

A 'new sms' screen should appear. Once Send is pressed, the thread for contact B should be displayed.

Comment 1

[(Inactive after June) George Duan [:gduan] [:喬智]]

it looks like bug 880115 ?

Comment 2

[(Inactive after June) George Duan [:gduan] [:喬智]]

I found patch of bug 880115 was already merged into master and v1-train.  And I tried to flash the gecko with past version (~ build b2g18 20130710 ) with this gaia code, everything works normal.

So, I tracing the code and also found if I sms app is opened, then below code(sms) doesn't work anymore.
window.navigator.mozSetMessageHandler('activity', this.global.bind(this));

which means that below code(contact) only bring us to sms app but doesn't not trigger the callback inside mozSetMessageHandler.

var activity = new MozActivity({
  name: 'new',
  data: {
          type: 'websms/sms',
          number: number
        }
});

My first thought is that might be a common issue over all apps. So, I also test email app with below steps.

precondition: setup email config, setup email for one contact
1. open contact app, and click one's email
2. email app should open with new email and set contact's mail as receiver
3. return to email list(click top-left button)
4. go to contact app and repeat step 1

expect: just like step 2
actual: switch to email app, but remain on email list.

Comment 3

[(Inactive after June) George Duan [:gduan] [:喬智]]

The reason might be from system app or gecko. I will keep investigating.

Comment 4

[Gene Lian (I already quit Mozilla)]

Sadly, this sounds like a regression of Bug 892708... The 'open-app' only switches the app to that page for activity but the system message handler isn't triggered.

Antonio, could you please take a look? This is also critically blocking.

Comment 5

[Antonio Manuel Amaya Calvo (:amac)]

Hmm... I tested exactly this use case (in fact I used the gallery/camera, contacts/gallery and contacts/sms pairings to test it) with Bug 892708 and I would have swear it did work for me. It doesn't work now, not even the basic case.

The problem is that we're queuing the messages now waiting for the app to be reloaded. And the queued messages only are requested/delivered when the handler is called/set. 

I have a very small patch for window_manager.js that fixes this (basically it forces a reload of the page even if the loaded page is the one that's going to be handling the message, that way the app behaves the same way always). It works on 1.1, I'm compiling m-c/master now to try it. Will do a PR once I've tested it on m-c/master.

Comment 6

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: bug-896945.patch

This fixes the problem by forcing activities to load the handler page (as opposed to just show it) always. As a secondary benefit, this also ensures the page is on an always known state when processing the activity so it should make activities development easier.

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

Stealing the bug, George. Sorry about that :)

Comment 8

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

Why not figure out "the reason the system message isn't pass to the handler"? If this works well before bug 892708.

Also there's reload function in mozbrowser API if you want that.

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

I tried the reload method first and it didn't work, that's why I did the change of URIs. And the reason why it isn't working is because the message is queued and queued messages are delivered when setmessagehandler is called. Since the page is already loaded the method was already called so queued messages aren't delivered.

Comment 10

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

Comment on attachment 780294 [details] [diff] [review]
bug-896945.patch

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

I tend to think this is a design failure of system message(only deliver once). But...anyway.

Comment 11

[Gene Lian (I already quit Mozilla)]

Hi George,

Could you please verify the patch at the same time? I heard you were saying we had different kinds of scenarios happening with the same issue, not only the contact-message scenario. Thanks!

Comment 12

[(Inactive after June) George Duan [:gduan] [:喬智]]

Hi Gene,

Verified. This patch can fix bug 897398 and bug 897180 .

Comment 13

[[:fabrice] Fabrice Desré]

I agree with Alive that this looks weird to fix that in gaia. Reloading the app will have side effects that can be pretty annoying. Hold on before landing anything.

Comment 14

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

Comment on attachment 780294 [details] [diff] [review]
bug-896945.patch

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

Per comment 13 cancel the review.
Could you guys discuss with :fabrice for any better solution? Thanks.

Comment 15

[bhavana bajaj [:bajaj]]

Blocking on this given its a regression from 892708.We obviously prefer a low risk path forward to avoid any further regression's.Hence if backing out something may fix this, please consider that path forward.

Comment 16

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fabrice Desré [:fabrice] from comment #13)
> I agree with Alive that this looks weird to fix that in gaia. Reloading the
> app will have side effects that can be pretty annoying. Hold on before
> landing anything.

Ok... this is fixed on Gaia because the activities flow is divided between Gecko and Gaia. Gecko delegates loading the app on Gaia. The way it used to work for activities and system messages before the system message patch that broke this was (and this is divided on Gecko and Gaia):

* If the handler page is not running/loaded:
  1 The message is queue (Gecko)
  2 Gecko->Gaia (open-app)
  3 Gaia opens a new frame loading the handler URL. If the handler URL is == 
    the app launching URL, then this is equivalent to launching the app. Otherwise,
    it isn't.
  4 Once the app is loaded, it calls setMessageHandler. The message is delivered at 
    this point
* If the handler page is running/loaded:
  A The message is delivered (Gecko).
  B The handler will start running. 
  C Gecko->Gaia (open-app)
  D Gaia: verifies if the URL loaded at the top frame of the app is == the handling 
    URL. If it is, it doesn't do anything. If it isn't, it *reloads* the handling 
    page on that frame (breaking the handler if it was still running, and discarding
    any work it had done in any case)
  E Gaia: If it's an activity, it brings the page to the top. 

What the original patch fixed was NOT sendd the message on C because it was breaking apps that had the handler loaded on a non root frame (for example, cost control). Note that loading the handler on a non root frame *is* a valid use case. That broke activities because activities needed step E to come forward.

At that point I proposed two possible changes. One was a pure Gecko patch, that would make activities work as they worked originally (so it would leave them broken if the handler was not index.html). Another was a Gecko + Gaia patch (instead of sending an open-app always, send a load-app with the meaning of just bring the app forward, don't change what's loaded). And finally implemented a third option, which was:

For Activities only:
 A' The message is queued (NOT delivered)
 B' Nothing
 C' Gecko->Gaia (open-app)
 D' Gaia: do the same as D
 E' Gaia: do the same as E

Now, the problem with this is that queued messages are delivered *only* when setMessageHandler is invoked. So if the app was already loaded and set, it would come forward but it wouldn't do any activity related changes because it would not receive the app. 

So the change that I wrote for this patch was just: return the app to a known good state before start processing the activity.

As for the 'side effects' with this patch the application will behave *exactly* the same way when an activity is received and the app is NOT running, and when an activity is received and the app IS running. Before this patch, your activity code had to take into account that the app may be already running, already half initialized, or whatever. After this patch, you don't have to take that into account, when an activity is received you'll be *always* on the same known state. If anything, I believe this makes actually *easier* to develop activities. And it should not break anything... cause with this the activities work the same way on both the app/running and app not running cases.

Comment 18

[Gene Lian (I already quit Mozilla)]

The main difference between non-activity and activity system message is: the activity needs a way to bring the page to the top no matter if the message is handled instantly or after the app is waken up. The rest should be all the same. I'd suggest:

A. If the handler page is not running/loaded:
   1. (Gecko) Queue up the message.
   2. (Gecko) Fire 'open-app' no matter which kind of system message it is.
   3. (Gaia)  Receives 'open-app' and load the app. Later, the setMessageHandler will retrieve all the pending messages and handle them for sure.

B. If the handler page is running/loaded:
   1. (Gecko) Directly deliver the message via IPC and let the handler handle it.
   2. (Gecko) Don't fire 'open-app' but fire a new 'bring-up-page' for activity only.
   3. (Gaia)  Receives 'bring-up-page' and brings the page to the top. Note that it doesn't mean to load it again. Instead, just bring it up, to avoid breaking the running message handler at B-1.

P.S. We can use configurator to do B-2 for activity only in Gecko.

This needs both Gecko and Gaia changes. Does it sound practical? I'm not sure if the step B-3 is feasible.

Comment 19

[Antonio Manuel Amaya Calvo (:amac)]

Yeah, it is pretty much feasible. In fact, that was the second option for the last patch. If you prefer it that way, I'll have a gecko+Gaia patch in a couple of hours.

Comment 20

[Gene Lian (I already quit Mozilla)]

Sound great! Let's do it.

Honestly, I don't like the last solution of queuing message for activity only, even if we're sure it can be delivered and handled at that moment. The window_manager.js should only be in charge of opening app or bring up page, such the page switching things, and shouldn't know any black-box behaviours done in the Gecko.

Comment 21

[Antonio Manuel Amaya Calvo (:amac)]

BTW, I still think that yesterday's patch make activity handlers easier to develop, since you only have to cater to the base case (app/page just loaded) without having to take into account the what-ifs (what if the activity to send a new SMS is invoked while a new SMS was being written... And so on). 

I'll get on with your proposed solution anyway :)

Comment 22

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Gecko patch: Sends the system message always and pass two new flags to the frontend to customize the behavior

This implements Gene's suggestion. Instead of sending a new type of message, it adds a couple of flags to the message sent to Gaia:
* showApp: The app should be put on foreground, on top on whatever process Gaia does
* showOnly: The only process Gaia should take is to put the app on foreground

There are two separate flages because showOnly implies showApp, but the reverse isn't true (an activity for a non running app will have showApp=true, showOnly=false, for example).

Comment 23

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Gaia patch: Customize open-app behavior based on the showApp and showOnly flags

This is the corresponding Gaia patch

Comment 24

[Antonio Manuel Amaya Calvo (:amac)]

The Gecko + Gaia patch implements the solution proposed by Gene in comment #20. This solution has some advantages compared to yesterday one... and some disadvantages.

As an advantage:

* It leaves activities on a closer state to how they were originally. 
* If the app is running, it's not reloaded. That allows specialized behavior, 
  like the one in email where activities 'stack' (you can start composing an 
  email to user A, switch to contacts and select user B, press his email address
  and you'll be taken back to email composing an email to user B. If you hit the
  back button (or send the mail), you'll be brought back to the original mail).

As a disadvantage:
A It leaves activities on a closer state to how they were originally. This means that inline activities cannot be set on an URI that's already loaded on the page, or they won't run correctly when invoked if the app is running already. On the bright side, all of Gaia is already working around this problem (by using specific URIs for the inline handlers).
B The change is bigger.
C It makes activity handler harder to write.

Honestly... I think that the original solution (leaving Gecko as it's now and just applying yesterday patch) leaves system message and activities on a more correct state (as in they'll behave more like someone that hasn't used them yet would expect). Sadly, I think that yesterday patch would break some behaviors that depend specifically on being able to distinguish between activity invocation from a cold start and activity invocation from a running app (like the email one).

So, all things considered, I think today's patch is safer since it leaves activities on a state closer to the original one.

Comment 25

[Antonio Manuel Amaya Calvo (:amac)]

Tests I've done with the patch applied:

* Send SMS from the contacts app, with the SMS app stopped, and running (and on different screens)
* Send email from the contacts app, with the email app stopped and running, and on different screens (mail list, mail composing, inside another inline activity)
* Add/view contact from the email app
* Camera->Gallery->Camera->Gallery
* Share a photo from the Gallery app using bluetooth

Some of those are inline, some are not. By the way, I think I know now why my original tests worked... I think the phone was OOMing so when I thought I was testing with a running app I was actually testing with a freshly loaded app... happened several times during these tests also (I think I have too many mails :) ).

What doesn't work, though, is:
Open the mail app, select a mail from one of  your existing contacts, press on the contact, select view contact. The View contacts activity opens inline correctly. But the buttons on that view doesn't seem to do anything (the dial one just kills the window, while the send sms and send mail one don't do anything).

I don't think that's a system message bug, but a contacts bug, since there's no system message being generated when clicking on anything on that screen.

Comment 26

[Gene Lian (I already quit Mozilla)]

Comment on attachment 780950 [details] [diff] [review]
Gecko patch: Sends the system message always and pass two new flags to the frontend to customize the behavior

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

::: b2g/chrome/content/shell.js
@@ +555,5 @@
>        url: msg.uri,
>        manifestURL: msg.manifest,
>        isActivity: (msg.type == 'activity'),
> +      showOnly: msg.showOnly,
> +      showApp: msg.showApp,

Do we really need |showApp|? Please see below.

::: dom/messages/SystemMessageInternal.js
@@ +243,5 @@
>  
>          // Queue this message in the corresponding pages.
>          this._queueMessage(aPage, aMessage, messageID);
>  
> +        // Open app pages to handle their pending messages.

Don't need this comment since _openAppPage() isn't doing what the comment says now.

@@ +518,5 @@
>        aPage.pendingMessages.splice(0, 1);
>      }
>    },
>  
> +  _openAppPage: function _openAppPage(aPage, aMessage, aMessageStatus) {

s/aMessageStatus/aMsgSentStatus/

@@ +520,5 @@
>    },
>  
> +  _openAppPage: function _openAppPage(aPage, aMessage, aMessageStatus) {
> +    // We should send the open-app message if the system message was
> +    // not sent, or if it was sent but we should show the app anyway

nit: add a period at the end of each complete sentence.

@@ +522,5 @@
> +  _openAppPage: function _openAppPage(aPage, aMessage, aMessageStatus) {
> +    // We should send the open-app message if the system message was
> +    // not sent, or if it was sent but we should show the app anyway
> +
> +    // This means the app must be brought to the foreground

nit: period.

@@ +523,5 @@
> +    // We should send the open-app message if the system message was
> +    // not sent, or if it was sent but we should show the app anyway
> +
> +    // This means the app must be brought to the foreground
> +    let showApp = this._getMessageConfigurator(aPage.type).mustShowRunningApp;

I'd prefer using another equivalent condition and doing early return here:

-----
// This means the app must be brought to the foreground.
let showApp = ...;

// We should send the open-app message if the system message was
// not sent, or if it was sent but we should show the app anyway.
if ((aMsgSentStatus === MSG_SENT_SUCCESS) && !showApp) {
  return;
}

-----
Because:

1. This is what you wrote in the comments.
2. Double negative is hard to read (!== MSG_SENT_FAILURE_APP_NOT_RUNNING).
3. You're still using (aMessageStatus === MSG_SENT_SUCCESS) below.

@@ +525,5 @@
> +
> +    // This means the app must be brought to the foreground
> +    let showApp = this._getMessageConfigurator(aPage.type).mustShowRunningApp;
> +
> +    // And this means the app must *only* be brought to the foreground

nit: period.

@@ +526,5 @@
> +    // This means the app must be brought to the foreground
> +    let showApp = this._getMessageConfigurator(aPage.type).mustShowRunningApp;
> +
> +    // And this means the app must *only* be brought to the foreground
> +    let showOnly = (aMessageStatus === MSG_SENT_SUCCESS) && showApp;

I'd prefer s/showOnly/showAppOnly:

let showAppOnly = (aMessageStatus === MSG_SENT_SUCCESS) && showApp;

@@ +528,5 @@
> +
> +    // And this means the app must *only* be brought to the foreground
> +    let showOnly = (aMessageStatus === MSG_SENT_SUCCESS) && showApp;
> +
> +    if ((aMessageStatus === MSG_SENT_FAILURE_APP_NOT_RUNNING) || showApp ) {

Don't need this line because of the above early return.

@@ +536,5 @@
> +                   manifest: aPage.manifest,
> +                   type: aPage.type,
> +                   target: aMessage.target,
> +                   showOnly: showOnly,
> +                   showApp: showApp

I don't understand why we need two flags here. From the Gecko's point of view, we only need to tell Gaia whether we need to show the app on the top. And we only need to do this extra notification for activity when the app is running and message is sent.

So I think we only need to pass |showAppOnly| (note I'd prefer s/showOnly/showAppOnly as mentioned above).

Please correct me if I'm wrong.

@@ +537,5 @@
> +                   type: aPage.type,
> +                   target: aMessage.target,
> +                   showOnly: showOnly,
> +                   showApp: showApp
> +                 };

nit: move "};" to the above line and leave a space before it.

@@ +610,5 @@
> +                                   msg: aMessage,
> +                                   manifest: aManifestURI,
> +                                   uri: aPageURI,
> +                                   msgID: aMessageID });
> +                                  }

nit: align this "}".

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +56,5 @@
>  interface nsISystemMessagesConfigurator: nsISupports
>  {
>    /*
> +   * Will be true if this type of system messages assumes/requires
> +   * that the app will be brought to the front always

nit: add a period.

Comment 27

[Gene Lian (I already quit Mozilla)]

Comment on attachment 780954 [details] [diff] [review]
Gaia patch: Customize open-app behavior based on the showApp and showOnly flags

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

::: apps/system/js/window_manager.js
@@ +1336,5 @@
>          }
>  
> +        // If the message specifies we only have to show the app,
> +        // then we don't have to do anything here
> +        if (!e.detail.showOnly) {

Please see the comments in the Gecko. I'd prefer:

if (!e.detail.showAppOnly) {
  ...
}

@@ +1373,5 @@
>          }
>  
> +        // We will only bring apps to the foreground when the message
> +        // specifically requests it
> +        if (!e.detail.showApp)

Can we keep it as it was?

  if (!e.detail.isActivity)

From the Gecko's point of view, we only need to pass one |showAppOnly| to specify this 'open-app' is for the purpose of showing app and nothing else (i.e., shouldn't load page to handle messages as the above if-block does).

IMO, the |showApp| is something very redundant to |isActivity|.

Comment 28

[Gene Lian (I already quit Mozilla)]

Btw, please rebase the gecko code on top of bug 887650 which was just landed few hours ago.

Comment 29

[Antonio Manuel Amaya Calvo (:amac)]

Thanks for the review, Gene. Concerning the showApp/showOnly, I need either two booleans or a enumerated because there are three different cases:

showApp showOnly
  false   false   ---> Normal system message, app wasn't running
  false   true    ---> Invalid case, can't happen
  true    false   ---> Activity, app wasn't running
  true    true    ---> Activity, app was running.

I can do that with an enumerated value, or a string value, but if I use a enumerated then I have to replicate the definition, and string comparison is more expensive. Booleans on the other hand are universal :) 

As for the e.detail.isActivity, *currently* showApp <==> isActivity (because there's only one Configurator in Gecko that returns true, which is the activity one). But since we have showApp, the correct value to use here is that, if we leave showApp, we're actually using MessageConfigurator both on Gecko and Gaia (otherwise, adding a the messageconfigurator was just a very convoluted way of not putting aType === 'activity' in Gecko :) ). That is, if we added a new message type no Gecko that had a configurator that returned true, leaving showApp on that check would mean not having to change that part of Gaia at least. Leaving isActivity would mean having to change it. 

That's also why I didn't use isActivity instead of showApp on the table above, although currently they're one and the same. Because 

showApp = this._getMessageConfigurator(aPage.type).mustShowRunningApp;

and not

showApp = aPage.type === 'activity';

Comment 30

[Gene Lian (I already quit Mozilla)]

OK. I see. Sounds good to me. You can keep |showApp|. Can we change |showOnly| to a better name? It sound ambiguous if the followers don't know its relation to |showApp|. Maybe, |showAppOnly|?

Comment 31

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Gene Lian [:gene] from comment #30)
> OK. I see. Sounds good to me. You can keep |showApp|. Can we change
> |showOnly| to a better name? It sound ambiguous if the followers don't know
> its relation to |showApp|. Maybe, |showAppOnly|?

I changed it tentatively to |onlyShowApp|. Is that ok with you?

Comment 32

[Gene Lian (I already quit Mozilla)]

Looks good to me. Thanks!

Comment 33

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

I haven't read all the comments but in "inline" activity case I don't care the app is opened or not.
We just create a new page for the "inline" activity usage.
For the "window" activity we switch to the app or open the app if it's not opened.

Comment 34

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Alive Kuo [:alive][Paris work week 7/22-26] from comment #33)
> I haven't read all the comments but in "inline" activity case I don't care
> the app is opened or not.
> We just create a new page for the "inline" activity usage.
> For the "window" activity we switch to the app or open the app if it's not
> opened.

I know. If you check the patch for window_manager.js, it doesn't touch the first two conditions (so if the message goes to the system app window_manager.js doesn't do anything, and if it's an activity it does exactly the same it was doing, regardless what showApp and onlyShowApp say.

Comment 35

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

Keep the |isActivity| in gecko please.
BTW have you guys done the discussion? I still see showApp, showOnly in gecko patch(confusing me also..)

Comment 36

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Alive Kuo [:alive][Paris work week 7/22-26] from comment #35)
> Keep the |isActivity| in gecko please.
> BTW have you guys done the discussion? I still see showApp, showOnly in
> gecko patch(confusing me also..)

Hey.

Yeah, I have a new patch, waiting on Fabrice's review before uploading it (in case he's started reviewing it). I can upload it now without obsoleting the other one if you prefer)

And I can leave the isActivity if you prefer it, but I still think it's an error, now that Gecko has a way to decide if any message type should bring the app up to leave Gaia using the wrong flag to decide if it should show or not an app (and that's what that if (!isActivity) return actually means ).

The way Gecko is with this patch, we can modify if an message should bring the app up or not without modifying the System Messages core working. But if we leave |isActivity| on that check instead of |showApp| then we would have to modify Gaia even if that's the only change.

Comment 37

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Gecko patch, includes Gene's comments

Not setting this up for review yet since I'm waiting on Fabrice's review also

Comment 38

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Gaia patch: Customize the open-app treatment based on the showApp and onlyShowApp flags, includes Gene's comments

Same as the Gecko patch, not setting this for review until Fabrice's review is done

Comment 39

[Antonio Manuel Amaya Calvo (:amac)]

Ok, I uploaded the reviewed version without obsoleting the others :) That's what I had talked with Gene, I think.

Comment 40

[Gene Lian (I already quit Mozilla)]

Comment on attachment 781643 [details] [diff] [review]
Gecko patch, includes Gene's comments

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

Looks good to me. Directly review+. Thank you! :)

::: b2g/chrome/content/shell.js
@@ +554,5 @@
>        type: 'open-app',
>        url: msg.uri,
>        manifestURL: msg.manifest,
>        isActivity: (msg.type == 'activity'),
> +          onlyShowApp: msg.onlyShowApp,

nit: please fix the alignment.

::: dom/messages/SystemMessageInternal.js
@@ +533,5 @@
> +    if ((aMsgSentStatus === MSG_SENT_SUCCESS) && !showApp) {
> +      return;
> +    }
> +
> +    // And this means the app must *only* be brought to the foreground.

*only* is for what? Sounds not clear. Please add more words:

// This flag means the app must *only* be brought to the foreground
// and don't need to load the app to handle messages.

Comment 41

[Antonio Manuel Amaya Calvo (:amac)]

Thanks for the review, Gene. I made those last changes, waiting on Fabrice to upload a new version.

Comment 42

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

(In reply to Antonio Manuel Amaya Calvo from comment #36)
> (In reply to Alive Kuo [:alive][Paris work week 7/22-26] from comment #35)
> > Keep the |isActivity| in gecko please.
> > BTW have you guys done the discussion? I still see showApp, showOnly in
> > gecko patch(confusing me also..)
> 
> Hey.
> 
> Yeah, I have a new patch, waiting on Fabrice's review before uploading it
> (in case he's started reviewing it). I can upload it now without obsoleting
> the other one if you prefer)
> 
> And I can leave the isActivity if you prefer it, but I still think it's an
> error, now that Gecko has a way to decide if any message type should bring
> the app up to leave Gaia using the wrong flag to decide if it should show or
> not an app (and that's what that if (!isActivity) return actually means ).
> 
> The way Gecko is with this patch, we can modify if an message should bring
> the app up or not without modifying the System Messages core working. But if
> we leave |isActivity| on that check instead of |showApp| then we would have
> to modify Gaia even if that's the only change.

It's UI requirement, activity frame has a different transition than normal app. It's up to backend to replace isActivity to any other things but system needs to know which is activity.

Comment 44

[Antonio Manuel Amaya Calvo (:amac)]

Ok, I think we might have a misunderstanding here. I haven't removed isActivity from the message, and in fact it's used on the launch inline activity part still. What I've changed is that to check if the app should be brought to the top or not I check showApp instead of isActivity. The actual 'bringing-to-top' function is unchanged (and so if it used isActivity it'll be used still).

Comment 45

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

I don't want to be picky to the variable name.
But 'showApp' and 'showOnly' occurs at the same really confuses.
I think your |showOnly| means |doNotReload|.

For the system message use case, I think every app frame now brings itself to the foreground by calling app.launch() "if they want to" when they get system message. (I complain this in webAPI maillist but it's another thing.) So in the long run why do we need this? Sorry I'm really confusing.

In my imagination:
```pseudo code
// system-message-handler
if (theTargetIsNotRunning) {
  if (this-is-an-activity)
    startInlineActivity();
  else
    appendFrame();
} else if (!doNotReload) {
  replace the URL
} else {
  // the app is running but we don't want to replace the URL so do nothing
}
```
Gene, we shall talk about this after I back to Taipei...

Comment 46

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

Updated pseudo code of comment 45
https://gist.github.com/alivedise/6089691

Comment 47

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Alive Kuo [:alive][Paris work week 7/22-26] from comment #45)
> I don't want to be picky to the variable name.
> But 'showApp' and 'showOnly' occurs at the same really confuses.
> I think your |showOnly| means |doNotReload|.

On the new patch it's called |onlyShowApp| and it means... that the only thing the open-app handler should do is show the app. So in short, yeah, it means |doNotReload|.

 
> For the system message use case, I think every app frame now brings itself
> to the foreground by calling app.launch() "if they want to" when they get
> system message. (I complain this in webAPI maillist but it's another thing.)
> So in the long run why do we need this? Sorry I'm really confusing.

Because Fabrice didn't want (and rightly so) to tie that behavior into Gecko to just activities, so on Gecko if a message type should auto-bring to top the app or not is extensible (and not necessarily limited to activities). And also giving the app the *option* to bring itself to top (which is what app.launch() does) and making the system *force* it to come to top (which is what showApp means) are different things.

> 
> In my imagination:
> ```pseudo code
> // system-message-handler
> if (theTargetIsNotRunning) {
>   if (this-is-an-activity)
>     startInlineActivity();
>   else
>     appendFrame();
> } else if (!doNotReload) {
>   replace the URL
> } else {
>   // the app is running but we don't want to replace the URL so do nothing
> }
> ```

What's actually done is:

if (system is the target)
  return;

if (isAnInlineActivity)
  launchInlineActivity(); // This happens *even* if the target is running

// Equivalent to if (!doNotReloead) without the double negation
if (!onlyShowApp) {
  if (alreadyRunning) {
    launchNewApp();
  } else {
    reloadTheMainFrameOfTheApp();
  }
}

if (showApp) {
  bringTheAppToTopWithTheAdequateTransition()
}

That's what's implemented on the patch. The only changes to the current behavior was adding the 
if (!onlyShowApp) {

verification, and changing the condition to bring up the app (it was 

if (isActivity) {

and is now

if (showApp) {

so as to not restrict the showing just to activities (since it isn't restricted on the Gecko System Message code). As I said before, the actual 
  bringTheAppToTopWithTheAdequateTransition() 
part hasn't changed at all, so the activity transition is intact :).

Hope that clears this up!

Comment 48

[Antonio Manuel Amaya Calvo (:amac)]

s/  if (alreadyRunning) {/  if (!alreadyRunning) {

:)

Comment 49

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

Comment on attachment 780954 [details] [diff] [review]
Gaia patch: Customize open-app behavior based on the showApp and showOnly flags

I'm giving this r+ since the (system message X activity) bundling chaos seems un-resolvable for now.

Comment 50

[Antonio Manuel Amaya Calvo (:amac)]

Thanks for the review, Alive!

Comment 51

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Gecko patch, including all of Gene's comments

This is the patch that was r+ by Gene, with his latest nits included. It's also rebased with the latest System Messages changes as requested.

Comment 52

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 781645 [details] [diff] [review]
Gaia patch: Customize the open-app treatment based on the showApp and onlyShowApp flags, includes Gene's comments

This is the patch alive r+'ed, with Gene's comments (just a change of the variable name from showOnly to onlyShowApp, and rebased).

Comment 53

[Antonio Manuel Amaya Calvo (:amac)]

Thanks for the review, Fabrice!
I'll upload the patches for v1-train and b2g18 in a few minutes.

Comment 54

[Antonio Manuel Amaya Calvo (:amac)]

As commented on the PR [1], the Gecko patch should land before the Gaia one. 

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

Comment 55

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Gaia patch, for v1-train

This is the same patch, rebased to apply cleanly on v1-train

Comment 56

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Gecko patch, B2G18 version

This is the same patch r+'ed by Gene and Fabrice, rebased for B2G18.

Comment 57

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/projects/birch/rev/6f6a4ea042c0

Comment 58

[Francisco Jordano [:arcturus] [:francisco]]

Gaia part landed:

https://github.com/mozilla-b2g/gaia/commit/217a9482bdba8d51f9c5d0ca65b7e85266769eda

Comment 59

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6f6a4ea042c0

Comment 60

[Ryan VanderMeulen [:RyanVM]]

Leaving status-b2g18 set to affected for the Gaia uplift.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/150c3f4ad24b

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/150c3f4ad24b

Comment 63

[[:fabrice] Fabrice Desré]

And... it looks like that broke the view activity for the browser app (and probably others), the browser is not coming to the foreground anymore.

Comment 64

[Antonio Manuel Amaya Calvo (:amac)]

I think that bug is previous to this landing, isn't it? I mean, that should be broken the same way that all the others were, and for the same reason, before this patch.

Comment 65

[Antonio Manuel Amaya Calvo (:amac)]

I'm making a fresh build now, but that bug was from the 23rd... and it was broken then :)

Comment 66

[Kevin Grandon :kgrandon]

Looking at this, it appears that we always early return if (!e.detail.showApp). This means we never get to the setDisplayedApp() method, and never bring the app into the foreground. I still haven't quite wrapped my head around the gecko changes, but it seems that we shouldn't have this second check?

Comment 67

[Antonio Manuel Amaya Calvo (:amac)]

For activities showApp will be true, so it won't return. See https://mxr.mozilla.org/mozilla-central/source/dom/activities/src/ActivityMessageConfigurator.js#26 that's what will get passed to Gaia on showApp

Comment 68

[bov]

Tested on Unagi device. 
Build:unagi-ICS.user.manifest.V1-train.Rel0.4.Sprint12.B-332.Gecko-f89d83b.Gaia-6221737
Build ID:20130730104759
Commercial RIL: AU 172

Steps: 

1. Open sms app and start a thread with contact A.
2. Leave sms app open, and open contacts app.
3. For contact B, click the 'send sms' icon.


Expected results:
A 'new sms' screen should appear. Once Send is pressed, the thread for contact B should be displayed.

Actual results:
It works well, as expected.

Comment 69

[Gene Lian (I already quit Mozilla)]

How can it be verified without the Gaia v1-train merge? Has the merge already been done?

Comment 70

[Antonio Manuel Amaya Calvo (:amac)]

Without the Gaia part it will work well for most app. Gaia patch is needed for handlers to work well when/if the page is loaded on a non root iframe. For all other cases, it'll work without the Gaia part.

Comment 71

[Gene Lian (I already quit Mozilla)]

Yes, I know but we need both Gecko and Gaia patches to fix this bug. Right? Just suspecting the verification at comment #68 if we didn't have the v1-train merge yet.

Hi John, I'd like to ask what's the current merging progress of the Gaia patch (for v1-train)?

Comment 73

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 217a9482bdba8d51f9c5d0ca65b7e85266769eda to:
v1-train: 9516dbf7bd60a36ede2ec120158ddb9befe365b4

Comment 76

[Scoobidiver (away)]

Is the status-b2g-v1.1hd flag right?

Comment 77

[Gene Lian (I already quit Mozilla)]

Hi John, who is in charge of uplifting patch to b2g-v1.1hd on the Gaia side?

Comment 78

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

v1.1.0hd: 9516dbf7bd60a36ede2ec120158ddb9befe365b4

Comment 79

[Jeni]

Verified Fixed on Leo

Build ID: 20130814041202
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/15813d776a69
Gaia: dd3959fa74e356a528daa76ffee14c2c728a4b56
Platform Version: 18.1
RIL Version: 01.01.00.019.190 

A message now displays letting user know they have a unsent message which says 'Your previous message was  not sent.  Do you want to edit the unsent message or discard it and continue?' Also has options to edit or discard