774216 - B2G can't launch app since it's seen as an XP_UNIX by WebappOSUtils.jsm

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Etienne Segonzac (:etienne)]

So we don't go through the |Services.obs.notifyObservers(this, "webapps-launch", JSON.stringify(aData))| path.

Comment 1

[Etienne Segonzac (:etienne)]

Attachment: Workarround

Comment 2

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

#ifndef ANDROID will not work on the desktop build.

Comment 3

[Etienne Segonzac (:etienne)]

(In reply to Vivien Nicolas (:vingtetun) from comment #2)
> #ifndef ANDROID will not work on the desktop build.

Yep, it doesn't.

Happy to update the patch if there is a better way.

Comment 4

[Andreas Gal :gal]

How about #ifdef MOZ_WIDGET_GONK?

Comment 5

[Andreas Gal :gal]

r=me with that hack, until we have a better fix

Comment 6

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal

This is a more stable fix for B2G desktop *and* on the phone.
Thanks to :vingtetun for the pointer.

Comment 7

[Andreas Gal :gal]

Comment on attachment 642583 [details] [diff] [review]
Patch proposal

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

Nice. r=me but please add a comment explaining the event flow.

Comment 8

[Ed Lee :Mardak]

Doesn't this patch revert the change for Firefox?

Comment 9

[Ed Lee :Mardak]

Comment on attachment 642583 [details] [diff] [review]
Patch proposal

>+++ b/webapprt/WebappsHandler.jsm
>+Cu.import("resource://gre/modules/WebappOSUtils.jsm");
..
>       case "webapps-launch":
>+        WebappOSUtils.launch(data);

This change would also need to made for Firefox I believe?

http://mxr.mozilla.org/mozilla-central/source/browser/modules/webappsUI.jsm#36

Comment 10

[Etienne Segonzac (:etienne)]

(In reply to Edward Lee :Mardak from comment #8)
> Doesn't this patch revert the change for Firefox?

Either https://mxr.mozilla.org/mozilla-central/source/browser/modules/webappsUI.jsm#36 was dead code or this patch will "fix" firefox too.

Comment 11

[Ed Lee :Mardak]

It seems like the patch that landed with bug 772600 was lacking in various ways including removing the dead code from Firefox. I'm not sure why there are 5 bugs open for the same feature, but originally it was filed as bug 745924 then split into bug 763740, bug 763786, and bug 763789; and then was implemented in bug 772600 -- all of those were targeting Firefox 16.

I'll double check with drivers to figure out (hopefully within an hour) for how we can make sure b2g isn't broken while fixing the intended platforms and runtimes.

Comment 12

[Etienne Segonzac (:etienne)]

Attachment: Patch v2

Comment 13

[Etienne Segonzac (:etienne)]

(In reply to Edward Lee :Mardak from comment #11)
> I'll double check with drivers to figure out (hopefully within an hour) for
> how we can make sure b2g isn't broken while fixing the intended platforms
> and runtimes.

Thanks for tracking this!

In the meantime here is a patch proposal ensuring that we do not have side effects outside of the b2g runtime.

Comment 14

[Ed Lee :Mardak]

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

Switching r?felipe for browser code, although I don't think it'll get r+d with keeping the pinned tab launch behavior.

>+++ b/browser/modules/webappsUI.jsm
>       case "webapps-launch":
>+        WebappOSUtils.launch(data);
This line looks fine although I can't r+ for browser/.

>         DOMApplicationRegistry.getManifestFor(data.origin, (function(aManifest) {
>           if (!aManifest)
>             return;
>           let manifest = new DOMApplicationManifest(aManifest, data.origin);
>           this.openURL(manifest.fullLaunchPath(), data.origin);
>         }).bind(this));
We actually want to get rid of this code because we don't want a native launch /and/ a pinned tab launch.

>+++ b/toolkit/webapps/WebappOSUtils.jsm
>@@ -1,15 +1,21 @@
>+/* Only used for webapprt and browser runtimes
>+   webapps-launch events are still systematically sent. */
Not sure if this comment is necessary or exactly what you intend with "systematically sent."

>+const Cu = Components.utils;
>+Cu.import("resource://gre/modules/Services.jsm");
Thanks for fixing this.

Comment 15

[Ed Lee :Mardak]

felipe: To help with the review, this patch switches back to always notifying, and we have 3 consumers for the notification:

1) b2g: works as it did before
2) firefox: switch to using os utils launching (and remove pinned tab launch)
3) webapprt: keep the os util launching

Comment 16

[Etienne Segonzac (:etienne)]

(In reply to Edward Lee :Mardak from comment #14)
> Comment on attachment 642604 [details] [diff] [review]
> Patch v2
> 
> Switching r?felipe for browser code, although I don't think it'll get r+d
> with keeping the pinned tab launch behavior.
> 
> >+++ b/browser/modules/webappsUI.jsm
> >       case "webapps-launch":
> >+        WebappOSUtils.launch(data);
> This line looks fine although I can't r+ for browser/.
> 
> >         DOMApplicationRegistry.getManifestFor(data.origin, (function(aManifest) {
> >           if (!aManifest)
> >             return;
> >           let manifest = new DOMApplicationManifest(aManifest, data.origin);
> >           this.openURL(manifest.fullLaunchPath(), data.origin);
> >         }).bind(this));
> We actually want to get rid of this code because we don't want a native
> launch /and/ a pinned tab launch.

Make sense.
I'll remove the dead code call, but I'll let somebody more aware of the situation remove the openURL function.

> 
> >+++ b/toolkit/webapps/WebappOSUtils.jsm
> >@@ -1,15 +1,21 @@
> >+/* Only used for webapprt and browser runtimes
> >+   webapps-launch events are still systematically sent. */
> Not sure if this comment is necessary or exactly what you intend with
> "systematically sent."

Don't want to let somebody think that it's a good place to change something canonical to all webapps-launch.
Happy to reformulate/remove.

> 
> >+const Cu = Components.utils;
> >+Cu.import("resource://gre/modules/Services.jsm");
> Thanks for fixing this.

np.

Comment 17

[Etienne Segonzac (:etienne)]

Attachment: Patch v3

Comment 18

[Tony Chung [:tchung]]

this is a blocker for b2g daily builds.  I cannot launch any apps on the phone. 

we need this fixed in m-c asap.

Comment 19

[Ed Lee :Mardak]

Attachment: for checkin

Updated patch to be against mozilla-central (no ActivitiesService.jsm) and updated commit message.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/mozilla-central/rev/c918ff2f0994

Comment 21

[Andreas Gal :gal]

Thanks for the quick fix.

Comment 22

[Tony Chung [:tchung]]

Verified fix on 7-17-2012 daily build

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Moving to Core to request aurora-approval

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 642654 [details] [diff] [review]
for checkin

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 772600

User impact if declined: None if the 16 branch won't be used by B2G or Android. (Bug does not happen on desktop, but it'd still be good to have the code in 16 and trunk to match).
 If used by Android then launching apps from inside the about:apps dashboard (not the homescreen) won't work.  

Testing completed (on m-c, etc.): landed on m-c at the beginning of the week and verified to have fixed the problem

Risk to taking this patch (and alternatives if risky): very little
String or UUID changes made by this patch: none

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 642654 [details] [diff] [review]
for checkin

Low risk and we definitely don't want to break app launching on Android, approving.

Comment 27

[Jason Smith [:jsmith]]

We still need this uplift in order to get apps working on FF 16. Can someone get this uplifted?

Comment 28

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/27235727dfbf

Comment 29

[Jason Smith [:jsmith]]

Aaron - Can you verify this is fixed on Aurora when a new build comes out for Aurora on Firefox for Android?