Closed Bug 257162 Opened 20 years ago Closed 19 years ago

Enable standalone XUL applications via XULRunner

Categories

(Toolkit Graveyard :: XULRunner, enhancement, P1)

enhancement

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.8beta2

People

(Reporter: darin.moz, Assigned: darin.moz)

References

Details

Attachments

(1 file, 5 obsolete files)

Enable standalone XUL applications via XULRunner. I will be using this bug to track landing the XULRunner development branch on the trunk. I will be attaching patches to this bug for the various affected modules.
Attached patch v1 - chrome registry changes (obsolete) (deleted) — Splinter Review
This patch enables support for multiple "app"-level chrome directories. It adds the new directory key: NS_APP_CHROME_DIR_LIST, which corresponds to a list of alternate chrome directories. It supplements the default NS_APP_CHROME_DIR. installed-chrome.txt is read from each extra chrome directory, and added to the per-profile RDF chrome data source. This seemed better than trying to make the chrome registry aggregate a new data source for each chrome directory. This patch also includes some 'carried-away' cleanup to the chrome protocol handler.
Attached patch v1 - widget changes for XUL window icons (obsolete) (deleted) — Splinter Review
This patch changes nsIWidget::SetIcon to take a string identifier instead of a resource:// URL. This allows the widget code to search multiple locations for the icon(s) matching the given string identifier. This patch causes the widget code to search all of the application chrome directories for "icons/default/$id.$ext" We could extend this API in the future to support stock GNOME icons as well. I discussed this change with blizzard and the design is based on our discussions. nsXULWindow.cpp is the only caller of SetIcon. The widget implementations were anyways stripping the resource://chrome/ prefix, and manually talking to the directory service to find NS_APP_CHROME_DIR, so this patch just avoids the unnecessary overhead of passing a URL that is mostly ignored. NS_APP_CHROME_DIR_LIST is searched ahead of NS_APP_CHROME_DIR so that XUL apps can override builtin window icons if desired.
Attached patch v1 - toolkit and resource: changes (obsolete) (deleted) — Splinter Review
This patch includes changes to nsAppRunner.cpp to support a new command line argument, -app that can be used to specify a .moz file that is an INI file containing fields corresponding to the members of nsXREAppData. With this change, any toolkit based application will respond to a -app command line switch. Additional directories will be added to the following directory service keys: NS_APP_CHROME_DIR_LIST - $app/chrome NS_APP_PREFS_DEFAULTS_DIR_LIST - $app/defaults/pref NS_XPCOM_COMPONENT_DIR_LIST - $app/components Default preferences in $app/defaults/pref can be used to override browser.chromeURL to the appropriate default chrome URL for the app. $app is derived from the location of the .moz file. The .moz file is assumed to live in the $app directory, so the chrome, components, and defaults directories will be assumed to be found in there. This patch also introduces resource://app/, which corresponds to the directory containing the .moz file, or $app as I have been calling it here. To support this new substitution, nsResProtocolHandler::GetSubstitution is modified such that if a substitution is not known, the directory service is queried for the key "resource:$root", where $root is "app" in this case. nsXREDirProvider.cpp, which implements nsIDirectoryServiceProvider, returns $app when queried for "resource:app". While resource:// URLs are not limited to corresponding to file:// URLs, I felt like it was a natural thing to bind unknown resource: mapping to the directory service instead of introducing some additional callback mechanism. Oh, I should have said... it also turned out to be quite inconvenient to try to call nsIResProtocolHandler::SetSubstitution at the right times during startup (more than one place would have been required, had to deal with chrome registration happening during xpcom startup, etc.).
NOTE: none of these patches are specific to a XULRunner executable. In fact, with these patches alone, it is possible to use Firefox as XULRunner. XULRunner itself is really a matter of better factoring, packaging, and installation foo. To make XULRunner a reality, we need to eliminate #ifdef MOZ_PHOENIX and friends that appear in core gecko and toolkit code. We also need to decide on what is "in" the core XULRunner (i.e., how do we define the XUL runtime?).
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta
> To make XULRunner a reality, we need to eliminate #ifdef MOZ_PHOENIX and friends > that appear in core gecko and toolkit code. We also need to decide on what is > "in" the core XULRunner (i.e., how do we define the XUL runtime?). Heh, this is the hard part ;) Actually at this point, MOZ_THUNDERBIRD ifdefs cause more grief than MOZ_PHOENIX/MOZ_XUL_APP.
Attachment #157192 - Flags: review?(bsmedberg)
Attachment #157193 - Flags: review?(bsmedberg)
Attachment #157195 - Flags: review?(bsmedberg)
Comment on attachment 157193 [details] [diff] [review] v1 - widget changes for XUL window icons +++ widget/src/xpwidgets/nsBaseWidget.cpp 18 Aug 2004 00:06:32 -0000 1.126.4.1 + * Modifies aDir to point at icon file with given name and extension. + * Returns true if the icon file exists and can be read. + */ +static PRBool +ResolveIconNameHelper(nsILocalFile *aFile, there is no aDir :) ideally this comment would mention that aIconExt must include the dot
Thanks biesi!
Attachment #157193 - Flags: superreview?(blizzard)
Attachment #157193 - Flags: review?(cbiesinger)
Attachment #157193 - Flags: review?(bsmedberg)
Comment on attachment 157192 [details] [diff] [review] v1 - chrome registry changes > nsChromeProtocolHandler::GetScheme(nsACString &result) > { >- result = "chrome"; >+ result.AssignLiteral("chrome"); If you really are thinking about the aviary branch, you can't use the new *Literal methods. >+static nsresult >+GetFileAndLastModifiedTime(nsIProperties *aDirSvc, >+ const char *aDirKey, >+ const nsCSubstring &aLeafName, >+ nsILocalFile **aFile, >+ PRTime *aLastModified) >+{ >+ *aLastModified = LL_ZERO; >+ >+ nsresult rv; >+ nsCOMPtr<nsILocalFile> file; >+ rv = aDirSvc->Get(aDirKey, NS_GET_IID(nsILocalFile), getter_AddRefs(file)); >+ if (NS_FAILED(rv)) >+ return rv; >+ rv = file->AppendNative(aLeafName); >+ if (NS_SUCCEEDED(rv)) { >+ // if the file does not exist, this returns zero >+ file->GetLastModifiedTime(aLastModified); >+ file.swap(*aFile); >+ } >+ return rv; Why do you need the extra COMPtr here? (just use aFile throughout). Or, what I have done in some places, make aFile a nsCOMPtr<nsILocalFile> &aFile. > NS_IMETHODIMP > nsChromeRegistry::CheckForNewChrome() >+ // we assume that any other list files will only reference chrome that >+ // should be installed in the profile directory. we might want to add >+ // some actual checks to this effect. Why do we assume that? Maybe we're making a bad assumption about where the chrome.rdf and overlayinfo cache are going to live for xulrunner apps? IMO they should live in the "resource://app/" directory, not the resource:/// directory. When CheckForNewChrome is called, we probably don't have a current profile (or at least you can't assume that we have a profile). Answer me this, otherwise this looks good.
Comment on attachment 157195 [details] [diff] [review] v1 - toolkit and resource: changes >+static nsXREAppData *LoadAppData(const char *appDataFile) I don't like the style, can it be nsXREAppData* LoadAppData ? >+{ >+ static nsXREAppData data; >+ static char vendor[256], name[256], version[32], buildID[32], copyright[256]; copyright[512] perhaps? >+ parser.GetString("Main", "Vendor", vendor, sizeof(vendor)); >+ parser.GetString("Main", "Name", name, sizeof(name)); >+ parser.GetString("Main", "Version", version, sizeof(version)); >+ parser.GetString("Main", "BuildID", buildID, sizeof(buildID)); >+ parser.GetString("Main", "Copyright", copyright, sizeof(copyright)); Need error-checking here. If the name doesn't exist, INIParser doesn't null-out the buffer, and I presume you would want to bail anyway in some cases: Name/Version are required Vendor/BuildID/Copyright are optional, I think I'm a bit worried about forwards-compatibility, especially if this lands for aviary... maybe we can include a magic INI key of some sort, to prevent "old" versions of the xulrunner from running new/incompatible XUL apps, e.g.: MinVersion=2 would cause us to bail. (Assuming ffox1.0 is "1", and libxul/xulrunner is "2", and allowing for decimals. >+ { >+ nsCOMPtr<nsILocalFile> lf; >+ >+ if (appDataFile) { >+ NS_GetFileFromPath(appDataFile, getter_AddRefs(lf)); >+ lf->SetNativeLeafName(NS_LITERAL_CSTRING("")); Whoa, that can't be right. Maybe you mean lf->GetParent ?
Attachment #157195 - Flags: review?(bsmedberg) → review-
Comment on attachment 157195 [details] [diff] [review] v1 - toolkit and resource: changes >+static nsXREAppData *LoadAppData(const char *appDataFile) >+{ >+ static nsXREAppData data; >+ static char vendor[256], name[256], version[32], buildID[32], copyright[256]; Oh, and you can save some code here: >+ data.appVendor = vendor; >+ data.appName = name; >+ data.appVersion = version; >+ data.appBuildID = buildID; >+ data.copyright = copyright; >+ data.useStartupPrefs = PR_FALSE; static char vendor[256], name[256], version[32], buildID[32], copyright[256]; static nsXREAppData data = { vendor, name, version, buildID, copyright };
and you can actually make "data" const once you do that ;)
Comment on attachment 157193 [details] [diff] [review] v1 - widget changes for XUL window icons Index: widget/src/gtk/nsWindow.cpp + NS_ConvertUCS2toUTF8 iconKey(aIconSpec); can you make this UTF16 instead of UCS2 while you're here? + ResolveIconName(aIconSpec, NS_LITERAL_STRING("16.xpm"), + getter_AddRefs(iconFile)); hm, passing "16.xpm" as extension is slightly abusing the API I guess, but this is in the same module, so ok. + printf("Looking for small icon file: %s\n", path.get()); + printf("Loaded small icon file: %s\n", path.get()); they seem to be now in the same if, so either none of them or both will be printed. is it useful to have them both, then? Index: widget/src/windows/nsWindow.cpp - nsCAutoString cPath; cPath.AssignWithConversion(iconPath); thanks! :) HICON bigIcon = (HICON)::LoadImageW( NULL, hm... wonder why this works on win9x... didn't think it had this function Index: widget/src/xpwidgets/nsBaseWidget.cpp + * Resolve the given icon name into a local file object. This method is hm... can you mention that it also checks whether the file exists? that wasn't obvious to me at first. is there a reason to put this comment into the .cpp file instead of the .h file?
Attachment #157193 - Flags: review?(cbiesinger) → review+
> HICON bigIcon = (HICON)::LoadImageW( NULL, > > hm... wonder why this works on win9x... didn't think it had this function The code tries LoadImageW first, and then falls back to LoadImageA if the GetLastError() returns ERROR_CALL_NOT_IMPLEMENTED.
> is there a reason to put this comment into the .cpp file instead of the .h > file? convention. the header file doesn't document any of the other methods. any documentation appears to be in the .cpp file.
Thanks bsmedberg and biesi for the reviews! > Why do we assume that? Maybe we're making a bad assumption about where the > chrome.rdf and overlayinfo cache are going to live for xulrunner apps? IMO > they should live in the "resource://app/" directory, not the resource:/// > directory. When CheckForNewChrome is called, we probably don't have a current > profile (or at least you can't assume that we have a profile). Answer me this, > otherwise this looks good. I discussed this issue with bsmedberg over IRC. Here's the deal: By putting chrome.rdf (and overlayinfo) for xulapps in the profile directory, we allow the user to create them. This is good because it means thatapp developers neither have to include chrome.rdf in the installation package nor provide some sort of installation process that generates chrome.rdf when the xulapp is installed. As for profile selection details, toolkit/xre/nsAppRunner.cpp ensures that a profile exists before invoking application logic. It either shows the profile selection dialog, uses the default profile, or expects a profile to be selected on the command line. A xulapp should not need to show chrome before this process occurs. Moreover, chrome.rdf is just a cache, so we could move it in the future if appropriate. > Name/Version are required Well, the code seems to require all of the fields if you count DumpVersion. But, otherwise only Name and BuildID (not Version) are required. I'll add code to require those fields and use an empty string for the others when they don't appear in the INI file.
Attachment #157193 - Flags: superreview?(blizzard) → superreview+
Attached patch v1.1 patch - combined (obsolete) (deleted) — Splinter Review
OK, here's the revised patch for the trunk.
Attachment #157192 - Attachment is obsolete: true
Attachment #157193 - Attachment is obsolete: true
Attachment #157195 - Attachment is obsolete: true
Comment on attachment 157837 [details] [diff] [review] v1.1 patch - combined widget/ and xpfe/ changes since they have already been reviewed by biesi and blizzard.
Attachment #157837 - Flags: review?(bsmedberg)
Attachment #157192 - Flags: review?(bsmedberg)
> widget/ and xpfe/ changes since they have already been reviewed by biesi and > blizzard. er... prefix that with "You can ignore"
Comment on attachment 157837 [details] [diff] [review] v1.1 patch - combined Please file a followup bug about the windows DDE mutex string and window class name.
Attachment #157837 - Flags: review?(bsmedberg) → review+
> Please file a followup bug about the windows DDE mutex string and window class > name. see bug 258217.
v1.1 patch checked in on trunk
Depends on: 258217
Comment on attachment 157837 [details] [diff] [review] v1.1 patch - combined a=ben@mozilla.org valid for today only, skip the CPH changes since they're not necessary - if it causes hell we can yank it quickly. The areas being touched are CR, nsAppRunner, ResHandler and Window
Attachment #157837 - Flags: approval-aviary+
Oops. Darin will land this after 1.0PR - sometime next week. Setting blocking1.0 flag.
Flags: blocking-aviary1.0+
I copied the sample app and tried this: ./firefox -app /home/varga/myapp/myapp.moz it didn't work first time, I had to add [XRE] to myapp.moz [XRE] Version=0.1 [Main] has been renamed to [App] ok, now it works but it uses /home/varga/.firefox to store myapp's profiles it would be better to use /home/varga/.myvendor/myapp IMHO
This happens only when Firefox uses old profile scheme e.g. ~/.firefox I fixed my profiles manually and now it works (almost). Anyway, there are other problems like bug 258470 and bug 258476.
(In reply to comment #25) > This happens only when Firefox uses old profile scheme e.g. ~/.firefox > I fixed my profiles manually and now it works (almost). > Anyway, there are other problems like bug 258470 and bug 258476. > Yikes! What sample are you using? The format of the INI file changed since I blogged about this stuff. Moreover, the sample app under mozilla/xulrunner has not been updated!
(In reply to comment #26) > Yikes! What sample are you using? The format of the INI file changed since I > blogged about this stuff. Moreover, the sample app under mozilla/xulrunner has > not been updated! I updated it according to the code in nsAppRunner.cpp :) Anyway, those problems with old profile locations and bug 258470 seem to be valid.
This caused Bug 258473.
Depends on: 258491
Depends on: 258473
Depends on: 258470, 258476
Attached patch build fu to enable xulrunner executable (obsolete) (deleted) — Splinter Review
Comment on attachment 158285 [details] [diff] [review] build fu to enable xulrunner executable This is likely to evolve significantly over time. Consider this an initial stab at carving out XULRunner. This accompanies the code that is already on the trunk under mozilla/xulrunner.
Attachment #158285 - Flags: review?(bsmedberg)
Comment on attachment 158285 [details] [diff] [review] build fu to enable xulrunner executable >--- client.mk 5 Sep 2004 19:39:02 -0000 1.212 >+++ client.mk 9 Sep 2004 07:04:42 -0000 I think it would make much more sense to just pull mozilla/xulrunner with mozilla/toolkit (have everyone pull this little bit of code), instead of adding a separate option. If you really want to go with the patch you have, you need to add XULRUNNER_CO_TAG to the TAG list near the top of the file. >Index: Makefile.in >+ifdef MOZ_XULRUNNER >+tier_99_dirs += xulrunner xpfe/bootstrap/init.d >+endif The init.d stuff isn't going to work with xulrunner, and I don't want to pretend to support it. I told gisburn as much when he added it (that it was only good as a temporary hack). >Index: xpfe/Makefile.in > ifdef MOZ_PHOENIX > DIRS += \ > browser/public \ > browser/src \ > $(NULL) > else >+ifdef MOZ_XULRUNNER >+DIRS += \ >+ browser/public \ >+ browser/src \ >+ $(NULL) Makefile trick: ifneq (,$(MOZ_PHOENIX)$(MOZ_XULRUNNER)) >Index: xpfe/browser/src/Makefile.in > ifdef MOZ_PHOENIX > REQUIRES += toolkitcomps history >+else >+ifdef MOZ_XULRUNNER >+REQUIRES += toolkitcomps history >+endif > endif Same >Index: xpfe/components/Makefile.in > ifdef MOZ_PHOENIX > # Firefox uses this flag to stop the automatic processing of the jar.mn file > # that lives parallel to this Makefile. The jar.mn is responsible for the > # packaging of a large number of chrome files Firefox does not need. > NO_DIST_INSTALL=1 > NO_INSTALL=1 > endif >+ifdef MOZ_XULRUNNER >+NO_DIST_INSTALL=1 >+NO_INSTALL=1 >+endif Same >+ifdef MOZ_XULRUNNER >+DIRS += bookmarks/public >+endif Ick, this hurts, can you make sure there is a followup bug on the toolkit dependencies on nsIBookmarksService or whatever needs this?
Attachment #158285 - Flags: review?(bsmedberg) → review+
bsmedberg: thanks for all the tips! i applied your suggested fixes, and checked this patch in on the trunk. as for xpfe/components/bookmarks/public, that is required for xpfe/components/search, which is currently built by firefox (and hence, i'm currently building it for xulrunner). in the future, we might end up not building the search component -- hard to say right now.
Depends on: 258638
*** Bug 206358 has been marked as a duplicate of this bug. ***
This is great work, thank you. Now that its cooking, here's my 2c. Some feedback and simple observations from the land of technology uptake. My interest is seeing this technology become user-friendly so that mere humans can play with it. As easy as XML/Scripting for Beginners. Hope these notes help. I'm focussing here on xulrunner, not XRE. It's easy to be disgusted by newbies. The problem is that everyone's a newbie sometime, and newbies will always outweigh experts in number. Newbie coders are the real case; expert coders are the rare case. Even experts appreciate simple corner-cutting features. *Initial* use of this feature set should be smooth and easy, or else people will reject the idea of a xulrunner AND we'll be back in the complexity we are currently in with -chrome AND they'll never graduate to the more complete features offered. If this is to be widely successful, then new programmers need to be able to use it "out of the box" with limited config. That requires fallback cases that apply if the newbie programmer does nothing. I ask you to cast your minds back to your first C program. You didn't start with Makefiles or CVS. You just typed: cc test.c ./a.out Perl was even easier. Turning this logic on app/xulrunner: - If the newbie types moz -app [url|path|filename] and nothing else, what then? Something sane should happen. A window should appear. - Could NS_APP_CHROME_DIR_LIST be NS_APP_CHROME_PATH or something equivalent? People know what a path is. I have in mind that when the newbie sets up his environment, he's going to want to set maybe 2 environment variables at most. (zero ideally). So could there be a $MOZILLA_CHROME_PATH envvar that overrides (or defaults to) NS_APP_CHROME_PATH? Already the newbie must set maybe LD_LIBRARY_PATH (or equiv), maybe MOZILLA_FIVE_HOME and maybe PATH. That is, if they just want to experiment in their home directory "normally and safely". A MOZILLA_CHROME_PATH should only have to be touched in intermediate cases. "." should also be in the default chrome path. I think for bundled Fedora installs, et al, MOZILLA_CHROME_PATH would be the first non-default envvar a newbie needs. But if the newbie pulls down a "recent" version, and installs it in /foo/bar then it gets harder straight away and MOZILLA_CHROME_PATH really needs to default sensibly. The idea of a .ini file for XRE is a good intermediate solution, but not for beginners. Please no .ini for xulrunner. The idea of requiring installed-chrome.txt edits is a post-beginner solution. If that file is missing, xulrunner should scan the chrome top directories for package names and deduce the (probably only one) package the user's trying to make. There should also be a default package name (called default?) for $PWD. Similarly a chrome.rdf or contents.rdf is way too hard for beginners. Beginners are going to be stressed just identifying that they need a content/skin/locale split. At least they can drop the skin & locale features til later, leaving an (alas meaningless) "content" directory. Please don't make them build a contents.rdf file. For xulrunner, if there's no .rdf file in "." or in "./<package>/content", xulrunner should try to auto-generate the needed RDF facts. They can be deduced from the above scan. Since overlays aren't beginner stuff, that much at least isn't required, only ordinary top-level package detection. Maybe a regxpcom-style solution? Can I also observe (for brendan) that a chrome-interpretation phase, whether scanning or merely reading existing constructed information, is a "baking" kind of process that is preparatory to any actual app code interpretation. It moves moz app development workflow a bit towards a compile-and-run model, and a bit away from "pure" JIT-interpret-on-the-spot model. That's an architectural question that needs a viewpoint, since all the chrome RDF baking stuff is a performance and complexity cost. From the point of view of beginners, either that baking process needs to stay completely hidden, or else it should be ditched (no chrome). I think in Perl, the site_perl stuff is only engaged once the coder starts using the 'use' or 'require' keywords. That doen't happen at step 1. And in perl, hacking site_perl is only for highly organised intermediate perl programmers. Not that moz is anything like perl ;-). If you look also at the example of Java, the baking process is still out of the way of beginners, but for any sizeable programming task, the baking process (making EARs, etc) is still entirely formal and necessary. Not that mozilla is anything like Java either ;-). I'll log bugs for any of the points above that survive feedback & discussion here. Or we can go to the NG (if only we had one). - N.
(In reply to comment #34) > - If the newbie types moz -app [url|path|filename] and nothing > else, what then? > > Something sane should happen. A window should appear. well, of course? why do you think anything else would be needed? > - Could NS_APP_CHROME_DIR_LIST be NS_APP_CHROME_PATH > or something equivalent? People know what a path is. Why, this is purely a mozilla-internal. > I have in mind that when the newbie sets up his environment, It's not an environment variable. > Already the newbie must set maybe LD_LIBRARY_PATH (or equiv), this should be done by a ./xulrunner script, like with ./mozilla on unix. > Similarly a chrome.rdf or contents.rdf is way too hard for beginners. No chrome.rdf is needed, see xulrunner/examples/simple
comment 35 doesn't really help, alas. - I'm aware of the difference between internal directory defines and envvars. I'm pointing out there's value in exposing this config via an envvar. - Currently (or else I'm behind) a window won't appear if the -chrome / -app argument is a filename. It has to be a URL. - A startup script won't help a user that's got a custom install. My point is that envvars are a config hurdle that needs consideration for newbies. - I take your point on chrome.rdf. My point, however, is that xulrunner/examples/simple is simpler but still not at all simple. Can we have a more considered review of comment 34 from the bug owner or other lurkers? There's no other forum that springs to mind without a newsgroup being created. - N.
Nigel, we can discuss this in npm.seamonkey or npd.xul, but not here. If you want something much simpler than xulrunner/samples/simple then it's going to take a long time to rearchitect the various pieces. In particular something that has *no* configuration files at all is at this point infeasible, because of the way we handle profiles and chrome registration. I have a plan, in 1.9a, to reduce the complexity of the installed-chrome.txt/contents.rdf so that no RDF is needed. Note that the xulrunner is a very different beast than the -chrome flag. You are not passing a URI, you are passing the filename of your app.xulapp file. What is this "user that has a custom install" mumbo-jumbo? The idea is that you install the xulrunner (through an RPM on most linux systems), and xulrunner is in your path. You don't have to think about the fact that at this point in time it's a shell script.
Mumbo jumbo is my job - sigh. According to comment 3, -app works for Fx, Tb, MAS. In my view newbies will try out XUL from that angle. Fx will always be more popular than xulrunner. Think MS Excel. RPM is way too obscure for most newbies. To summarize my position, I think the chrome init code could *deduce* config files for simple beginner cases, now that we have multi-tier chrome support. For this bug, my comments amount to a request that $PWD be auto-added to the list of alternate chrome directories, and that $PWD be treated to special-case chrome registration. For more text -> n.p.d.xul as Benjamin recommended. See <news://news.mozilla.org:119/ciap4i$mst7@ripley.netscape.com> - N.
I don't know if this is the best place, or if I should file a new bug, but there's a bunch of binary files in mozilla/xulrunner/app that don't have -kb set, meaning that they are completely corrupted on Windows. The ones I've had a problem with are xulrunner.ico (which breaks the build!), document.ico and document.png. I would guess the os2 versions of the icos are similarily affected.
We've decided that this is too much to take on aviary, pushing out.
Flags: blocking-aviary1.0+ → blocking-aviary1.0-
Attachment #157837 - Flags: approval-aviary+
Implement gecko version checking (part 1): - move LoadAppData into nsXULRunnerApp.cpp - remove [XRE]/Version - add [Gecko]/{MinVersion,MaxVersion} where MinVersion is required - add comments to simple.xulapp Some info about XULRunner version checking here: http://wiki.mozilla.org/index.php/XUL:Xul_Runner_Versioning
Attachment #157837 - Attachment is obsolete: true
Attachment #158285 - Attachment is obsolete: true
Comment on attachment 169147 [details] [diff] [review] Implement gecko version checking (part 1) bsmedberg: note, i decided to not bother support the '+' form of maxVersion. instead, 1.8 will match 1.8.2 as well as 1.8b and the like.
Attachment #169147 - Flags: review?(bsmedberg)
Comment on attachment 169147 [details] [diff] [review] Implement gecko version checking (part 1) >Index: xulrunner/app/nsXULRunnerApp.cpp >+static PRUint32 geckoVersion = 0; It seems a little wasteful that we have to do this at runtime. Is there any chance we can do it at build-time instead? >+static PRUint32 ParseVersion(const char *versionStr) >+{ >+ PRUint16 major, minor; >+ if (PR_sscanf(versionStr, "%hu.%hu", &major, &minor) != 2) { If we feed this a string like 1.8a or 1.8.3, this will still work correctly and get "1.8", correct? >+ if (!hasAppOption) >+ { >+ // fixup argv to start with -app. >+ char **argv2 = NULL; >+ if (argv[1][0] != '-') Do we need to check "/" for win32? >Index: xulrunner/examples/simple/Makefile.in >+# generate various fields for simple.xulapp > BUILD_ID=`date +%""Y%""m%d%H` >+GECKO_VERSION=`echo $(MOZILLA_VERSION) | sed 's|\(^[0-9]*\.[0-9]*\).*|\1|'` If we accept "1.8a5" etc, why not use the version string directly, instead of munging int? >Index: xulrunner/examples/simple/simple.xulapp >+; This field specifies your organization's name. This field is optional. recommended, but optional.
Attachment #169147 - Flags: review?(bsmedberg) → review+
> It seems a little wasteful that we have to do this at runtime. Is there any > chance we can do it at build-time instead? Yeah, I wasn't too worried about optimizing that but I suppose we could do something at build time. > If we feed this a string like 1.8a or 1.8.3, this will still work correctly > and get "1.8", correct? Yes, I verified that it works. > >+ if (!hasAppOption) > >+ { > >+ // fixup argv to start with -app. > >+ char **argv2 = NULL; > >+ if (argv[1][0] != '-') > > Do we need to check "/" for win32? Well... in that case, hasAppOption would be true. Hmm... which means that checking argv[1][0] here is just redundant. That was leftover from the old code. I'll remove the |if| check. > If we accept "1.8a5" etc, why not use the version string directly, instead of > munging int? We don't accept "[Gecko]/MinVersion=1.8a5" The idea is for XUL apps to specify compatability granularity of major.minor and no more. It's an implementation detail that "1.8a5" would be treated as "1.8" :-/ > recommended, but optional. Yeah, I'll make that change. Thanks for the quick review!
Attachment 169147 [details] [diff] has been committed to the trunk. I also renamed xulrun.exe to xulrunner.exe under Windows (and OS/2) since there's no reason for the abbreviated executable name.
Depends on: 284955
Depends on: 283766
-> Toolkit : XULRunner
Component: XP Miscellany → XULRunner
Flags: superreview+
Flags: review-
Flags: review+
Flags: blocking-aviary1.0-
Product: Core → Toolkit
Target Milestone: mozilla1.8beta1 → mozilla1.8beta2
Version: Trunk → unspecified
QA Contact: brendan → xulrunner
Depends on: 285799
Depends on: 285789
Depends on: 257777
Depends on: 286013
Depends on: 286147
Priority: -- → P1
Depends on: 288370
I think we can close this bug as fixed. There is still work to be done to make XULRunner more usable, but in its current state it is definitely usable.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
See also bug 302101 - Xulrunner 1.8 tracking bug and bug 299986 - xulrunner 1.9 tracking bug
Status: RESOLVED → VERIFIED
Flags: in-testsuite-
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: