889123 - Gaia system app no longer loads in Firefox Nightly

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Kevin Grandon :kgrandon]

In gaia, we have a build script that installs applications into a firefox profile. We then launch this firefox profile inside of Nightly for rapid development. The gaia system app is no longer loading in Firefox Nightly.

I believe these new ifdefs seem to be the cause: http://hg.mozilla.org/mozilla-central/rev/d1fd89491708

This was landed in bug 860782. We may want to look at removing the ifdefs there in favor of preferences.

One alternative could be to bring back the old manifest permissions installer into gaia. That might be a quick bandaid fix for it.

Comment 1

[Kevin Grandon :kgrandon]

It seems like the permissions are not being registered, causing some messages like this to appear in the logs: 

Std out:
No permission to use the keyboard API for http://system.gaiamobile.org:8080
No permission to use the keyboard API for http://keyboard.gaiamobile.org:8080

JS console:
[16:00:40.992] NO SETTINGS PERMISSION FOR: http://keyboard.gaiamobile.org:8080
 @ resource://gre/components/SettingsManager.js:394

Comment 2

[Wesley Johnston (:wesj)]

gaia can setup these permissions in the profile its creating. Or we could change this ifdef, but without it, desktop firefox is incorrectly setting permissions in the desktop firefox profile when apps are installed.

Comment 3

[Alexandre Poirot [:ochameau]]

TBH, I'd prefer using a pref (or any runtime way of doing) instead of an hardcoded #ifdef in order:
- to avoid complexifying gaia's build system,
- ease working on packaged apps support in firefox desktop!

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Alexandre Poirot (:ochameau) from comment #3)
> TBH, I'd prefer using a pref (or any runtime way of doing) instead of an
> hardcoded #ifdef in order:
> - to avoid complexifying gaia's build system,
> - ease working on packaged apps support in firefox desktop!

I didn't want the #ifdef either when I reviewed this code, but we have no other choice until we get a proper glue between the apps backend and our various frontends. And even then, here we are in a situation where we except a different behavior for gaia devs and usual firefox users.

Using a pref may be ok if is set only in gaia-dev profiles, with people understanding that they will loose the capability to install apps properly in this setup.

I guess the other option is to have the gaia add-on to the permissions installation itself.

Comment 5

[Kevin Grandon :kgrandon]

Attachment: Patch part 1 - Implement gecko preference

Comment 6

[Kevin Grandon :kgrandon]

Attachment: Patch part 2 - Set gaia preference during desktop build

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 770363 [details] [diff] [review]
Patch part 1 - Implement gecko preference

>diff -r 4ffb23062b3b dom/apps/src/Webapps.jsm

>+let cachedInstalledAppsPref = null;
>+function supportInstalledApps() {
>+  if (cachedInstalledAppsPref === null) {
>+    cachedInstalledAppsPref = Services.prefs.getBoolPref("dom.webapps.installedApps");
>+  }
>+  return cachedInstalledAppsPref;
>+}

drive-by:
* the prefs service already caches the value in a hash table. you probably don't need the extra cache here.
* supportsInstalledApps is somewhat vague about it's intent. makes it sounds like i want to set it to true to install any apps, which is not true.

Comment 8

[Kevin Grandon :kgrandon]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Comment on attachment 770363 [details] [diff] [review]
> drive-by:
> * the prefs service already caches the value in a hash table. you probably
> don't need the extra cache here.

Gotcha - I was following the other example in this same file. If this is the case we can remove the in-file caching for both preferences. 

> * supportsInstalledApps is somewhat vague about it's intent. makes it sounds
> like i want to set it to true to install any apps, which is not true.

Any idea for a better name? Would 'supportsPreInstalledApps' be better?

Comment 9

[Kevin Grandon :kgrandon]

Attachment: Patch part 1 - Implement gecko preference

Renames pref 'installedApps' -> 'preInstalledApps'.

Comment 10

[Kevin Grandon :kgrandon]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=8a3753e3a5a4

Comment 11

[Kevin Grandon :kgrandon]

Comment on attachment 770765 [details] [diff] [review]
Patch part 1 - Implement gecko preference

Fabrice, would you be able to review these?

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Given what the preference really does, maybe "dom.webapps.installInCurrentProfile" or "dom.webapps.useCurrentProfile" ?

Comment 13

[[:fabrice] Fabrice Desré]

Comment on attachment 770765 [details] [diff] [review]
Patch part 1 - Implement gecko preference

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

I also agree with mfinkle that installInCurrentProfile or useCurrentProfile are more descriptive names. Maybe even installPermissionsInCurrentProfile ?

::: dom/apps/src/Webapps.jsm
@@ +37,5 @@
> +  if (cachedPreInstalledAppsPref === null) {
> +    cachedPreInstalledAppsPref = Services.prefs.getBoolPref("dom.webapps.preInstalledApps");
> +  }
> +  return cachedPreInstalledAppsPref;
> +}

It looks like you didn't change to just return the Services.prefs.getBoolPref(). Why?

@@ +43,3 @@
>  let cachedSysMsgPref = null;
>  function supportSystemMessages() {
>    if (cachedSysMsgPref === null) {

Same here.

Comment 14

[Kevin Grandon :kgrandon]

Attachment: Patch part 1 - Implement gecko preference

Hi Fabrice. Here's the updated patch using the 'useCurrentProfile' pref name, as well as removing the cache layer. This has been pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=74e17ff248ec

Please let me know if you think there's anything else that could be better. Thanks!

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 771064 [details] [diff] [review]
Patch part 1 - Implement gecko preference

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

r=me with nits addressed.

::: b2g/app/b2g.js
@@ +533,4 @@
>  // Enable device storage
>  pref("device.storage.enabled", true);
>  
> +// Enable installed manifests

What about "Enable pre-installed applications." ?

::: modules/libpref/src/init/all.js
@@ +778,4 @@
>  // Enables system messages and activities
>  pref("dom.sysmsg.enabled", false);
>  
> +// Enable installed manifests

Same as in b2g.js

Comment 16

[Kevin Grandon :kgrandon]

Attachment: Patch part 1 - Implement gecko preference

Thanks for the review fabrice! Nits have been addressed in this patch.

Comment 17

[Kevin Grandon :kgrandon]

Need a checkin for the gecko patch here: https://bug889123.bugzilla.mozilla.org/attachment.cgi?id=771093

Thank you!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/190651bea394

To make life easier for those checking in on your behalf, please make sure that you have Mercurial configured to generate patches in a checkin-friendly format. Also, please make sure the attached patch has an usable commit message. Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/190651bea394

Comment 20

[Kevin Grandon :kgrandon]

Gaia patch has landed in master: https://github.com/mozilla-b2g/gaia/commit/5759318c0b3d18e20ba957cdb86bccd096476864