Last Comment Bug 774216 - B2G can't launch app since it's seen as an XP_UNIX by WebappOSUtils.jsm
: B2G can't launch app since it's seen as an XP_UNIX by WebappOSUtils.jsm
Status: VERIFIED FIXED
[blocking-webrtandroid1+]
: regression
Product: Core
Classification: Components
Component: DOM: Apps (show other bugs)
: Trunk
: All Linux
: -- blocker (vote)
: mozilla17
Assigned To: Etienne Segonzac (:etienne)
: Tony Chung [:tchung]
: [:fabrice] Fabrice Desré
Mentors:
: 777843 (view as bug list)
Depends on:
Blocks: Blocking-FFA-WebRT1+ 772600
  Show dependency treegraph
 
Reported: 2012-07-16 03:26 PDT by Etienne Segonzac (:etienne)
Modified: 2012-08-01 13:56 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
verified


Attachments
Workarround (1.51 KB, patch)
2012-07-16 03:27 PDT, Etienne Segonzac (:etienne)
no flags Details | Diff | Splinter Review
Patch proposal (3.75 KB, patch)
2012-07-16 07:43 PDT, Etienne Segonzac (:etienne)
gal: review+
Details | Diff | Splinter Review
Patch v2 (5.27 KB, patch)
2012-07-16 09:00 PDT, Etienne Segonzac (:etienne)
edilee: feedback+
Details | Diff | Splinter Review
Patch v3 (5.37 KB, patch)
2012-07-16 10:11 PDT, Etienne Segonzac (:etienne)
felipc: review+
Details | Diff | Splinter Review
for checkin (5.66 KB, patch)
2012-07-16 11:23 PDT, Ed Lee :Mardak
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Etienne Segonzac (:etienne) 2012-07-16 03:26:46 PDT
So we don't go through the |Services.obs.notifyObservers(this, "webapps-launch", JSON.stringify(aData))| path.
Comment 1 Etienne Segonzac (:etienne) 2012-07-16 03:27:28 PDT
Created attachment 642522 [details] [diff] [review]
Workarround
Comment 2 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-16 06:27:22 PDT
#ifndef ANDROID will not work on the desktop build.
Comment 3 Etienne Segonzac (:etienne) 2012-07-16 06:37:44 PDT
(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 2012-07-16 06:59:01 PDT
How about #ifdef MOZ_WIDGET_GONK?
Comment 5 Andreas Gal :gal 2012-07-16 07:03:01 PDT
r=me with that hack, until we have a better fix
Comment 6 Etienne Segonzac (:etienne) 2012-07-16 07:43:21 PDT
Created attachment 642583 [details] [diff] [review]
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 2012-07-16 07:47:10 PDT
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 2012-07-16 08:08:37 PDT
Doesn't this patch revert the change for Firefox?
Comment 9 Ed Lee :Mardak 2012-07-16 08:15:46 PDT
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) 2012-07-16 08:22:21 PDT
(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 2012-07-16 08:32:30 PDT
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) 2012-07-16 09:00:11 PDT
Created attachment 642604 [details] [diff] [review]
Patch v2
Comment 13 Etienne Segonzac (:etienne) 2012-07-16 09:01:04 PDT
(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 2012-07-16 09:43:18 PDT
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 2012-07-16 09:56:50 PDT
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) 2012-07-16 10:07:48 PDT
(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) 2012-07-16 10:11:39 PDT
Created attachment 642626 [details] [diff] [review]
Patch v3
Comment 18 Tony Chung [:tchung] 2012-07-16 11:08:19 PDT
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 2012-07-16 11:23:53 PDT
Created attachment 642654 [details] [diff] [review]
for checkin

Updated patch to be against mozilla-central (no ActivitiesService.jsm) and updated commit message.
Comment 20 :Felipe Gomes (needinfo me!) 2012-07-16 11:24:48 PDT
https://hg.mozilla.org/mozilla-central/rev/c918ff2f0994
Comment 21 Andreas Gal :gal 2012-07-16 11:28:30 PDT
Thanks for the quick fix.
Comment 22 Tony Chung [:tchung] 2012-07-17 21:03:52 PDT
Verified fix on 7-17-2012 daily build
Comment 23 :Felipe Gomes (needinfo me!) 2012-07-19 15:29:52 PDT
Moving to Core to request aurora-approval
Comment 24 :Felipe Gomes (needinfo me!) 2012-07-19 15:36:10 PDT
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 2012-07-24 14:50:23 PDT
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 26 Jason Smith [:jsmith] 2012-07-28 09:27:48 PDT
*** Bug 777843 has been marked as a duplicate of this bug. ***
Comment 27 Jason Smith [:jsmith] 2012-07-28 09:29:47 PDT
We still need this uplift in order to get apps working on FF 16. Can someone get this uplifted?
Comment 28 :Felipe Gomes (needinfo me!) 2012-07-31 17:32:04 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/27235727dfbf
Comment 29 Jason Smith [:jsmith] 2012-07-31 17:35:11 PDT
Aaron - Can you verify this is fixed on Aurora when a new build comes out for Aurora on Firefox for Android?

Note You need to log in before you can comment on or make changes to this bug.