256560 - Patch for gtkmozembed for compiling in win32 using Gtk+ 2.x

Status: RESOLVED WONTFIX

(Core Graveyard :: Embedding: GTK Widget, defect)

Description

[Todd A. Fisher]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3

I've put together a page documenting how to get gtkmozembed to compile in win32
and run with a few issues.  The main issue being changing focus between the
mozembed widget and other gtk widgets.  

here's the link: http://severna.homeip.net/gtkmozwin32.php

The patches include changes to the makefiles and source files.

It was also necessary to modify the configure script as it seems to only 
search for glib-1.2 instead of glib-2.x

The area that I'm most concerned with is in gtk_moz_embed_realize.  It appears
it will be difficult to notification of a focus change of the child widget.
Currently, gtkmozembed looks to its child to know when a focus change is
happening.  And it appears, it assumes a gtk+ backend, which is not the case in
win32.  So, I'm hoping someone who understands the win32 backend better can fix
this :-)

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Todd A. Fisher]

Attachment: Patch for compiling gtkmozembed in win32

Comment 2

[Todd A. Fisher]

Attachment: Patch for compiling mozilla against gtk+ 2.x in win32

Comment 3

[Benjamin Smedberg]

Comment on attachment 156766 [details] [diff] [review]
Patch for compiling gtkmozembed in win32

Here are some drive-by comments:

>Index: EmbedContentListener.cpp

>-#include <strings.h>
>+#ifdef XP_PC
>+  #include <string.h>
>+  #define strcasecmp(x,y) (_stricmp(x,y))
>+#else
>+  #include <strings.h>
>+#endif
>+

Use XP_WIN instead of XP_PC
Also, prevailing mozilla stylo does not indent nested preprocessor directives.

>+++ EmbedPrivate.cpp	6 Jul 2004 15:36:28 -0000

>+#ifndef XP_PC
>   // get the native drawing area
>   GdkWindow *tmp_window =
>     NS_STATIC_CAST(GdkWindow *,
>@@ -269,6 +270,9 @@
>   gpointer data = nsnull;
>   gdk_window_get_user_data(tmp_window, &data);
>   mMozWindowWidget = NS_STATIC_CAST(GtkWidget *, data);
>+#else
>+	mMozWindowWidget = NULL;
>+#endif

1) could you explain this a bit
2) moz code should never have tabs, even if the existing file uses tabs (use
spaces)

>Index: EmbedWindow.cpp

> nsresult
>-EmbedWindow::CreateWindow(void)
>+EmbedWindow::CreateMozWindow(void)

Why did you rename this method?

> {
>   nsresult rv;
>   GtkWidget *ownerAsWidget = GTK_WIDGET(mOwner->mOwningWidget);
>@@ -73,7 +82,17 @@
>   // Get the base window interface for the web browser object and
>   // create the window.
>   mBaseWindow = do_QueryInterface(mWebBrowser);
>-  rv = mBaseWindow->InitWindow(GTK_WIDGET(mOwner->mOwningWidget),
>+  rv = mBaseWindow->InitWindow(
>+        #ifdef XP_PC
>+            #ifdef MOZ_WIDGET_GTK2
>+               nsNativeWidget( GDK_WINDOW_HWND( GTK_WIDGET( mOwner->mOwningWidget )->window ) ),
>+            #endif
>+            #ifdef MOZ_WIDGET_GTK
>+               nsNativeWidget(GDK_DRAWABLE_WIN32DATA (GTK_WIDGET(mOwner->mOwningWidget)->window)->xid),
>+            #endif
>+        #else
>+             GTK_WIDGET(mOwner->mOwningWidget),
>+        #endif

Putting #ifdefs in the middle of a function call makes this unreadable. Go
ahead and wrap the entire call, even if that involves duplicating a few lines
of code.

>Index: Makefile.in

>-SHARED_LIBRARY_LIBS= \
>-		$(DIST)/lib/libembed_base_s.$(LIB_SUFFIX) \
>-		$(DIST)/lib/libprofdirserviceprovider_s.$(LIB_SUFFIX) \
>-		$(NULL)
>+ifeq ($(OS_ARCH), WINNT)
>+  SHARED_LIBRARY_LIBS= \
>+  		$(DIST)/lib/embed_base_s.$(LIB_SUFFIX) \
>+		  $(DIST)/lib/xpcom.$(LIB_SUFFIX)\
>+		  $(DIST)/lib/profdirserviceprovider_s.$(LIB_SUFFIX) \
>+  		$(NULL)
>+else
>+  SHARED_LIBRARY_LIBS= \
>+  		$(DIST)/lib/libembed_base_s.$(LIB_SUFFIX) \
>+  		$(DIST)/lib/libprofdirserviceprovider_s.$(LIB_SUFFIX) \
>+  		$(NULL)
>+endif

moz makefiles can do this for you: $(LIB_PREFIX)embed_base_s.$(LIB_SUFFIX), no
ifdefs needed

>Index: gtkmozembed.h

>+#ifndef NS_EXPORT
>+	#ifdef _MSC_VER
>+		#define NS_EXPORT  __declspec( dllexport )
>+	#else
>+		#define NS_EXPORT
>+	#endif
>+#endif

NS_EXPORT is declared in nscore.h, you should #include this file rather than
declaring it here separately.

Comment 4

[Todd A. Fisher]

Attachment: Revised patch

Addressed the issues with the previous patch for /embedding/browser/gtk/src

Comment 5

[Todd Fisher]

Attachment: Focus Handler WinProc

I added this class which hooks into the window proc of the main window
embedding the gtkmozembed widget.  This makes it possible to send the
appropriate signals to the gtk widgets and to the mozembed window.  It is very
rough around the edges but I wanted to post it here to get feed back.

Comment 6

[Todd Fisher]

Attachment: Header file for FocusHandler

Comment 7

[Todd Fisher]

Attachment: Patch for attaching the FocusHandler object

Comment 8

[Todd Fisher]

Attachment: Updated FocusHandler

This appears to have fixed the issue of switching focus.  I left a lot of
commented out code in there so show the different ideas I tried.  But, what I
found seems to really work is a combination of SendMessage,
gdk_keyboard_ungrab, and gdk_pointer_ungrab.

I think someone with a better understand of win32 event loop can probably make
a pass through this and clean it up and fill any corner cases I'm probably
missing.  But so far as I can tell with my test program gtkmoz.exe this seems
to fix things, so that a user can switch via a click between any gtkwidget and
the embedded mozilla window.

Comment 9

[Todd Fisher]

Attachment: Cleaned up FocusHandler

My posting above was slightly incorrect.  It doesn't seem like
gdk_key/point_ungrab's are necessary.  
Just the combination of SetFocus and SendNotifyMessage.

I also, removed the static variables from the WinProc into the FocusHandler
object.  

What I'd like to fix next is that the Focus needs to be taken away from a
GtkWidget when the mozilla widget requires it.	Otherwise, the user is left
wondering why.

1. there are two cursors one in the moz embed window and one and in the
gtkentry.
2. It can be confusing when you start to type and type ahead starts working and
you think you should have changed the text in a gtkentry instead.

I think this change should happen in the EmbedPrivate.cpp Focus methods.  What
i'm not sure how to do yet is get the currently focused gtkwidget and tell it
to lose focus when the mozembed widget requests it.

Comment 10

[Todd Fisher]

Attachment: Updated FocusHandler.h

Comment 11

[Todd Fisher]

Attachment: Fixed a Gtk Critical caused by win32 not having a child widget

In the realize function the gtkmozembed widget does not have a child widget so
instead of calling gtk_signal_connect_while_alive we call g_signal_connect.  I
think this should be ok and am not really sure why
gtk_signal_connect_while_alive was used to begin with; but can only guess that
perhaps it had to do with gtk+1.2 support.

Comment 12

[Todd Fisher]

Attachment: Patch to fix widgets left with focus when loading a new gtkmozembed page

This patchs adds a simulated mouse click when the user loads a new page in the
mozembed widget.  This means that pages such as google that request default
focus on their search form will not keep that focus when the page is finished
loading.  The up side of it is that any gtkwidget that currently has focus is
not left in a state between focus and not focus.

Comment 13

[Todd A. Fisher]

Attachment: Fix reparenting bug

Alright, I used the following to wrap a HWND in a GtkWidget

HWND hwnd = NS_STATIC_CAST(HWND,mozWidget->GetNativeData(NS_NATIVE_WINDOW));
mMozWindowWidget = gtk_window_new( GTK_WINDOW_TOPLEVEL ); 
mMozWindowWidget->window = gdk_window_foreign_new( (GdkNativeWindow)hwnd );

This might make it possible to remove the whole FocusHandler kludge.

Comment 14

[Todd A. Fisher]

Attachment: improved patch

I've run a number of different tests from reparenting to viewing many different

websites at the same time.  The only issue this patch and it's accompaning
FocusHandler.cpp/.h has, as far as I can tell, is that the page being loaded is
not given keyboard focus. And any Gtkwidget that currently held focus will not
keep focus after loading the page.  I had to do this because there was a
problem where a window might have a cursor visible, but not have keyboard
focus.

Comment 15

[Todd A. Fisher]

Attachment: C++ file for FocusHandler object

Comment 16

[Todd A. Fisher]

Attachment: C++ header for FocusHandler

Comment 17

[Todd A. Fisher]

Attachment: recreated patch using -up6

Comment 18

[Todd A. Fisher]

Attachment: recreated patch stripping out unnecessary comments and converting all tabs to 2 spaces

Comment 19

[Zac Bowling]

I can confirm that the gtkembedmoz on Win32 does work nearly perfectly with this
patch.

Comment 20

[Zac Bowling]

http://www.polystimulus.com/screenshot-TestGtkEmbed.PNG here is an image of
gtkembedmoz working in win32. I would love to see this patch make into the build
because over at mono we provide a "gecko-sharp" package that wraps gtkembedmoz
and I'm currently porting it to work in WIN32. WIN32 support would also require
that mozilla to build with gtk2 on WIN32 as well. It can be done right now by
tricking it with some other tricky little config file changes are you would have
to overwrite the gtk lib vars locally in the makefile to be the only package
built with gkt2 in mozilla on WIN32. I was thinking of forking gtkmozembed off
temporarly to make it independent in order to make it easier to build without
tricking up your current mozilla sources for WIN32.  

Comment 21

[Benjamin Smedberg]

Zac, are you willing to maintain the gtk/win32 port (compile regularly, fix
problems, etc)? If so, please click "edit" on your last patch and mark
review?[e-mail]

Comment 22

[Todd A. Fisher]

Attachment: Crash test case for gtkmozembed in win32

Erik, a developer of FTD4Linux has used this patch to get his application
working in win32.  He has reported some issues, that may or may not have been
resolved by Zacs work.	He is having problem when gtkmozembed is placed in a
GtkNotebook. 

From his mail:

"My program shows several instances of the gtkmozembed component, each in a
seperate GtkNotebook page. The notebook tabs contain an label and an close
button.
When the user clicks on the close button, the tab should be destroyed.
Under Linux (and other OS'es under my which program works succesfully) this
works without any problem. But under win32 this can result in an crash."

He also provides a test case for the problem, which i've attached

Comment 23

[Zac Bowling]

(In reply to comment #21)
> Zac, are you willing to maintain the gtk/win32 port (compile regularly, fix
> problems, etc)? If so, please click "edit" on your last patch and mark
> review?[e-mail]

I would be happy to. I'm not the world's best C/C++ programmer when it comes to
writing new code from scratch but I'm awesome at porting and bug fixing. I'm
also very peristant in getting things done, and will go out of my way to find
the solution to a bug to get it to work as well as possible in every situation.
I've got the entire support of the Mono project behind me (as a member of
development team) as well as Novell (the official sponser of the Mono project)
which has a interest in this working as it allows their documentaion system,
IDE, any application that uses our wrapper for .NET/Mono for gtkembedmoz
(codename gecko#) to work in WIN32, like the new application Beagle from the
Gnome team so it can work under Windows natively. 

I was just about to post a new gtkembedmoz.dll to my site that I compiled that
use the latest MOZILLA_1_7_BRANCH code for gtkembedmoz (HEAD has some issues I
need to work on later, right before the 1.8 release). 

I haven't had any issues with the gtknotebook issue discribed before with my
builds, and the patch to fix the possible error thrown when destoying seemed to
fix the issue of that happening everytime on WIN32. I have yet to discover why
that is, just trusting the patch and rebuilding. :-)

You can get updates to my dll at http://polystimulus.com/gtkembedmoz-sharp/
right now. It focuses on the gecko# aspect of it (since this was my main focus
in getting this to work) but the dll and included manual install instructions
should work for everyone regardless.

Hope that helps :-)

Zac Bowling
zac@@zacbowling..com

Comment 24

[Benjamin Smedberg]

Comment on attachment 157303 [details] [diff] [review]
recreated patch stripping out unnecessary comments and converting all tabs to  2 spaces

In that case, let's get it reviewed and landed. Caillon, can you do the honors?
It's pretty simple.

Comment 25

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 157303 [details] [diff] [review]
recreated patch stripping out unnecessary comments and converting all tabs to  2 spaces

>@@ -168,13 +181,13 @@ EmbedPrivate::Init(GtkMozEmbed *aOwningW
> 
>   // Create our progress listener object, make an owning reference,
>   // and initialize it.  It is assumed that this progress listener
>   // will be destroyed when we go out of scope.
>   mProgress = new EmbedProgress();
>   mProgressGuard = NS_STATIC_CAST(nsIWebProgressListener *,
>-				       mProgress);
>+               mProgress);

Nit: can you align this up better?  e.g.

  mProgressGuard = NS_STATIC_CAST(nsIWebProgressListener *,
				  mProgress);

This occurs a bit in the file, so please extend the nit to the other places.




>@@ -207,25 +220,72 @@ EmbedPrivate::Init(GtkMozEmbed *aOwningW
>     if (watcher)
>       watcher->SetWindowCreator(windowCreator);
>   }
>   return NS_OK;
> }
> 
>+#ifdef XP_WIN
>+static void 
>+embed_reparent( GtkWidget *widget, 
>+                GtkWidget *new_parent, 
>+                int hide )

Might it make more sense to always define this method, and then #ifdef the body
to call gtk_widget_reparent if not on windows?	You could (de)attach the focus
handler in the method body as well.  This will reduce the random #ifdefing
throughout the actual code flow.

>+{    
>+  if( widget->parent ){
>+    g_print( "EmbedPrivate: gtk-reparent\n" );
>+    gtk_widget_reparent( widget, new_parent );
>+  }
>+  else if( !GTK_WIDGET_TOPLEVEL( widget ) ){
>+    g_print( "EmbedPrivate: gtk-set_parent\n" );
>+    gtk_widget_set_parent( widget, new_parent );
>+  }
>+  else{
>+    g_object_ref( widget );
>+    gtk_widget_unparent( widget );
>+    g_print( "EmbedPrivate: gdk-reparent\n" );
>+    gdk_window_reparent( widget->window, new_parent->window,
>+                         new_parent->allocation.x, 
>+                         new_parent->allocation.y );
>+    g_object_notify( G_OBJECT( widget ), "parent" );
>+    if( !hide ){
>+      HWND hwnd = (HWND)GDK_WINDOW_HWND( widget->window );
>+      gdk_window_show( widget->window );
>+//    This is done to simulate a window resize which appears to be
>+//    the only way to get the mozembed window to show itself
>+      MoveWindow( hwnd, new_parent->allocation.x, 
>+                        new_parent->allocation.y,
>+                        new_parent->allocation.width,
>+                        new_parent->allocation.height,TRUE );
>+    }
>+  }
>+}
>+#endif
>+
> nsresult
> EmbedPrivate::Realize(PRBool *aAlreadyRealized)
> {
> 
>   *aAlreadyRealized = PR_FALSE;
> 
>   // create the offscreen window if we have to
>   EnsureOffscreenWindow();
> 
>-  // Have we ever been initialized before?  If so then just reparetn
>+  // Have we ever been initialized before?  If so then just reparent
>   // from the offscreen window.
>   if (mMozWindowWidget) {
>+#ifndef XP_WIN
>     gtk_widget_reparent(mMozWindowWidget, GTK_WIDGET(mOwningWidget));
>+#else
>+    embed_reparent( mMozWindowWidget, GTK_WIDGET(mOwningWidget), 0 );
>+    focus_handler->reattach();

Do we need this extra commented out code?


>+//    gtk_widget_unparent( GTK_WIDGET( mMozWindowWidget ) );
>+//    gtk_widget_show( GTK_WIDGET(mMozWindowWidget) );
>+//    gdk_window_show( mMozWindowWidget->window );
>+//    gdk_window_move_resize( mMozWindowWidget->window, 
>+//    GTK_WIDGET(mOwningWidget)->window = mMozWindowWidget->window;
>+//  gtk_widget_show( GTK_WIDGET(mOwningWidget) );
>+#endif
>     *aAlreadyRealized = PR_TRUE;
>     return NS_OK;
>   }
> 
>   // Get the nsIWebBrowser object for our embedded window.
>   nsCOMPtr<nsIWebBrowser> webBrowser;
>@@ -246,41 +306,82 @@ EmbedPrivate::Realize(PRBool *aAlreadyRe
>   // bind the progress listener to the browser object
>   nsCOMPtr<nsISupportsWeakReference> supportsWeak;
>   supportsWeak = do_QueryInterface(mProgressGuard);
>   nsCOMPtr<nsIWeakReference> weakRef;
>   supportsWeak->GetWeakReference(getter_AddRefs(weakRef));
>   webBrowser->AddWebBrowserListener(weakRef,
>-				    nsIWebProgressListener::GetIID());
>+            nsIWebProgressListener::GetIID());
> 
>   // set ourselves as the parent uri content listener
>   nsCOMPtr<nsIURIContentListener> uriListener;
>   uriListener = do_QueryInterface(mContentListenerGuard);
>   webBrowser->SetParentURIContentListener(uriListener);
> 
>   // save the window id of the newly created window
>   nsCOMPtr<nsIWidget> mozWidget;
>   mWindow->mBaseWindow->GetMainWidget(getter_AddRefs(mozWidget));
>+#ifndef XP_WIN
>   // get the native drawing area
>   GdkWindow *tmp_window =
>-    NS_STATIC_CAST(GdkWindow *,
>-		   mozWidget->GetNativeData(NS_NATIVE_WINDOW));
>+     NS_STATIC_CAST(GdkWindow *,
>+     mozWidget->GetNativeData(NS_NATIVE_WINDOW));
>   // and, thanks to superwin we actually need the parent of that.
>   tmp_window = gdk_window_get_parent(tmp_window);
>   // save the widget ID - it should be the mozarea of the window.
>   gpointer data = nsnull;
>   gdk_window_get_user_data(tmp_window, &data);
>   mMozWindowWidget = NS_STATIC_CAST(GtkWidget *, data);
>+#else
>+  // We cannot get the native drawing area here and treat it as though it was
>+  // a GtkWidget or even a GdkWindow as is done above.  So, we need to do something
>+  // specific to win32 here.  Perhaps a SetWindowLong call since all the above is
>+  // basically doing is storing the GdkWindow for later use. 
>+  // for the mean time it works to just initialize mMozWindowWidget to null
>+  // this gets us past the initialization and seems to let everything run
>+  // with the exception of notification of focus-in and focus-out.  The commented
>+  // code is my attempt at making a wrapper GdkWindow for the parent window(HWND).
>+/*  HWND hwnd = NS_STATIC_CAST(HWND,mozWidget->GetNativeData(NS_NATIVE_WINDOW));
>+  HWND parent = GetParent( hwnd );
>+  mMozWindowWidget = gtk_window_new( GTK_WINDOW_TOPLEVEL );
>+  mMozWindowWidget->window = gdk_window_foreign_new( (GdkNativeWindow)parent );
>+  gtk_window_set_decorated( GTK_WINDOW( mMozWindowWidget ), FALSE );*/


Ok, this is unreadable.  Do not comment out code with /* */.  That just gets
hairy if someone else comments a larger chunk of code near here with the same
notation. (e.g. /* ... /* ... */ ... */ and then things get screwed!)  Use an
#if 0 if you have to, but really, why do we even need this to begin with?  It's
apparently a failed attempt, so it might not be that useful, and the comment
does a decent enough job of explaining things.


>+  HWND hwnd = NS_STATIC_CAST(HWND,mozWidget->GetNativeData(NS_NATIVE_WINDOW));
>+  mMozWindowWidget = gtk_window_new( GTK_WINDOW_TOPLEVEL );
>+//  mMozWindowWidget = gtk_window_new( GTK_WINDOW_POPUP );
>+//  mMozWindowWidget = gtk_event_box_new();
>+//  SetParent( hwnd, NULL );

Same here.  I think we can lose the comment.

>+  mMozWindowWidget->window = gdk_window_foreign_new( (GdkNativeWindow)hwnd );
>+  mMozWindowWidget->parent = 0;
>+//  gtk_widget_set_parent_window( mMozWindowWidget, gdk_window_foreign_new( (GdkNativeWindow)hwnd ) );
>+ 
>+  //mMozWindowWidget = 0;
>+  if( !attached ){
>+    GtkWidget *toplevel = gtk_widget_get_toplevel(GTK_WIDGET(mOwningWidget));
>+    g_print( "EmbedPrivate: attaching focus handler\n" );
>+    focus_handler->attach( toplevel, GTK_WIDGET( mOwningWidget ), hwnd );
>+
>+    attached = 1;
>+  }
>+#endif
> 
>   return NS_OK;
> }
> 
> void
> EmbedPrivate::Unrealize(void)
> {
>+  g_print( "ENTER: EmbedPrivate::Unrealize\n" );
>   // reparent to our offscreen window
>+#ifndef XP_WIN
>   gtk_widget_reparent(mMozWindowWidget, sOffscreenFixed);
>+#else
>+  embed_reparent( mMozWindowWidget, sOffscreenFixed, 1 );
>+  focus_handler->deattach();
>+#endif
>+
>+  g_print( "LEAVE: EmbedPrivate::Unrealize\n" );
> }
> 
> void
> EmbedPrivate::Show(void)
> {
>   // Get the nsIWebBrowser object for our embedded window.
>@@ -305,14 +406,14 @@ EmbedPrivate::Hide(void)
> }
> 
> void
> EmbedPrivate::Resize(PRUint32 aWidth, PRUint32 aHeight)
> {
>   mWindow->SetDimensions(nsIEmbeddingSiteWindow::DIM_FLAGS_POSITION |
>-			 nsIEmbeddingSiteWindow::DIM_FLAGS_SIZE_INNER,
>-			 0, 0, aWidth, aHeight);
>+       nsIEmbeddingSiteWindow::DIM_FLAGS_SIZE_INNER,
>+       0, 0, aWidth, aHeight);
> }
> 
> void
> EmbedPrivate::Destroy(void)
> {
>   // This flag might have been set from
>@@ -328,13 +429,13 @@ EmbedPrivate::Destroy(void)
>   // Release our progress listener
>   nsCOMPtr<nsISupportsWeakReference> supportsWeak;
>   supportsWeak = do_QueryInterface(mProgressGuard);
>   nsCOMPtr<nsIWeakReference> weakRef;
>   supportsWeak->GetWeakReference(getter_AddRefs(weakRef));
>   webBrowser->RemoveWebBrowserListener(weakRef,
>-				       nsIWebProgressListener::GetIID());
>+               nsIWebProgressListener::GetIID());
>   weakRef = nsnull;
>   supportsWeak = nsnull;
> 
>   // Release our content listener
>   webBrowser->SetParentURIContentListener(nsnull);
>   mContentListenerGuard = nsnull;
>@@ -376,18 +477,25 @@ EmbedPrivate::SetURI(const char *aURI)
> #endif
> }
> 
> void
> EmbedPrivate::LoadCurrentURI(void)
> {
>-  if (mURI.Length())
>+  if (mURI.Length()){

Nit: the rest of the file spaces between close parenthesis and open brace.  Do
so here as well.

>+
>     mNavigation->LoadURI(mURI.get(),                        // URI string
>                          nsIWebNavigation::LOAD_FLAGS_NONE, // Load flags
>                          nsnull,                            // Referring URI
>                          nsnull,                            // Post data
>                          nsnull);                           // extra headers
>+#ifdef XP_WIN
>+    // for pages that take a long time to load this
>+    // prevents the cursor from flickering in a gtk widget
>+    NotifyFocusChange(WM_SETFOCUS);
>+#endif
>+  }
> }
> 
> /* static */
> void
> EmbedPrivate::PushStartup(void)
> {
>@@ -399,13 +507,13 @@ EmbedPrivate::PushStartup(void)
>     nsresult rv;
>     nsCOMPtr<nsILocalFile> binDir;
>     
>     if (sCompPath) {
>       rv = NS_NewNativeLocalFile(nsDependentCString(sCompPath), 1, getter_AddRefs(binDir));
>       if (NS_FAILED(rv))
>-	return;
>+  return;

Spacing needs fixing.

>     }
> 
> #ifdef _BUILD_STATIC_BIN
>     // Initialize XPCOM's module info table
>     NSGetStaticModuleInfo = gtk_getModuleInfo;
> #endif
>@@ -580,16 +688,16 @@ EmbedPrivate::FindPrivateForBrowser(nsIW
>   PRInt32 count = sWindowList->Count();
>   // This function doesn't get called very often at all ( only when
>   // creating a new window ) so it's OK to walk the list of open
>   // windows.
>   for (int i = 0; i < count; i++) {
>     EmbedPrivate *tmpPrivate = NS_STATIC_CAST(EmbedPrivate *,
>-					      sWindowList->ElementAt(i));
>+                sWindowList->ElementAt(i));
>     // get the browser object for that window
>     nsIWebBrowserChrome *chrome = NS_STATIC_CAST(nsIWebBrowserChrome *,
>-						 tmpPrivate->mWindow);
>+             tmpPrivate->mWindow);
>     if (chrome == aBrowser)
>       return tmpPrivate;
>   }
> 
>   return nsnull;
> }
>@@ -607,41 +715,81 @@ EmbedPrivate::ContentStateChange(void)
>   if (!mEventReceiver)
>     return;
>   
>   AttachListeners();
> 
> }
>+#ifdef XP_WIN
>+void 
>+EmbedPrivate::NotifyFocusChange( int wm_msg )
>+{
>+  // Simulate a mouse click either on the mozembed widget or off the 
>+  // mozembed widget.  This allows us to unfocus a currently focused gtkwidget,
>+  // which might otherwise be left in a focus state; but not have keyboard focus.
>+  // NOTE: this is not exactly a real mouse click because we use the allow_focus
>+  // to force a focus on browser or prevent one.  If we can get this to be a real
>+  // simulated click I think it will allow things such as default focus on a 
>+  // form feild, which pages such as google have.
>+  POINT pt;
>+  RECT rect;
>+  HWND tophwnd = focus_handler->hwnd;
>+
>+  GetWindowRect( focus_handler->moz_hwnd, &rect );
>+  if( wm_msg == WM_SETFOCUS ){
>+    pt.x = rect.left + 1;
>+    pt.y = rect.top + 1;
>+    focus_handler->focus_on_moz = TRUE;
>+    focus_handler->allow_focus = TRUE;
>+  }
>+  else{
>+    pt.x = rect.left - 1;
>+    pt.y = rect.top - 1;
>+    focus_handler->focus_on_moz = FALSE;
>+    focus_handler->allow_focus = FALSE;
>+  }
>+  g_print( "sending simulated mouse clicked to (%d,%d): 0x%x\n", pt.x, pt.y, focus_handler->moz_hwnd );


We don't need to see stuff on the console.  Add an #ifdef DEBUG or use PR
Logging if you think we really need the information in non-debug builds.


>+  SendMessage( tophwnd, WM_PARENTNOTIFY, (WPARAM)WM_LBUTTONDOWN, MAKELPARAM( pt.x, pt.y ) );
>+  SendMessage( tophwnd, WM_MOUSEACTIVATE, (WPARAM)focus_handler->moz_hwnd, 1 );
>+}
>+#endif
> 
> void
> EmbedPrivate::ContentFinishedLoading(void)
> {
>+  #ifdef XP_WIN
>+   // notify is to get the focus from any gtk+ widgets
>+   NotifyFocusChange(WM_SETFOCUS);
>+  #endif
>+

Start the preproccessor commands at column 0, please.  =)


>@@ -159,12 +163,20 @@ class EmbedPrivate {
>   PRBool                         mChromeLoaded;
>   // saved window ID for reparenting later
>   GtkWidget                     *mMozWindowWidget;
>   // has someone called Destroy() on us?
>   PRBool                         mIsDestroyed;
> 
>+#ifdef XP_WIN
>+  guint focus_in_top;
>+  guint focus_out_top;
>+  int attached;

I think this wants to be a PRBool, not an int.

>+  struct FocusHandler *focus_handler;
>+  void NotifyFocusChange( int wm_msg );
>+#endif
>+
>  private:
> 
>   // is the chrome listener attached yet?
>   PRBool                         mListenersAttached;
> 
>   void GetListener    (void);

>@@ -60,23 +70,41 @@ EmbedWindow::Init(EmbedPrivate *aOwner)
>   
>   nsCOMPtr<nsIDocShellTreeItem> item = do_QueryInterface(mWebBrowser);
>   item->SetItemType(nsIDocShellTreeItem::typeContentWrapper);
> 
>   return NS_OK;
> }
>+nsNativeWidget EmbedWindow::GetOwnerNativeWidget()
>+{
>+  #ifdef XP_WIN
>+
>+  // unfortunately there are two different ways to get the native HWND in win32 depending
>+  // on the version of gtk

I think I'd prefer getting the HWND separate from the return statement, just to
stress the fact that the difference in code is the way to get the HWND, not the
rest of the statement.


>+  #ifdef MOZ_WIDGET_GTK2
>+  return nsNativeWidget (GDK_WINDOW_HWND( GTK_WIDGET(mOwner->mOwningWidget)->window));
>+  #endif
>+  #ifdef MOZ_WIDGET_GTK
>+  return nsNativeWidget (GDK_DRAWABLE_WIN32DATA (GTK_WIDGET(mOwner->mOwningWidget)->window)->xid);
>+  #endif
>+
>+  #else
>+  return nsNativeWidget (GTK_WIDGET(mOwner->mOwningWidget));
>+  #endif
>+}
> 

The rest looks OK enough, provided it actually does work on windows,  but I'd
appreciate you making these changes, then posting *both* a diff -u and a diff
-uw for me to look at again.  Minusing for now just to get it out of the queue
and waiting on a new patch.

Comment 26

[Zac Bowling]

Attachment: Updated with improvements

Here is the patches. I changed the name of that function to follow the name
scheme and did everything else asked to. I have not had a chance to test this
on my other OSs, but it works in Windows very nicely. 

This patch is a diff -u

Comment 27

[Zac Bowling]

Attachment: diff-uw for gtkembedmoz

this is the same patch with diff -uw

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

+++ src/EmbedWindow.cpp	27 Jan 2005 19:40:54 -0000
@@ -1,6 +1,4 @@
 /*
- * vim:ts=2:et:sw=2
- *

why are you removing the vim modeline?

Comment 29

[Zac Bowling]

(In reply to comment #28)
> +++ src/EmbedWindow.cpp	27 Jan 2005 19:40:54 -0000
> @@ -1,6 +1,4 @@
>  /*
> - * vim:ts=2:et:sw=2
> - *
> 
> why are you removing the vim modeline?

my version of vim on win32 freaked out to it for some reason. I forgot to change
it back. 

Comment 30

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 172598 [details] [diff] [review]
Updated with improvements

Wait, this isn't what the old code, or even your old patch used to do on linux.
 I was thinking along the lines of your function body being something like

#ifndef XP_WIN
  gtk_widget_reparent(...);
#else
  /* stuff */ 
#endif

Is there a reason to change this?


+void 
>+EmbedPrivate::EmbedReparent( GtkWidget *widget, 
>+                             GtkWidget *new_parent, 
>+                             PRBool hide )
>+{		
>+  if( widget->parent ){
>+    g_print( "EmbedPrivate: gtk-reparent\n" );
>+    gtk_widget_reparent( widget, GTK_WIDGET(new_parent) );
>+  }
>+  else if( !GTK_WIDGET_TOPLEVEL( widget ) ){
>+    g_print( "EmbedPrivate: gtk-set_parent\n" );
>+    gtk_widget_set_parent( widget, GTK_WIDGET(new_parent) );
>+  }
>+  else{
>+    g_object_ref( widget );
>+    gtk_widget_unparent( widget );
>+    g_print( "EmbedPrivate: gdk-reparent\n" );
>+    gdk_window_reparent( widget->window, new_parent->window,
>+                         new_parent->allocation.x, 
>+			 new_parent->allocation.y );
>+    g_object_notify( G_OBJECT( widget ), "parent" );
>+    if( hide != PR_TRUE ){
>+      HWND hwnd = (HWND)GDK_WINDOW_HWND( widget->window );
>+      gdk_window_show( widget->window );
>+
>+//    This is done to simulate a window resize which appears to be
>+//    the only way to get the mozembed window to show itself
>+      MoveWindow( hwnd, new_parent->allocation.x, 
>+	          new_parent->allocation.y, new_parent->allocation.width,
>+		  new_parent->allocation.height, TRUE );
>+#ifdef XP_WIN
>+      focus_handler->attach( widget, GTK_WIDGET( new_parent ), hwnd );
>+#endif
>+    }
>+  }
>+#ifdef XP_WIN
>+  if ( hide == PR_TRUE )	  
>+    focus_handler->deattach();
>+#endif
>+}

Comment 31

[Erik van Pienbroek]

Zac, I am the one that reported the GtkNotebook issue to Todd. I just tried your
binary distribution of gtkmozembed
(http://polystimulus.com/gtkembedmoz-sharp/gtkmozembed.zip) and my GtkNotebook
issue still exists in that package. Maybe Todd was a little too incomplete with
his explanation, so here is the full description of the problem :

My application uses an GtkNotebook for displaying HTML pages in several tabs.
Every tab header contains an label and an close button. When the user clicks the
close button, the tab should close. Under Linux and MacOSX this works without
problems, but with Win32 the program crashes under some conditions. For example
when you have 3 tabs in the GtkNotebook, the first one is active and you close
the second and then the third tab. To be sure the problem isn't in my code I've
created an stripped down version with only an GtkNotebook en 3 tabs (this is the
crash test case that Todd added to this page).

I see you have added the GtkNotebook test from gtkmozembed to your
gtkmozembed.zip distribution, but with this program the bug can not be reproduced.

I hope you can look into the code and fix this issue.
If you want I can send you my binary of the crash test case.

Comment 32

[Zac Bowling]

I think I should also post my build instructions here so everyone has them.

You will need a GTK2 (GTK+-2.4) installer for WIN32. You can use one of the 
complete devel installer packages from http://gladewin32.sf.net/ or using the 
official Tor builds at http://www.gimp.org/~tml/gimp/win32/downloads.html 
(which the gladewin32 builds are based on). What I did was install an older 
gladewin32 install a while back, and now I unzip Tor's runtimes and devels for 
GTK, Glib, Pango, Atk, libart_lgpl, Glade, and etc. 

After you have GTK2, you will need to build Mozilla exactly as discribed as the 
WIN32 instructions say. Do it completely. Forget about gtkembedmoz until after 
you build a complete version of mozilla. 

After the build apply the patches to <moz>/embedding/browser/gtk.

Then change the <moz>/config/automake.mk to say 1 for MOZ_ENABLE_GTK2 and set 
your GTK CFLAGS and LIBS with LIB and INCLUDE files for gtk.
(I know this could really be done in the makefile for gtkembedmoz or in a 
standalone-win32 directory, but I haven't explored that method yet.)

It shoud look something like this: reflect something like this: 
.......
MOZ_ENABLE_GTK		= 
MOZ_ENABLE_GTK2		= 1 
MOZ_ENABLE_XLIB		= 
MOZ_ENABLE_PHOTON	= 
MOZ_ENABLE_COCOA	= 
MOZ_ENABLE_XREMOTE	= 

MOZ_GTK_CFLAGS		= 
MOZ_GTK_LDFLAGS		= 

MOZ_GTK2_CFLAGS= -DMOZ_WIDGET_GTK2 -Ic:/PROGRA~1/COMMON~1/GTK/2.4/include -
Ic:/PROGRA~1/COMMON~1/GTK/2.4/include/atk-1.0 -
Ic:/PROGRA~1/COMMON~1/GTK/2.4/include/gtk-2.0 -
Ic:/PROGRA~1/COMMON~1/GTK/2.4/include/glib-2.0 -
Ic:/PROGRA~1/COMMON~1/GTK/2.4/include/pango-1.0 -
IC:/PROGRA~1/COMMON~1/GTK/2.4/include/gtk-2.0 -
IC:/PROGRA~1/COMMON~1/GTK/2.4/lib/gtk-2.0/include -
IC:/PROGRA~1/COMMON~1/GTK/2.4/include/atk-1.0 -
IC:/PROGRA~1/COMMON~1/GTK/2.4/include/pango-1.0 -
IC:/PROGRA~1/COMMON~1/GTK/2.4/include/glib-20 -
IC:/PROGRA~1/COMMON~1/GTK/2.4/lib/glib-2.0/include

MOZ_GTK2_LIBS		= /libpath:C:/PROGRA~1/COMMON~1/GTK/2.4/lib 
C:/PROGRA~1/COMMON~1/GTK/2.4/lib/gtk-win32-2.0.lib 
C:/PROGRA~1/COMMON~1/GTK/2.4/lib/gdk-win32-2.0.lib 
C:/PROGRA~1/COMMON~1/GTK/2.4/lib/atk-1.0.lib 
C:/PROGRA~1/COMMON~1/GTK/2.4/lib/gdk_pixbuf-2.0.lib 
C:/PROGRA~1/COMMON~1/GTK/2.4/lib/pangowin32-1.0.lib 
C:/PROGRA~1/COMMON~1/GTK/2.4/lib/pango-1.0.lib 
C:/PROGRA~1/COMMON~1/GTK/2.4/lib/gobject-2.0.lib 
C:/PROGRA~1/COMMON~1/GTK/2.4/lib/gmodule-2.0.lib 
C:/PROGRA~1/COMMON~1/GTK/2.4/lib/glib-2.0.lib 
C:/PROGRA~1/COMMON~1/GTK/2.4/lib/intl.lib 
C:/PROGRA~1/COMMON~1/GTK/2.4/lib/iconv.lib

.........

Then cd to <moz>/embedding/browser/gtk and run make. You should have your fresh 
new GtkEmbedMoz.dll set to go. :-)

Comment 33

[Zac Bowling]

I'm not sure why but when I posted it put new lines in my config file. The 
CFLAGS and LIB settings should be all on one line or have "\" to continue on to 
the next line as normal. :-)

Comment 34

[Zac Bowling]

Another issue I can't figure out is this funny little bug in the official GRE 
vs my GRE. For some reason the scrollbars disapear and focus bugs on web forms 
come out really nasty with the official GRE but my GRE works fine. The official 
doesn't crash or anything just acts funny. http://polystimulus.com/gtkembedmoz-
sharp/screenshot-funnybug.PNG is an example of the bug. Its the whole reason I 
included my GRE on my site as well :-). If anyone has a clue where it would be, 
any help would be awesome.

Comment 35

[Geoff Schmidt]

This is working great for me so far, but I noticed that when I build in debug
mode I see random intermittant NS_ASSERT's ("This shouldn't happen") on page
load at nsFocusController.cpp:485.

Here's the scoop. In this class, mActive is a flag manipulated with
GetActive/SetActive. mCurrentWindow is a DOM window pointer manipulated with
GetFocusedWindow/SetFocusedWindow, and it may be null.

The idea is that every time mActive is toggled on with SetActive(TRUE), a
function UpdateWWActiveWindow() should be called. However, if mCurrentWindow is
null, that call to UpdateWWActiveWindow() must be deferred until mCurrentWindow
becomes non-null as a result of a call to SetFocusedWindow with a non-null
argument. This is accomplished by having SetActive set a flag
mUpdateWindowWatcher when mActive has become true but mCurrentWindow is null,
and having SetFocusedWindow check this flag when its argument is non-null, and
if the flag is set call UpdateWWActiveWindow() and clear the flag.

The problem is that in this last case, where SetFocusedWindow generates an
UpdateWWActiveWindow() call, it first does a NS_ASSERT(mActive, "This shouldn't
happen") As far as I can tell, the assert is mistaken and simply should be
removed. Actually, mActive would typically be true is this case, because
mUpdateWindowWatcher is set (and the assert triggered) in response to mActive
becoming true. On the other hand, mActive could be false if mActive were set and
then cleared, all before mCurrentWindow became true, so the opposite sense of
the assert isn't an invariant either.

So, to repeat, the fix is simply to remove the mActive assert in
nsFocusController::SetFocusedWindow.

I don't know if a spurious NS_ASSERT is something that is worthy of attention by
a committer, but I thought I'd post what I'd learned in case others wonder about it.

Comment 36

[Tor Lillqvist]

So what's the latest status on this? Can one find gtkmozembed binaries for Win2
anywhere? That polystimulus.com site mentioned above seems to be broken now.
Does one really have to build all of Mozilla to get gtkmozembed?

Comment 37

[Todd A. Fisher]

(In reply to comment #36)
> So what's the latest status on this? Can one find gtkmozembed binaries for Win2
> anywhere? That polystimulus.com site mentioned above seems to be broken now.
> Does one really have to build all of Mozilla to get gtkmozembed?

Status:
I haven't done any work on this since my last patch update... I was under the
optimisic impression that Zac was charging forward with this effort.  Perhaps, I
was wrong if his link is dead :-(

Building:
Very likely yes...  The binaries I have on my site for example,
http://severna.homeip.net/gtkmozwin32.php are built against a mozilla and gtk+
compiled using vc7 - so msvcr71.dll is needed.  Sadly, I did things this way at
the time to both better understand gtk+ and in hopes of reducing the binary size
of gtk+, making my work very difficult to impossible to use with your gtk+
binaries.  From the looks of it though Zac figured out how to get this working
with your binaries and only needed to modify the mozilla make to use your gtk+
binaries glib2, etc... instead of glib1. 

I actually, don't think it would take too much work to get things to work with
your binaries and mozilla.  If you need help getting mozilla to compile I'd be
glad to help you out.

I think the only part of mozilla that really needs to be compiled is the
gtkmozembed part.  What I'm not sure about is glib and whether it matters that
the rest of mozilla use glib1 while the mozembed part uses glib2.  In fact I'm
not even sure how much of mozilla actually uses glib1 and whether or not that's
just needed for some tools.  Some clarifcation here would be helpful.  

Anyways, that's what I know if I can help in any other ways I'd be glad.

Comment 38

[Zac Bowling]

Sorry about that. I still manage to build this at least once every few weeks on the mozilla 1.7 trunk. I was in the middle of making this work on 1.8 with all the changes when my laptop blew up. The polystimulus.com site will be up in about 2 hours. I'm rebuilding the directory structure as we speak (I had to reimage the server a little while back and forgot to finish this). Tons of good stuff there when I'm done. When I get the disaster recovery team at my company to extract the data off that failed drive, I will post everything. All dependencies and all object files. I also have a version I was compiling with MSVC 6 (sp6) for binary compatablity with the windows build of mozilla so I didn't have to release the GRE. 


Tor: I'm on almost all the time on irc.gimp.org as zbowling in the #mono channel if you want to get ahold of me at anytime.

Comment 39

[Zac Bowling]

(In reply to comment #36)
> So what's the latest status on this? Can one find gtkmozembed binaries for Win2
> anywhere? That polystimulus.com site mentioned above seems to be broken now.
> Does one really have to build all of Mozilla to get gtkmozembed?

Yes, this is a required step unless you feel liking being a l33t h4x0r of pain. You can maybe try and use the mozilla source 1.7 source tarball and the procompiled mozilla win32 sdk and extract the sdk over the same parts of the tree in mozilla. Then maybe you could hybred something up, but I doubt it. There are a lot of things I believe are generated in order to generate other files that are stacked and compilied in with the gtkembedmoz source. 

Comment 40

[Zac Bowling]

(In reply to comment #39)
> (In reply to comment #36)
> > So what's the latest status on this? Can one find gtkmozembed binaries for Win2
> > anywhere? That polystimulus.com site mentioned above seems to be broken now.
> > Does one really have to build all of Mozilla to get gtkmozembed?
> Yes, this is a required step unless you feel liking being a l33t h4x0r of pain.
> You can maybe try and use the mozilla source 1.7 source tarball and the
> procompiled mozilla win32 sdk and extract the sdk over the same parts of the
> tree in mozilla. Then maybe you could hybred something up, but I doubt it.
> There are a lot of things I believe are generated in order to generate other
> files that are stacked and compilied in with the gtkembedmoz source. 

The files are back up:
http://www.polystimulus.com/polystimulus/

Everything is there.

Comment 41

[Zac Bowling]

http://zacbowling.com/gtkembedmoz/ Info page links work again.

Comment 42

[Axel]

(In reply to comment #38)
> Sorry about that. I still manage to build this at least once every few weeks on
> the mozilla 1.7 trunk. I was in the middle of making this work on 1.8 with all
> the changes when my laptop blew up. The polystimulus.com site will be up in
> about 2 hours. I'm rebuilding the directory structure as we speak (I had to
> reimage the server a little while back and forgot to finish this). Tons of good
> stuff there when I'm done. When I get the disaster recovery team at my company
> to extract the data off that failed drive, I will post everything. All
> dependencies and all object files. I also have a version I was compiling with
> MSVC 6 (sp6) for binary compatablity with the windows build of mozilla so I
> didn't have to release the GRE. 
> 
> 
> Tor: I'm on almost all the time on irc.gimp.org as zbowling in the #mono
> channel if you want to get ahold of me at anytime.
> 

Hi, at first thanks for your great work so far! Is there any progress in the 1.8 trunk? 

Comment 43

[H.C.]

Is there any way to make this work under Vista with Visual Studio 8 (SP1) and the 1.9 Trunk (Firefox 3)? I tried to make the patch files run, but there's always an error occurring saying "can't find file to patch in line 19", while Line 19 contains "@@ -1,7 +1,8 @@". 
I'm not really experienced with these-patch files, but is there anyway I can make this files work anyway?