300139 - Need mac implementation of XRE_InstallXULApplication

Status: RESOLVED FIXED

(Toolkit Graveyard :: XULRunner, defect)

Description

[Benjamin Smedberg]

The function prototype is

/**
 * Installs a XUL application from a directory or a ZIP file.
 * @param aXULApp A file pointing to a directory or a zipfile.
 * @param aInstallDirectory The directory in which the app should be installed.
 *                          This would be "Program Files\Application Name" or
 *                          "/Applications/AppName.app".
 */
nsresult
XRE_InstallXULApplication(nsIFile* aXULApp, nsIFile* aInstallDirectory);

This code should exist in toolkit/xre (the function declaration should go
nsXULAppAPI.h

The function needs also to install a (yet unwritten) xulrunner stub executable
in the final app bundle (this will the the executable file for the bundle), see
bug 299991.

I'll file separate bugs for the windows/unix implementations.

Comment 1

[Benjamin Smedberg]

On further reflection, I don't see why this needs to be an exported symbol
instead of an interface, so I'm thinking that it will live on

interface nsIXULAppInstallService {
  void installApplication(in nsIFile aApplicationFile, in nsIFile aDirectory);
};

But if you write the code, I can make sure it ends up living in the right place.

Comment 2

[Benjamin Smedberg]

Attachment: Filelist for application bundle

This is an application bundle I hand-created for testing the stub executable.
You can see that the entire xulapp (application.ini and all support files) are
located at App.app/Contents/Resources. It is also possible (though you don't
need to support this in the initial revision of your install code) to stick the
XUL.framework into App.app/Contents/Frameworks/XUL.framework and have the
entire application self-contained.

Comment 3

[Benjamin Smedberg]

BTW, I think I'd be perfectly fine with an implementation of this interface
written in JS intead of C++, if that's easier (it sure sounds easier to me).

Comment 4

[Todd A. Fisher]

here's a link to my initial work, still needs testing. 
http://severna.homeip.net/mactest/

my plan is to write some js to test this and then create a patch to post here

Comment 5

[Todd A. Fisher]

Some questions:

Is there code for parsing application.ini files, that can be reused?

Should the following be symbolic links
  App.app/Contents/MacOS/xulrunner
  App.app/Contents/MacOS/xulrunner-bin
Should the symtem call symlink be used?  If so, is there an xpcom interface that
exposes this call to javascript?

Comment 6

[Benjamin Smedberg]

> Is there code for parsing application.ini files, that can be reused?

Yes, see the iniparser patch in bug 299992.

> Should the following be symbolic links
>   App.app/Contents/MacOS/xulrunner

no. This should be copied.

>   App.app/Contents/MacOS/xulrunner-bin

There should be no xulrunner-bin in the app bundle.

Comment 7

[Benjamin Smedberg]

*** Bug 304228 has been marked as a duplicate of this bug. ***

Comment 8

[Benjamin Smedberg]

Attachment: nsIXULAppInstall service (cross-platform), rev. 1

This is actually tested on linux, I'm still building/testing mac/windows but it
should be ready for a review. This is pretty rough code ATM and could use a
whole lot of error handling, but it does seem to work when it works, and I'd
like to get something in now which can create runnable mac application bundles.

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 198406 [details] [diff] [review]
nsIXULAppInstall service (cross-platform), rev. 1

>+   * Install a XUL application into a form that can be run by the native
>+   * operating system.
nit: 'into a form' is ambiguous.

>+   * @param aAppFile   Directory or a zip file containing a 
>+   *                   XULRunner package (application.ini file).
nit: aAppFile could be confused with the application.ini file. Perhaps "with
the required application.ini file" instead of "(application.ini file)'?

>+const nsIFile          = Components.interfaces.nsIFile;
>+const nsIINIParser     = Components.interfaces.nsIINIParser;
>+const nsIINIParserFactory = Components.interfaces.nsIINIParserFactory;
>+const nsILocalFile     = Components.interfaces.nsILocalFile;
>+const nsISupports      = Components.interfaces.nsISupports;
>+const nsIXULAppInstall = Components.interfaces.nsIXULAppInstall;
>+const nsIZipEntry      = Components.interfaces.nsIZipEntry;
>+const nsIZipReader     = Components.interfaces.nsIZipReader;
nit: line these up.

>+function copy_recurse(aSource, aDest) {
>+  dump("copy_recurse(" + aSource.path + ", " + aDest.path + ")\n");
nit: do you want to keep this?

>+        this.mINIParser = createINIParser(f);
>+      }
>+      catch (e) {
>+        try {
>+          f.remove();
nit: it would be nice to remove the application.ini file extracted to the temp
for the zipExtractor success case though for now this is ok. Still lots of
other things to do here and this can be fixed later.

>+    var isdir = aZipEntry
isDir isn't used.

>+  canUnload : function mod_unload(aCompMgr) {
>+    return false;
>+  }
Unless I'm mistaken canUnload is ignored for JS components and the usual
convention is to return true.

r=me with the nits picked and an explanation regarding canUnload.

Comment 10

[Benjamin Smedberg]

> Unless I'm mistaken canUnload is ignored for JS components and the usual
> convention is to return true.

canUnload is actually ignored for all components (at the current time we don't
do component unloading at all). I'm planning on changing this in the 1.9
timeframe, but I've changed this to "true" for the moment until I can decide
exactly how xpconnect ought to handle this.

Comment 11

[Benjamin Smedberg]

Attachment: Add -install-app command-line flag, and fix mac bustages, rev. 1

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 198701 [details] [diff] [review]
Add -install-app command-line flag, and fix mac bustages, rev. 1

+	    "  --install-app <application> [<destination>] [<directoryname>]\n"
+	    "				  Install a XUL application.\n"
The optional args are actually [<destination> [<directoryname>]]

+
+  {
+    nsCOMPtr<nsIXULAppInstall> install
+      (do_GetService("@mozilla.org/xulrunner/app-install-service;1"));
+    if (!install) {
+      rv = NS_ERROR_FAILURE;
+    }
+    else {
+      rv = install->InstallApplication(appLocation, installTo, leafName);
+    }
+  }
extra brackets

+      char *leafName = nsnull;
+      if (argc > 4) {
+	 leafName = argv[5];
You want argv[4] here

-      aDirectory = getDirectoryKey("Progs");
+      aDirectory = getDirectoryKey("ProgF");
This is surprising to discover :)

+    var getInfoString = "";
+    if (vendor) {
+      getInfoString = vendor + " ";
+    }
+    getInfoString += appName + " " + version;
hmmm... the var describes CFBundleGetInfoString though a get prefix seems wrong
here. infoString may be more appropriate - I'm fine either way.

+  char *lastSlash = strrchr(greDir, '/');
+  if (lastSlash) {
+    *lastSlash = '\0';
+  }
Not sure what / why this is for?

r=me with these items addressed and an explanation re: lastSlash or its
removal.

BTW: copy_recurse doesn't create empty directories for the flat file structure
case.

Comment 13

[Benjamin Smedberg]

> extra brackets

Local style uses extra brackets, I'd like to keep them.

> -      aDirectory = getDirectoryKey("Progs");
> +      aDirectory = getDirectoryKey("ProgF");
> This is surprising to discover :)

See bug 311346.

> +  char *lastSlash = strrchr(greDir, '/');
> +  if (lastSlash) {
> +    *lastSlash = '\0';
> +  }
> Not sure what / why this is for?

GRE_GetGREPathWithProperties returns the XPCOM path, and we want the directory.

> BTW: copy_recurse doesn't create empty directories for the flat file structure
> case.

Hrm, should it?

Comment 14

[Robert Strong (they/them - no direct email)]

(In reply to comment #13)
> > extra brackets
> 
> Local style uses extra brackets, I'd like to keep them.
For if statements etc. but for this?

+  rv = NS_InitXPCOM2(nsnull, aXULRunnerDir, nsnull);
+  if (NS_FAILED(rv))
+    return 3;
+
+  { <-- this one
+    nsCOMPtr<nsIXULAppInstall> install
+      (do_GetService("@mozilla.org/xulrunner/app-install-service;1"));
+    if (!install) {
+      rv = NS_ERROR_FAILURE;
+    }
+    else {
+      rv = install->InstallApplication(appLocation, installTo, leafName);
+    }
+  } <-- and this one

> > BTW: copy_recurse doesn't create empty directories for the flat file structure
> > case.
> 
> Hrm, should it?
It may be important to some and it would be a good thing to be consistent with
the two methods but I don't think it is necessary at this time or possibly
ever... just a heads up.

Comment 15

[Benjamin Smedberg]

> +  { <-- this one
> +    nsCOMPtr<nsIXULAppInstall> install
> +      (do_GetService("@mozilla.org/xulrunner/app-install-service;1"));

> +  } <-- and this one

That is a scoping bracket, so that the nsCOMPtr goes out of scope before we shut
down xpcom.

Comment 16

[Robert Strong (they/them - no direct email)]

Sorry about that... I should have noticed the call to NS_ShutdownXPCOM and put 2
and 2 together. :/

Comment 17

[Benjamin Smedberg]

Fixed on trunk.

Comment 18

[Benjamin Smedberg]

Fixed on 1.8 branch as well.