Closed Bug 889123 Opened 11 years ago Closed 11 years ago

Gaia system app no longer loads in Firefox Nightly

Categories

(Core Graveyard :: DOM: Apps, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla25

People

(Reporter: kgrandon, Assigned: kgrandon)

References

Details

(Keywords: regression, Whiteboard: c= p=3)

Attachments

(2 files, 3 obsolete files)

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.
Blocks: 860782
Keywords: regression
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: nobody → kgrandon
Status: NEW → ASSIGNED
Attachment #770364 - Attachment description: Patch part 1 - Set gaia preference during desktop build → Patch part 2 - Set gaia preference during desktop build
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.
(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?
Renames pref 'installedApps' -> 'preInstalledApps'.
Attachment #770363 - Attachment is obsolete: true
Attachment #770765 - Attachment is patch: true
Attachment #770765 - Attachment mime type: text/x-patch → text/plain
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)
Attachment #770364 - Flags: review?(fabrice)
Given what the preference really does, maybe "dom.webapps.installInCurrentProfile" or "dom.webapps.useCurrentProfile" ?
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-
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+
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+
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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: