Closed Bug 455247 Opened 12 years ago Closed 12 years ago

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

Categories

(Core Graveyard :: Embedding: APIs, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tristan.van.berkom, Unassigned)

References

Details

Attachments

(1 file, 8 obsolete files)

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.
Attached patch rough update (obsolete) — Splinter Review
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 :)
Attachment #346787 - Flags: review?(dcamp)
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?
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.
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
Attachment #346787 - Attachment is obsolete: true
Attachment #346787 - Flags: review?(dcamp)
Attached patch Same patch + GetDocContent() (obsolete) — Splinter Review
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
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...
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?
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.
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)
Attached patch Patch updated to latest trunk (obsolete) — Splinter Review
Attachment #346971 - Attachment is obsolete: true
Attachment #348057 - Attachment is obsolete: true
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
Attachment #356769 - Attachment is obsolete: true
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.
Attached patch Second try* (obsolete) — Splinter Review
not really, just for this day! :)
Attachment #360945 - Attachment is obsolete: true
Attached patch 3rd try on a row (obsolete) — Splinter Review
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
Attachment #360971 - Attachment is obsolete: true
Attached patch X11 and win32 GTK backend update (obsolete) — Splinter Review
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
Attachment #360974 - Attachment is obsolete: true
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.
I know, but I am not the author of the original code, so you know, i tried to keep the status quo :)
Juan, if removing the G_OBJECT macro is a good fix, let's do it.
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?
(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.
r=mark.finkle@gmail.com
Same patch but without the unnecessary G_OBJECT() casts.
Attachment #362423 - Attachment is obsolete: true
Attachment #365436 - Flags: review?(mark.finkle)
Attachment #365436 - Flags: review?(mark.finkle) → review+
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
http://hg.mozilla.org/incubator/embedding/rev/0daaac93ea35
Status: UNCONFIRMED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Duplicate of this bug: 477173
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.