913362 - When more than one app can handle an activity, the first displayed app won't launch

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

Steps to reproduce:

1) Long press on the homescreen to change the wallpaper. You'll see 3 choices: wallapper, gallery, camera.  The first choice (wallpaper) does nothing. The other two work.

2) Open the SMS app, start to compose a message, and tap the attach (paperclip) button.  You'll see many choices. They all seem to work, except for the first one (video).

If you select the first app listed, the app doesn't even appear to ever run. It is just never started and the activity request fails somehow.

Comment 1

[David Flanagan [:djf]]

Needinfo on Alive, who was working on activities lately and Fabrice who knows activities well.

Comment 2

[David Flanagan [:djf]]

I forgot to mention that I can reproduce this on tonight's nightly mozilla-central-eng build for unagi

Comment 3

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

I wander this is a gecko problem...because from my point of view, I see the mozContentEvent is dispatched.
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/activities.js#L71

I will check from b2g runtime later.

Comment 4

[David Flanagan [:djf]]

I figured it out.

This is a regression caused by the big third party keyboard patch. That patch changed list_menu.js by adding this one line:

value = parseInt(value)

So list_menu is now passing a number to the callback instead of a string. 

Activities.js used to pass that string to gecko, but now it passes a number.  The number 0 is falsy, but the string "0" is truthy. 

The workaround is to convert back to a string in activities.js, and I'll attach a patch to do that.

Fabrice: if you want you could fix gecko to handle the numeric value 0.

Comment 5

[David Flanagan [:djf]]

Attachment: link to patch on github

This patch fixes a regression caused by the list_menu.js change from https://github.com/mozilla-b2g/gaia/commit/9fb5802df60a9081846d704def01df814ed8fbd4

Comment 6

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

Comment on attachment 800657 [details]
link to patch on github

Thanks for spending time on this.r=me.

Comment 7

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

(In reply to David Flanagan [:djf] from comment #4)
> I figured it out.
> 
> This is a regression caused by the big third party keyboard patch. That
> patch changed list_menu.js by adding this one line:
> 
> value = parseInt(value)
> 
> So list_menu is now passing a number to the callback instead of a string. 
> 
> Activities.js used to pass that string to gecko, but now it passes a number.
> The number 0 is falsy, but the string "0" is truthy. 
> 
> The workaround is to convert back to a string in activities.js, and I'll
> attach a patch to do that.
> 
> Fabrice: if you want you could fix gecko to handle the numeric value 0.

http://mxr.mozilla.org/mozilla-central/source/b2g/components/ActivitiesGlue.js#50

I think http://hg.mozilla.org/mozilla-central/rev/ba5b2970ceb5 has resolved this from gecko?

Comment 8

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

Anyway, IMO ensure data type correctness shall be done from both frontend and backend, so this patch is still a valid fix.

Comment 9

[Jason Smith [:jsmith]]

Isn't this a dupe of bug 912065 even though the patches are different? Are we taking two different patches here to fix this or should we take only one patch?

Comment 10

[David Flanagan [:djf]]

(In reply to Alive Kuo [:alive] from comment #8)
> Anyway, IMO ensure data type correctness shall be done from both frontend
> and backend, so this patch is still a valid fix.

Actually, it looks like gecko is expecting a numeric array index, not a string, so coercing to a string seems wrong now that gecko is handling the 0 case correctly.

Whath should we do here?

- close this bug as a dupe.

- land my patch as it is. (but I don't recommend that)

- use this bug as a followup to the gecko patch and change activities.js:29 so that it passes the number 0 instead of the string '0'.  I suppose we should let the gecko patch propagate out a bit before making that gaia change, though, since this would break single-choice activity invocations for people who don't have up-to-date gekco.

Fabrice: will you decide which option is best here?

Comment 11

[[:fabrice] Fabrice Desré]

Yep, I changed gecko to expect a numeric index, since this is what it actually means, and our code there was fragile...

My preference is to make sure that gaia send the number 0 and not a string.

Comment 12

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

Per comment 11, let's close this since there's gecko patch already.