As part of the B2G OS Transition Project we will be removing the mozApps API https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/Navigator/mozApps mozApps will be deprecated in favour of W3C web apps https://www.w3.org/TR/appmanifest/

("fixlater" just means "ideally within the next few months")

I have been working towards a patch for this, and I'm very close...

ehsan, are you removing also the appId from nsIPrincipal?

Sounds like we're working on the same thing, see bug 1291291. Ehsan if you're more far along than me - and it sounds like you are - I'll close that bug as a dup.

(In reply to Andrea Marchesini [:baku] from comment #3) > ehsan, are you removing also the appId from nsIPrincipal? Not yet. I'm removing navigator.mozApps and all of the test/code that depends on it. (I'm doing this work so that we can remove all notions of apps from Gecko, including appId from nsIPrincipal.)

(In reply to Gabriele Svelto [:gsvelto] from comment #4) > Sounds like we're working on the same thing, see bug 1291291. Ehsan if > you're more far along than me - and it sounds like you are - I'll close that > bug as a dup. I commented there. I think my work here is a subset of that bug, but we should be able to collaborate towards the greater goal. :-)

AppID is currently in used by some addons (mine, actually). I'm OK to remove it, but maybe would be awesome if this happens at the same time with exposing Containers to WebExtensions. I'm happy to help, of course.

(In reply to Andrea Marchesini [:baku] from comment #7) > AppID is currently in used by some addons (mine, actually). I'm OK to remove > it, but maybe would be awesome if this happens at the same time with > exposing Containers to WebExtensions. I'm happy to help, of course. What's the plan for exposing containers to WebExtensions? (Note that this is off topic for this bug as appId won't be removed here.)

What these patches do is remove navigator.mozApps and everything in the codebase which relied on that (including swaths of tests that were disabled before.) I have also removed the JSMs in dom/apps that weren't used by anything any more, but left alone the ones that were still being used to be removed in the future.

Comment on attachment 8796624 [details] [diff] [review] Part 1: Remove support for running marionette tests as apps Review of attachment 8796624 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to forward this review to Maja because I don't normally review Marionette code.

Comment on attachment 8796624 [details] [diff] [review] Part 1: Remove support for running marionette tests as apps Review of attachment 8796624 [details] [diff] [review]: ----------------------------------------------------------------- r+wc: please also remove test_container stuff from testing/marionette/harness/session, thanks! Yay, removing coding.

Comment on attachment 8796625 [details] [diff] [review] Part 2: Remove the webapps devtools actor Review of attachment 8796625 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good, thanks for the cleanup! ::: devtools/client/webide/test/test_runtime.html @@ -87,5 @@ > - }); > - > - win.AppManager.update("runtime-list"); > - > - let packagedAppLocation = getTestFilePath("app"); Portions of this test appear to still be useful, so let's try to only remove the parts about installing apps, since that's what can't be done anymore. I would say try removing a portion of the test starting here... @@ -132,5 @@ > - is(Object.keys(DebuggerServer._connections).length, 0, "Disconnected"); > - > - ok(win.AppManager.selectedProject, "A project is still selected"); > - ok(!isPlayActive(), "play button is disabled 4"); > - ok(!isStopActive(), "stop button is disabled 4"); ...and ending here. The rest of it doesn't seem to relate to apps.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15) > ::: devtools/client/webide/test/test_runtime.html > @@ -87,5 @@ > > - }); > > - > > - win.AppManager.update("runtime-list"); > > - > > - let packagedAppLocation = getTestFilePath("app"); > > Portions of this test appear to still be useful, so let's try to only remove > the parts about installing apps, since that's what can't be done anymore. I > would say try removing a portion of the test starting here... > > @@ -132,5 @@ > > - is(Object.keys(DebuggerServer._connections).length, 0, "Disconnected"); > > - > > - ok(win.AppManager.selectedProject, "A project is still selected"); > > - ok(!isPlayActive(), "play button is disabled 4"); > > - ok(!isStopActive(), "stop button is disabled 4"); > > ...and ending here. The rest of it doesn't seem to relate to apps. Given that WebIDE is intended to develop "apps" and we're going to remove all notions of apps from Gecko soon, it seems to me that WebIDE is also slated for a full deletion from the code base soon. Given that, is the above really worth it? I mostly chose to remove the whole test because of this reason.

(In reply to :Ehsan Akhgari from comment #16) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15) > > ::: devtools/client/webide/test/test_runtime.html > > @@ -87,5 @@ > > > - }); > > > - > > > - win.AppManager.update("runtime-list"); > > > - > > > - let packagedAppLocation = getTestFilePath("app"); > > > > Portions of this test appear to still be useful, so let's try to only remove > > the parts about installing apps, since that's what can't be done anymore. I > > would say try removing a portion of the test starting here... > > > > @@ -132,5 @@ > > > - is(Object.keys(DebuggerServer._connections).length, 0, "Disconnected"); > > > - > > > - ok(win.AppManager.selectedProject, "A project is still selected"); > > > - ok(!isPlayActive(), "play button is disabled 4"); > > > - ok(!isStopActive(), "stop button is disabled 4"); > > > > ...and ending here. The rest of it doesn't seem to relate to apps. > > Given that WebIDE is intended to develop "apps" and we're going to remove > all notions of apps from Gecko soon, it seems to me that WebIDE is also > slated for a full deletion from the code base soon. Given that, is the > above really worth it? I mostly chose to remove the whole test because of > this reason. If WebIDE were just about apps, I would agree. It is certainly true that apps are its main focus and the reason WebIDE was originally created. However, WebIDE is also home to our currently recommended way of connecting to remote devices like Firefox for Android to debug web pages. There is a plan to move this remote debugging to the newer about:debugging (bug 1212802). I believe WebIDE will likely stick around until that's done. (Many people agree it's awkward that WebIDE has these two mostly unrelated focuses of creating apps and remote debugging.) The rest of the test is about connecting to runtimes in general, which is the part that seems to still be relevant without apps.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #17) > (In reply to :Ehsan Akhgari from comment #16) > > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15) > > > ::: devtools/client/webide/test/test_runtime.html > > > @@ -87,5 @@ > > > > - }); > > > > - > > > > - win.AppManager.update("runtime-list"); > > > > - > > > > - let packagedAppLocation = getTestFilePath("app"); > > > > > > Portions of this test appear to still be useful, so let's try to only remove > > > the parts about installing apps, since that's what can't be done anymore. I > > > would say try removing a portion of the test starting here... > > > > > > @@ -132,5 @@ > > > > - is(Object.keys(DebuggerServer._connections).length, 0, "Disconnected"); > > > > - > > > > - ok(win.AppManager.selectedProject, "A project is still selected"); > > > > - ok(!isPlayActive(), "play button is disabled 4"); > > > > - ok(!isStopActive(), "stop button is disabled 4"); > > > > > > ...and ending here. The rest of it doesn't seem to relate to apps. > > > > Given that WebIDE is intended to develop "apps" and we're going to remove > > all notions of apps from Gecko soon, it seems to me that WebIDE is also > > slated for a full deletion from the code base soon. Given that, is the > > above really worth it? I mostly chose to remove the whole test because of > > this reason. > > If WebIDE were just about apps, I would agree. It is certainly true that > apps are its main focus and the reason WebIDE was originally created. > > However, WebIDE is also home to our currently recommended way of connecting > to remote devices like Firefox for Android to debug web pages. There is a > plan to move this remote debugging to the newer about:debugging (bug > 1212802). I believe WebIDE will likely stick around until that's done. > (Many people agree it's awkward that WebIDE has these two mostly unrelated > focuses of creating apps and remote debugging.) > > The rest of the test is about connecting to runtimes in general, which is > the part that seems to still be relevant without apps. OK, I didn't know that. I'll test removing just those parts on try...

I'm running into some issues figuring out how to take out the app specific parts of this test. The test starts by pushing three things onto win.AppManager.runtimeList.usb, at lines 52, 65 and 76. It then expects to find three buttons corresponding to the usb runtime list thing here: <http://searchfox.org/mozilla-central/source/devtools/client/webide/test/test_runtime.html#98> If I remove everything from <http://searchfox.org/mozilla-central/source/devtools/client/webide/test/test_runtime.html#91> down to <http://searchfox.org/mozilla-central/source/devtools/client/webide/test/test_runtime.html#136>, then items variable is now undefined. If I refactor out the definition of items (and panelNode) from <http://searchfox.org/mozilla-central/source/devtools/client/webide/test/test_runtime.html#96>, this line fails: <http://searchfox.org/mozilla-central/source/devtools/client/webide/test/test_runtime.html#172> since in that case, items.length is 0. The same problem of course applies to this block of code as well: <http://searchfox.org/mozilla-central/source/devtools/client/webide/test/test_runtime.html#185>. It's not really possible to find out what element this is pointing to and how it gets populated using grep, but the three things being added to the "usb runtime list" makes it sound like that's where the three expected items here come from... Does that mean that the code from <http://searchfox.org/mozilla-central/source/devtools/client/webide/test/test_runtime.html#171> to here <http://searchfox.org/mozilla-central/source/devtools/client/webide/test/test_runtime.html#192> also needs to get removed? That basically only leaves <http://searchfox.org/mozilla-central/source/devtools/client/webide/test/test_runtime.html#138> to <http://searchfox.org/mozilla-central/source/devtools/client/webide/test/test_runtime.html#159> but most of those check seem meaningless without adding stuff to the runtime list... I'm completely lost as to what this test is trying to check and how to remove the app specific parts. Can you please help me? Thanks!

(In reply to :Ehsan Akhgari from comment #19) > I'm running into some issues figuring out how to take out the app specific > parts of this test. > > I'm completely lost as to what this test is trying to check and how to > remove the app specific parts. Can you please help me? Thanks! Sorry, it seems that ended up being more complex that I was hoping. After looking at the test more closely, it seems like the main change is just that the play buttons will always be disabled (because there's no way to install an app anymore). I applied your patch (and restored the test to its previous state). The attached test changes seem to fix it for me, feel free to fold into your patch. Let me know if more assistance is needed!

Comment on attachment 8796626 [details] [diff] [review] Part 3: Remove Navigator.mozApps and the code depending on it Review of attachment 8796626 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/runapp.js @@ -1,1 @@ > -"use strict"; I'm unfamiliar with this file (and others in b2g/), but presumably removing it breaks B2G. I know that's according to plan, but we should give the B2G folks notice about this change (and see if there's anything we should do to mitigate the impact). Perhaps it makes more sense to leave this code in the tree, even if it's using nonexistent APIs, because it's easier for the B2G folks to fix it up (presumably in conjunction with moving b2g/ to another repository) than to recreate it. ::: b2g/chrome/content/settings.js @@ -639,5 @@ > resetToPref: true > }, > > 'dom.mozApps.use_reviewer_certs': false, > - 'dom.mozApps.signed_apps_installable_from': 'https://marketplace.firefox.com', Nit: You should also remove dom.mozApps.use_reviewer_certs, since you're removing the only two other references to it in the code. ::: b2g/chrome/content/shell.html @@ -36,5 @@ > <script type="application/javascript;version=1.8" > src="chrome://b2g/content/screen.js"> </script> > - <!-- this script handles the "runapp" argument for desktop builds --> > - <script type="application/javascript;version=1.8" > - src="chrome://b2g/content/runapp.js"> </script> If we leave runapp.js, then we'd leave this too. In general, I'm unsure we should touch b2g/. Either way, we'll break it when we remove mozApps from dom/ and elsewhere in the tree. But if we leave the b2g/ code in place, then it's presumably easier for B2G folks to fix it up. Let's check with the B2G folks. Requesting feedback from Fabrice. ::: devtools/shared/apps/moz.build @@ -2,5 @@ > # This Source Code Form is subject to the terms of the Mozilla Public > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > -MOCHITEST_MANIFESTS += [ I defer to jryans on all the devtools/ changes. Requesting review from him. ::: dom/apps/AppsService.js @@ +44,5 @@ > debug("GetManifestCSPByLocalId( " + localId + " )"); > if (this.isInvalidId(localId)) { > return null; > } > + throw Cr.NS_ERROR_NOT_IMPLEMENTED; Nit: here and below, it'd be helpful to include the original line in a comment, so it's obvious why the function throws without having to dig through history. ::: dom/apps/OperatorApps.jsm @@ -7,5 @@ > const Cu = Components.utils; > const Cc = Components.classes; > const Ci = Components.interfaces; > > this.EXPORTED_SYMBOLS = ["OperatorAppsRegistry"]; Nit: Although b2g/chrome/content/shell.js imports OperatorApps.jsm, OperatorAppsRegistry isn't being used anywhere in b2g/ (nor elsewhere in the codebase), so it should be possible to remove this file entirely. ::: dom/downloads/tests/mochitest.ini @@ -7,5 @@ > serve_file.sjs > clear_all_done_helper.js > file_app.template.webapp > file_app.sjs > common_app.js Nit: You should be able to remove file_app.template.webapp, file_app.sjs, and common_app.js too.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #20) > Created attachment 8797304 [details] [diff] [review] > Clean up test_runtime.html after webapps actor removal > > (In reply to :Ehsan Akhgari from comment #19) > > I'm running into some issues figuring out how to take out the app specific > > parts of this test. > > > > I'm completely lost as to what this test is trying to check and how to > > remove the app specific parts. Can you please help me? Thanks! > > Sorry, it seems that ended up being more complex that I was hoping. After > looking at the test more closely, it seems like the main change is just that > the play buttons will always be disabled (because there's no way to install > an app anymore). > > I applied your patch (and restored the test to its previous state). The > attached test changes seem to fix it for me, feel free to fold into your > patch. > > Let me know if more assistance is needed! Thank you, this is great!

(In reply to Myk Melez [:myk] [@mykmelez] from comment #21) > Perhaps it makes more sense to leave this code in the tree, even if it's > using nonexistent APIs, because it's easier for the B2G folks to fix it up > (presumably in conjunction with moving b2g/ to another repository) than to > recreate it. That'd be fine with me if that's what Fabrice prefers. > ::: dom/apps/AppsService.js > @@ +44,5 @@ > > debug("GetManifestCSPByLocalId( " + localId + " )"); > > if (this.isInvalidId(localId)) { > > return null; > > } > > + throw Cr.NS_ERROR_NOT_IMPLEMENTED; > > Nit: here and below, it'd be helpful to include the original line in a > comment, so it's obvious why the function throws without having to dig > through history. Even given that we know that this file will be completely removed very soon? > ::: dom/apps/OperatorApps.jsm > @@ -7,5 @@ > > const Cu = Components.utils; > > const Cc = Components.classes; > > const Ci = Components.interfaces; > > > > this.EXPORTED_SYMBOLS = ["OperatorAppsRegistry"]; > > Nit: Although b2g/chrome/content/shell.js imports OperatorApps.jsm, > OperatorAppsRegistry isn't being used anywhere in b2g/ (nor elsewhere in the > codebase), so it should be possible to remove this file entirely. You're right! > ::: dom/downloads/tests/mochitest.ini > @@ -7,5 @@ > > serve_file.sjs > > clear_all_done_helper.js > > file_app.template.webapp > > file_app.sjs > > common_app.js > > Nit: You should be able to remove file_app.template.webapp, file_app.sjs, > and common_app.js too. Good catch!

(In reply to :Ehsan Akhgari from comment #23) > > ::: dom/apps/AppsService.js > > @@ +44,5 @@ > > > debug("GetManifestCSPByLocalId( " + localId + " )"); > > > if (this.isInvalidId(localId)) { > > > return null; > > > } > > > + throw Cr.NS_ERROR_NOT_IMPLEMENTED; > > > > Nit: here and below, it'd be helpful to include the original line in a > > comment, so it's obvious why the function throws without having to dig > > through history. > > Even given that we know that this file will be completely removed very soon? There's also an instance in dom/secureelement/gonk/ACEService.js, but if we know we're going to remove both files very soon (and won't reuse them elsewhere), then it's fine to leave the patch as-is.

One last note: the patch makes changes to a bunch of directories for which I'm not a peer. I suppose you could justify these as necessary bustage fixes for changes that I do have the authority to review (i.e. the removal of services from dom/apps/). But it would make sense to get review (or at least a rubber stamp) from someone who does have authority over those dirs, like peterv for dom/ and mcmanus for netwerk/.

Comment on attachment 8796626 [details] [diff] [review] Part 3: Remove Navigator.mozApps and the code depending on it Review of attachment 8796626 [details] [diff] [review]: ----------------------------------------------------------------- devtools changes look good to me! It might make more sense in the part 2 patch if you intend to land them separately since the test being removed is a test of the DevTools webapps actor, but I don't feel strongly about it.

(In reply to Myk Melez [:myk] [@mykmelez] from comment #25) > One last note: the patch makes changes to a bunch of directories for which > I'm not a peer. I suppose you could justify these as necessary bustage > fixes for changes that I do have the authority to review (i.e. the removal > of services from dom/apps/). But it would make sense to get review (or at > least a rubber stamp) from someone who does have authority over those dirs, > like peterv for dom/ and mcmanus for netwerk/. I'm not sure if those reviews are technically necessary, but OK, I'll ask them to follow the rules...

(In reply to Myk Melez [:myk] [@mykmelez] from comment #21) > Comment on attachment 8796626 [details] [diff] [review] > Part 3: Remove Navigator.mozApps and the code depending on it > > Review of attachment 8796626 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: b2g/chrome/content/runapp.js > @@ -1,1 @@ > > -"use strict"; > > I'm unfamiliar with this file (and others in b2g/), but presumably removing > it breaks B2G. I know that's according to plan, but we should give the B2G > folks notice about this change (and see if there's anything we should do to > mitigate the impact). I'm fine with removing that file. The current b2g doesn't use any of the Apps code anymore. > ::: b2g/chrome/content/settings.js > @@ -639,5 @@ > > resetToPref: true > > }, > > > > 'dom.mozApps.use_reviewer_certs': false, > > - 'dom.mozApps.signed_apps_installable_from': 'https://marketplace.firefox.com', > > Nit: You should also remove dom.mozApps.use_reviewer_certs, since you're > removing the only two other references to it in the code. I agree. > ::: b2g/chrome/content/shell.html > @@ -36,5 @@ > > <script type="application/javascript;version=1.8" > > src="chrome://b2g/content/screen.js"> </script> > > - <!-- this script handles the "runapp" argument for desktop builds --> > > - <script type="application/javascript;version=1.8" > > - src="chrome://b2g/content/runapp.js"> </script> > > If we leave runapp.js, then we'd leave this too. In general, I'm unsure we > should touch b2g/. Either way, we'll break it when we remove mozApps from > dom/ and elsewhere in the tree. But if we leave the b2g/ code in place, > then it's presumably easier for B2G folks to fix it up. Let's check with > the B2G folks. Requesting feedback from Fabrice. As commented above, we can remove this. > ::: dom/apps/OperatorApps.jsm > @@ -7,5 @@ > > const Cu = Components.utils; > > const Cc = Components.classes; > > const Ci = Components.interfaces; > > > > this.EXPORTED_SYMBOLS = ["OperatorAppsRegistry"]; > > Nit: Although b2g/chrome/content/shell.js imports OperatorApps.jsm, > OperatorAppsRegistry isn't being used anywhere in b2g/ (nor elsewhere in the > codebase), so it should be possible to remove this file entirely. That's correct.

Comment on attachment 8796626 [details] [diff] [review] Part 3: Remove Navigator.mozApps and the code depending on it Trying also Johnny for the dom/ parts, let the review race begin!

Comment on attachment 8796626 [details] [diff] [review] Part 3: Remove Navigator.mozApps and the code depending on it \o/

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b9b472734bfa Part 1: Remove support for running marionette tests as apps; r=maja_zf https://hg.mozilla.org/integration/mozilla-inbound/rev/02d134905768 Part 2: Remove the webapps devtools actor; r=jryans https://hg.mozilla.org/integration/mozilla-inbound/rev/144625d6fafa Part 3: Remove Navigator.mozApps and code depending on it; r=myk,jryans,fabrice,mcmanus,peterv

Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/4e5b5087c13e No bug - Clobber on OS X to fix bustage caused by files removed in bug 1261019. r=bustage-fix on a CLOSED TREE

https://hg.mozilla.org/mozilla-central/rev/b9b472734bfa https://hg.mozilla.org/mozilla-central/rev/02d134905768 https://hg.mozilla.org/mozilla-central/rev/144625d6fafa https://hg.mozilla.org/mozilla-central/rev/4e5b5087c13e

The patches for this seem to have broken push notifications on Android; see bug 1311445.

I've updated the docs to reflect this change: Navigator.mozApps archived here: https://developer.mozilla.org/en-US/docs/Archive/Firefox_OS/API/Navigator Relevant interfaces archived here: https://developer.mozilla.org/en-US/docs/Archive/Marketplace/API https://developer.mozilla.org/en-US/docs/Archive/Marketplace/API/App_installation_and_management_APIs Note added to Fx 52 release notes: https://developer.mozilla.org/en-US/Firefox/Releases/52#Others Let me know if this looks OK - cheers!