46852 - nsIBaseWindow needs to be split into two interfaces

Status: VERIFIED FIXED

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

Description

[Judson Valeski]

This will remove confusion about the bi-directional nature of the current 
semantics.



     Needs to be split into two interfaces: 
          The methods implemented by the embedding application should stay in
          nsIBaseWindow - destroy(), setPosition(), getPosition(), setSize(),
          getSize(), setPositionAndSize(), getPositionAndSize(), visibility,
          setFocus(), title, repaint() 

          The down call methods used by the embedding application could be
          moved into a new public interface - initWindow(), create(), destroy(),
          setSize(), repaint(), setFocus(). Both nsIBaseWindow and the new
          interface will be implemented by nsWebBrowser.

Comment 1

[Judson Valeski]

Now that I'm diving into this one, I'm not sure of it's relevance anymore. We
want embedding windows to look and feel just like regular windows (nsIBaseWindow
as is). We also want embedding apps to be able to manipulate browser windows,
thus both parties impl nsIBaseWindow. Can someone refresh me on the relevance
and logic surrounding the split?

Comment 2

[lchiang]

per email with Jud, changing nsbeta3+ to nsbeta3- on all "embed" keyword bugs
since embedding changes will not be made in the mn6 branch. If you feel this bug
fix needs to go into mn6 branch, please list the reasons/user impact/risk and
nominate for rtm. Thanks.

Comment 3

[Judson Valeski]

over to adam.

Comment 4

[Paul Wyskoczka]

Updating QA Contact

Comment 5

[Adam Lock]

Attachment: New nsIWebBrowserSiteWindow interface and impl

Comment 6

[Adam Lock]

This new interface contains a subset of the methods on nsIBaseWindow and is 
intended for implementation by the embedders chrome/site object. I've changed 
nsDocShellTreeOwner.cpp so that the embedder can choose which interface to 
implement - nsIWebBrowserSiteWindow or nsIBaseWindow. The implementation of 
nsIBaseWindow in nsDocShellTreeOwner has been changed to work with either.

Comment 7

[Judson Valeski]

couple questions:
1. why support either? shouldn't we just support the sitewindow?
2. the comment at the top of nsIWebBrowserSiteWindow.idl says "nsIBaseWindow"
where I think you want to say "nsIWebBrowserSiteWindow."?

Comment 8

[Adam Lock]

1. We could just support just the site window but at the risk of breaking every 
embedders existing code. If that isn't an issue I can chop the backwards 
support.
2. That's fixed.

If you think I should change the code for 1) I'll resubmit a new patch.

Comment 9

[Judson Valeski]

please re-submit. we're not shooting for backwards compat right now.

Comment 10

[Adam Lock]

Attachment: Patch without backwards compatibility. gtkEmbed patch still to do

Comment 11

[Adam Lock]

Attachment: Updated patch including changes to gtkEmbed

Comment 12

[Judson Valeski]

can you do a diff -u . It's hard to tell what methods have been moved where.

cc'ing conrad as he wanted to make sure there was a particular method being 
propegated.

Comment 13

[Adam Lock]

Attachment: Unified diff of the above

Comment 14

[Adam Lock]

Conrad, can you check to see if what you need is there?

If it is I'll submit this for review

Comment 15

[Conrad Carlen (not reading bugmail)]

So far, it looks good. I'll work it into my embedding app, and see how it fits
together.

Comment 16

[Adam Lock]

Attachment: Patch for the gtk widget

Comment 17

[Sebastian Späth]

Will the embedding API web page on mozilla.org be updated to reflect these API changes? That would be great...

Comment 18

[Adam Lock]

Everything has been checked in now except for the ActiveX control

Comment 19

[Adam Lock]

It's all checked in now, marking FIXED

Comment 20

[Michael Dunn]

Verified fixed -- source changed

Comment 21

[Conrad Carlen (not reading bugmail)]

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 shouldn't complain because I said the patch looked fine but I had
not actually applied it and tested the JS dialog case.