We should remove the "webapps-ask-install", "webapps-launch" and "webapps-uninstall" observer notifications. The WebappManager.jsm module will be the interface between the dom/ code and the different applications. In another bug I'll make the app-specific modules inherit from DOMApplicationRegistry and do a refactoring of the install flow (we may also change the module names, we'll see). I've decided to break bug 910473 up because it's a considerable amount of work and we want to unblock some other bugs that are waiting on the refactoring. In particular, in bug 1000315, we'll avoid to add yet another observer notification and just add a "askUninstall" function to WebappManager.

This is almost ready, I've manually tested it on Desktop, B2G Desktop, Android (with synthetic APKs). Automated tests are passing. If you want you can start to look at the patch (the files under b2g/, dom/, toolkit/, webapprt/ probably won't change much) otherwise feel free to wait until I'm finished. I know there's a lot of room for cleanup, my plan is to do it in follow-up bugs.

I'll upload a new patch shortly. Everything works but uninstalling apps on Android when MOZ_ANDROID_SYNTHAPKS is undefined, but it's a pre-existing bug (that I've just filed: bug 1008325).

Comment on attachment 8420309 [details] [diff] [review] Patch Review of attachment 8420309 [details] [diff] [review]: ----------------------------------------------------------------- I only looked at dom/apps/ and b2g/. You'll need various people to review to it would be easier to split this kind of big patch in chunks. Also, push to try. ::: b2g/components/WebappManager.jsm @@ +22,5 @@ > + return; > + } > + > + let installer = this._installers[detail.id]; > + delete this._installers[detail.id]; You will get many events that you are not interested in, so bail out if detail.type is not what you expect before touching _installers. @@ +59,5 @@ > + __exposedProps__: { > + timestamp: "r", > + url: "r", > + manifestURL: "r" > + }, no need for exposedProps, we do the wrapping in _sendCustomEvent ::: dom/apps/src/Webapps.jsm @@ +1211,5 @@ > } > ); > }, > > + launch: function(aManifestURL, aStartPoint, aTimeStamp, aOnSuccess, aOnFailure) { I know why you're doing that but please don't do unrelated changes. We should clean them all at once rather. @@ +1235,5 @@ > + let manifest = new ManifestHelper(aManifest, app.origin); > + > + WebappManager.launch(app, manifest.fullLaunchPath(aStartPoint), > + aTimeStamp); > + That was not possible with observers, but we could have launch() return success/failure. Not sure if that can be sync though. @@ +1245,3 @@ > debug("close"); > > + if (WebappManager.close) { test that close is a function @@ +1485,5 @@ > .then(() => this._saveApps()).then(() => { > // Update the handlers and permissions for this app. > this.updateAppHandlers(aOldManifest, aData, app); > > + if (WebappManager.update) { test that update is a function @@ +1903,5 @@ > let manifest; > if (aNewManifest) { > this.updateAppHandlers(aOldManifest, aNewManifest, aApp); > > + if (WebappManager.update) { ditto ::: toolkit/devtools/apps/tests/unit/test_webappsActor.js @@ +96,1 @@ > why do tests have to be so gross :(

Comment on attachment 8420309 [details] [diff] [review] Patch Review of attachment 8420309 [details] [diff] [review]: ----------------------------------------------------------------- r=myk on the parts I can review: browser/modules/WebappManager.jsm dom/apps/src/AppsUtils.jsm dom/apps/src/Webapps.jsm dom/tests/mochitest/webapps/test_bug_771294.xul toolkit/webapps/WebappOSUtils.jsm webapprt/Startup.jsm webapprt/WebappManager.jsm webapprt/content/mochitest-shared.js webapprt/test/chrome/browser_geolocation-prompt-noperm.js webapprt/test/chrome/browser_geolocation-prompt-perm.js webapprt/test/chrome/head.js These files need Fabrice's review: b2g/chrome/content/shell.js b2g/components/WebappManager.jsm b2g/components/moz.build These need wesj's or mfinkle's review: mobile/android/chrome/content/aboutApps.js mobile/android/chrome/content/browser.js mobile/android/locales/en-US/chrome/browser.properties mobile/android/modules/WebappManager.jsm mobile/android/modules/moz.build And these need review from a peer of the Devtools module <https://wiki.mozilla.org/Modules/All#Devtools>: toolkit/devtools/apps/tests/unit/head_apps.js toolkit/devtools/apps/tests/unit/test_webappsActor.js ::: dom/apps/src/Webapps.jsm @@ +1245,3 @@ > debug("close"); > > + if (WebappManager.close) { Ideally we wouldn't have to check that the method even exists here. But that would require us to design the WebappManager implementations differently, either ensuring that each implementation has a stub for every method (which also means changing them all every time we add a method) or making them inherit from a prototype that has such stubs (like DOMApplicationRegistry itself). Anyway, this is fine for now, but there's more to consider doing here in followups. ::: toolkit/devtools/apps/tests/unit/test_webappsActor.js @@ +94,2 @@ > run_next_test(); > + }; It doesn't particularly matter in this case, but note that you can use a "let statement" <https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.7#let_statement> to reduce the scope of a variable like oldLaunch to the code that accesses it: let (oldLaunch = DOMApplicationRegistry.launch) { DOMApplicationRegistry.launch = (aManifestURL, aStartPoint, aTimeStamp, aOnSuccess, aOnFailure) => { DOMApplicationRegistry.launch = oldLaunch; aOnSuccess(); do_check_eq(aManifestURL, manifestURL); do_check_eq(aStartPoint, startPoint); run_next_test(); }; } Also, I could imagine the tests creating a fake WebappManager to capture these calls, although that's out-of-scope for this bug. ::: webapprt/WebappManager.jsm @@ +46,1 @@ > }, Given that the implementations of these methods are identical in browser/modules/WebappManager and webapprt/WebappManager, and they call the same-named methods in WebappOSUtils, I would make WebappOSUtils the prototype for the WebappManager objects (but perhaps in a followup, if you're planning larger changes to WebappOSUtils anyway).

(In reply to Myk Melez [:myk] [@mykmelez] from comment #5) > Also, I could imagine the tests creating a fake WebappManager to capture > these calls, although that's out-of-scope for this bug. I wanted to, but I couldn't find a simple way to do it.

Comment on attachment 8420309 [details] [diff] [review] Patch Requesting review for mobile/android/ and toolkit/devtools changes.

Comment on attachment 8420309 [details] [diff] [review] Patch >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js > Cu.import("resource://gre/modules/Webapps.jsm"); Just curious if this can be delay loaded now. Or if it could be moved out of the startup method. If not, that's fine. I'm just thinking about ancillary code cleanup. >diff --git a/mobile/android/modules/WebappManager.jsm b/mobile/android/modules/WebappManager.jsm >+ makeBase64Icon: function(aIconURL, aCallbackFunction) { >+ let window = Services.wm.getMostRecentWindow("navigator:browser") >+ .browserDOMWindow.contentWindow; I think this is a web content DOM window. The old code used the browser.xul window. I don't know if there would be any issue with using a web content DOM window. You might want to consider switching back to the XUL browser window just in case. I guess the canvas is never added to the DOM document though. I might be worried for nothing. The sooner we land this the better so we can catch any regressions.

Comment on attachment 8420309 [details] [diff] [review] Patch Review of attachment 8420309 [details] [diff] [review]: ----------------------------------------------------------------- Looks good but it breaks gaia's Firefox nightly support. This addon, is installed within Firefox nightly profile in order to be able to run gaia. The following code ends up breaking with your patch: https://github.com/mozilla-b2g/gaia/blob/master/tools/extensions/browser-helper%40gaiamobile.org/content/shell.js#L33-L53 Please submit a patch against this addon to prevent breaking gaia in Firefox. (I would suggest keeping this observer notification listener, while adding new code to accomodate your modification. It will to ease landing the gaia patch) ::: toolkit/devtools/apps/tests/unit/head_apps.js @@ +92,1 @@ > } Do you think we can remove this overload by using MOZ_B2G here instead of GONK? http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#214 ::: toolkit/devtools/apps/tests/unit/test_webappsActor.js @@ +96,1 @@ > Given that this test is only executed against b2g, may be we can, instead of overloading stuff like that, listen for DOM events via the SystamAppProxy.addEventListener("webapps-launch"/"webapps-close", ...) ?

(In reply to Alexandre Poirot (:ochameau) from comment #9) > Looks good but it breaks gaia's Firefox nightly support. > This addon, is installed within Firefox nightly profile in order to be able > to run gaia. > The following code ends up breaking with your patch: > > https://github.com/mozilla-b2g/gaia/blob/master/tools/extensions/browser- > helper%40gaiamobile.org/content/shell.js#L33-L53 > > Please submit a patch against this addon to prevent breaking gaia in Firefox. > (I would suggest keeping this observer notification listener, while adding > new code to accomodate your modification. It will to ease landing the gaia > patch) Is this addon used in b2g desktop? If not, how can I test it? I guess I'll keep this observer notification and remove it in a followup together with the update to this addon. > > ::: toolkit/devtools/apps/tests/unit/head_apps.js > @@ +92,1 @@ > > } > > Do you think we can remove this overload by using MOZ_B2G here instead of > GONK? > http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#214 We need to mock |getAppInfo| because the application package isn't in the directory where it is supposed to be (on Desktop, it shouldn't be in |app.basePath + "/" + app.id|, but the webapps actor skips the "native" installation that would move the zip file in its proper directory). > > ::: toolkit/devtools/apps/tests/unit/test_webappsActor.js > @@ +96,1 @@ > > > > Given that this test is only executed against b2g, > may be we can, instead of overloading stuff like that, > listen for DOM events via the > SystamAppProxy.addEventListener("webapps-launch"/"webapps-close", ...) ? Right now this test is only executed on !b2g (bug 916874 re-enabled the test on !b2g, bug 1006106 will re-enable it on b2g).

(In reply to Marco Castelluccio [:marco] from comment #10) > (In reply to Alexandre Poirot (:ochameau) from comment #9) > > Looks good but it breaks gaia's Firefox nightly support. > > This addon, is installed within Firefox nightly profile in order to be able > > to run gaia. > > The following code ends up breaking with your patch: > > > > https://github.com/mozilla-b2g/gaia/blob/master/tools/extensions/browser- > > helper%40gaiamobile.org/content/shell.js#L33-L53 > > > > Please submit a patch against this addon to prevent breaking gaia in Firefox. > > (I would suggest keeping this observer notification listener, while adding > > new code to accomodate your modification. It will to ease landing the gaia > > patch) > > Is this addon used in b2g desktop? If not, how can I test it? > I guess I'll keep this observer notification and remove it in a followup > together with the update to this addon. The addon is in gaia repo, over here: https://github.com/mozilla-b2g/gaia/blob/master/tools/extensions/browser-helper%40gaiamobile.org/ you can test it by running gaia with: $ cd .../gaia $ DEBUG=1 make $ .../firefox -profile profile-debug

(In reply to Mark Finkle (:mfinkle) from comment #8) > Comment on attachment 8420309 [details] [diff] [review] > Patch > > >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js > > Cu.import("resource://gre/modules/Webapps.jsm"); > > Just curious if this can be delay loaded now. Or if it could be moved out of > the startup method. If not, that's fine. I'm just thinking about ancillary > code cleanup. Unfortunately, I don't think so. I tried to delay loading Webapps.jsm in bug 970215, but Webapps.js depends on it being loaded so it can send it messages, and it doesn't have a way to ensure that it's loaded (because the two files are evaluated in separate processes and communicate via the cross-process message manager).

I've applied your suggestions. I'll submit a pull request with the changes to the browser-helper addon as soon as this lands.

Comment on attachment 8425834 [details] [diff] [review] Patch Review of attachment 8425834 [details] [diff] [review]: ----------------------------------------------------------------- I would like to see a try build. ::: b2g/components/WebappManager.jsm @@ +19,5 @@ > + let detail = aEvt.detail; > + > + if (!detail || !detail.id || > + detail.type != "webapps-install-granted" || > + detail.type != "webapps-install-denied") { This will always return true because one of the |detail.type != ...| will always be true for the values you expect. You want (!detail || !detail.id || ["webapps-install-granted", "webapps-install-denied"].indexOf(detail.type) == -1) or something equivalent.

(In reply to Marco Castelluccio [:marco] from comment #13) > I'll submit a pull request with the changes to the browser-helper addon as > soon as this lands. You have to land a fix on gaia first, listening for both obs notifications and new chrome event, or you will break nightly support until you land the gaia patch.

(In reply to Alexandre Poirot (:ochameau) from comment #15) > (In reply to Marco Castelluccio [:marco] from comment #13) > > I'll submit a pull request with the changes to the browser-helper addon as > > soon as this lands. > > You have to land a fix on gaia first, listening for both obs notifications > and new chrome event, or you will break nightly support until you land the > gaia patch. So I should land in Gaia a first patch to support both things and then a second patch to remove the observer? I wish I could land everything at the same time, this procedure doesn't look ideal to me. (In reply to Fabrice Desré [:fabrice] from comment #14) > I would like to see a try build. Here it is: https://tbpl.mozilla.org/?tree=Try&rev=eccbee55779a > ::: b2g/components/WebappManager.jsm > @@ +19,5 @@ > > + let detail = aEvt.detail; > > + > > + if (!detail || !detail.id || > > + detail.type != "webapps-install-granted" || > > + detail.type != "webapps-install-denied") { > > This will always return true because one of the |detail.type != ...| will > always be true for the values you expect. Oops, you're right...

(In reply to Marco Castelluccio [:marco] from comment #16) > So I should land in Gaia a first patch to support both things and > then a second patch to remove the observer? > > I wish I could land everything at the same time, this procedure > doesn't look ideal to me. It's not, but we can't do atomic changes to gaia+gecko so we have to do that. > (In reply to Fabrice Desré [:fabrice] from comment #14) > > I would like to see a try build. > > Here it is: https://tbpl.mozilla.org/?tree=Try&rev=eccbee55779a I retriggered the failing gaia ones, but I think we're busted.

(In reply to Fabrice Desré [:fabrice] from comment #17) > I retriggered the failing gaia ones, but I think we're busted. Looks like "Gaia Unit Test" was successful when retriggered, "Marionette WebAPI Tests" wasn't.

(In reply to Marco Castelluccio [:marco] from comment #16) > So I should land in Gaia a first patch to support both things and > then a second patch to remove the observer? Unfortunately, we all have to go throught such steps while we have two distinct repos without explicit revisions being set in each others...

Comment on attachment 8426607 [details] [diff] [review] Patch Review of attachment 8426607 [details] [diff] [review]: ----------------------------------------------------------------- clearing review until try results are green.

It is failing waiting for the "mozbrowserloadend" event. Looks like also the "mozbrowserloadstart" event isn't being sent (I've added another event listener for this event here: http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/emulator.py?rev=d065ab496e3d#368). I don't know why, but the latest try runs that I'm spinning don't contain Gecko logs (the old one does contain them (https://tbpl.mozilla.org/?tree=Try&rev=eccbee55779a) the new one doesn't (https://tbpl.mozilla.org/?tree=Try&rev=e07ae241b04d)). In the old try run, that contains more logging messages, I've noticed the following message: "16:34:06 INFO - 05-21 23:32:04.662 44 44 I Gecko : 1400715124666 Marionette INFO could not load listener into content for page: chrome://b2g/content/shell.html" Is this normal? Could this be the issue?

(In reply to Marco Castelluccio [:marco] from comment #21) > It is failing waiting for the "mozbrowserloadend" event. > Looks like also the "mozbrowserloadstart" event isn't > being sent (I've added another event listener for this > event here: > http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/ > marionette/emulator.py?rev=d065ab496e3d#368). Are you sure the launch() call actually goes up to launch the app? > I don't know why, but the latest try runs that I'm > spinning don't contain Gecko logs (the old one does > contain them (https://tbpl.mozilla.org/?tree=Try&rev=eccbee55779a) > the new one doesn't (https://tbpl.mozilla.org/?tree=Try&rev=e07ae241b04d)). See bug 1015487 ...

(In reply to < away until June 17 > from comment #22) > (In reply to Marco Castelluccio [:marco] from comment #21) > > It is failing waiting for the "mozbrowserloadend" event. > > Looks like also the "mozbrowserloadstart" event isn't > > being sent (I've added another event listener for this > > event here: > > http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/ > > marionette/emulator.py?rev=d065ab496e3d#368). > > > Are you sure the launch() call actually goes up to launch the app? > > > I don't know why, but the latest try runs that I'm > > spinning don't contain Gecko logs (the old one does > > contain them (https://tbpl.mozilla.org/?tree=Try&rev=eccbee55779a) > > the new one doesn't (https://tbpl.mozilla.org/?tree=Try&rev=e07ae241b04d)). > > See bug 1015487 ... Logging is working again, the problem is: 18:42:32 INFO - 06-03 01:41:00.380 196 196 I Gecko : ###################################### forms.js loaded 18:42:32 INFO - 06-03 01:41:00.790 44 154 D : HostConnection::get() New Host Connection established 0x44bd72e0, tid 154 18:42:32 INFO - 06-03 01:41:00.980 196 196 I Gecko : ############################### browserElementPanning.js loaded 18:42:32 INFO - 06-03 01:41:01.190 196 196 I Gecko : ######################## BrowserElementChildPreload.js loaded 18:42:32 INFO - 06-03 01:41:01.400 44 44 I Gonk : Setting nice for pid 249 to 18 18:42:32 INFO - 06-03 01:41:01.400 44 44 I Gonk : Changed nice for pid 249 from 0 to 18. 18:42:32 INFO - 06-03 01:41:01.490 44 44 I Gonk : Setting nice for pid 249 to 1 18:42:32 INFO - 06-03 01:41:01.500 44 44 I Gonk : Changed nice for pid 249 from 18 to 1. 18:42:32 INFO - 06-03 01:41:13.120 44 44 I Gonk : Setting nice for pid 249 to 7 18:42:32 INFO - 06-03 01:41:13.130 44 44 I Gonk : Changed nice for pid 249 from 1 to 7. 18:42:32 WARNING - 06-03 01:41:15.990 196 196 E GeckoConsole: [JavaScript Error: "Failed to load module resource:///modules/WebappManager.jsm." {file: "resource://gre/modules/XPCOMUtils.jsm" line: 229}] 18:42:32 WARNING - 06-03 01:41:16.020 196 196 E GeckoConsole: [JavaScript Error: "NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]" {file: "resource://gre/modules/XPCOMUtils.jsm" line: 202}] 18:42:32 INFO - 06-03 01:41:16.020 196 196 I Gecko : !! Creating a dummy channel for ftu.gaiamobile.org (no appInfo)

Finally green on try: https://tbpl.mozilla.org/?tree=Try&rev=d56513952786 (The problem was that WebappManager.jsm on B2G was loading Webapps.jsm in the child. I've fixed the problem by loading Webapps.jsm only when needed, after bug 910473 we won't need to load Webapps.jsm anymore)

Comment on attachment 8436494 [details] [diff] [review] Patch Review of attachment 8436494 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +1255,5 @@ > + > + let manifest = new ManifestHelper(aManifest, app.origin); > + > + WebappManager.launch(app, manifest.fullLaunchPath(aStartPoint), > + aTimeStamp); We are still breaking DEBUG=1 mode in gaia as WebappManager here is firefox one, and fails launching the app as it is being looking for webappsrt file path (getLaunchTarget returns null as it searches for apps in home folder) Again, the Mulet is going to solve these weird issues with DEBUG=1 mode, but it is not ready to replace b2g desktop yet, so in the meantime, we have to keep supporting DEBUG=1. ::: toolkit/devtools/apps/tests/unit/head_apps.js @@ +87,5 @@ > + let app = DOMApplicationRegistry.webapps[aAppId]; > + > + return { "path": app.basePath + "/" + app.id, > + "isCoreApp": false }; > + }; Don't you want to do like most other tests and overload WebappManager.getPackagePath instead? ::: toolkit/devtools/apps/tests/unit/test_webappsActor.js @@ +92,5 @@ > + do_check_eq(aStartPoint, startPoint); > + > + run_next_test(); > + }; > + } This reduce the code coverage of this test. I wish we could test all code up to the very precide interaction we have with gaia. So that we would have a b2g specific test, checking that we correctly send 'webapps-launch' custom event. I think we can listen from here via SystemAppProxy.addEventListener. (it would have the benefit to be less hacky than overloading methods like that) Othewise, we should at least ensure that WebappManager.launch is called. (similar comment applies to close and uninstall)

(note that the patch needs to be rebased against last master)

(In reply to Alexandre Poirot (:ochameau) from comment #25) > Comment on attachment 8436494 [details] [diff] [review] > Patch > > Review of attachment 8436494 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/apps/src/Webapps.jsm > @@ +1255,5 @@ > > + > > + let manifest = new ManifestHelper(aManifest, app.origin); > > + > > + WebappManager.launch(app, manifest.fullLaunchPath(aStartPoint), > > + aTimeStamp); > > We are still breaking DEBUG=1 mode in gaia as WebappManager here is firefox > one, and fails launching the app as it is being looking for webappsrt file > path (getLaunchTarget returns null as it searches for apps in home folder) I haven't sent the pull request to Gaia yet, but I will do it as soon as this gets r+ed. > > ::: toolkit/devtools/apps/tests/unit/head_apps.js > @@ +87,5 @@ > > + let app = DOMApplicationRegistry.webapps[aAppId]; > > + > > + return { "path": app.basePath + "/" + app.id, > > + "isCoreApp": false }; > > + }; > > Don't you want to do like most other tests and overload > WebappManager.getPackagePath instead? I can't import WebappManager.jsm here (NS_ERROR_FILE_NOT_FOUND, presumably because its URL is "resource:///modules/WebappManager.jsm", so it tries to load the file in an app-specific directory, that doesn't exist in xpcshell tests). > > ::: toolkit/devtools/apps/tests/unit/test_webappsActor.js > @@ +92,5 @@ > > + do_check_eq(aStartPoint, startPoint); > > + > > + run_next_test(); > > + }; > > + } > > This reduce the code coverage of this test. I wish we could test all code up > to the very precide interaction we have with gaia. > So that we would have a b2g specific test, checking that we correctly send > 'webapps-launch' custom event. I think we can listen from here via > SystemAppProxy.addEventListener. (it would have the benefit to be less hacky > than overloading methods like that) > > Othewise, we should at least ensure that WebappManager.launch is called. > > (similar comment applies to close and uninstall) The problem is that I didn't manage to overload WebappManager in here, so I can't ensure |launch| is called. At the same time, this test doesn't work on B2G, so we can't yet write a B2G specific test. Given that this test is meant as a unit test, can't we just test that the correct information is passed to DOMApplicationRegistry's |launch|? In another bug we could make the test work on B2G (bug 1006106) and then build an integration test that tests all the code up to the interaction with Gaia.

Comment on attachment 8436494 [details] [diff] [review] Patch Review of attachment 8436494 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/components/WebappManager.jsm @@ +57,5 @@ > + }, > + > + launch: function(aApp, aFullLaunchPath, aTimeStamp) { > + SystemAppProxy._sendCustomEvent("webapps-launch", { > + __exposedProps__: { __exposedProps__ is a thing of the past. We use Cu.cloneInto automagically in _sendCustomEvent when there is no __exposedProps__ @@ +70,5 @@ > + }, > + > + close: function(aApp) { > + SystemAppProxy._sendCustomEvent("webapps-close", { > + __exposedProps__: { idem. ::: webapprt/test/chrome/browser_geolocation-prompt-noperm.js @@ +10,5 @@ > win.addEventListener("load", function onLoadWindow() { > win.removeEventListener("load", onLoadWindow, false); > + > + if (win.location == "chrome://global/content/commonDialog.xul") { > + promptShown = true; why are these changes related to this bug?

I would easily r+ with a gaia patch ensuring that we can land this gecko patch without breaking gaia. You can ignore the comments about the tests, we are working on a new test script for the webapps actor for b2g.

(In reply to Fabrice Desré [:fabrice] from comment #28) > __exposedProps__ is a thing of the past. We use Cu.cloneInto automagically > in _sendCustomEvent when there is no __exposedProps__ I did remove __exposedProps__, probably I've reintroduced it by mistake while debugging... > > ::: webapprt/test/chrome/browser_geolocation-prompt-noperm.js > @@ +10,5 @@ > > win.addEventListener("load", function onLoadWindow() { > > win.removeEventListener("load", onLoadWindow, false); > > + > > + if (win.location == "chrome://global/content/commonDialog.xul") { > > + promptShown = true; > > why are these changes related to this bug? Removing the observer notifications, we have to modify a bit how the webapprt tests work (simplifying them). Before this patch, a dialog asking to install the app was opened for every test. Now we override askInstall, so the dialog is no longer opened.

Here's the Gaia patch.

Comment on attachment 8447556 [details] [diff] [review] Patch Review of attachment 8447556 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, works great with DEBUG=1.

Comment on attachment 8447557 [details] [diff] [review] Gaia patch Review of attachment 8447557 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for this gaia patch! You may land it before the gecko patch and surround it with try/catch in order to prevent breaking people using older gecko.

I've sent the patch to try once again, and there's a new failure... INFO - Non-local network connections are disabled and a connection attempt to marketplace.allizom.org (63.245.217.111) was made. You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server. Then b2g crashes (because it's not allowed to make the connection). I wonder how the patch is affecting this behavior.

Strange indeed that this patch could change that. I would have say that this connection would happen when checking for updates since the marketplace is preinstalled in gaia profiles. Can you link to the try build?

(In reply to Fabrice Desré [:fabrice] from comment #36) > Strange indeed that this patch could change that. I would have say that this > connection would happen when checking for updates since the marketplace is > preinstalled in gaia profiles. > > Can you link to the try build? https://tbpl.mozilla.org/?tree=Try&rev=291f5ad51a89

This is now green again on try: https://tbpl.mozilla.org/?tree=Try&rev=3c2c360de14e I'm going to land the patch in Gaia first and then this one in the following days.

Erm, is this still relevant?

I don't think it's really useful anymore.