Gaia system app no longer loads in Firefox Nightly

RESOLVED FIXED in mozilla25

Status

()

Core
DOM: Apps
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kgrandon, Assigned: kgrandon)

Tracking

({regression})

Trunk
mozilla25
x86
Mac OS X
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: c= p=3)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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.

Updated

4 years ago
Blocks: 860782
Keywords: regression
(Assignee)

Comment 1

4 years ago
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
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.
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!
(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.
(Assignee)

Updated

4 years ago
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
(Assignee)

Comment 5

4 years ago
Created attachment 770363 [details] [diff] [review]
Patch part 1 - Implement gecko preference
(Assignee)

Comment 6

4 years ago
Created attachment 770364 [details]
Patch part 2 - Set gaia preference during desktop build
(Assignee)

Updated

4 years ago
Attachment #770364 - Attachment description: Patch part 1 - Set gaia preference during desktop build → Patch part 2 - Set gaia preference during desktop build
(Assignee)

Updated

4 years ago
Attachment #770363 - Attachment is patch: true
Attachment #770363 - Attachment mime type: text/x-patch → text/plain
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.
(Assignee)

Comment 8

4 years ago
(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?
(Assignee)

Comment 9

4 years ago
Created attachment 770765 [details] [diff] [review]
Patch part 1 - Implement gecko preference

Renames pref 'installedApps' -> 'preInstalledApps'.
Attachment #770363 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #770765 - Attachment is patch: true
Attachment #770765 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Comment 10

4 years ago
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=8a3753e3a5a4
(Assignee)

Comment 11

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

Fabrice, would you be able to review these?
Attachment #770765 - Flags: review?(fabrice)
(Assignee)

Updated

4 years ago
Attachment #770364 - Flags: review?(fabrice)
Given what the preference really does, maybe "dom.webapps.installInCurrentProfile" or "dom.webapps.useCurrentProfile" ?

Updated

4 years ago
Component: Gaia::System → DOM: Apps
Product: Boot2Gecko → Core
Version: unspecified → Trunk
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.
Attachment #770765 - Flags: review?(fabrice) → review-
(Assignee)

Comment 14

4 years ago
Created attachment 771064 [details] [diff] [review]
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!
Attachment #770765 - Attachment is obsolete: true
Attachment #771064 - Flags: review?(fabrice)
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
Attachment #771064 - Flags: review?(fabrice) → review+
(Assignee)

Comment 16

4 years ago
Created attachment 771093 [details] [diff] [review]
Patch part 1 - Implement gecko preference

Thanks for the review fabrice! Nits have been addressed in this patch.
Attachment #771064 - Attachment is obsolete: true
Attachment #771093 - Flags: review+
Attachment #770364 - Flags: review?(fabrice) → review+
(Assignee)

Comment 17

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

Thank you!
Keywords: checkin-needed
Attachment #771093 - Attachment is patch: true
Attachment #771093 - Attachment mime type: text/x-patch → text/plain
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/190651bea394
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Comment 20

4 years ago
Gaia patch has landed in master: https://github.com/mozilla-b2g/gaia/commit/5759318c0b3d18e20ba957cdb86bccd096476864
You need to log in before you can comment on or make changes to this bug.