Closed
Bug 237745
(app-startup)
Opened 21 years ago
Closed 20 years ago
split app-startup from core appshell functionality
Categories
(SeaMonkey :: Build Config, defect, P1)
SeaMonkey
Build Config
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.7final
People
(Reporter: benjamin, Assigned: benjamin)
References
Details
Attachments
(6 files, 5 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
darin.moz
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
I'm moving appshell out of tier 9, and need to keep nsIWindowWatcher and implementation in tier 9, for xpinstall and other random crap (ugh, but I don't have time/inclination to fix the interface to not suck). embedding/components/windowwatcher is the logical destination. cls/bz, does this sound OK to you?
WindowMediator is very much an appshell thing. appshell is a good place for it because it lives and breathes XUL Window. embedding/components is a bad place for it because, if for no other reason, that would require a compilation dependency on appshell in embedding/components. WindowMediator belongs in appshell. If I understand you correctly, I think what you actually want to do is consider making all the files spread out all over the codebase outside of appshell which use WindowMediator, use WindowWatcher instead. And be sure to check in with me before you get the inclination to make one of these interfaces not suck, whichever one you were talking about.
Assignee | ||
Comment 3•21 years ago
|
||
ok, here's the big picture: the startup sequence for firefox is changing dramatically: overview is at http://bdsmedberg.no-ip.org/single-profile/2004-02-25.html This means that we need to fork nsIAppShellService.idl in the new toolkit, because the current interface makes no sense for the new toolkit startup sequence. So xpfe/appshell and toolkit/appshell are going to be forked and live in tier 50 (the toolkit). However, there are some interfaces which currently live in appshell that need to be in tier 9, because gecko internals depend on them: nsIWindowMediator is used xpinstall for the xpinstall UI (a dependency that could probably broken) and DOM for window.find (a dependency that might be breakable, but I don't know enough to say). This involves some dicing-up of appshell. We can put the nsIWindowMediator and nsIXULWindow interfaces in embedding/components but leave the implementations (forked) in xxx/appshell... or we can break the xulwindow dependency on nsiwebshellwindow and move the windowmediator and xulwindow impls into embedding/components. (I also need to move nsIPopupWindowManager.idl, which is pretty simple because appshell doesn't implement or use it anyway, bug 237744).
![]() |
||
Comment 4•21 years ago
|
||
Whoa. We're forking appshell? Why? Is it not possible to simply modify appshell to do what's needed? Why not?
Comment 5•21 years ago
|
||
Perhaps you could attach your overview to this bug? I can't see it because it "has been temporarily blocked", and as an attachment it'll probably enjoy a longer life (for all future readers to see) than on an external server.
I've read your proposal. Gotta say, replacing the "implementations we don't want to try" paragraph with one that explains why it's a good idea in the first place would have aided my comprehension. I gather this is something you've been working on with Ben for a long time, so me quickly catching up is unlikely. You may have heard, there's been a flurry of email about this project. I've said my piece; I don't much like any of it. Despite my objections, assuming you go ahead with separating the Window part of appshell from the rest, I suggest just making that a new module in whichever build tier you need. I continue to believe that the real fix is to make core code not dependent on XUL appshell code. That's a bad dependency, anyway. embedding/components is a terrible place for XULWindow et.al. Terrible. It goes there over my dead body. And please don't split the interface files away from the implementation. That's an unhappy build hack. And the XUL interface doesn't belong in embedding any more than the implementation. PS The sincerely chrome registry specific methods that are commented "xxxbsmedberg Move me to nsIWindowMediator" only add to my discontent.
Assignee | ||
Comment 7•21 years ago
|
||
AOL network blocks your DNS lookups to my host :( danm: Ben and I talked, and I think we all agree that the goal is to fork as little as possible. Here's the alternative that currently looks best: We keep xpfe/appshell in tier 9 (I would like to move the directory out of xpfe/ at some point, but that's not really important). We split+fork the following stuff into xpfe/components/startup or someplace equivalent: nsICmdLine* nsIAppSupport* and the parts of nsIAppShellService that deal with startup sequencing (these would move into a new interface "nsIXULAppStartup" or something like that): initialize doProfileStartup nativeAppSupport hideSplashScreen createStartupState ensure1Window
Assignee | ||
Comment 8•21 years ago
|
||
This splits appstartup out of appshell. It builds and works in seamonkey. It does *not* work in toolkit yet, but it will be simple to get it working in toolkit once it's reviewed for seamonkey.
Assignee | ||
Updated•21 years ago
|
Attachment #144676 -
Attachment is patch: false
Attachment #144676 -
Attachment mime type: text/plain → application/x-gzip
Assignee | ||
Comment 9•21 years ago
|
||
Comment on attachment 144676 [details]
split appstartup out of appshell NOTE: IGNORE toolkit changes for the moment
please review ONLY the seamonkey portions of this patch... I will attach the
toolkit/profile-startup portions separately.
Attachment #144676 -
Flags: superreview?(brendan)
Attachment #144676 -
Flags: review?(bugs)
Assignee | ||
Comment 10•21 years ago
|
||
Comment on attachment 144676 [details]
split appstartup out of appshell NOTE: IGNORE toolkit changes for the moment
This is a crappy patch... I've figured out a way to avoid all that toolkit
crud.
Attachment #144676 -
Flags: superreview?(brendan)
Attachment #144676 -
Flags: review?(bugs)
Assignee | ||
Comment 12•21 years ago
|
||
Assignee | ||
Comment 13•21 years ago
|
||
Comment on attachment 144688 [details]
updated with, one JS error caught by beng, and without the toolkit cruft
This is more reviewable ;)
I have only built and tested this on linux... I will build win32 tomorrow, and
upload to planetoid to test there.
Attachment #144688 -
Flags: superreview?(brendan)
Attachment #144688 -
Flags: review?(bugs)
Assignee | ||
Updated•21 years ago
|
Priority: -- → P1
Summary: move nsIWindowMediator and implementation from appshell to windowwatcher → split app-startup from core appshell functionality
Target Milestone: --- → mozilla1.7final
Comment 14•21 years ago
|
||
[noscript] - void createHiddenWindow(); + void createHiddenWindow(in nsIAppShell aAppShell); + + void destroyHiddenWindow(); hmm, why is destroyHiddenWindow scriptable when createHiddenWindow isn't? + [noscript] void initialize(in long argc, out string argv); whoa. is there a reason why [array, size_is(argc)] was avoided for argv? Yes, you probably just copied it, but could you fix it?
Assignee | ||
Comment 15•21 years ago
|
||
> hmm, why is destroyHiddenWindow scriptable when createHiddenWindow isn't? I don't know why createHiddenWindow is [noscript]... probably historical accident from when nsIAppShell was not a real interface. It doesn't matter... even if you were doing something unusual like bootstrapping a browser from xpcshell, you would have to go through the appstartup component, instead of manually using nsIAppShellService. > + [noscript] void initialize(in long argc, out string argv); > whoa. is there a reason why [array, size_is(argc)] was avoided for argv? Yes, > you probably just copied it, but could you fix it? size_is has to be an unsigned long. It's a stupid signature, as is nsIAppShell.Init, but let's fix that later, shall we? I have a whole boatload of potential cleanup I avoided for this patch. There's a decent amount of deCOMification that can happen in appshell next cycle, as well.
Assignee | ||
Comment 16•21 years ago
|
||
update: I had to make a few more changes for win32 and mac; I moved building the activex control from tier 9 to tier 99 (it is now built with the rest of the embedding "clients" like mfcembed). There were errors in xpfe/components/build/Makefile.in where I was overriding makefile vars using = instead of +=. And there were a couple places in mac-specific code where I needed to do an nsIAppShellService->nsIAppStartup conversion (nsNativeAppSupportMac and the appleevents code). And I had to change appstartup to use a threadsafe isupports implementation, because we proxy it in a few places. I can post a new patch, but none of those changes were major or dangerous... also, if we can reach agreement on which files are going to move, I can go ahead and get the files copied in CVS, then redo the patch with only the changed portions of those files, which ought to make detailed reviewing a lot easier.
Assignee | ||
Comment 17•21 years ago
|
||
This patch is readable, but not apply-able... I put the files which need to be moved back in their original locations and took diffs from there.
Attachment #144688 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #145518 -
Flags: review?(bugs)
Assignee | ||
Updated•21 years ago
|
Blocks: semi-single-profile
Comment 18•21 years ago
|
||
Index: xpfe/bootstrap/nsNativeAppSupportWin.cpp + rv = compMgr->CreateInstanceByContractID(NS_COMMANDLINESERVICE_CONTRACTID, + nsnull, + NS_GET_IID(nsICmdLineService), + (void**)aResult ); why not use CallCreateInstance while you're here?
Assignee | ||
Updated•21 years ago
|
Attachment #144688 -
Flags: superreview?(brendan)
Attachment #144688 -
Flags: review?(bugs)
Assignee | ||
Updated•21 years ago
|
Attachment #145518 -
Flags: superreview?(brendan)
Comment 19•21 years ago
|
||
It seems like you should have changes corresponding to the client.mk activex changes to embedding/browser/activex/src/Makefile.in , but I don't see any in the patch. (Also, in the client.mk activex changes, the activex comment is dangling a few lines after the activex stuff.)
Comment 20•21 years ago
|
||
Comment on attachment 145518 [details] [diff] [review] Readable, but not apply-able What biesi and dbaron said, plus my terse list o' issues: Factory, etc. CamelCaps in nsComposerCmdLineHandler.js #ifndef MOZ__THUNDERBIRD in msgMapiHook.cpp nsIAppStartup.idl createHiddenWindow comment cut off nsAppStartup::OpenBrowserWindow's body is overindented one c-basic-offset Get mscott to look at the MOZ_THUNDERBIRD changes and test 'em if you haven't. Otherwise, the main thing is to test all the apps before landing. /be
Assignee | ||
Comment 21•21 years ago
|
||
Nits picked, except the one about MOZ__THUNDERBIRD which I didn't understand. This builds with tbird, and mscott said that I could land little mail/mailnews changes like this without further review.
Assignee | ||
Updated•21 years ago
|
Attachment #145518 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #148129 -
Flags: superreview?(brendan)
Assignee | ||
Comment 22•21 years ago
|
||
Clarification: I added that tbird #ifdef because tbird doesn't have quicklaunch, and will always have a profile when processing the eventloop. It therefore doesn't need any special logic to "ensure" that there is a profile selected. This patch needs to land together with the semi-single-profile work, because this patch only covers the xpfe tine of the fork.
Assignee | ||
Comment 23•21 years ago
|
||
Also, I had to revert back to compMgr->CreateInstanceByContractID instead of using CallCreateInstance... that function links directly to the deprecated nsComponentManager:: symbols, which won't work when we're using the XPCOM glue in seamonkey.
Assignee | ||
Updated•21 years ago
|
Attachment #145518 -
Flags: superreview?(brendan)
Attachment #145518 -
Flags: review?(bugs)
Comment 24•21 years ago
|
||
Comment on attachment 148129 [details] [diff] [review] app-startup, revision 4 New, reduced trunk-only patch coming up? /be
Attachment #148129 -
Flags: superreview?(brendan)
Assignee | ||
Updated•20 years ago
|
Alias: app-startup
Assignee | ||
Comment 25•20 years ago
|
||
It's revived! This should build, with the same symlinks listed earlier in this bug. I have not tested the changes to nsNativeAppSupportWin.cpp yes, that code is untested but will be tested soon. Also I didn't build tbird, though the changes there are minor. Will build before checkin ;) Who wants to review? darin should, danm or dveditz, are you up to looking this over?
Assignee | ||
Updated•20 years ago
|
Attachment #148129 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #160387 -
Flags: review?(darin)
Comment 26•20 years ago
|
||
Comment on attachment 160387 [details] [diff] [review] app-startup, revision 5 >Index: editor/ui/nsComposerCmdLineHandler.js >+ _factory: { >+ createInstance: function(outer, iid) { >+ if (outer != null) { >+ throw R.NS_ERROR_NO_AGGREGATION; >+ } >+ >+ return new nsComposerCmdLineHandler(); return new nsComposerCmdLineHandler().QueryInterface(iid); more to come...
I think you probably mean return (new nsComposerCmdLineHandler()).QueryInterface(iid); or something similar.
Comment 28•20 years ago
|
||
shaver, darin had it right: return new nsComposerCmdLineHandler().QueryInterface(iid); ECMA-262 goes out of its way to make this work. See the productions NewExpression : MemberExpression new NewExpression and MemberExpression : new MemberExpression Arguments and CallExpression : MemberExpression Arguments Once a new foo(args) has been reduced to a MemberExpression, subsequent . or [] operators use that MemberExpression as left operand. No need to parenthesize the new expression with arguments. /be /be
Well, blow me over. Apologies!
Comment 30•20 years ago
|
||
moreover, it works just fine in practice. i use that pattern all the time ;)
Updated•20 years ago
|
Attachment #160387 -
Flags: review?(darin) → review+
Assignee | ||
Comment 32•20 years ago
|
||
> why is this interface being forked? why don't we make seamonkey
> inherit the interface from toolkit? or do you mean to remove the
Actually, forking the interface was the whole point of this exercize. The many
methods for dealing with the splash screen, profile migration, and
command-line-handling need to change (rapidly and radically, as we've
discussed), and rewriting the seamonkey profile migrator is likely to be the
very last step in porting it to toolkit. We should consider
xpfe/components/startup a short-term solution until seamonkey gets its porting
in gear.
Comment 33•20 years ago
|
||
> We should consider xpfe/components/startup a short-term solution until seamonkey
> gets its porting in gear.
ok, works for me.
Comment 34•20 years ago
|
||
>Also, I had to revert back to compMgr->CreateInstanceByContractID instead of >using CallCreateInstance... that function links directly to the deprecated >nsComponentManager:: symbols, which won't work when we're using the XPCOM glue >in seamonkey. you could make CallCI use NS_GetComponentManager! ;) (quoting review comment) >>+ url.AssignWithConversion(urlToLoad); > nit: use CopyASCIItoUTF16 instead Absolutely! please do that :) (or AssignASCII, given your IsASCII check) > splash screen will toolkit offer some way to show a splash screen? aiui it doesn't have a way now. (quoting patch v5) client.mk +# Configure + +configure:: $(OBJDIR)/Makefile why this dependency? configure creates the makefile surely, not depends on it? ah... so "make -f client.mk" would trigger a configure run if needed? nsComposeCmdLineHandler.js: +const R = Components.results; hm, I don't think this is common... I'm not sure it increases readability + appStartup->Quit(nsIAppStartup::eAttemptQuit); app _startup_ quits the app? heh. nsSystemPref.cpp + if (!gSysPrefLog) return NS_ERROR_OUT_OF_MEMORY; please put the return on a second line profile/build/Makefile.in -GRE_MODULE = 1 (also public/) ? profileSelection.js: + var appStartup = Components.classes["@mozilla.org/seamonkey/app-startup;1"]. + getService(Components.interfaces.nsIAppStartup); usual style is: + var appStartup = Components.classes["@mozilla.org/seamonkey/app-startup;1"] + .getService(Components.interfaces.nsIAppStartup); with the dot from getService under the dot from .classes, which I'm sure I did wrong in this comment Yeah... why kill nsAppShellCIDs.h? put the contracts there, rather that into the idl :) nsIAppShellService.idl: nsIXULWindow createTopLevelWindow(in nsIXULWindow aParent, this function has documentation (hard to see in the patch), mind updating it to explain your new nsIAppShell arg? nsXULWindow + obssvc->NotifyObservers(nsnull, "xul-window-visible", nsnull); nice, this could probably be used by gnome's libappstartup-notification (or whatever that's called) xpfe/bootstrap/nsAppRunner.cpp + do_GetService(NS_COMMANDLINESERVICE_CONTRACTID,&rv); missing space before &rv xpfe/browser/src/nsBrowserInstance.cpp + rv = pIProxyObjectManager->GetProxyForObject(NS_UI_THREAD_EVENTQ, NS_GET_IID(nsIAppStartup), + appStartup, PROXY_ASYNC | PROXY_ALWAYS, + getter_AddRefs(appStartupProxy)); while you are touching this line, you could fix its indentation... xpfe/components/build/Makefile.in $(DIST)/lib/$(LIB_PREFIX)related_s.$(LIB_SUFFIX) \ + ../startup/src/$(LIB_PREFIX)appstartup_s.$(LIB_SUFFIX) \ hmm, why is this not using $DIST/lib? > Reviewers made me add this comment. hehe. xpfe nsAppStartup.cpp: + NS_ASSERTION(0, "Failed to get a platform charset"); make that NS_ERROR, please + printf("default args: %s\n", NS_ConvertUCS2toUTF8(defaultArgs).get()); maybe UCS2 -> UTF16 + nsCOMPtr<nsIInterfaceRequestor> thing(do_QueryInterface(newWindow)); + if (thing) + thing->GetInterface(NS_GET_IID(nsIWebBrowserChrome), (void **) _retval); CallGetInterface? (sorry if all this is copied. it shows with + in the patch! ;) ) + var cmdLineService = Components.classes[ "@mozilla.org/app-startup/commandLineService;1" ] the appstartup service has /toolkit/ in its contractid, but cmdline is in /app-startup/? why not put both into /app-startup/, the service as /app-startup/service;1 or something? toolkit/xre/nsINativeAppSupport.idl + * The interface provides these functions: now that's an interesting way to document the methods. anyway, not your fault. but maybe you want to fix it anyway? ;) (i.e. put the comments for each function directly in front of it)
Assignee | ||
Comment 35•20 years ago
|
||
> you could make CallCI use NS_GetComponentManager! ;) Followup bug 263360 filed. > will toolkit offer some way to show a splash screen? aiui it doesn't have a way now. No, unless there is great wailing and gnashing of teeth. > (quoting patch v5) > client.mk > +# Configure > + > +configure:: $(OBJDIR)/Makefile > > why this dependency? configure creates the makefile surely, not depends on it? > ah... so "make -f client.mk" would trigger a configure run if needed? Actually, configure is a "fake" target, so that I can get client.mk to run configure without doing any more building steps. e.g. "make -f client.mk configure" > profile/build/Makefile.in > -GRE_MODULE = 1 > (also public/) > > ? Yes. nsIProfile.idl, though frozen, should not be part of the GRE, since it is not part of the toolkit, only part of seamonkey. > xpfe/components/build/Makefile.in > $(DIST)/lib/$(LIB_PREFIX)related_s.$(LIB_SUFFIX) \ > + ../startup/src/$(LIB_PREFIX)appstartup_s.$(LIB_SUFFIX) \ > > hmm, why is this not using $DIST/lib? Because I am not exporting this lib. Basically, I want to stop exporting a lot of the intermediate libs that we use to build larger libs. It helps save a lot of disk space on win32, and it's gratuitous. > + nsCOMPtr<nsIInterfaceRequestor> thing(do_QueryInterface(newWindow)); > + if (thing) > + thing->GetInterface(NS_GET_IID(nsIWebBrowserChrome), (void **) _retval); > > CallGetInterface? This was copied, and I can't figure out how to make CallGetInterface work.
Comment 36•20 years ago
|
||
(In reply to comment #35) > > ah... so "make -f client.mk" would trigger a configure run if needed? > > Actually, configure is a "fake" target, so that I can get client.mk to run > configure without doing any more building steps. e.g. "make -f client.mk configure" whoops, that's what I meant to write. but it will not always run configure, right? maybe that'd be more useful? > Because I am not exporting this lib. Basically, I want to stop exporting a lot > of the intermediate libs that we use to build larger libs. It helps save a lot > of disk space on win32, and it's gratuitous. ah, makes sense. > > + nsCOMPtr<nsIInterfaceRequestor> thing(do_QueryInterface(newWindow)); > > + if (thing) > > + thing->GetInterface(NS_GET_IID(nsIWebBrowserChrome), (void **) _retval); > > > > CallGetInterface? > > This was copied, and I can't figure out how to make CallGetInterface work. (rv = ) CallGetInterface(thing.get(), _retval); (I'm not quite sure whether the .get() is needed...) oh, nevermind. that won't help with a void**.
Comment 37•20 years ago
|
||
Comment on attachment 160387 [details] [diff] [review] app-startup, revision 5 >+const C = Components.classes; >+const I = Components.interfaces; >+const R = Components.results; These aren't prevalent style. Declaring a particular interface or result, e.g. const nsISupports = Components.interfaces.nsISupports is preferred. >+ _CID: Components.ID("{f7d8db95-ab5d-4393-a796-9112fe758cfa}"), >+ _contractIDPrefix: "@mozilla.org/commandlinehandler/general-startup;1?type=", These should be top-level consts. The scope is private to the component anyway. >+ _factory: { >+ createInstance: function(outer, iid) { >+ if (outer != null) { >+ throw R.NS_ERROR_NO_AGGREGATION; >+ } >+ >+ return new nsComposerCmdLineHandler(); >+ } This could be a top-level object too. >+ if (!iid.equals(I.nsICmdLineHandler) && >+ !iid.equals(I.nsISupports)) { >+ throw R.NS_ERROR_NO_INTERFACE; >+ } You braced this but not the one above... I'm no fan of braces but this is a new file so you get to choose the style, as long as you're consistent. >+ get defaultsArgs() { Nit: should be defaultArgs. While I mention it, you could probably use JS properties rather than writing each getter out, it's not as if anyone's going to be able to change these values. >+ catMan.deleteCategoryEntry("command-line-argument-handlers", >+ "nsComposerCmdLineHandler", true); Looks like this will be the first command line handler to unregister itself correctly! >+ rv = compMgr->CreateInstanceByContractID( >+ NS_COMMANDLINESERVICE_CONTRACTID, >+ nsnull, NS_GET_IID(nsICmdLineService), >+ (void**) aResult); I don't claim to have followed the create instance debate but I assume at some point this will become rv = CallCreateInstance(NS_COMMANDLINESERVICE_CONTRACTID, aResult); >+ nsCOMPtr<nsIInterfaceRequestor> thing(do_QueryInterface(newWindow)); >+ if (thing) >+ thing->GetInterface(NS_GET_IID(nsIWebBrowserChrome), (void **) _retval); Bah, why is it you can do nsCOMPtr<nsIWebBrowserChrome> wbc = do_GetInterface(newWindow) but not CallGetInterface(newWindow, _retval); :-/
Comment 38•20 years ago
|
||
> oh, nevermind. that won't help with a void**.
actually do mind, as _retval is NOT a void** - so CallGetInterface should work
Assignee | ||
Comment 39•20 years ago
|
||
Fixed on trunk. Onwards to command-line handling!
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 40•20 years ago
|
||
>+ if (!iid.equals(I.nsICmdLineHandler) && >+ !iid.equals(I.nsISupports)) { >+ throw R.NS_ERROR_NO_INTERFACE; >+ } Bah, I failed to notice the bad indenting of "throw" :-[ >+ get defaultsArgs() { >+ return "about:blank" >+ }, I was hoping for defaultArgs: "about:blank", :-/
Comment 41•20 years ago
|
||
Doh, I also overlooked this :-[
>+ var appStartup = Components.classes["@mozilla.org/toolkit/app-startup;1"].
>+ getService(Components.interfaces.nsIAppStartup);
which should of course line up .getService with .classes
Comment 42•20 years ago
|
||
On top of the build bustage fixes that dbaron already checked in this gets it working again for OS/2. Can someone check this in?
Comment 43•20 years ago
|
||
The Thunderbird windows build is still busted on Tinderbox from these changes. Can you take a look? Re-opening until the bustage clears...Thanks!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 44•20 years ago
|
||
Not trying to be a pest, but the Thunderbird windows trunk build is still in flames from this checkin. It's been a couple days: http://tinderbox.mozilla.org/showbuilds.cgi?tree=Thunderbird any chance of an ETA on when we can expect to get Windows builds again? Thanks! -Scott
Comment 45•20 years ago
|
||
This change also busted the Sunbird build process. I get the same error message as the WINNT 5.0 patrocles Clbr Tbird tinderbox. Although it is not directly related to this bustage, the change to splash.rc (and other changes as appropriate) should probably also be made for Sunbird.
Comment 46•20 years ago
|
||
As this is causing "red" on patrocles and prevents cvs builds from compiling Thunderbird and Sunbird changing to "blocker".
Severity: normal → blocker
Comment 47•20 years ago
|
||
Comment 48•20 years ago
|
||
(In reply to comment #47) > Created an attachment (id=164125) > MOZ_XUL_APP winhooks bustage fix With this fix in my Thunderbird tree, I now get :- Creating Resource file: module.res /cygdrive/e/mozilla/source/thunderbird/mozilla/build/cygwin-wrapper windres -O c off --use-temp-file -DMOZ_THUNDERBIRD --include-dir /cygdrive/e/mozilla/source/t hunderbird/mozilla/mail/app -DTHUNDERBIRD_ICO=\"../../dist/branding/thunderbird. ico\" -DOSTYPE=\"WINNT5.1\" -DOSARCH=\"WINNT\" -DAPP_VERSION=\"0.6+\" -DBUILD_ID =\"0000000000\" --include-dir ../../dist/include/string --include-dir ../../dist /include/xpcom --include-dir ../../dist/include/xulapp --include-dir ../../dist/ include/xpinstall --include-dir ../../dist/include/appshell --include-dir ../../ dist/include --include-dir ../../dist/include --include-dir ../../dist/include/n spr -o module.res /cygdrive/e/mozilla/source/thunderbird/mozilla/mail/app/module .rc e:/mozilla/source/thunderbird/mozilla/mail/app/module.rc:84:35: nsNativeAppSuppo rtWin.h: No such file or directory e:\gnu\mingw\bin\windres.exe: e:\gnu\mingw\bin\gcc exited with status 1 make[4]: *** [module.res] Error 1 make[4]: Leaving directory `/cygdrive/e/mozilla/source/thunderbird/mozilla/mail/ app' make[3]: *** [libs] Error 2 make[3]: Leaving directory `/cygdrive/e/mozilla/source/thunderbird/mozilla/mail' make[2]: *** [libs] Error 2 make[2]: Leaving directory `/cygdrive/e/mozilla/source/thunderbird/mozilla' make[1]: *** [alldep] Error 2 make[1]: Leaving directory `/cygdrive/e/mozilla/source/thunderbird/mozilla' make: *** [alldep] Error 2
Just having a trunk Seamonkey Mail window with no Navigator window, trying to _get_ a Navigator window fails, and yields: Error: uncaught exception: [Exception... "Could not convert JavaScript argument (NULL value cannot be used for a C++ reference type) arg 0 [nsISupports.QueryInterface]" nsresult: "0x8057000b (NS_ERROR_XPC_BAD_CONVERT_JS_NULL_REF)" location: "JS frame :: chrome://communicator/content/tasksOverlay.js :: OpenBrowserWindow :: line 120" data: no] Warning: reference to undefined property Components.interfaces.nsICmdLineHandler Source File: chrome://communicator/content/tasksOverlay.js Line: 120 Trying to quit the app via File | Quit fails as well, and yields: Error: uncaught exception: [Exception... "Component returned failure code: 0x80570018 (NS_ERROR_XPC_BAD_IID) [nsIJSCID.getService]" nsresult: "0x80570018 (NS_ERROR_XPC_BAD_IID)" location: "JS frame :: chrome://global/content/globalOverlay.js :: goQuitApplication :: line 23" data: no] Warning: reference to undefined property Components.interfaces.nsIAppStartup Source File: chrome://global/content/globalOverlay.js Line: 23
Comment 50•20 years ago
|
||
David, does adding LOCAL_INCLUDES += -I$(topsrcdir)/toolkit/xre to mail/app/Makefile.in fix the bustage? My VC8 build is busted in addrbook currently, so I couldn't verify if anything further along built before attaching the previous patch.
Comment 51•20 years ago
|
||
That fix allows my build to finish OK. (In reply to comment #50) > David, does adding LOCAL_INCLUDES += -I$(topsrcdir)/toolkit/xre to > mail/app/Makefile.in fix the bustage? My VC8 build is busted in addrbook > currently, so I couldn't verify if anything further along built before attaching > the previous patch.
Assignee | ||
Comment 52•20 years ago
|
||
stephend, are you using an installer build? I think I probably forgot to add some .xpt files to the packaging manifests which would cause the error you're describing.
Assignee | ||
Comment 53•20 years ago
|
||
I fixed mail/app/Makefile.in adding the LOCAL_INCLUDES line, and fixed the installer packaging for seamonkey/firefox/thunderbird. I'm going to mark this bug FIXED. If there are additional issues, can you open a separate bug (mark the dependency) so that I can keep track of things?
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
(In reply to comment #52) > stephend, are you using an installer build? I think I probably forgot to add > some .xpt files to the packaging manifests which would cause the error you're > describing. Yes, I am using an installer build, and upgrading to 2004-11-01-04 fixed this. Thanks.
Updated•20 years ago
|
Product: Browser → Seamonkey
Comment 55•20 years ago
|
||
I've been crashing on shutdown for a while, in js_PurgeDeflatedStringCache. This fixes it.
Updated•20 years ago
|
Attachment #167161 -
Flags: superreview?(darin)
Attachment #167161 -
Flags: review?(bsmedberg)
Comment 56•20 years ago
|
||
(Which was essentially a regression of bug 249737, so I'm just going to check it in. Never mind the reviews. That's what you get for copying an old copy of a file and just checking it in.)
Updated•20 years ago
|
Attachment #167161 -
Flags: superreview?(darin)
Attachment #167161 -
Flags: review?(bsmedberg)
Comment 57•20 years ago
|
||
Darin saw the js_PurgeDeflatedStringCache signature too -- thanks for finding the culprit, dbaron. /be
Comment 58•20 years ago
|
||
I received: E:/cvs/work/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp:883: warning: invalid conversion from `const char* const' to `char*' E:/cvs/work/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp: At global scope: E:/cvs/work/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp:943: prototype for ` nsresult nsNativeAppSupportOS2::Quit()' does not match any in class ` nsNativeAppSupportOS2' E:/cvs/work/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp:303: candidate is: virtual void nsNativeAppSupportOS2::Quit() E:/cvs/work/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp:943: `nsresult nsNativeAppSupportOS2::Quit()' and `virtual void nsNativeAppSupportOS2::Quit()' cannot be overloaded make.exe[4]: *** [nsNativeAppSupportOS2.o] Error 1 make.exe[4]: Leaving directory `E:/cvs/work/mozilla/sbobj/toolkit/xre' make.exe[3]: *** [libs] Error 2 make.exe[3]: Leaving directory `E:/cvs/work/mozilla/sbobj/toolkit' make.exe[2]: *** [tier_50] Error 2 make.exe[2]: Leaving directory `E:/cvs/work/mozilla/sbobj' make.exe[1]: *** [default] Error 2 make.exe[1]: Leaving directory `E:/cvs/work/mozilla/sbobj' make.exe: *** [build] Error 2 building FireFox and Sunbird (clean builds). Is this because the Final build bustage for OS/2 patch has not been commited? Andy P.S. I would test it but as I am building the suite right now (which doesn't have this issue) it will be tomorrow before I can start a build with the patch in place and hours from then before I would be certain if this was a fix for this issue. Therefore I am not reopening unless I test it before I hear back here.
Comment 59•20 years ago
|
||
Comment on attachment 163959 [details] [diff] [review] Final build bustage fix for OS/2 This never went in like that.
Attachment #163959 -
Attachment is obsolete: true
Comment 60•20 years ago
|
||
This was not checked into the the Aviary Branch for OS/2 (neither was 258217). I found issue was NS_IMETHOD Quit; was changed to void Quit(); Changing it back cleans up this error when building Firefox and Sunbird on OS/2 from the Trunk. As I get an error from 258217 I can't say for certain that it fixes everything but as best I can tell it should. Not sure why it was switched to void as the windows version was not changed so I don't know if it was a mistake or intentional but something else wasn't done to complete that change.
Keywords: aviary-landing
Comment 61•20 years ago
|
||
Relanded relevant parts of patch for globalOverlay.js and nsExtensionManager.js.in following landing of aviary branch.
Keywords: aviary-landing
You need to log in
before you can comment on or make changes to this bug.
Description
•