Bug 237745 (app-startup)

split app-startup from core appshell functionality

RESOLVED FIXED in mozilla1.7final

Status

SeaMonkey
Build Config
P1
blocker
RESOLVED FIXED
15 years ago
12 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

Trunk
mozilla1.7final
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 5 obsolete attachments)

(Assignee)

Description

15 years ago
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?
This is danm's thing more than mine...

Comment 2

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

15 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).
Whoa.  We're forking appshell?  Why?  Is it not possible to simply modify
appshell to do what's needed?  Why not?

Comment 5

15 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.

Comment 6

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

15 years ago
Created attachment 144252 [details]
semi-single-profile proposal, for posterity

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

14 years ago
Created attachment 144676 [details]
split appstartup out of appshell NOTE: IGNORE toolkit changes for the moment

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

14 years ago
Attachment #144676 - Attachment is patch: false
Attachment #144676 - Attachment mime type: text/plain → application/x-gzip
(Assignee)

Comment 9

14 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

14 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 11

14 years ago
Created attachment 144688 [details]
updated with, one JS error caught by beng, and without the toolkit cruft
Attachment #144676 - Attachment is obsolete: true
(Assignee)

Comment 12

14 years ago
Created attachment 144689 [details]
document describing the implementation of this patch
(Assignee)

Comment 13

14 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

14 years ago
Priority: -- → P1
Summary: move nsIWindowMediator and implementation from appshell to windowwatcher → split app-startup from core appshell functionality
Target Milestone: --- → mozilla1.7final
   [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

14 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

14 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

14 years ago
Created attachment 145518 [details] [diff] [review]
Readable, but not apply-able

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

14 years ago
Attachment #145518 - Flags: review?(bugs)
(Assignee)

Updated

14 years ago
Blocks: 239929
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

14 years ago
Attachment #144688 - Flags: superreview?(brendan)
Attachment #144688 - Flags: review?(bugs)
(Assignee)

Updated

14 years ago
Attachment #145518 - Flags: superreview?(brendan)
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 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

14 years ago
Created attachment 148129 [details] [diff] [review]
app-startup, revision 4

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

14 years ago
Attachment #145518 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #148129 - Flags: superreview?(brendan)
(Assignee)

Comment 22

14 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

14 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

14 years ago
Attachment #145518 - Flags: superreview?(brendan)
Attachment #145518 - Flags: review?(bugs)
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

14 years ago
Alias: app-startup
(Assignee)

Comment 25

14 years ago
Created attachment 160387 [details] [diff] [review]
app-startup, revision 5

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

14 years ago
Attachment #148129 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #160387 - Flags: review?(darin)

Comment 26

14 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.
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

14 years ago
moreover, it works just fine in practice.  i use that pattern all the time ;)

Comment 31

14 years ago
Created attachment 161049 [details]
review comments for "app-startup, revision 5"

just a bunch of nits

Updated

14 years ago
Attachment #160387 - Flags: review?(darin) → review+
(Assignee)

Comment 32

14 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

14 years ago
> We should consider xpfe/components/startup a short-term solution until seamonkey 
> gets its porting in gear.

ok, works for me.
>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

14 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.
(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

14 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); :-/
> 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

14 years ago
Fixed on trunk. Onwards to command-line handling!
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 40

14 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

14 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

14 years ago
Created attachment 163959 [details] [diff] [review]
Final build bustage fix for OS/2

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

Updated

14 years ago
Blocks: 266873

Comment 43

14 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 → ---
(Assignee)

Updated

14 years ago
Blocks: 266829

Comment 44

14 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
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

14 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

14 years ago
Created attachment 164125 [details] [diff] [review]
MOZ_XUL_APP winhooks bustage fix

Comment 48

14 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

14 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

14 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

14 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

14 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
Last Resolved: 14 years ago14 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.
(Assignee)

Updated

14 years ago
Blocks: 267455
Product: Browser → Seamonkey
Created attachment 167161 [details] [diff] [review]
fix reliable crash-on-shutdown caused by previous patches

I've been crashing on shutdown for a while, in js_PurgeDeflatedStringCache. 
This fixes it.
Attachment #167161 - Flags: superreview?(darin)
Attachment #167161 - Flags: review?(bsmedberg)
(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.)
Attachment #167161 - Flags: superreview?(darin)
Attachment #167161 - Flags: review?(bsmedberg)
Darin saw the js_PurgeDeflatedStringCache signature too -- thanks for finding
the culprit, dbaron.

/be
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

14 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
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.

Updated

14 years ago
Keywords: aviary-landing

Comment 61

14 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.