From bug 46582: Is there separate bug for the fact that the visibility attribute needs to be on nsIWebBrowserSiteWindow? Since it's not there now, the embedding app, if it ever wants it window to show, has to create it visible. If it's a JS dialog and then the dialog is moved and resized after it's created (normal case), it looks really bad.

I have attached a prototype for the new nsIEmbeddingSiteWindow interface, the replacement for the shortlived nsIWebBrowserSiteWindow. This has a visibility attribute and simplifed methods for setting size and position. I haven't put any support in for chrome yet.

Dan can you comment on the latest plans for nsIEmbeddingSiteWindow. Jud, Conrad and myself agreed that the destroy() method should be moved to nsIWebBrowserChrome and renamed as destroyBrowserWindow() as a companion for createBrowserWindow(). The belief is that destroy() is specific to webbrowsers (e.g. to enable JS to close a window) and it would make sense to move it out of the generic interface. Do you have any thoughts on this plan?

My take: basically, I don't mind. Chrome has other window-type methods on it, so I'd say the distinction between Chrome and more windowy interfaces is a fuzzy one. Destroy goes with Create sure enough (except that they're different in the sense that Create doesn't refer to the current object, while Destroy does). But I'm not convinced that Create itself belongs on Chrome. It seems weird to me that to make a new A object, first you have to have an A object. (You probably know I've just spent days-'n-days exorcising this situation from the DOM window.) There is, of course, already a Destroy method on nsIBaseWindow. And JS uses this method to close a DOM window. It strikes me that putting another similar method on nsIWebBrowserChrome won't help the scripting situation; current glue code doesn't know about it and doesn't seem to have any need to. nsContentTreeOwner and the WebBrowserChrome in gtk and photon; the only objects in the tree (that I know of) which implement both nsIBaseWindow and nsIWebBrowserChrome, will have a curious implementation of one of these destroy methods. But I ramble. In summary I'm saying that the window interfaces don't seem so clearly defined to me that this change is necessarily righteous, but I'd say it doesn't hurt, either.

> There is, of course, already a Destroy method on nsIBaseWindow. The point of this was to hide nsIBaseWindow from the embeddor. True, the JS glue needs nsIBaseWindow::Destroy to close a window but the webbrowser dll will still implement nsIBaseWindow and map that to a call to nsIWebBrowserChrome::DestroyBrowserWindow(). So, it makes things a little more clear externally for the embeddor while a little less clear internally for us :-/ As far as gtk and photon, they should just be made to not use nsIBaseWindow.

Okay I'll go with that the "move Destroy to nsIWebBrowserChrome and rename as DestroyBrowserWindow" plan. I have no idea why the gtk widget still implements nsIBaseWindow - you'd better ask Chris. Photon implements it because it is bitrotten.

The gtk widget implements nsIBaseWindow because it is also the docshelltreeowner. If you don't implement nsIBaseWindow and you are the tree owner then all hell breaks loose. There are some nasty assumptions made in that regard. I'm rewriting the gtk embedding widget as we speak. I'm going to break it up and see if I can remove the XUL dependencies and use a much more sane design since the current one is pretty organic. Doing that should remove that particular dependency. Off topic but the only thing that I'm concerned about is making sure that key bindings work when I'm done. I don't mind receiving the DOM events and processing them and turning them into scroll events myself but I'm just not sure if the mechanism is there or not to do this.

> The gtk widget implements nsIBaseWindow because it is also the > docshelltreeowner. If it's both the docshelltreeowner *and* implements nsIBaseWindow, it's bypassing a lot of the webBrowser dll. Is there something we're missing with the embedding APIs that forced you to do this, or what?

I think that when I first did the embedding work the answer was yes there were a lot of things missing that I had to implement the tree owner to do. That might not be the case anymore. I'm going to work on my new impl and try to find holes. If I do I'll try and address them and get them fixed in the dll.

Hang on a sec. I've been working under the assumption that nsIWebBrowserSiteWindow is what I should be using. Should I be using the new inteface here? I'm refactoring the embedding widget code and it's much cleaner. I can include these changes as well.

nsIWebBrowserSiteWindow is still in flux. It's being renamed for starters, adam can ellaborate.

Can you please? I'm right in the middle of a rewrite.

Chris, I'll hold off making checkins until your widget has been updated. It'll probably be 3-4 days to complete all the platforms and get approval for me anyway.

Setting for 0.8.1

Adam, do you want me to supply the diffs for the PowerPlant sample?

Conrad, I was going to do the Mac next, but if you would rather do it, feel free! Aside from Powerplant you need to modify the mac embedding project to build and export nsIEmbeddingSiteWindow.idl instead of nsIWebBrowserSiteWindow.idl

Here are the diffs for the PowerPlant embedding sample. While testing it, I found a few oddities: (1) When nsDocShellTreeOwner::SetPositionAndSize calls SetDimensions(), it only sets the DIM_FLAGS_POSITION flag. Since there is no DIM_FLAGS_SIZE, how is this supposed to be interpreted? SIZE is always returned and if DIM_FLAGS_POSITION is set, position as well as size are supposed to be used/returned? Wouldn't it make more sense if there was a DIM_FLAGS_SIZE? (2) From GlobalWindowImpl, the inner size of the window is always set through docShellTreeOwner->SizeShellTo(). It seems that nsDocShellTreeOwner should translate that into a call to nsEmbeddingSiteWindow::SetDimensions() with the DIM_FLAGS_SIZE_INNER flag set. Then we could get rid of nsIWebBrowserChrome::SizeBrowserTo(). (3) All of the Get/SetSize, Get/SetPosition(), etc nsIBaseWindow methods implemented by nsDocShellTreeOwner should use the DIM_FLAGS_SIZE_OUTER flag. This is from looking at the way they are called from GlobalWindowImpl.

Just FYI I've checked in my new embedding code. You'll have to change the code in EmbedWindow.cpp + EmbedWindow.h and it should be pretty straight forward. I haven't implemented the VISIBILITY signal yet because I'm waiting for the API change in this bug.

Conrad, in response to your issues. 1. nsDocShellTreeOwner::SetPositionAndSize does pass both the SIZE and POSITION flags. The line was a bit long so I've wrapped the code to make it clearer both bits are set. 2. Only embedders implement nsIEmbeddingSiteWindow at the moment. The global window can't call methods on that instead of the chrome because XUL windows don't implement the new interface. 3. I will investigate that one. I've written all of the client code so calls to set the inner/outer size are dealt with the same way.

Chris, I'm having a problem with the gtk widget and I think it might be to do with the content docshell. I'm running TestGtkEmbed and it's crashing in GtkMozEmbedChrome::GetPrimaryContentShell apparantly where it tries to AddRef a non-existant mContentShell. Do you have any ideas what might be up? Do I have to build anything else? Other than that the patch is ready for review.

The old widget bitrotted quickly in a period of weeks. I haven't bothered fixing it because I've been rewriting it. Can you try building the new widget? You just have to set a variable but I don't remember what the name is off the top of my head. It's in the Makefile.in in the src/ directory for the widget. Can you patch that code instead and just stub the old one so it builds when you land it? Before I can turn on the new code I have to fix nsIPrompt related problems which I described to Valeski on the phone.

The changeover is pretty boilerplate, but would everyone be kind enough to scan over it for errors? Chris, can I have an sr if the concensus is that it's good?

You're violating 80th columns all over the place. :) I can clean that up after you check in if you want, though. Other than that I don't see anything that's really bad except that EmbedWindow::SetVisibility() doesn't actually emit the VISIBILITY signal. I'll fix that after you check it in, though since I need to test it anyway and make sure that it works in all my cases. Other than that, sr=blizzard.

From nsDocShellTreeOwner.cpp: - return mOwnerWin->SetPositionAndSize(aX, aY, aCX, aCY, aRepaint); + return mOwnerWin->SetDimensions(nsIEmbeddingSiteWindow::DIM_FLAGS_SIZE_OUTER | + nsIEmbeddingSiteWindow::DIM_FLAGS_POSITION, + aX, aY, aCX, aCY); It's still only passing the position flag. Does that mean that in a call to nsIEmbeddingSiteWindow::SetDimensions(), the size (dimension) is always set? I still think it would be clearer if there was DIM_FLAGS_SIZE bit which was set in that case. Otherwise, how does the position get set without changing the size?

Conrad it's passing DIM_FLAGS_SIZE_OUTER and DIM_FLAGS_POSITION. I'm not sure what you think is wrong with that.

Isn't this call setting both the position and size? Maybe I'd better apply the patch to this file to see in more context...

Any progress on this? I need this for my new embedding widget and 0.8.1 is looming.

I rebuilt all platforms today and all looks good so it will be checked in first thing tomorrow (the middle of the night for you).

Thanks everyone. The changes are checked in.

Correction: Changing QA contact for the Embed API bugs to David Epstein.

visibility attribute in nsIEmbeddingSiteWindow. regarding one of the other changes discussed here, I noticed that destroy() is in nsIWebBrowserSiteWindow, but wasn't renamed to destroyBrowserWindow(). was it supposed to be rename?

nsIWebBrowserSiteWindow.idl is obselete. I've just removed it to avoid further confusion.