Need mac implementation of XRE_InstallXULApplication

RESOLVED FIXED

Status

Toolkit Graveyard
XULRunner
RESOLVED FIXED
13 years ago
2 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

({fixed1.8})

Details

Attachments

(3 attachments)

(Assignee)

Description

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

Comment 1

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

Comment 2

13 years ago
Created attachment 189049 [details]
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.
(Assignee)

Updated

13 years ago
Blocks: 299986
(Assignee)

Comment 3

13 years ago
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

13 years ago
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

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

Comment 6

13 years ago
> 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.
Depends on: 299992
(Assignee)

Comment 7

13 years ago
*** Bug 304228 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

13 years ago
Assignee: toddf → benjamin
(Assignee)

Updated

13 years ago
Depends on: 310590
(Assignee)

Comment 8

13 years ago
Created attachment 198406 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Attachment #198406 - Flags: first-review?(bugspam)
(Assignee)

Updated

13 years ago
Attachment #198406 - Flags: first-review?(bugspam) → first-review?(robert.bugzilla)
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.
Attachment #198406 - Flags: first-review?(robert.bugzilla) → first-review+
(Assignee)

Comment 10

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

Updated

13 years ago
Depends on: 311346
(Assignee)

Comment 11

13 years ago
Created attachment 198701 [details] [diff] [review]
Add -install-app command-line flag, and fix mac bustages, rev. 1
Attachment #198701 - Flags: first-review?(robert.bugzilla)
(Assignee)

Updated

13 years ago
Blocks: 302101
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.
Attachment #198701 - Flags: first-review?(robert.bugzilla) → first-review+
(Assignee)

Comment 13

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

Comment 15

13 years ago
> +  { <-- 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.
Sorry about that... I should have noticed the call to NS_ShutdownXPCOM and put 2
and 2 together. :/
(Assignee)

Comment 17

13 years ago
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

13 years ago
Fixed on 1.8 branch as well.
Keywords: fixed1.8
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.