Closed Bug 269323 Opened 20 years ago Closed 14 years ago

Freeze nsIBaseWindow or provide compatible alternative

Categories

(Core Graveyard :: Embedding: APIs, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX
Future

People

(Reporter: darin.moz, Unassigned)

References

()

Details

Attachments

(1 file)

Freeze nsIBaseWindow or provide compatible alternative.

All of our embedding samples use nsIBaseWindow.  In fact, it isn't possible to
embed Gecko without using this interface.  Unfortunately, the interface is not
exactly in great shape for freezing.  Certainly, we don't want to freeze it with
dependencies on nsIWidget because we don't want to freeze that interface.  But,
yet... if we are to support Eclipse and other embedders going forward, then we
need to continue to support nsIBaseWindow (in at least some capacity).

I propose copying nsIBaseWindow and calling it nsIEmbedBaseWindow, and then
replace nsIWidget with nsISupports.  Maybe that'd be sufficient.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta
Would embeddors ever need the nsIWidget functionality on this interface?  That
is, can we not remove it entirely?
Yes, embedders need to be able to call nsIBaseWindow::InitWindow, and that
function takes a nsIWidget parameter.  NOTE: none of the embedders actually pass
a non-null nsIWidget value though.

However, gtkmozembed does call nsIBaseWindow::GetMainWidget because it needs to
get the native widget corresponding to that nsIWidget.  I don't think Eclipse
uses this API, and none of the embeddings under embedding/tests uses it.
(In reply to comment #2)
> Yes, embedders need to be able to call nsIBaseWindow::InitWindow

Sure.

> and that function takes a nsIWidget parameter.

The point is, the version on the embedding interface should not, probably.  It
can call the same internal function and pass null for the widget and all that. 
But I can't think of a reason an embedder would want to init a basewindow with
an nsIWidget parent.

> NOTE: none of the embedders actually pass a non-null nsIWidget value though.

Exactly.  ;)

> However, gtkmozembed does call nsIBaseWindow::GetMainWidget because it needs
> to get the native widget corresponding to that nsIWidget.

We could probably have a way to get that directly off our interface (similar to
how initWindow takes a parent beastie of that sort directly already).
It would be nice if nsIWidget did not appear in any frozen interfaces. It needs
to evolve considerably.

If embedders want to get a native widget, just give it to them directly as a void*.
It wouldn't be "nice" to avoid nsIWidget, it is an absolute requirement.
nsIWidget is not a freezable interface. We need to come up with alternate
freezable solutions for getting native widget pointers and munging event loops,
if embedders need to call into those tasks.

I don't mind the idea of exposing a [noscript] getNativeWidget(out voidptr);
Folks, much to my dismay we are not in a position to be able to change
nsIBaseWindow.  nsIBaseWindow2 is always an option, but nsIBaseWindow is
effectively frozen already.  We dropped the ball by not providing an alternative
for Moz 1.4, and now we have real embedders to support.  (Please see comment #0.)

Now, that doesn't mean that nsIWidget needs to remain in the frozen version of
the interface.  (We must preserve the ABI, which is not equivalent to the
compile time API.)  We could replace it with nsISupports.  We could say that
NULL must be passed for the second parameter to InitWindow.  We could say that
GetMainWidget always returns NULL, provided we determine that none of the
existing embedders use that method for anything.  (Like I said, I think that
Eclipse and other embedders do not -- so we should be safe.)

Moreover, we could rename the frozen nsIBaseWindow to nsIEmbedBaseWindow or
nsIBaseWindowObsolete (though I prefer nsIEmbedBaseWindow since we don't yet
have a replacement for this interface).  Preserving the vtable and the UUID of
the current nsIBaseWindow is what is important.  We could then change the UUID
of the nsIBaseWindow used internally, and preserve nsIEmbedBaseWindow as the
frozen "base window" interface.

Is everyone OK with this plan?
The problem is that we then saddle all embedders going forward with api cruft...

I think the right thing to do is to create the api we actually want embeddors to
use (more or less nsIBaseWindow, but with nsIWidget removed).  Then we declare
that the official embedding api.  We do what you suggested for the existing api
and mark it as deprecated.  That way embeddors can use the new, cleaner, api if
they want...
OK, is anyone volunteering to create the new-and-improved nsIBaseWindow for
embedders in the 1.8 timeframe? ;-)  I am not sure that I will have the cycles
for that.  I will proceed with nsIEmbedBaseWindow, and we can mark it deprecated
once we have an alternative.
> Now, that doesn't mean that nsIWidget needs to remain in the frozen version of
> the interface.  (We must preserve the ABI, which is not equivalent to the
> compile time API.)  We could replace it with nsISupports.  We could say that
> NULL must be passed for the second parameter to InitWindow.  We could say that
> GetMainWidget always returns NULL, provided we determine that none of the
> existing embedders use that method for anything.  (Like I said, I think that
> Eclipse and other embedders do not -- so we should be safe.)

These all sound good. Assuming there are no users of parentWidget, that should
be defined to always return null too.

> Moreover, we could rename the frozen nsIBaseWindow to nsIEmbedBaseWindow or
> nsIBaseWindowObsolete (though I prefer nsIEmbedBaseWindow since we don't yet
> have a replacement for this interface).

Sounds good but I suggest we use nsIBaseWindowObsolete.
> Sounds good but I suggest we use nsIBaseWindowObsolete.

Create a frozen alternative, and I'll happily use that name. ;-)

It is wrong to deprecate an interface (or call it obsolete) without first
providing an alternative.
Here's a patch that basically implements what I suggested.  The existing
nsIBaseWindow is copied to nsIEmbeddingBaseWindow, and all occurances of
nsIWidget are replaced by nsISupports.	Finally, the IID of nsIBaseWindow is
changed.

nsWebBrowser and nsDocShell are modified to implement nsIEmbeddingBaseWindow. 
nsWebBrowser no longer implements nsIBaseWindow, but nsDocShell implements both
interfaces.  Right now, I hack it so that nsDocShell::QI returns the same impl
for both interfaces.
Drive-by comments:

Do we want to document whether it's gecko that implements
nsIEmbeddingBaseWindow, or embedders, or either?  If embedders can implement it,
we should probably say something about what other sorts of interfaces they're
likely to need to implement...

What units are the coordinates and lengths in?

Are setting positions and sizes sync, async, or unspecified?

What do |fRepaint| and |repaint| actually do?

The documentation on parentWidget says that it corresponds to
parentNativeWindow... but it actually corresponds to whatever was passed to
initWindow, no?  Garbage in, garbage out...

What happens when SetVisibility is called on non-toplevel windows?  Similar for
SetEnabled?  And SetBlurSuppression?  Note that every single docshell implements
this interface, for instance.

What does "The implementation should allow for blur events to be suppressed
multiple times" mean, exactly?
cvs history suggests that i should ask travis those questions :-/, but maybe...

danm: do you have any idea how to answer bz's questions in comment #12 (in
particular the one about blur suppression)?  thanks!
Are you sure CVS didn't credit Travis merely for adjusting the indentation?

Frankly I've become quite fuzzy on the subject of embedding Gecko. I think that
nsIBaseWindow, and by extension the new nsIEmbeddingBaseWindow, *can* be
implemented by embedd(e/o)rs but that will be unnecessary for all but the most
ambitious projects. The best information I know of is the sample embedding
projects in the CVS repository. I suggest leaving a discussion of the interfaces
required to be implemented to the -- ahem -- documentation.

I don't know of any exceptions to the statement that all units in this interface
are expressed in pixels.

Well... |fRepaint| and |repaint| allow the window to be resized without issuing
paint or invalidation events. It's used in a few places, mostly with windows not
yet visible. It could be reasonable to at least give the two parameters the same
name.

> Are setting positions and sizes sync, async, or unspecified?

Let's take door #3. All implementations that I know of just toddle off and get
it done but I don't want to have to guarantee what happens when we cross the OS
border.

> ... [parentWidget] actually corresponds to whatever was passed to initWindow

I don't follow. We want the caller to supply either a parentWidget or a
parentNativeWindow. Embedding apps typically supply the latter, Mozilla the former.

> What happens when SetVisibility is called on non-toplevel windows?

I'm not certain. Some of these functions are probably redundant. I know that the
process of setting visibility on a top-level window involves setting the
visibility on both the window widget itself and on its docshell. There's an old
comment by Travis in nsXULWindow::SetVisibility wondering about the necessity.
Got me. The practice predates both of us. Whether it's still important; good
question.

> What does "The implementation should allow for blur events to be suppressed
> multiple times" mean, exactly?

It means it needs to be a counter, not a flag. Feel free to adjust for clarity.
Danm: Thanks for the comments.  Much appreciated.  I'll try to work those into
the documentation for nsIEmbeddingBaseWindow.
Priority: -- → P1
Target Milestone: mozilla1.8beta1 → mozilla1.8beta2
Target Milestone: mozilla1.8beta2 → mozilla1.9alpha
Target Milestone: mozilla1.9alpha → Future
Assignee: darin → nobody
Status: ASSIGNED → NEW
Priority: P1 → --
QA Contact: apis
We no longer freeze interfaces.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: