Last Comment Bug 889123 - Gaia system app no longer loads in Firefox Nightly
: Gaia system app no longer loads in Firefox Nightly
Status: RESOLVED FIXED
c= p=3
: regression
Product: Core
Classification: Components
Component: DOM: Apps (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla25
Assigned To: Kevin Grandon :kgrandon
:
: [:fabrice] Fabrice Desré
Mentors:
Depends on:
Blocks: 860782
  Show dependency treegraph
 
Reported: 2013-07-01 15:46 PDT by Kevin Grandon :kgrandon
Modified: 2013-07-08 12:09 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch part 1 - Implement gecko preference (6.34 KB, patch)
2013-07-02 12:47 PDT, Kevin Grandon :kgrandon
no flags Details | Diff | Splinter Review
Patch part 2 - Set gaia preference during desktop build (220 bytes, text/html)
2013-07-02 12:50 PDT, Kevin Grandon :kgrandon
fabrice: review+
Details
Patch part 1 - Implement gecko preference (6.37 KB, patch)
2013-07-03 06:31 PDT, Kevin Grandon :kgrandon
fabrice: review-
Details | Diff | Splinter Review
Patch part 1 - Implement gecko preference (6.47 KB, patch)
2013-07-03 15:23 PDT, Kevin Grandon :kgrandon
fabrice: review+
Details | Diff | Splinter Review
Patch part 1 - Implement gecko preference (6.49 KB, patch)
2013-07-03 16:16 PDT, Kevin Grandon :kgrandon
kevin+bugzilla: review+
Details | Diff | Splinter Review

Description Kevin Grandon :kgrandon 2013-07-01 15:46:02 PDT
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 2013-07-01 16:02:52 PDT
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) 2013-07-01 20:43:04 PDT
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] 2013-07-02 09:27:51 PDT
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é 2013-07-02 09:41:23 PDT
(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 2013-07-02 12:47:08 PDT
Created attachment 770363 [details] [diff] [review]
Patch part 1 - Implement gecko preference
Comment 6 Kevin Grandon :kgrandon 2013-07-02 12:50:19 PDT
Created attachment 770364 [details]
Patch part 2 - Set gaia preference during desktop build
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2013-07-02 13:08:09 PDT
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 2013-07-02 13:59:58 PDT
(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 2013-07-03 06:31:06 PDT
Created attachment 770765 [details] [diff] [review]
Patch part 1 - Implement gecko preference

Renames pref 'installedApps' -> 'preInstalledApps'.
Comment 10 Kevin Grandon :kgrandon 2013-07-03 09:03:12 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=8a3753e3a5a4
Comment 11 Kevin Grandon :kgrandon 2013-07-03 09:03:42 PDT
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?) 2013-07-03 09:06:35 PDT
Given what the preference really does, maybe "dom.webapps.installInCurrentProfile" or "dom.webapps.useCurrentProfile" ?
Comment 13 [:fabrice] Fabrice Desré 2013-07-03 14:47:37 PDT
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 2013-07-03 15:23:37 PDT
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!
Comment 15 [:fabrice] Fabrice Desré 2013-07-03 15:53:16 PDT
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 2013-07-03 16:16:02 PDT
Created attachment 771093 [details] [diff] [review]
Patch part 1 - Implement gecko preference

Thanks for the review fabrice! Nits have been addressed in this patch.
Comment 17 Kevin Grandon :kgrandon 2013-07-03 16:21:09 PDT
Need a checkin for the gecko patch here: https://bug889123.bugzilla.mozilla.org/attachment.cgi?id=771093

Thank you!
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-07-07 13:44:35 PDT
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] 2013-07-07 18:31:00 PDT
https://hg.mozilla.org/mozilla-central/rev/190651bea394
Comment 20 Kevin Grandon :kgrandon 2013-07-08 11:09:34 PDT
Gaia patch has landed in master: https://github.com/mozilla-b2g/gaia/commit/5759318c0b3d18e20ba957cdb86bccd096476864

Note You need to log in before you can comment on or make changes to this bug.