68581 - [API]We need visibility attribute on nsIWebBrowserSiteWindow

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P2)

Description

[Conrad Carlen (not reading bugmail)]

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.

Comment 1

[Adam Lock]

Attachment: Proposed new nsIEmbeddingSiteWindow.idl

Comment 2

[Adam Lock]

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.

Comment 3

[Adam Lock]

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?

Comment 4

[Dan M]

  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.

Comment 5

[Conrad Carlen (not reading bugmail)]

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

Comment 6

[Adam Lock]

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.

Comment 7

[Christopher Blizzard (:blizzard)]

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.

Comment 8

[Conrad Carlen (not reading bugmail)]

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

Comment 9

[Christopher Blizzard (:blizzard)]

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.

Comment 10

[Adam Lock]

Attachment: Linux diffs for new interface

Comment 11

[Christopher Blizzard (:blizzard)]

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.

Comment 12

[Judson Valeski]

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

Comment 13

[Christopher Blizzard (:blizzard)]

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

Comment 14

[Adam Lock]

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.

Comment 15

[Adam Lock]

Setting for 0.8.1

Comment 16

[Adam Lock]

Attachment: Win32 and Linux diffs

Comment 17

[Conrad Carlen (not reading bugmail)]

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

Comment 18

[Adam Lock]

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

Comment 19

[Conrad Carlen (not reading bugmail)]

Attachment: PowerPlant Diffs

Comment 20

[Conrad Carlen (not reading bugmail)]

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.

Comment 21

[Christopher Blizzard (:blizzard)]

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.

Comment 22

[Adam Lock]

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.

Comment 23

[Adam Lock]

Attachment: Diffs for Linux, Win32 & Powerplant

Comment 24

[Adam Lock]

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.

Comment 25

[Christopher Blizzard (:blizzard)]

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.

Comment 26

[Adam Lock]

Attachment: Update to the new gtk widget

Comment 27

[Adam Lock]

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?

Comment 28

[Christopher Blizzard (:blizzard)]

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.

Comment 29

[Conrad Carlen (not reading bugmail)]

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?

Comment 30

[Adam Lock]

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

Comment 31

[Conrad Carlen (not reading bugmail)]

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

Comment 32

[Christopher Blizzard (:blizzard)]

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

Comment 33

[Adam Lock]

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

Comment 34

[Adam Lock]

Thanks everyone. The changes are checked in.

Comment 35

[Michael Dunn]

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

Comment 36

[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?

Comment 37

[Adam Lock]

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