67496 - Embedding APIs and JS window.open() with size specified...

Status: VERIFIED INVALID

(Core Graveyard :: Embedding: APIs, defect)

Description

[Chak Nanga]

Currently when one opens a JS window with the height and width specified in the
embedding mode the following happens:

1. The nsIWebBrowserChrome::CreateBrowserWindow() is called with default
height/width specified even though the JS window.open() specifies a size

2. The embedder's code creates/shows a window with a default size(since no
sizing info. was passed in to CreateBrowserWindow())

3. Then a call to nsIWebBrowserChrome::SizeBrowserTo() is made.

This call sequence results in window "flicker"  since the embedder first
creates/shows a window with a default size and then resizes it in response to
the next interface method invocation.

The embeddor could hide the window in the Create() phase and then show it during
the SizeBrowserTo() call which will work in the case of the size specified. It
will fail in the case of no size being provided since a call to SizeBrowserTo()
will not be made and hence the window will never be displayed.

This used to work before the nsIWebBrowserSiteWindow() changes since the
nsIBaseWindow::SetVisibility() method was being invoked after the Create/Resize
calls and the emebedder could use that to decide when to actually show the
window on the screen(thus doing the resize in the background)

nsIWebBrowserSiteWindow() does not have the SetVisibility() fucntionality any more.

We should either specify the given sizing info. in the first call to
CreateBrowserWindow() or provide some other kind of support to handle this scenario.

Comment 1

[Judson Valeski]

sizing info in the initial createwindow() call is the right move. who's up for 
doing this?

Comment 2

[Chak Nanga]

Per Jud's suggestion, i'm going to take a look at this one to get my feet wet in
the interal embedding API imlementations.

Could you please give me your comments/suggestions on the best way to go about
getting this one fixed?....Thanks

Comment 3

[Chak Nanga]

Here's some additional info. on this one:

In response to a JS window.open() the following takes place:

<skipping some misc. calls here>

0. GlobalWindowImpl::OpenInternal() is invoked

1. GlobalWindowImpl::OpenInternal() calls DocShellTreeOwner::GetNewWindow()
to create a new window with default size, loads the URI if one's
specified and then calls GlobalWindowImpl::SizeOpenedDocShellItem()
to size the window. (I'm guessing there's a specific reason for this
call order?)

In addition to other things the SizeOpenedDocShellItem() checks with
the Security manager to validate the window sizes before issuing a call
to size the browser window.

Finally, the SetVisibility() method is called to show the window (which
is no longer there after we switched to the  nsISiteWindow interface but
one which Adam's bringing back to life in nsIEmbeddingSiteWindow)

If we were to pass in the window sizing info. via
nsDocShellTreeOwner::GetNewWindow() then we'd need to do the following:

1. Change the nsDocShellTreeOwner::GetNewWindow() interface method
to specify the window sizes as args
By doing this we may be bypassing the window size security checks and need to
change others who're already implementing this interface.

Or

We could use the SetVisibility() method in nsIEmbeddingSiteWindow interface and
leave everything as is.

Please comment....

Comment 4

[Conrad Carlen (not reading bugmail)]

> We could use the SetVisibility() method in nsIEmbeddingSiteWindow interface and
> leave everything as is.
This is definately the way to go. Once nsIEmbeddingSiteWindow is in and we have
SetVisibility() again, things will work and be compatible with the rest of the code.

Comment 5

[Dan M]

  I'm not sure what this bug is objecting to. Is it the flicker when a window is 
sized, or the strange situation we have where 
nsIWebBrowserChrome::CreateBrowserWindow has size and position parameters, but 
the functions that call this method don't have that information to send along? 
Probably both.
  I remember complaining about that second thing myself once. But now that I 
think about it, we (currently) have no use in Mozilla for those parameters. Are 
we sure we want them in the embedding API, then? We can't necessarily know the 
size of the window before opening it (if its size was specified by giving the 
size of the content area (since the size of the chrome is unknowable until it has 
been loaded)). An embedding app may have fixed chrome and be able to adjust, but 
Mozilla-the-app can't. (Also note the given interface, with just one set of size 
parameters, isn't rich enough to handle sizing by content, anyway.)
  In Mozilla, windows are always created invisible. There'd be a festival of 
flashing otherwise. We calculate the window's correct size and position after it 
has been opened, and may even delay making the window visible until well past the 
call to SetVisibility that you've noticed in SizeOpenedDocShellItem, all to 
reduce flashing.
  It would be more efficient if we did take our best shot at a correct size (and 
perhaps position) and pass it along to the CreateBrowserWindow call. We would 
often not need to correct that after opening the window. Seems like that could 
possibly speed window opening (though it didn't seem to in a quick stopwatch test 
I just ran.)
  In the end, the important thing is for embedding apps to create their windows 
invisible and support SetVisibility, like Mozilla. (And conceivably be prepared 
to defer the SetVisibility, like Mozilla, though that's done for reasons that 
likely won't apply to an embedding app.)

Comment 6

[Conrad Carlen (not reading bugmail)]

> This call sequence results in window "flicker"  since the embedder first
> creates/shows a window with a default size and then resizes it in response to
> the next interface method invocation.

Basically, the embeddor should not create the window to be visible in
CreateBrowserWindow(). When nsIWebBrowserSiteWindow was created, without a
visibility attribute, the embeddor was forced to, and the result was flicker.
The lack of a visibility attribute is a temporary regression and once that's
back, things will be good. That's bug 68581.

So, I don't think this bug is so valid. BTW, if there's never a case in which
the actual size will be known when CreateBrowserWindow is called, maybe we
should remove the size parameters from it. This would make it clear that the
size is unspecified and that the embeddor must depend on the call to
SizeBrowserTo() - or some sizing method. If, at the point that SetVisibility()
is called and the size is still not known, the embeddor needs to wait until the
chrome loads, and determine the size from that. That's what the Powerplant
embedding sample does. Problem is, it's using
nsIMarkupDocumentViewer::SizeToContent() to handle this case and that interface
is not sanctioned. The SizeToContent() functionality should be moved elsewhere.

Comment 7

[Chak Nanga]

Marking it invalid. Please see Conrad's/Danm's and my comments below on why this 
is invalid.

This issue will be resolved once Adam lands his nsIEmbeddingSiteWindow changes.

Comment 8

[Michael Dunn]

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

Comment 9

[David Epstein]

verifying as invalid.