Closed
Bug 300139
Opened 20 years ago
Closed 19 years ago
Need mac implementation of XRE_InstallXULApplication
Categories
(Toolkit Graveyard :: XULRunner, defect)
Toolkit Graveyard
XULRunner
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: benjamin, Assigned: benjamin)
References
Details
(Keywords: fixed1.8)
Attachments
(3 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
robert.strong.bugs
:
first-review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
robert.strong.bugs
:
first-review+
|
Details | Diff | Splinter Review |
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•20 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•20 years ago
|
||
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 | ||
Comment 3•20 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•20 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•19 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•19 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•19 years ago
|
||
*** Bug 304228 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•19 years ago
|
Assignee: toddf → benjamin
Assignee | ||
Comment 8•19 years ago
|
||
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•19 years ago
|
Attachment #198406 -
Flags: first-review?(bugspam)
Assignee | ||
Updated•19 years ago
|
Attachment #198406 -
Flags: first-review?(bugspam) → first-review?(robert.bugzilla)
Comment 9•19 years ago
|
||
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•19 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 | ||
Comment 11•19 years ago
|
||
Attachment #198701 -
Flags: first-review?(robert.bugzilla)
Assignee | ||
Updated•19 years ago
|
Blocks: xulrunner-1.8
Comment 12•19 years ago
|
||
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•19 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?
Comment 14•19 years ago
|
||
(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•19 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.
Comment 16•19 years ago
|
||
Sorry about that... I should have noticed the call to NS_ShutdownXPCOM and put 2
and 2 together. :/
Assignee | ||
Comment 17•19 years ago
|
||
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•