Johnny Stenback (:jst) Assignee
	Description • 17 years ago

See bz's comment #79 in bug 194404. W/o this apps like Camino won't be able to support modal content dialogs.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 17 years ago

So I don' see how we can make this actually _work_ in embedding apps without changes on their end.  I guess our options are either to have it work incorrectly (arguments/retval not working) or to have it throw.  Unless the embeddor takes steps, of course.  The former is what we have now.  I think the latter is a better idea.

All I can think of is yet another window creator interface that would have to be implemented to open these sorts of windows...  Anyone have a better idea?

	Johnny Stenback (:jst) Assignee
	Comment 2 • 17 years ago

This implements what bz and I discussed on irc the other day. This eliminates the need for the XUL window code to tell the docshell that it's modal by making the docshell ask the chrome window (through the embedding API nsIWebBrowserChrome), and makes the window watcher throw if the tree owner doesn't implement that API and we're trying to open a modal window. This also contains the followup fixes for bug 194404 that Jonas found in his original sr (and I overlooked).

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 17 years ago

Comment on attachment 275504 [details] [diff] [review]
Remove XUL dependency.

>+++ b/docshell/base/nsDocShell.cpp
>+        chromeFlags & nsIWebBrowserChrome::CHROME_MODAL &&
>+        !(chromeFlags & nsIWebBrowserChrome::CHROME_OPENAS_CHROME);

Stick parens around |chromeFlags & nsIWebBrowserChrome::CHROME_MODAL| so people don't have to remember whether & or && binds tighter?

>+++ b/embedding/components/windowwatcher/src/nsWindowWatcher.cpp
>+  if (chromeFlags & nsIWebBrowserChrome::CHROME_MODAL &&

Same here.

r=bzbarsky with that

	Johnny Stenback (:jst) Assignee
	Comment 4 • 17 years ago

Fixed.

	neil@parkwaycc.co.uk
	Comment 5 • 17 years ago

This caused a regression in Thunderbird/SeaMonkey Mail and News. msgMail3PaneWindow.js moves a <browser type="content"> element in the XUL document. desiredParent.appendChild(messagePane); triggers something in layout which my build doesn't have symbols for, so I'll need to retest with symbols. Anyway, at some point layout tries to getInterface the nsPIDOMWindow from the docShell. This calls nsDocShell::EnsureScriptEnvironment() which calls nsContentTreeOwner::GetChromeFlags() which calls nsXULWindow::GetChromeFlags which calls nsXULWindow::GetContentScrollbarVisibility() which calls nsDocShell::GetInterface for nsIDOMWindow... can you see what this is yet?

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 17 years ago

Hmm... We probably need to fix that on the docshell end too, in case other treeowners do something similar, eh?

	neil@parkwaycc.co.uk
	Comment 7 • 17 years ago

The irony is that the scrollbars flag is actually a member of nsDocShell!

	Joe Sabash [:JoeS1]
	Comment 8 • 17 years ago

Could this be causing stack overflow crash on Winxp on sweitching views.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007081304 Thunderbird/3.0a1pre ID:2007081304
Crash report here:
http://crash-stats.mozilla.com/report/index/fc7871ca-49e5-11dc-8ee8-001a4bd43ed6?date=2007-08-13-21

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 17 years ago

Yes.  See comment 5.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 17 years ago

I filed bug 392532 on comment 5.