439252 - Embedding gtk+ widget for win32

Status: RESOLVED FIXED

Description

[Tristan Van Berkom]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14
Build Identifier: 

I am attaching here a gtk+ embedding widget for win32

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Tristan Van Berkom]

Created attachment 325117 [details]
hg bundle including core modifications

These patches are against:
  http://hg.mozilla.org/users/blassey_mozilla.com/embedding/

I'll attach the full bundle first (includes changes from bug 436234).

Comment 2

[Tristan Van Berkom]

Created attachment 325118 [details] [diff] [review]
same as above, but generated as "hg outgoing -p" (incremental patch)

Comment 3

[Tristan Van Berkom]

Created attachment 325119 [details] [diff] [review]
human readable patch

This patch assumes that modifications in bug 436234 are already applied.

So it only adds new files, mozarea.[cpp,h], mozarea-marshal.[ch],
win32gtk.def, and visual studio files... plus a test subdir including
a little test program and a makefile to compile it with mingw.

Comment 4

[Dave Camp (:dcamp)]

It would be nice if this code shared at least a header file/API with the widget in the gtk/ tree.

Comment 5

[Dave Camp (:dcamp)]

And I don't know the windows build environment very well, but if it's possible, including the original marshaller list and processing it during the build is preferable to including a pre-generated marshaller file (and you should be able to reuse the one in gtk/ if the APIs match).

Comment 6

[Tristan Van Berkom]

Ok, I'll make a patch that has:
  gtk/
  gtk/x11/
  gtk/win32/
with the shared header in gtk/, match the api, and hopefully 
get the marshalers generated (I kind of choked on this faced
with MSVS).

Comment 7

[Tristan Van Berkom]

Ahh yes, we should also share a test program in the gtk/ directory...

Comment 8

[Dave Camp (:dcamp)]

for consistency with the rest of the tree, maybe gtk/include for the headers and gtk/tests for the test stuff.

Thanks!

Comment 9

[Tristan Van Berkom]

Created attachment 330348 [details]
latest hg bundle

Ok in this new patch we have the directory structure:

gtk/x11     <-- original gtk code minus test.cpp
gtk/win32   <-- the win32 embedding widget
gtk/include <-- moz-web-view.h
gtk/common  <-- moz-web-view-marshal.list and generated files
gtk/tests   <-- test.cpp, Makefile.x11, Makefile.win32

At this point I make a guess at the Makefile in gtk/x11 and made
it build a shared lib to link to from gtk/tests/Makefile.x11, which
is also untested, I was hopeing someone working on the x11 backend
could try it and fix a probable error or two (I figure it should save
some work that I include that part...).

Will attach the plain text version in a second...

Comment 10

[Tristan Van Berkom]

Created attachment 330349 [details] [diff] [review]
Plain text version (outgoing incremental patch)

Enjoy ;-)
    -Tristan

Comment 11

[Tristan Van Berkom]

Created attachment 330530 [details]
latest hg bundle


This patch is the same as the last but also adds the init/term embedding
and set/get prefs apis to moz-web-view.h, and implements them in
gtk/common/moz-web-view-common.cpp.

The previous patch was also missing my promised gtk/tests/Makefile.x11 (oops)

Comment 12

[Tristan Van Berkom]

Created attachment 330531 [details] [diff] [review]
plain text version

Comment 13

[Tristan Van Berkom]

Created attachment 333866 [details] [diff] [review]
Here is a manual patch against a clean checkout

This will contain moves of files from repo/gtk/ to repo/gtk/x11/, those
files have no changes, except for the Makefile that I make a guess at
fixing.

Theres also a big fat compreg.dat diff to ignore in there.

Oh and Id say be careful ! the patch has a one line 
modification to .hg/branches.cache .

This one should be nice and readable.

Comment 14

[Dave Camp (:dcamp)]

Comment on attachment 333866 [details] [diff] [review]
Here is a manual patch against a clean checkout

Looks good to me, a few nitpicks in the comments.

>diff -rupNb embedding_repo_clean/gtk/common/moz-web-view-common.cpp embedding_repo/gtk/common/moz-web-view-common.cpp
>--- embedding_repo_clean/gtk/common/moz-web-view-common.cpp	Wed Dec 31 19:00:00 1969
>+++ embedding_repo/gtk/common/moz-web-view-common.cpp	Sun Jul 20 07:23:30 2008

>+gboolean
>+moz_web_view_set_char_pref  (const char  *name, 
>+			     const char  *value)

Indentation problem here, and in general you should be using spaces rather than tabs.
>+{
>+    nsresult rv;
>+
>+    g_return_val_if_fail (init_count > 0, FALSE);
>+
>+    rv = app->SetCharPref (name, value);
>+    
>+    return !NS_FAILED(rv);
>+}

You can use NS_SUCCEEDED(rv), and you don't really need the tmeporary variable there.


>diff -rupNb embedding_repo_clean/gtk/win32/moz-web-view.cpp embedding_repo/gtk/win32/moz-web-view.cpp
>--- embedding_repo_clean/gtk/win32/moz-web-view.cpp	Wed Dec 31 19:00:00 1969
>+++ embedding_repo/gtk/win32/moz-web-view.cpp	Sun Jul 20 06:41:34 2008

>+/*******************************************************************
>+ *                      MOZListener here                           *
>+ *******************************************************************/
>+class MOZListener : public MozViewListener

In the x11 widget we call this ViewListener, is there any particular reason to change it?

>+
>+void MOZListener::SetTitle(const char *new_title)
>+{
>+    GtkWidget *widget = this->GetWidget();
>+    
>+    g_object_set (G_OBJECT (widget), 
>+		  "page-title", new_title,
>+		  NULL);
>+    g_signal_emit(widget, moz_web_view_signals[TITLE_CHANGED], 0, new_title);
>+}

Is there a specific reason why you save the title here, but not the status and location?

Also, the style here is different then the implementation of the same file in x11/, we should probably use the same style in either one.

>+PRBool MOZListener::OpenURI(const char* new_uri)
>+{
>+    GtkWidget *widget = this->GetWidget();
>+    gboolean   abort_load = FALSE;
>+    
>+    g_signal_emit (widget, moz_web_view_signals[URI_REQUESTED], 0, 
>+		   new_uri, &abort_load);
>+    
>+    return abort_load;
>+}

Any particular reason for the signal name change? (OpenURI -> uri-requested).  If so, maybe we should be changing the common API too?

>+void MOZListener::DocumentLoaded()
>+{
>+    GtkWidget *widget = this->GetWidget();
>+    g_signal_emit (widget, moz_web_view_signals[DOCUMENT_LOADED], 0);
>+}
>+
>+GtkWidget *MOZListener::GetWidget()
>+{
>+    HWND hwnd = (HWND)pMozView->GetParentWindow();
>+    GtkWidget *widget = (GtkWidget *)g_hash_table_lookup (hwnd_hash, hwnd);
>+    return widget;
>+}

Any reason why you're not just storing a (possibly weak) reference to the widget in the MozListener object, rather than an HWND->widget lookup?

>+/*******************************************************************
>+ *                      Class Initialization                       *
>+ *******************************************************************/
>+static void
>+moz_web_view_class_init (MozWebViewClass *klass)
...
>+    g_object_class_install_property (object_klass,
>+				     PROP_REQUESTED_URI,
>+				     g_param_spec_string
>+				     ("requested-uri",
>+				      "Requested URI",
>+				      "the requested uri",
>+				      NULL,
>+				      (GParamFlags)G_PARAM_READWRITE));
>+    
>+    g_object_class_install_property (object_klass,
>+				     PROP_TITLE,
>+				     g_param_spec_string
>+				     ("page-title",
>+				      "Page title",
>+				      "the current webpage title",
>+				      NULL,
>+				      (GParamFlags)G_PARAM_READWRITE));

Could you file a followup bug to get these properties (and the extra signals) added to the x11 version if you don't have a linux environment to do it?

>+static void
>+moz_web_view_hierarchy_changed (GtkWidget         *widget,
>+			    GtkWidget         *previous_toplevel)
>+static gboolean 
>+handle_toplevel_configure (GtkWidget         *toplevel,
>+			   GdkEventConfigure *event,
>+			   GtkWidget         *widget)
>+{

As mentioned on irc, please explain this focus hack in comments.

>+
>+gchar *
>+moz_web_view_get_title (MozWebView *view)
>+gboolean
>+moz_web_view_set_user_agent (MozWebView  *view,
>+			     const gchar *user_agent)

File a followup bug for these methods that aren't implement in the x11 version please if you don't have a linux environment please?

Actually, are these marshallers even

Comment 15

[Tristan Van Berkom]

(In reply to comment #14)
[...]
> >diff -rupNb embedding_repo_clean/gtk/win32/moz-web-view.cpp embedding_repo/gtk/win32/moz-web-view.cpp
> >--- embedding_repo_clean/gtk/win32/moz-web-view.cpp	Wed Dec 31 19:00:00 1969
> >+++ embedding_repo/gtk/win32/moz-web-view.cpp	Sun Jul 20 06:41:34 2008
> 
> >+/*******************************************************************
> >+ *                      MOZListener here                           *
> >+ *******************************************************************/
> >+class MOZListener : public MozViewListener
> 
> In the x11 widget we call this ViewListener, is there any particular reason to
> change it?

This widget was not originally intended for the mozilla tree, I basically took
my widget, fixed the tree, and implemented the api common in the header - this
one is a minor detail but the signals and properties admittedly need dealing
with ;-)  (... I'll change this to match ViewListener ofcourse...)

> >+void MOZListener::SetTitle(const char *new_title)
> >+{
> >+    GtkWidget *widget = this->GetWidget();
> >+    
> >+    g_object_set (G_OBJECT (widget), 
> >+		  "page-title", new_title,
> >+		  NULL);
> >+    g_signal_emit(widget, moz_web_view_signals[TITLE_CHANGED], 0, new_title);
> >+}
> 
> Is there a specific reason why you save the title here, but not the status and
> location?
> 
> Also, the style here is different then the implementation of the same file in
> x11/, we should probably use the same style in either one.

Right, I had a requirement for _get_title() here, I could:
  - move it into our third party wrapping widget (we have one for api purposes)
  - add _get_status() and _get_location() along with the read-only props

which do you prefer ?

> >+PRBool MOZListener::OpenURI(const char* new_uri)
> >+{
> >+    GtkWidget *widget = this->GetWidget();
> >+    gboolean   abort_load = FALSE;
> >+    
> >+    g_signal_emit (widget, moz_web_view_signals[URI_REQUESTED], 0, 
> >+		   new_uri, &abort_load);
> >+    
> >+    return abort_load;
> >+}
> 
> Any particular reason for the signal name change? (OpenURI -> uri-requested). 
> If so, maybe we should be changing the common API too?

Kindof a reason, IMO in a gtk+ widget its unclear weather a vfunc is
a notification or an implementation - "open-uri" would be seemingly 
synonymous with "load-uri", while "uri-requested" is clearly a request
from the mozilla backend.

In contrast, the C++ api has a listener object - and all of that is
clear.

Could change the name in the backend, or not, or change the signal name,
your call ;-)

> >+void MOZListener::DocumentLoaded()
> >+{
> >+    GtkWidget *widget = this->GetWidget();
> >+    g_signal_emit (widget, moz_web_view_signals[DOCUMENT_LOADED], 0);
> >+}
> >+
> >+GtkWidget *MOZListener::GetWidget()
> >+{
> >+    HWND hwnd = (HWND)pMozView->GetParentWindow();
> >+    GtkWidget *widget = (GtkWidget *)g_hash_table_lookup (hwnd_hash, hwnd);
> >+    return widget;
> >+}
> 
> Any reason why you're not just storing a (possibly weak) reference to the
> widget in the MozListener object, rather than an HWND->widget lookup?

No, that thought crossed my mind too late, sorry I'll dig that mess out
of there.

I should have another patch in a few days, Im off from the 20-27 on a trip
so Ill do my damndest to take an afternoon for this before I leave.

Comment 16

[Dave Camp (:dcamp)]

(In reply to comment #15)
> 
> > >+void MOZListener::SetTitle(const char *new_title)
> > >+{
> > >+    GtkWidget *widget = this->GetWidget();
> > >+    
> > >+    g_object_set (G_OBJECT (widget), 
> > >+		  "page-title", new_title,
> > >+		  NULL);
> > >+    g_signal_emit(widget, moz_web_view_signals[TITLE_CHANGED], 0, new_title);
> > >+}
> > 
> > Is there a specific reason why you save the title here, but not the status and
> > location?
> > 
> > Also, the style here is different then the implementation of the same file in
> > x11/, we should probably use the same style in either one.
> 
> Right, I had a requirement for _get_title() here, I could:
>   - move it into our third party wrapping widget (we have one for api purposes)
>   - add _get_status() and _get_location() along with the read-only props

The second sounds good... maybe this should be pushed down to the common code?
 
> > Any particular reason for the signal name change? (OpenURI -> uri-requested). 
> > If so, maybe we should be changing the common API too?
> 
> Kindof a reason, IMO in a gtk+ widget its unclear weather a vfunc is
> a notification or an implementation - "open-uri" would be seemingly 
> synonymous with "load-uri", while "uri-requested" is clearly a request
> from the mozilla backend.

OK yeah, that makes sense.  I think it's fine how it is.

> > Any reason why you're not just storing a (possibly weak) reference to the
> > widget in the MozListener object, rather than an HWND->widget lookup?
> 
> No, that thought crossed my mind too late, sorry I'll dig that mess out
> of there.

Alrighty.

Comment 17

[Tristan Van Berkom]

Created attachment 338541 [details]
Latest bundle against the hg repo

Comment 18

[Tristan Van Berkom]

Created attachment 338542 [details] [diff] [review]
Manual diff (human readable version)

Ok heres the latest version:
   - Fixed out the hash map, now we store it as a win32 window
     property and save the widget pointer in the ViewListener as well
   - Made an internal interface that creates the signals and properties
     in the api.
   - win32 backend publishes the properties and emits the signals.

Comment 19

[Tristan Van Berkom]

Created attachment 338545 [details]
another bundle

Comment 20

[Tristan Van Berkom]

Created attachment 338546 [details] [diff] [review]
Last patch had errors

oops last patch was untested with a last change... heres
the last snapshot that compiles ;-)

Comment 21

[Dave Camp (:dcamp)]

Pushed with a merge changeset of b8ca42ef7d03.