455247 - x11 backend of the gtk+ embedding widget needs to be updated

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Tristan Van Berkom]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.16) Gecko/20080702 Firefox/2.0.0.16 (.NET CLR 3.5.30729)
Build Identifier: 

  - I wrote a guess at the makefile, since the directories changed
    and it now builds a shared lib the makefile needs to be tested and
    possibly fixed.

  - New signals/properties from MozViewable interface need to be 
    implemented:
       signals: title-changed, status-changed, location-changed,
       uri-requested and document-loaded (from moz-web-view-common.cpp)
       They "just exist", see gtk/win32/moz-web-view.c the
       ViewListener code for an example (and note that uri-requested
       takes a return value).
       properties: requested-uri, title, status and location.
       these properties are read-only, the widget needs only to call
       g_object_class_override_property() from class init and handle
       the properties in _get_property(), its also up to the widget
       to call g_object_notify() when the properties change.


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Juan Pablo]

Attachment: rough update

Hello, this a quick patch that makes this widget "work"
most features are untested except for "uri-requested" and "document-loaded" signals which are the ones i am using :)

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 346787 [details] [diff] [review]
rough update

Pinging Dave for a review. Is the overall patch consistent with what you and Pelle started?

Comment 3

[Dave Camp (:dcamp)]

Aside from some whitespace issues (seems to change the prevailing 2-space style to 4 spaces in some places, tabs in others) it looks fine to me.

Comment 4

[Juan Pablo]

Attachment: New patch, with proper indentation

Hello guys, new patch with consistent indent!
Btw i would like to have a function to get the serialized content of the current document the browser is displaying.

Would be ok for me to add a function like gchar *moz_web_view_get_content ()?

greets

Comment 5

[Juan Pablo]

Attachment: Same patch + GetDocContent()

Hello guys, this is the same patch with an extra function to get the content of the document, I know it is probably not something in the scope of this interface,
so i have no problem with that you can commit the previous patch

Comment 6

[Boris Zbarsky [:bzbarsky]]

Er...  Is there a reason to use the XML serializer even if the document is HTML?  That won't so much do the right thing, typically...

Comment 7

[Juan Pablo]

hmm, well I am a newbie in mozilla, do you know a better way to get the source of the current document?
anyways, do you think this is appropriate to the MozWebview interface?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Serializing is ok if what you want is the serialization of the DOM (which is not the same thing as the original source), but you need to use an HTML serializer for HTML documents.

No idea about the interface; I haven't seen any sort of design goal for this interfacem what its target audience is, etc.

Comment 9

[Juan Pablo]

ok then, just to keep this on topic, Dave do you want me to do something else?
btw gtk/x11/Makefile needs some love :)
Maybe there is a environment variable to replace $(SDK)

Comment 10

[Juan Pablo]

Attachment: Patch updated to latest trunk

Comment 11

[Juan Pablo]

Attachment: Patch that implements common iface on GTK/X11

gtk/include/moz-web-view.h, gtk/common/moz-web-view-common.cpp:
 o added missing accesor moz_web_view_get_requested_uri()
 o unified accessor with static common function moz_web_view_get_prop()

gtk/x11/moz-web-view.cpp: updated to implement every interface property and signal
 o implemented uri-requested and document-loaded signals
 o override and implemented iface props (requested-uri, title, status, location)
 o added moz_web_view_load_uri() and moz_web_view_load_data() functions

Comment 12

[Dave Camp (:dcamp)]

Comment on attachment 360945 [details] [diff] [review]
Patch that implements common iface on GTK/X11

>diff -r d57a2f58764c gtk/x11/moz-web-view.cpp

> static guint signals[LAST_SIGNAL] = { 0 };
> 
>+static void
>+update_property (MozWebView  *view, gint prop_id, const gchar *new_value);
>+
> class ViewListener : public MozViewListener {
> public:
>     ViewListener(MozWebView *view) : mView(view) {}
>     virtual ~ViewListener() {}
>-
>+    
>     virtual void SetTitle(const char *new_title) {
>-        g_signal_emit(G_OBJECT(mView), signals[TITLE_CHANGED], 0, new_title);
>+      update_property (mView, PROP_TITLE, new_title);
>+      g_signal_emit (G_OBJECT(mView), signals[TITLE_CHANGED], NULL, new_title);
>     }
>-
>+    
>     virtual void StatusChanged(const char *new_status, PRUint32 flags) {
>-        g_signal_emit(G_OBJECT(mView), signals[STATUS_CHANGED], 0, new_status);
>+      update_property (mView, PROP_STATUS, new_status);
>+      g_signal_emit (G_OBJECT(mView), signals[STATUS_CHANGED], NULL, new_status);
>     }
>-
>+    
>     virtual void LocationChanged(const char *new_uri) {
>-        g_signal_emit(G_OBJECT(mView), signals[LOCATION_CHANGED], 0, new_uri);
>+      update_property (mView, PROP_LOCATION, new_uri);
>+      g_signal_emit (G_OBJECT(mView), signals[LOCATION_CHANGED], NULL, new_uri);
>     }

Here (and elsewhere through this file) are some unnecessary whitespace changes, and some 2space tabs (the rest of the code in this file is 4space).

>+static void
>+update_property (MozWebView  *view, gint prop_id, const gchar *new_value)
...
>+  if (*ptr) g_free (*ptr);

g_free can handle a null pointer, g_free(*ptr) should be fine.

> GtkWidget *
>@@ -259,5 +339,37 @@
> void
> moz_web_view_load_uri(MozWebView *view, const char *uri)
> {
>-    view->priv->view->LoadURI(uri);
>+    MozWebViewPriv *priv; 
>+    
>+    g_return_if_fail (MOZ_IS_WEB_VIEW (view));
>+    g_return_if_fail (uri && uri[0]);
>+
>+    priv = view->priv;
>+
>+    if (priv->requested_uri && priv->requested_uri != uri)
>+        g_free (priv->requested_uri);
>+    
>+    priv->requested_uri = g_strdup (uri);

This will leak the old requested_uri if priv->requested_uri == uri.

Comment 13

[Juan Pablo]

Attachment: Second try*

not really, just for this day! :)

Comment 14

[Juan Pablo]

Attachment: 3rd try on a row

opps, that patch broke the the windows build
changelog follows

gtk/include/moz-web-view.h: deleted signals signatures from class struct since they are defined in gtk/common/moz-web-view-common.h

gtk/common/moz-web-view-common.cpp:
 o added missing accesor moz_web_view_get_requested_uri()
 o unified accessor with static common function moz_web_view_get_prop()

gtk/x11/moz-web-view.cpp: updated to implement every interface property and
signal
 o implemented uri-requested and document-loaded signals
 o override and implemented iface props (requested-uri, title, status,
location)
 o added moz_web_view_load_uri() and moz_web_view_load_data() functions

gtk/win32/moz-web-view.cpp: sync moz_web_view_load_uri(), moz_web_view_load_data() and update_property() with X11 version

Comment 15

[Juan Pablo]

Attachment: X11 and win32 GTK backend update

gtk/include/moz-web-view.h: deleted signals signatures from class struct since
they are defined in gtk/common/moz-web-view-common.h

gtk/common/moz-web-view-common.cpp:
 o added missing accesor moz_web_view_get_requested_uri()
 o unified accessor with static common function moz_web_view_get_prop()

gtk/x11/moz-web-view.cpp: updated to implement every interface property and
signal
 o implemented uri-requested and document-loaded signals
 o override and implemented iface props (requested-uri, title, status,
location)
 o added moz_web_view_load_uri() and moz_web_view_load_data() functions

gtk/win32/moz-web-view.cpp: sync moz_web_view_load_uri(),
moz_web_view_load_data() and update_property() with X11 version

gtk/win32/win32gtk.vcproj: added missing common sources files

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Though I am not a reviewer, 

>-	update_property(mView, PROP_TITLE, new_title);
>-	g_signal_emit_by_name(G_OBJECT(mView), "title-changed", new_title);
>+        update_property (mView, PROP_TITLE, new_title);
>+        g_signal_emit (G_OBJECT(mView), signals[TITLE_CHANGED],
>+                       NULL, new_title);

The first argument of g_signal_XX functions is a gpointer, so G_OBJECT macro is not needed.

Comment 17

[Juan Pablo]

I know, but I am not the author of the original code, so you know, i tried to keep the status quo :)

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Juan, if removing the G_OBJECT macro is a good fix, let's do it.

Comment 19

[Juan Pablo]

I am ok with it either way.
Adding such a runtime cast is not a performance hit in any way in this case since signal emission is very unfrequent (it would be in a loop)
Btw using g_signal_emmit_by_name() was way more offending (performance wise) but the same point above applies as well. 

So.. what else does do i have to do beside this trivial fix?

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #19)

> So.. what else does do i have to do beside this trivial fix?

Nothing. Just ask for a review from me or dave camp when you are ready.

Comment 21

[Juan Pablo]

Attachment: X11 and win32 GTK backend update

r=[e-mail]
Same patch but without the unnecessary G_OBJECT() casts.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

We should be on the lookout for any other common code in the gtk (x11 and win32) wrapper, like: update_property / moz_web_view_get_property methods and the ViewListener class

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

http://hg.mozilla.org/incubator/embedding/rev/0daaac93ea35

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

Not sure how I messed up the last push, but I did

http://hg.mozilla.org/incubator/embedding/rev/a6038143acd0