Shian-Yow Wu [:swu] Assignee
	Description • 10 years ago

When doing mochitest on Linux, install a packaged app and launch it in an iframe, the app path returned by getInstallPath in WebappOSUtils.jsm is wrong.

Since MOZ_B2G is not defined in such condition, we didn't get expected path as returned in MOZ_B2G condition.

WebappOSUtils.jsm:
150   getInstallPath: function(aApp) {
151 #ifdef MOZ_B2G
152     // All b2g builds
153     return aApp.basePath + "/" + aApp.id;
154 
155 #elifdef MOZ_FENNEC
156    // All fennec
157     return aApp.basePath + "/" + aApp.id;
158 
159 #elifdef MOZ_PHOENIX
160    // Firefox
161 
162 #ifdef XP_WIN
163     let execFile = this.getLaunchTarget(aApp);
164     if (!execFile) {
165       return null;
166     }
167 
168     return execFile.parent.path;
169 #elifdef XP_MACOSX
170     let [ bundleID, path ] = this.getLaunchTarget(aApp);
171     return path;
172 #elifdef XP_UNIX
173     let execFile = this.getLaunchTarget(aApp);
174     if (!execFile) {
175       return null;
176     }
177 
178     return execFile.parent.path;
179 #endif

	Shian-Yow Wu [:swu] Assignee
	Comment 1 • 10 years ago

Fabrice and Marco, do you have any comments?

	Marco Castelluccio [:marco]
	Comment 2 • 10 years ago

If you're using SpecialPowers.autoConfirmInstall, the pref that is set will make Firefox skip the local installation. This is why the package isn't in the directory returned by getInstallPath.
You could mock WebappOSUtils (see the patch in bug 997886 for an example).

I think to avoid confusion we ought to (but this is material for another bug):
1) Make autoConfirmInstall only do what it's supposed to do, that is to skip the confirmation prompt
2) Create a new function that only skips the local installation (it's extremely easy, because we have a pref to do that).
3) If many tests that load packaged apps in an iframe are going to be written, we should provide a function to simplify the mocking (so that if something changes in the future we'll just need to modify one function and not many tests).

	Shian-Yow Wu [:swu] Assignee
	Comment 3 • 10 years ago

Thank you for the moch skill!

	Shian-Yow Wu [:swu] Assignee
	Comment 4 • 10 years ago

When launching packaged app with "remote=true", the mock skill doesn't work.  Mocking the object in parent process leaves the object in child process intact.  According to Marco, bug 910473 should help because only the parent will call |WebappOSUtils.getInstallPath| and will hand the path to the child processes.

	Shian-Yow Wu [:swu] Assignee
	Comment 5 • 10 years ago

(In reply to Marco Castelluccio [:marco] from comment #2)
> If you're using SpecialPowers.autoConfirmInstall, the pref that is set will
> make Firefox skip the local installation. This is why the package isn't in
> the directory returned by getInstallPath.
> You could mock WebappOSUtils (see the patch in bug 997886 for an example).

If we return the desired path when "dom.mozApps.auto_confirm_install" is true,
then we don't need the mock.  Do you think this is feasible?

  getInstallPath: function(aApp) {
    let prefName = "dom.mozApps.auto_confirm_install";
    if (Services.prefs.prefHasUserValue(prefName) &&
        Services.prefs.getBoolPref(prefName)) {
      return aApp.basePath + "/" + aApp.id;
    }   
    ......
  },

	Marco Castelluccio [:marco]
	Comment 6 • 10 years ago

It may be a temporary workaround. I've only one doubt: since
this function is being called every time an app is launched,
reading the prefs each time may slow down app startup (I'm
not sure, but I think every time you use the prefs service
you're reading the prefs file).

Other temporary workarounds may be:
- caching the pref so that we only read it once;
- a DOMApplicationRegistry attribute, like we've done with
_allAppsLaunchable.

I think they're both uglier than a mock, but they look OK to
me as temporary workarounds.

	[:fabrice] Fabrice Desré
	Comment 7 • 10 years ago

Prefs are cached in memory (which is why we sometimes need to flush the pref file).

	Shian-Yow Wu [:swu] Assignee
	Comment 8 • 10 years ago

Per comment 7, slowing down app start by reading pref each time should not be a concern, so I keep it simple by reading pref as the current known temporary solution before bug 914073.
Could you help to review it?

	Marco Castelluccio [:marco]
	Comment 9 • 10 years ago

Comment on attachment 8444327 [details] [diff] [review]
Patch: Temporary workaround to get correct path.

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

Nit: I'd put this logic in AppsUtils.jsm.

	Shian-Yow Wu [:swu] Assignee
	Comment 10 • 10 years ago

(In reply to Marco Castelluccio [:marco] from comment #9)
> Comment on attachment 8444327 [details] [diff] [review]
> Patch: Temporary workaround to get correct path.
> 
> Review of attachment 8444327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nit: I'd put this logic in AppsUtils.jsm.

Thanks for your review!

There are two places calling WebappOSUtils.getPackagePath(), one at
AppsUtils.jsm and the other at Webapps.jsm.  If we move the logic to
upper layer, suppose we need to modify two places, is that right?

	Marco Castelluccio [:marco]
	Comment 11 • 10 years ago

We don't need to override the other use in Webapps.jsm because
|addInstalledApp| is a function only called by the desktop webapp
runtime (the function will only be called if the app has actually
been installed and executed).

	Shian-Yow Wu [:swu] Assignee
	Comment 12 • 10 years ago

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=0062a76f1d6c

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc98dff119e8

	Carsten Book [:Tomcat]
	Comment 13 • 10 years ago

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