905881 - Move the package in the local installation directory

Status: RESOLVED FIXED

(Firefox Graveyard :: Web Apps, defect, P2)

Description

[Marco Castelluccio [:marco]]

On desktop we want the package zip file in the app installation directory and not in the Firefox profile.

I see two options:
A) In the registry implementation, we need to set the basePath differently on desktop platforms and we need to modify AppProtocolHandler.js a bit.
B) We could just make AppProtocolHandler.js check |Services.appinfo.ID == "[e-mail]| and use the installation directory if that's true, leaving the basePath as it is now.

Fabrice, what do you prefer?

Comment 1

[[:fabrice] Fabrice Desré]

I think I prefer option A)

Comment 2

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 3

[Marco Castelluccio [:marco]]

Oops

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 791465 [details] [diff] [review]
Patch

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

I'd really like to have bug 910473 fixed before.

::: dom/apps/src/Webapps.jsm
@@ +152,5 @@
>                app.localId = this._nextLocalId();
>              }
>  
>              if (app.basePath === undefined) {
> +#ifndef MOZ_PHOENIX

That really makes me sad... I want to get rid of as many #ifdefs in this file, not add new ones.

@@ +2012,5 @@
>            this._saveApps();
>          }.bind(this);
>  
>          if (installSuccessCallback) {
> +          installSuccessCallback(aManifest, zipFile).then(sendInstalledMessage, sendErrorMessage);

Nothing guarantees that the success callback returns a promise (eg, this is not the case on android afaik). That's why we need a clear interface instead of this.

Comment 5

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> Comment on attachment 791465 [details] [diff] [review]
> Patch
> 
> Review of attachment 791465 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd really like to have bug 910473 fixed before.

The problem is that bug 910473 is going to take some time, this one instead should be fixed before users actually install packaged apps, otherwise we'll need to support two different kinds of installations.

> ::: dom/apps/src/Webapps.jsm
> @@ +152,5 @@
> >                app.localId = this._nextLocalId();
> >              }
> >  
> >              if (app.basePath === undefined) {
> > +#ifndef MOZ_PHOENIX
> 
> That really makes me sad... I want to get rid of as many #ifdefs in this
> file, not add new ones.

We could move this part in WebappOSUtils.jsm as a temporary fix (we will need a function to get the installation path also after the refactoring, so it shouldn't be a problem). 

> @@ +2012,5 @@
> >            this._saveApps();
> >          }.bind(this);
> >  
> >          if (installSuccessCallback) {
> > +          installSuccessCallback(aManifest, zipFile).then(sendInstalledMessage, sendErrorMessage);
> 
> Nothing guarantees that the success callback returns a promise (eg, this is
> not the case on android afaik). That's why we need a clear interface instead
> of this.

I wrote this patch after another that made the success callback return promises, I can move that part in this patch pretty easily.

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Patch

Would this solution be acceptable for you? (the patch depends on the patches in bug 910473)

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Tmp patch

Fabrice, I think the refactoring isn't going to land for Firefox 26, but to avoid compatibility problems I'd like to move the package in the local installation directory anyway (otherwise we'll have to deal with a bunch of problems because we'll need to support two different kinds of installations).
I promise that after the refactoring the code will look much more beautiful!

This patch is trying to modify as less code as possible and is trying to "shadow" what the approach will be after the refactoring (basically the main difference is that here we have a single WebappOSUtils module that returns the installation directory for all the platforms, instead in the future we'll have different components for the different platforms).

The webappActor test is disabled for desktop platforms because the webapps actor isn't really installing apps on desktop and so should be disabled (bug 914275).

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 802820 [details] [diff] [review]
Tmp patch

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

::: dom/apps/src/AppsUtils.jsm
@@ +202,5 @@
>  #ifdef MOZ_WIDGET_GONK
>      isCoreApp = app.basePath == this.getCoreAppsBasePath();
>  #endif
>      debug(app.basePath + " isCoreApp: " + isCoreApp);
> +    return { "path":  WebappOSUtils.getInstallPath(app),

that will probably break b2g desktop builds... Can you check that?

::: netwerk/protocol/app/AppProtocolHandler.js
@@ +10,5 @@
>  const Cr = Components.results;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/WebappOSUtils.jsm");

why do we need that?

::: toolkit/webapps/WebappOSUtils.jsm
@@ +33,5 @@
> +  }
> +
> +  // Convert the binary hash data to a hex string.
> +  return [toHexString(hash.charCodeAt(i)) for (i in hash)].join("");
> +}

can't you import AppsUtils.js for that?

Comment 9

[Marco Castelluccio [:marco]]

Attachment: Patch

(In reply to Fabrice Desré [:fabrice] from comment #8)
> ::: dom/apps/src/AppsUtils.jsm
> @@ +202,5 @@
> >  #ifdef MOZ_WIDGET_GONK
> >      isCoreApp = app.basePath == this.getCoreAppsBasePath();
> >  #endif
> >      debug(app.basePath + " isCoreApp: " + isCoreApp);
> > +    return { "path":  WebappOSUtils.getInstallPath(app),
> 
> that will probably break b2g desktop builds... Can you check that?

I've tested on b2g desktop, and it's working. I haven't tested on an actual device.

> ::: netwerk/protocol/app/AppProtocolHandler.js
> @@ +10,5 @@
> >  const Cr = Components.results;
> >  
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >  Cu.import("resource://gre/modules/Services.jsm");
> > +Cu.import("resource://gre/modules/WebappOSUtils.jsm");
> 
> why do we need that?

We don't need it, it's a leftover :D

> ::: toolkit/webapps/WebappOSUtils.jsm
> @@ +33,5 @@
> > +  }
> > +
> > +  // Convert the binary hash data to a hex string.
> > +  return [toHexString(hash.charCodeAt(i)) for (i in hash)].join("");
> > +}
> 
> can't you import AppsUtils.js for that?

Now AppsUtils.jsm is importing WebappOSUtils.jsm, so I can't import AppsUtils.jsm from WebappOSUtils.jsm anymore.

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 803402 [details] [diff] [review]
Patch

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

I did not review webappsUI.jsm and WebappsInstaller.jsm.
We need to update the tests if needed, but not disable them if possible.

::: toolkit/devtools/apps/tests/unit/xpcshell.ini
@@ +2,5 @@
>  head = head_apps.js
>  tail = tail_apps.js
>  
>  [test_webappsActor.js]
> +skip-if = (os == "win" || "linux" || "mac")

That's not great. What's failing?

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 803402 [details] [diff] [review]
Patch

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

(In reply to Fabrice Desré [:fabrice] from comment #10)
> I did not review webappsUI.jsm and WebappsInstaller.jsm.

In the interest of efficiency, I only reviewed these two files.


It worries me a bit that we're introducing a dependency between a module in dom/apps/ and one in toolkit/webapps/.  Historically, all Mozilla apps have built dom/, but they haven't all been toolkit/-based.  And such assumptions tend to run deep.

I know this is a minimal fix, so that's ok, but in the near future we should consider moving WebappOSUtils.jsm to dom/apps/, merging these two *Utils.jsm modules, or doing something else to break the dependency.

::: toolkit/webapps/WebappsInstaller.jsm
@@ +59,5 @@
>    /**
>     * Creates a native installation of the web app in the OS
>     *
>     * @param aData the data provided to the install function
>     * @param aManifest the manifest data provided by the web app

Nit: update this doc with a description of the aZipPath parameter.

@@ +95,5 @@
>   * @param aData the data object provided to the install function
>   *
>   */
>  function NativeApp(aData) {
> +  let manifest = new ManifestHelper(aData.app.updateManifest || aData.app.manifest, aData.app.origin);

Why does this no longer check aData.isPackage?  Other code checks that property, so this introduces an inconsistency, and it can mask a bug when isPackage is set but manifest is defined instead of updateManifest.  We should do what other code does here or change the behavior elsewhere as well.

@@ +200,5 @@
> +        let url = this.iconURI.QueryInterface(Ci.nsIURL);
> +        this.iconURI = Services.io.newURI("jar:file://" + this.installDir.path +
> +                                          "/application.zip!" + url.filePath,
> +                                          null, null);
> +      }

This should use OS.path.join, nsIFile.append, or some other platform-agnostic function to create the path to the JAR file, and then it should use nsIIOService.newFileURI to generate the file: URL before it starts doing string concatenation to turn it into a jar: URL.

@@ +237,5 @@
> +  _copyPackage: function(aZipPath) {
> +    if (aZipPath) {
> +      yield OS.File.move(aZipPath, OS.Path.join(this.installDir.path,
> +                                                "application.zip"));
> +    }

This feels like the wrong place to put the conditional.  Copying the package is the only thing this function does, so it's strange that its only parameter can be undefined, and it does nothing in that case.

It would be better to check aZipPath in the callers and only call this function if it's defined.  But it would be even better to unrefactor this code and repeat it three times in the three call sites, given it's a very small amount of code to factor out (especially without the conditional).  And perhaps best of all to factor out all of the common code in the three install functions, although that is a larger change that we should do separately.

Comment 12

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #10)
> We need to update the tests if needed, but not disable them if possible.
> 
> ::: toolkit/devtools/apps/tests/unit/xpcshell.ini
> @@ +2,5 @@
> >  head = head_apps.js
> >  tail = tail_apps.js
> >  
> >  [test_webappsActor.js]
> > +skip-if = (os == "win" || "linux" || "mac")
> 
> That's not great. What's failing?

Yeah, the problem is that the webapps actor isn't really installing apps on Desktop (and Android too), it's completely skipping the platform specific code. I think we should disable it (I mean, we should disable not only the test, but the webapps actor itself).

(In reply to Myk Melez [:myk] [@mykmelez] from comment #11)
> It worries me a bit that we're introducing a dependency between a module in
> dom/apps/ and one in toolkit/webapps/.  Historically, all Mozilla apps have
> built dom/, but they haven't all been toolkit/-based.  And such assumptions
> tend to run deep.
> 
> I know this is a minimal fix, so that's ok, but in the near future we should
> consider moving WebappOSUtils.jsm to dom/apps/, merging these two *Utils.jsm
> modules, or doing something else to break the dependency.

My idea was that in the future the AppsInstaller service (or *AnotherMoreBeautifulName*Service :D) will have a function that returns the installation path (so that the different implementations would be clearly separated, without preprocessor directives for B2G and Android).

> @@ +95,5 @@
> >   * @param aData the data object provided to the install function
> >   *
> >   */
> >  function NativeApp(aData) {
> > +  let manifest = new ManifestHelper(aData.app.updateManifest || aData.app.manifest, aData.app.origin);
> 
> Why does this no longer check aData.isPackage?  Other code checks that
> property, so this introduces an inconsistency, and it can mask a bug when
> isPackage is set but manifest is defined instead of updateManifest.  We
> should do what other code does here or change the behavior elsewhere as well.

I'll fix this now and the other places where we do the same after bug 910465.
In this case there isn't any difference between using updateManifest or manifest, because we're only using it to get the name.
Actually, the only other place where we did this was in WebappOSUtils.jsm and would be removed by this patch.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

(In reply to Marco Castelluccio [:marco] from comment #12)
> I'll fix this now and the other places where we do the same after bug 910465.
> In this case there isn't any difference between using updateManifest or
> manifest, because we're only using it to get the name.
> Actually, the only other place where we did this was in WebappOSUtils.jsm
> and would be removed by this patch.

WebappsUI.doInstall in mobile/android/chrome/content/browser.js also does it:

https://github.com/mozilla/mozilla-central/blob/bedab36aac74178e9e8bdc961fc054f1f5ed81ab/mobile/android/chrome/content/browser.js#L6793-L6794

And there are lots of uses of isPackage in Webapps.jsm, although for different purposes.  It seems better to be more explicit here, since the two states are mutually exclusive: when isPackage is true, updateManifest should be defined, while manifest should be undefined; and when isPackage is false, the reverse should be the case.

Comment 14

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #13)
> (In reply to Marco Castelluccio [:marco] from comment #12)
> > I'll fix this now and the other places where we do the same after bug 910465.
> > In this case there isn't any difference between using updateManifest or
> > manifest, because we're only using it to get the name.
> > Actually, the only other place where we did this was in WebappOSUtils.jsm
> > and would be removed by this patch.
> 
> WebappsUI.doInstall in mobile/android/chrome/content/browser.js also does it:
> 
> https://github.com/mozilla/mozilla-central/blob/
> bedab36aac74178e9e8bdc961fc054f1f5ed81ab/mobile/android/chrome/content/
> browser.js#L6793-L6794
> 
> And there are lots of uses of isPackage in Webapps.jsm, although for
> different purposes.  It seems better to be more explicit here, since the two
> states are mutually exclusive: when isPackage is true, updateManifest should
> be defined, while manifest should be undefined; and when isPackage is false,
> the reverse should be the case.

I incorrectly read your previous comment, I thought you were saying that there are other places that aren't checking isPackaged and that we should fix them too.
("We should do what other code does here or change the behavior elsewhere as well." -> "We should do what other code does here and change the behavior elsewhere as well.")

Comment 15

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Comment 16

[Myk Melez [:myk] [@mykmelez]]

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

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

> I incorrectly read your previous comment, I thought you were saying that
> there are other places that aren't checking isPackaged and that we should
> fix them too.
> ("We should do what other code does here or change the behavior elsewhere as
> well." -> "We should do what other code does here and change the behavior
> elsewhere as well.")

Sorry, I was imprecise!  I meant to say that we should continue to check isPackaged here, like similar code does; or, if we decide to stop checking isPackaged here, because it is unecessary, then we should also stop checking isPackaged in similar code; so that we're consistent, and our code here doesn't violate assumptions that code elsewhere is making about how to determine whether or not an app object represents a packaged app and which of the two manifest properties should be defined on it.


r=myk, but please attach an patch with an updated commit comment before requesting commission.

> Move package in the local installation directory. try: -b do -p all -u all -t none

Nits:
* in -> to
* remove *try* block
* add conventional references to bug number, reviewers

I.e. something like:

  bug 804090 - Move package to the local installation directory. r=fabrice, myk

Comment 17

[Marco Castelluccio [:marco]]

Attachment: Patch

Updated commit message.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e6a9da6e6a8c

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 20

[Alexandre Poirot [:ochameau]]

(In reply to Marco Castelluccio [:marco] from comment #12)
> (In reply to Fabrice Desré [:fabrice] from comment #10)
> > We need to update the tests if needed, but not disable them if possible.
> > 
> > ::: toolkit/devtools/apps/tests/unit/xpcshell.ini
> > @@ +2,5 @@
> > >  head = head_apps.js
> > >  tail = tail_apps.js
> > >  
> > >  [test_webappsActor.js]
> > > +skip-if = (os == "win" || "linux" || "mac")
> > 
> > That's not great. What's failing?
> 
> Yeah, the problem is that the webapps actor isn't really installing apps on
> Desktop (and Android too), it's completely skipping the platform specific
> code. I think we should disable it (I mean, we should disable not only the
> test, but the webapps actor itself).

This changeset actually completely disabled the test :'(
I don't know what is wrong with this xpcshell syntax `skip-if = (os == "win" || "linux" || "mac")`
but the test no longer runs, even on b2g.
See the very last test being run on m-c:
  https://tbpl.mozilla.org/php/getParsedLog.php?id=27894744&tree=Mozilla-Central
22:15:49     INFO -  TEST-PASS | /builds/slave/test/build/tests/xpcshell/tests/dom/system/gonk/tests/test_ril_worker_cdma_info_rec.js | test passed (time: 7085.545ms)
22:15:49     INFO -  TEST-INFO | skipping /builds/slave/test/build/tests/xpcshell/tests/toolkit/devtools/apps/tests/unit/test_webappsActor.js | skip-if: (os == "win" || "linux" || "mac")
22:15:49     INFO -  TEST-INFO | skipping /builds/slave/test/build/tests/xpcshell/tests/toolkit/devtools/apps/tests/unit/test_appInstall.js | skip-if: true

Also, we shouldn't stop testing the actor on desktop, most of its features are used to work on desktop.
It ensures that the app registry do works and that's better to have unit test that partially cover a given feature than no tests.
I haven't figured out why, but this changeset do break the test on desktop.
The test against getIconAsDataURL is no longer able to retrieve the icon.
The request app://actor-test/icon-128.png no longer works.

I easily agree to disable stuff on android, but I'd really like to keep stuff enabled on desktop,
as having apps working on firefox is really important for gaia. In firefox itself, not in webappsrt.
Many gaia contributors are using Firefox itself to run gaia, today we are using an addon to make it work nicely, but we would like to reach the goal where gaia would run (almost) natively on firefox.
So that we should do our best to keep stuff working on dekstop and avoid disabling/break features that are used to work.

Comment 21

[Marco Castelluccio [:marco]]

(In reply to Alexandre Poirot (:ochameau) from comment #20)
> Also, we shouldn't stop testing the actor on desktop, most of its features
> are used to work on desktop.

I think most of the actor features don't work on Desktop because they're assuming the same environment as b2g.

> It ensures that the app registry do works and that's better to have unit
> test that partially cover a given feature than no tests.

We already have tests for the app registry. In dom/apps/tests and in dom/tests/mochitest/webapps.

> I haven't figured out why, but this changeset do break the test on desktop.
> The test against getIconAsDataURL is no longer able to retrieve the icon.
> The request app://actor-test/icon-128.png no longer works.

The webapps actor is using internal DOMApplicationRegistry functions and is completely skipping the actual local installation. So in the actor we're moving the package in the wrong directory. The "app" protocol, instead, is trying to open the package in the directory where it's supposed to be (the local installation directory, that the webapps actor doesn't create).

> I easily agree to disable stuff on android, but I'd really like to keep
> stuff enabled on desktop,
> as having apps working on firefox is really important for gaia. In firefox
> itself, not in webappsrt.
> Many gaia contributors are using Firefox itself to run gaia, today we are
> using an addon to make it work nicely, but we would like to reach the goal
> where gaia would run (almost) natively on firefox.
> So that we should do our best to keep stuff working on dekstop and avoid
> disabling/break features that are used to work.

I don't think we want to support running packaged apps in Firefox without the Webapp Runtime. If we want that, we should consider Firefox as yet another platform (than Windows, Linux and OS X), because we should handle things in a really different way.

Comment 22

[Alexandre Poirot [:ochameau]]

(In reply to Marco Castelluccio [:marco] from comment #21)
> (In reply to Alexandre Poirot (:ochameau) from comment #20)
> > Also, we shouldn't stop testing the actor on desktop, most of its features
> > are used to work on desktop.
> 
> I think most of the actor features don't work on Desktop because they're
> assuming the same environment as b2g.

It does partially work, it works fine regarding the app registry.
I agree on the fact that it doesn't work as expected for an end user,
but that doesn't mean we should stop testing what already works.

> 
> > It ensures that the app registry do works and that's better to have unit
> > test that partially cover a given feature than no tests.
> 
> We already have tests for the app registry. In dom/apps/tests and in
> dom/tests/mochitest/webapps.

Last time I checked some tests were also disabled (by mistake or not) over time.
We have limited test coverage over apps API, we are starting to get advanced test via app actor, that's something we should leverage. I just found a general app regression thanks to this test, see bug 916604.

> 
> > I haven't figured out why, but this changeset do break the test on desktop.
> > The test against getIconAsDataURL is no longer able to retrieve the icon.
> > The request app://actor-test/icon-128.png no longer works.
> 
> The webapps actor is using internal DOMApplicationRegistry functions and is
> completely skipping the actual local installation. So in the actor we're
> moving the package in the wrong directory. The "app" protocol, instead, is
> trying to open the package in the directory where it's supposed to be (the
> local installation directory, that the webapps actor doesn't create).

Can we fix the actor instead of disabling it?

> 
> > I easily agree to disable stuff on android, but I'd really like to keep
> > stuff enabled on desktop,
> > as having apps working on firefox is really important for gaia. In firefox
> > itself, not in webappsrt.
> > Many gaia contributors are using Firefox itself to run gaia, today we are
> > using an addon to make it work nicely, but we would like to reach the goal
> > where gaia would run (almost) natively on firefox.
> > So that we should do our best to keep stuff working on dekstop and avoid
> > disabling/break features that are used to work.
> 
> I don't think we want to support running packaged apps in Firefox without
> the Webapp Runtime. If we want that, we should consider Firefox as yet
> another platform (than Windows, Linux and OS X), because we should handle
> things in a really different way.

I agree, that's another project.

Comment 23

[Marco Castelluccio [:marco]]

(In reply to Alexandre Poirot (:ochameau) from comment #22)
> It does partially work, it works fine regarding the app registry.
> I agree on the fact that it doesn't work as expected for an end user,
> but that doesn't mean we should stop testing what already works.

I don't agree, we're testing something with wrong assumptions in mind. The webapps actor shouldn't skip the local installation. After bug 910473 it would probably be simpler for you not to skip the local installation and so we'll be able to reenable the actor and the test.

> > 
> > > I haven't figured out why, but this changeset do break the test on desktop.
> > > The test against getIconAsDataURL is no longer able to retrieve the icon.
> > > The request app://actor-test/icon-128.png no longer works.
> > 
> > The webapps actor is using internal DOMApplicationRegistry functions and is
> > completely skipping the actual local installation. So in the actor we're
> > moving the package in the wrong directory. The "app" protocol, instead, is
> > trying to open the package in the directory where it's supposed to be (the
> > local installation directory, that the webapps actor doesn't create).
> 
> Can we fix the actor instead of disabling it?

The problem is that the actor should never have been enabled, because it has never worked as expected.
It's working as if it were in a b2g environment, but it isn't.
We can fix it, but while it doesn't work it should be disabled.

Comment 24

[[:fabrice] Fabrice Desré]

Disabling the actor on desktop is probably ok for now, but not disabling the tests on b2g.

Comment 25

[Alexandre Poirot [:ochameau]]

Please trust me when I say disabling this test on desktop is a mistake. Running on b2g desktop and in firefox desktop, even in this weird mode where we bypass the webappsrt, cover a lot of the platform codepath. When we run gaia on firefox, we run apps on firefox without webappsrt. Gaia unit tests are run on firefox.
I'm really sad to see existing codepath being broken unnecessarily. And I'm frustated to see unit test being disabled whereas we keep saying we need more tests for b2g related stuff!

Fabrice, you just replied to Jonas it was hard to run b2g tests, this xpcshell test covers a lot of b2g specific codepath **and** is extremelly easy to run on dekstop. `./mach xpcshell-test ...` without requiring an emulator build.

I'm fine disabling stuff on android as we don't invest much on it, but that's different for desktop as gaia depends on this support and b2g desktop [and device] is extremelly close to this desktop mode.

Btw, this patch ended up breaking b2g desktop on windows and linux for the same reason it broke the desktop support...

Comment 26

[Marco Castelluccio [:marco]]

(In reply to Alexandre Poirot (:ochameau) from comment #25)
> Please trust me when I say disabling this test on desktop is a mistake.
> Running on b2g desktop and in firefox desktop, even in this weird mode where
> we bypass the webappsrt, cover a lot of the platform codepath. When we run
> gaia on firefox, we run apps on firefox without webappsrt. Gaia unit tests
> are run on firefox.
> I'm really sad to see existing codepath being broken unnecessarily. And I'm
> frustated to see unit test being disabled whereas we keep saying we need
> more tests for b2g related stuff!
> 
> Fabrice, you just replied to Jonas it was hard to run b2g tests, this
> xpcshell test covers a lot of b2g specific codepath **and** is extremelly
> easy to run on dekstop. `./mach xpcshell-test ...` without requiring an
> emulator build.
> 
> I'm fine disabling stuff on android as we don't invest much on it, but
> that's different for desktop as gaia depends on this support and b2g desktop
> [and device] is extremelly close to this desktop mode.

I think we can't bypass the webapp runtime. Apps on desktop should always run in the webapp runtime.
The test is written with the bypassing idea in mind, so we can't do anything else than disabling it until we properly support the webapps actor on desktop.

This test is a unit test for the webapps actor, it happens to test also other things but its purpose is unit-testing the webapps actor. We should write other b2g tests and not rely on a unit test for a feature on a platform where the feature itself isn't supported.