Bug 14812 – [gtk] [request] add webkit_gtk_frame_get_title() function

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Diego Escalante Urrelo]

In Epiphany there are certain times when we want to get the page title on demand, listening to title_changed isn't that much of a good idea in our case.
Attached patch is pretty dirty, so I'm asking for some guidance on how to do it correctly.

[Diego Escalante Urrelo]

Created attachment 15739 [details]
Initial proposal

I guess there might be a better way of doing it. Adopted this idea after checking the forked gtk-webcore, they implement a signal listener to be between signals and  the app so you can ask for stuff like title on-demand.

[Holger Freyther]

Well, if we start to cache the title in WebFrameGtk we might just add a default handler and avoid connecting to our own sginals.

[Diego Escalante Urrelo]

Created attachment 15741 [details]
A better version

Following your suggestion of not abusing g_signal_connect :P

[Diego Escalante Urrelo]

Also, can we have a get_location() function here too?. Btw, I was wondering if this should be here or in gtkpage.

[Holger Freyther]

The easy bits:
   -Yes get_location makes sense (IMHO)
   -I think they belong into WebKitGtkFrame

Two style issues:
   -AFAIK you don't need a user_data in the signal signature. Please remove it
   -I would like to see another name for the closure, more similiar to the one in webkitgtkpage.cpp (or as in the pending patch for createFrame)
   -No new lines in the cb, I think they fit in one line on a display.

[Mark Rowe (bdash)]

Comment on attachment 15741 [details]
A better version

r- based on Holger's comments.

There is also very inconsistent placement of the * next to types or variable names.  The WebKit style is for the * to be next to the type in C and C++ code.  The code related to the Gtk API seems to be very inconsistent in this regard, but we should be trying to improve the situation rather than adding to the problem.

+    g_debug("title_changed: title=%s, location=%s", title, location);

We don't typically commit debugging printf's such as this.

[Diego Escalante Urrelo]

Created attachment 15806 [details]
Updated.

Ok, following your comments and including the webkitgtkprivate.h part that I forgot.
I didn't get this comment however:
   -I would like to see another name for the closure, more similiar to the one
in webkitgtkpage.cpp (or as in the pending patch for createFrame)

Can you explain it to me please?

PD: I couldn't try the patch, my webkit installation doesn't work anymore because of libWebKitGdk.so: undefined reference to `WebCore::Document::setDomain(WebCore::String const&, bool)'.
Anyway it should work, and for the moment we can at least discuss about it.

[Holger Freyther]

(In reply to comment #7)
> Can you explain it to me please?

Gtk+ and WebKitGtk* prefix such handlers with the normal prefix. So title_changed_cb should probably be webkit_gtk_frame_real_title_changed or just webkit_gtk_frame_title_changed.


> 
> PD: I couldn't try the patch, my webkit installation doesn't work anymore
> because of libWebKitGdk.so: undefined reference to
> `WebCore::Document::setDomain(WebCore::String const&, bool)'.
> Anyway it should work, and for the moment we can at least discuss about it.
>
Sometimes qmake from Qt4.2 has issues tracking dependencies. Qt4.3 seems to do it better but we might need to set a couple more DEPENDPATH and HEADERS in the .pro file.

[Juan A. Suarez]

This bug should have the Gtk keyword.

[Adam Roben (:aroben)]

Comment on attachment 15806 [details]
Updated.

r- given Holger's comments.

[Diego Escalante Urrelo]

Created attachment 16044 [details]
Updated to trunk and following naming comments

Updated.

[Mark Rowe (bdash)]

Comment on attachment 16044 [details]
Updated to trunk and following naming comments

Inside webkit_gtk_frame_title_changed_cb g_strdup is called but I don't see any corresponding calls to free the memory.

There are some minor coding style issues present:
+gchar*
+webkit_gtk_frame_get_title (WebKitGtkFrame* frame)

should be one line, for example.

You should also include a ChangeLog entry.  You can use WebKitTools/Scripts/prepare-ChangeLog to generate a template which you can then fill in.

[Cyril Brulebois]

Created attachment 16303 [details]
Updated with g_free() calls, and coding style fixes.

I added g_free on finalize() and on updating, and hopefully fixed coding style problems. I didn't include copyright information for webkitgtkframe.cpp since Diego might want to be added instead of me; waiting for a reply from him.

[Holger Freyther]

(In reply to comment #13)
> Created an attachment (id=16303) [edit]
> Updated with g_free() calls, and coding style fixes.
> 

+static void webkit_gtk_frame_title_changed_cb(WebKitGtkFrame* frame, gchar* title, gchar* location)
+{
+    WebKitGtkFramePrivate* frame_data = WEBKIT_GTK_FRAME_GET_PRIVATE(frame);
+    g_free (frame_data->title);
+    g_free (frame_data->location);
+    frame_data->title = g_strdup(title);
+    frame_data->location = g_strdup(location);
+}

frame_data -> frameData and "g_free (" -> "g_free(". but the memory leak spotted by Mark should be fixed.

[Holger Freyther]

Landed in r25593.

[Holger Freyther]

Comment on attachment 16303 [details]
Updated with g_free() calls, and coding style fixes.

The patch was landed and does not need further review.