161623 - Embedding APIs are overly burdensome on Mac

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Simon Fraser [no longer active]]

Gecko embedding for Mac applications seems overly burdensome and complex.
Embedders have to talk to nsIWidget and nsIBaseWidget directly (neither of which
are frozen APIs), and stuff nsIWidgets onto native WindowRefs.

Here's an approximation of the code required:

  nsCOMPtr<nsIWidget> newWidget(do_CreateInstance(kWindowCID, &rv));
    
  nsRect r(0, 0, 32000, 32000);
  rv = newWidget->Create(theWindow, r, nsnull, nsnull, nsnull, nsnull, nsnull);
  
  err = ::SetWindowProperty(theWindow, 'PPMZ', 'WIDG', sizeof(nsIWidget*),
(void*)&newWidget.get());
  
  mEventSink = do_QueryInterface(aWidget, &rv);
  
  Rect clientrect = {0, 0, m_height, m_width};
  nsRect   r(clientrect.left, clientrect.top, clientrect.right -
clientrect.left, clientrect.bottom - clientrect.top);  
  rv =
mWebBrowserAsBaseWin->InitWindow(aWidget->GetNativeData(NS_NATIVE_WIDGET),
nsnull, r.x, r.y, r.width, r.height);
  rv = mWebBrowserAsBaseWin->Create();
  rv = mWebBrowserAsBaseWin->SetVisibility(PR_TRUE);
  
  newWidget->Show(PR_TRUE);

Yuck. All this stuff should 'just happen'.

Comment 1

[Simon Fraser [no longer active]]

In addition to the above, the embedder has to make an nsILocalFileMac which
points to the location of the mozilla binaries, and implement an
nsIDirectoryServiceProvider to pass to NS_InitEmbedding(). It would be nice if
one could pass nil to NS_InitEmbedding() for both, and have it do some sensible
default behaviour (use process dir, and pick some sensible profile location).

It would also be nicer if NS_InitEmbedding() could use platform-native types,
rather than forcing the embeddder to have to deal with nsILocalFiles up front.

Another issue with Mac CFM embedders is that they have to do the CFM alias trick
to get 'Essential Files' into the search path, and they have to suck in our
NSStdLib/NSRuntime stuff as well. It's all too much.

Comment 2

[Conrad Carlen (not reading bugmail)]

> Embedders have to talk to nsIWidget and nsIBaseWidget directly

Yes. This does suck.
1. On the other platforms, in this line:
mWebBrowserAsBaseWin->InitWindow(aWidget->GetNativeData(NS_NATIVE_WIDGET)
the first param is just a native window so there's no need to see the widget.
Also, on the Mac, GetNativeData() is unusual in that it returns a widget, not a
WindowRef or WindowPtr.

2. The widget is also needed because that's where we get the nsIEventSink.

Here's what I think should happen: Instead of using nsIBaseWindow, we could have
nsIWebBrowserContainer. It's initialization would take a native window type. It
would also have a readonly "EventSink" attribute. And, it would be a freezable
embedding API.

Cleaning this up in the nsMacWindow code in widget will take some work.


> In addition to the above, the embedder has to make an nsILocalFileMac which
points to the location of the mozilla binaries, and implement an
nsIDirectoryServiceProvider to pass to NS_InitEmbedding()

Not true. You can pass null for either or both of these. The bin directory
defaults to the current process dir and the registry location is that of
mozilla. True, it would be easier to just pass a program name instead of a
directory service provider. Then, the code would make the registry location be a
directory in the platform-standard place, appending the program name. But, then
we're really limited. A directory service provider is more flexible in what it
chooses to override.

> It would also be nicer if NS_InitEmbedding() could use platform-native types,
rather than forcing the embeddder to have to deal with nsILocalFiles up front.

I don't see the advantage of this. nsILocalFile is standard across all platforms
so there is no need to #ifdef NS_InitEmbedding. Also, given a native type, all
that's required to make an nsILocalFile is one simple call to NS_NewLocalFile.

Comment 3

[Michael Dunn]

Some notes from Conrad

Comment 4

[Conrad Carlen (not reading bugmail)]

Some notes on the widget problem, which I think is the biggest:

At this point in PPEmbed:
http://lxr.mozilla.org/seamonkey/source/embedding/browser/powerplant/source/CBrowserShell.cpp#405,
it would look like the code is passing a native window.

That gets us to here in nsWebBrowser.cpp:
http://lxr.mozilla.org/seamonkey/source/embedding/browser/webBrowser/nsWebBrowser.cpp#1032
where a child window is created as passed what would still appear to be a native
window as the parent.

But, finally, we get to this point:
http://lxr.mozilla.org/seamonkey/source/widget/src/mac/nsWindow.cpp#414
and you can see the problem - why the embedding app needs to create a top-level
widget.

nsWindow::Create() needs to be fixed so that it can initialize with a native
window. This would mean that, in the embedded case, there would be no
nsMacWindow in the hierarchy. Problem is, the event dispatching is on that
object. I'm looking at how to split the event handling gear up so that it can be
attached to either a top-level or a child window. Any thoughts on this would be
helpful.

Comment 5

[Conrad Carlen (not reading bugmail)]

Attachment: Avoid having to create top-level widget

This allows a child window to be created without passing an nsIWidget* as the
parent. The child window, if created in a window without a top-level widget,
will make one.

Comment 6

[Simon Fraser [no longer active]]

Index: browser/webBrowser/nsWebBrowser.cpp
===================================================================

+   if (aIID.Equals(NS_GET_IID(nsIEventSink))) {
+      nsCOMPtr<nsIWidget> widget = mParentWidget ? mParentWidget : mInternalWidget;

Maybe a comment to say what nsIEventSink is, and that it only applies on Mac,
just to inform non-Mac hackers.

Index: src/mac/nsMacWindow.cpp
===================================================================

+nsMacContainer::nsMacContainer()
...

I think we should start breaking nsMacWindow.cpp into two separate classes in
different files; one for mozilla-only stuff, and one for embedders and mozilla.
Maybe this is a good time to start.


+        nsrefcnt refcnt;
+        NS_RELEASE2(topWidget, refcnt); // One for the getter,
+        NS_RELEASE2(topWidget, refcnt); // and one to balance Create()

Why not just two NS_RELEASE()? Do you need to get back the refcnt?

+    if (!::IsValidWindowPtr((WindowRef)aNativeParent))

I think IsValidWindowPtr is not a good way to test this; it is intended for
debugging, and could be very slow (checking the consistency of the heap etc).
Are you checking for nil, or could aNativeParent be an nsIWidget*?

Comment 7

[Mike Pinkerton (not reading bugmail)]

the IsValidWindowPtr() was my idea...i forgot that it could be potentially
expensive (heap checking, etc). basically it's either that or walk the window
list or widget list to find the ptr to determine its type.

Comment 8

[Simon Fraser [no longer active]]

How do we get into a state where aNativeParent can be one of two things? Isn't
that rather dangerous? If we _have_ to do it, how about a dynamic_cast to an
nsIWidget?

Comment 9

[Conrad Carlen (not reading bugmail)]

> Maybe a comment to say what nsIEventSink is, and that it only applies on Mac,
just to inform non-Mac hackers.

OK.

> I think we should start breaking nsMacWindow.cpp into two separate classes

I agree. I think the event handler needs to be able to exist on more than one
item in the hierarchy. A top level window made by mozilla may want handle a
click in the title bar or grow box, but not an embedding window.

> Maybe this is a good time to start.

Hmm. I already spent too long on this. There are other things which need to be
fixed in this area first:
* Handling the PLEventQueue with Carbon events (patch on bug 44768)
* Making it so that windows which Gecko makes for dropdown lists are completely
hidden from the embeddor. Right now, they're completely separate from browser
windows and need to be fed events thru some other machanism.

> Why not just two NS_RELEASE()? Do you need to get back the refcnt?

Though I don't care what the refcnt is, I do need what NS_RELEASE2 does: checks
for NULL before release and only sets the var to NULL is it refcnt reached 0.

> I think IsValidWindowPtr is not a good way to test this

Yeah, I looked it up and found that :-( The point of that is to distinguish a
WindowPtr from an nsIWidget*. dynamic_cast isn't available on gcc Carbon builds.

Comment 10

[Conrad Carlen (not reading bugmail)]

I now think that this patch is all wrong. I did it this way in order to avoid
putting #ifdef XP_MAC code into nsWebBrowser.cpp. If nsWebBrowser was passed a
native window (what we want), it could check for a top-level widget and create
it if it didn't exist. It would clearly own it and we would have none of the
icky circular reference problems in this patch. Avoiding #ifdefs is good but if
it makes things so completely unnatural, it's not quite worth it.

Distinguishing whether the param to nsWindow::Create() is an nsIWidget* or a
nsNativeWindow is also skanky, however it's done. The current signature works
given the result of GetNativeData(NS_NATIVE_WIDGET). As I have found, that
absolutely has to be an nsIWidget* on the Mac. The way to avoid this ambiguity
is to add another form of nsIWidget::Create() which would take *only* a native
window and would be used only where the code knew damn well what it had a native
window (nsWebBrowser.cpp). Adding another form of Create() isn't pretty but at
least it's not ambiguous.

Comment 11

[Conrad Carlen (not reading bugmail)]

Attachment: alternate patch

Uses #ifdefs but has clear ownership model, much safer, no need to change
Create(), etc.

Comment 12

[Conrad Carlen (not reading bugmail)]

-> 1.3a

Comment 13

[Kevin Ollivier]

I've been using the alternate patch Conrad uploaded for embedding Mozilla in a Mach-O 
Carbon application (wxWindows, actually), and I've found it to be very useful. The less 
embedding code I have to write, the less I have to maintain and the easier it is to 
understand. Also, since I'm working on wxWindows, I have to write Windows and Linux 
versions of the embedding code, and it helps make the implementations more consistent 
and reduces the amount of #ifdef'd code I have.

I hope this gets into the Mozilla codebase sometime soon so that others can build my 
app without applying this patch! Thanks, Kevin

Comment 14

[Conrad Carlen (not reading bugmail)]

Mass move of 1.3a bugs -> 1.4a.

Comment 15

[Conrad Carlen (not reading bugmail)]

-> 1.4b

Comment 16

[Simon Fraser [no longer active]]

Comment on attachment 95474 [details] [diff] [review]
alternate patch

This would clean up a lot of embedding code.

Comment 17

[Conrad Carlen (not reading bugmail)]

Comment on attachment 95474 [details] [diff] [review]
alternate patch

pink - still remember this stuff? sorry, couldn't think of anyone else.

Comment 18

[Simon Fraser [no longer active]]

Comment on attachment 95474 [details] [diff] [review]
alternate patch


>+NS_IMETHODIMP nsWebBrowser::EnsureTopLevelWidget(nativeWindow aWindow)
>+{
>+#if (defined(XP_MAC) || defined(XP_MACOSX)) && !defined(MOZ_WIDGET_COCOA)
>+    WindowPtr macWindow = NS_STATIC_CAST(WindowPtr, aWindow);
>+    nsIWidget *widget = nsnull;
>+    nsCOMPtr<nsIWidget> newWidget;
>+    
>+    ::GetWindowProperty(macWindow, 'MOSS', 'GEKO', sizeof(nsIWidget*), nsnull, (void*)&widget);

On second thoughts, I don't like this. nsMacWindow::StandardCreate() sets the
widget property on the window, and there is a getter for it in nsToolkit. I
don't like yet another place having code to do something that should be
centralized. We need some shared code that can get and set the widget propery
on a native window.

Comment 19

[Conrad Carlen (not reading bugmail)]

> We need some shared code that can get and set the widget propery
on a native window.

If we do that, it means linking the webbrowser lib with that shared code
(widget, gfx, or something). I don't think having the linkage is worth it here.

Commenting in the code that 'MOSS' and 'GEKO' are frozen seems better.

Comment 20

[Conrad Carlen (not reading bugmail)]

Attachment: updated patch

alternate patch + these changes:
1. EnsureTopLevelWidget now contains fix from bug 162885.
2. More #ifdefs in nsWebBrowser.cpp/.h for this stuff.
3. Window properties are now defined in a header - as discussed with Simon.

Comment 21

[Conrad Carlen (not reading bugmail)]

Attachment: updated patch v2

I removed the bit which constrained the size of the toplevel widget to that of
the gray rgn. Based mainly on this comment:
http://bugzilla.mozilla.org/show_bug.cgi?id=162885#c11, and the fact that not
all apps constrain the size of their windows as PowerPlant does.

Comment 22

[Mike Pinkerton (not reading bugmail)]

+      nsRect r(0, 0, 32000, 32000);
+      rv = newWidget->Create(macWindow, r, nsnull, nsnull, nsnull, nsnull, nsnull);

does this end up creating a buffer of that size anywhere in GFX? that would suck
memory for no good reason.

+      nsRect r(0, 0, 32000, 32000);
+      rv = newWidget->Create(macWindow, r, nsnull, nsnull, nsnull, nsnull, nsnull);
+      NS_ENSURE_SUCCESS(rv, rv);
+      widget = newWidget;

i don't really like the multiple returns from EnsureTopLevelWidget(), but i'll
deal i guess.

+  OSStatus swpStatus = ::SetWindowProperty ( mWindowPtr,
kTopLevelWidgetPropertyCreator,
+      kTopLevelWidgetRefPropertyTag, sizeof(nsIWidget*), &temp );

nit, i think you should indent the wrapped line more.

Comment 23

[Conrad Carlen (not reading bugmail)]

> does this end up creating a buffer of that size anywhere in GFX?

Not that I can measure using top and looking at VSIZE.
I recorded the #s at a few points:
* before and after the call to CreateWidget with huge bounds.
* after the content was loaded but before the window was shown
* after the window was shown
* after creating a 2nd window

During several of those steps, much of Gecko ends up being loaded, so it's a but
hard to tell. I then changed the size from 32,000 square to 500 and repeated the
measurements. The VSIZE #'s were the same for that run.

BTW, breaks in nsDrawingSurfaceMac::Init() were not hit at any point. If we were
allocating these buffers, I would think it would happen there?

Comment 24

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 125440 [details] [diff] [review]
updated patch v2

r=pink

Comment 25

[Conrad Carlen (not reading bugmail)]

Comment on attachment 95474 [details] [diff] [review]
alternate patch

canceling old r= requests

Comment 26

[Conrad Carlen (not reading bugmail)]

Comment on attachment 125421 [details] [diff] [review]
updated patch

canceling old r= requests

Comment 27

[Conrad Carlen (not reading bugmail)]

Checked in.