69922 - intrinsic sizing is broken in embedded apps

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P1)

Description

[Dan M]

Reported by -- errrr -- I forget who. Conrad? Windows sized intrinsically don't 
change their size in embedded apps; even those apps that have window sizing 
hooked up.

Comment 1

[Christopher Blizzard (:blizzard)]

I'm going to take a wild guess here as to why this is happening.  I'll bet that
since we changed the interface to nsIEmbeddingSiteWindow the code that does the
resizing assumes that you're still using the old interfaces and can't QI the
object to it and can't therfore resize.  Possible?  I haven't looked at any code
so I'm just flying blind here. :)

Comment 2

[Conrad Carlen (not reading bugmail)]

No, that's not it. Intrinsic sizing has never worked. Also, it's not exactly
broken but just not implemented. The embedding app has to know not only how to
do intrinsic sizing (by calling SizeToContent()) but when (if the window is
chrome, and if it's never had its size explicitly set). The reason this bug
exists is to move SizeToContent to a public iface OR to have the webbrowser lib
handle sizing to content automatically so the embeddor doesn't have to deal with
it. 

Comment 3

[Christopher Blizzard (:blizzard)]

Strange.  This used to work for me when I was using the old interfaces.  But at
that time I was returning an nsIDOMWindowInternal and I was using the
appshellservice ( ASS ) to get XUL windows.  I suppose those support intrinsic
resizing.

I'm still confused as to why the external interfaces require specific support to
handle intrinsic resizing.  To the window it just looks like a |SetDimensions|
and the flags for the chrome around the window are passed in as normal.  For
example, my embedding widget looks fine except for the size.  The content looks
great and seems to load just fine.

Is it as simple as finding the inner DOM window and calling |SizeToContent|? 
I've already got a progress listener attached to the window and I think that
should report when the load finishes.  I might have to attach to the inner
window in the chrome case but I haven't played with it enough to know yet.

Comment 4

[Conrad Carlen (not reading bugmail)]

> Strange.  This used to work for me when I was using the old interfaces.  But at
> that time I was returning an nsIDOMWindowInternal and I was using the
> appshellservice ( ASS ) to get XUL windows.  I suppose those support intrinsic
> resizing.

That's right - XUL windows with appshell do the intrinsic sizing. The webbrowser
lib does not.

> I'm still confused as to why the external interfaces require specific support
> to handle intrinsic resizing.

That's why I thought this was a pain and wondered if we could hide this inside
webbrowser and have it just call out to nsIEmbeddingSiteWindow to do the actual
sizing once it had been determined.


> Is it as simple as finding the inner DOM window and calling |SizeToContent|?

Almost. Another thing to watch for is that you may get a call to SetVisibility()
before the load happens and you have a chance to size to content. In this case
your impl of SetVisibiity should not immediately make the window visible but set
a flag which says it should be made visible as soon as you can determine the size.

Comment 5

[Christopher Blizzard (:blizzard)]

I'll do some playing once my build finishes and see if I can figure out a way to
work around this for 0.8.1 and something that might fit into the webbrowser lib.

Comment 6

[Christopher Blizzard (:blizzard)]

Thanks for the tips ccarlen.  I've implemented what you described in my new
embedding widget and it works very well.

I'd like to turn the new embedding widget on for 0.8.1 as I've described in bug
72224.  Can people from here review?  They are most qualified.

Comment 7

[Peter Trudelle]

->P1

Comment 8

[arik devens]

dedooming dan

Comment 9

[Marek Z. Jeziorek]

How's the progress on this one? I hope to see it by 0.9.

Comment 10

[Christopher Blizzard (:blizzard)]

It works in the gtk embedding widget.  I don't know about the other implementations.

Comment 11

[arik devens]

Attachment: i think this should fix it, not sure i got all the code changed though

Comment 12

[arik devens]

this should do the trick, if someone could verify i found all the code that was
using the old method that would be great

Comment 13

[Conrad Carlen (not reading bugmail)]

Here's another:
http://lxr.mozilla.org/seamonkey/source/embedding/browser/powerplant/source/CBrowserWindow.cp#559.

Comment 14

[Christopher Blizzard (:blizzard)]

And:

http://lxr.mozilla.org/seamonkey/source/embedding/browser/gtk/src/EmbedPrivate.cpp#488

Comment 15

[Christopher Blizzard (:blizzard)]

Lots of things here:

http://lxr.mozilla.org/seamonkey/ident?i=SizeToContent

Comment 16

[arik devens]

ok, attaching a new patch which fixes (i hope ;-) all the issues mentioned.

Comment 17

[Brendan Eich [:brendan]]

Cc'ing jst to bless the DOM IDL change in the patch.

/be

Comment 18

[Johnny Stenback (:jst)]

DOM changes look fine to me, please note that binary compatibility will be
broken for code that uses nsIDOMWindowInternal since this method is moved, but
if that's ok with you it's fine with me in this case.

r/sr/a=jst for the IDL changes (and r=jst for the other change), I'm making the
same interface change on the XPCDOM branch right now.

Comment 19

[Peter Trudelle]

Please update ETA, current status.

Comment 20

[Marek Z. Jeziorek]

we seem to have everything ready. when could this be checked-in?

Comment 21

[arik devens]

everything is not ready, i haven't tested (waiting for mozilla to build right
now to test) all the code path changes that i had to make to the code that was
using the old interface.

i also don't have an sr yet...

hopefully i will have both before i leave today

Comment 22

[Judson Valeski]

it would also be good to test this on mfcEmbed. we could really use this in 0.9.

Comment 23

[arik devens]

afaik mfcEmbed doesn't currently have any support for intrinsic sizing, it would
be great to add it but i don't know any mfc so someone else might have to do it
;-)

anyway modified patch (with a few more instances of size_to_content) should be
up in a few minutes, i am sorting through some stuff right now

Comment 24

[Brendan Eich [:brendan]]

Patch covering all SizeToContent calls coming up soon?

/be

Comment 25

[arik devens]

currently diffing, should post new patch rsn

Comment 26

[arik devens]

Attachment: this should do it ;-)

Comment 27

[Brendan Eich [:brendan]]

r/sr=[e-mail]

/be

Comment 28

[Mike Shaver (:shaver emeritus)]

Are we going to regress this when the xpcdom branch lands? Arik, could you whip
up a fix for that Brave New World as well?

Comment 29

[arik devens]

my understanding is that jst has already fixed this on the xpcdom branch and we
should be fine...

if i'm wrong i could do a fix ;-)

Comment 30

[arik devens]

i guess winEmbed added this feature today, will add new patch with patch for
winEmbed

Comment 31

[Dan Rosen]

One comment, from the "in my day, we had to walk to school in the snow, in our
underwear, uphill both ways" perspective: If you can hunt down the equivalent
methods in our other various and sundry testbeds which should or do call
SizeToContent(), and patch those calls, that would be good.

You want to make sure you get at least winEmbed, mfcEmbed, gtkEmbed, gtkmozembed
and powerplant, if appropriate for each. Alternatively, make sure the API is
well-documented -- this seems to be the exception rather than the norm, though.

See danm if you feel like whining ;)

Besides that, though, r=dr. If you *really* wanted, you could check in as the
patch stands, but you'd probably rather your work be visible to those who want
to use it...

Comment 32

[arik devens]

Attachment: updated patch that fixes winEmbed

Comment 33

[Johnny Stenback (:jst)]

I already took care of this on the XPCDOM branch...

Comment 34

[Christopher Blizzard (:blizzard)]

a=blizzard for 0.9  Let's get it in.

Comment 35

[arik devens]

landed on trunk.