Bug 13542 – gdklauncher doesnt change URL in adress GTKEntry

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Kulyk Nazar]

gdklauncher doesnt change adress ind adress line on startup, after input new URL and after clicking on link befor new page is loaded.

[Kulyk Nazar]

Created attachment 14267 [details]
change-url-0.1.patch

Just what i could find out at this moment. I dont find any possibility to change URL on starting new page load.

[Maciej Stachowiak]

I'm not an expert on GdkLauncher, but I don't think this is the right place to update the URL. You want it to happen at a point that is a bottleneck for loading, ideally.

[Holger Freyther]

Using .ascii() feels wrong.

[Maciej Stachowiak]

Comment on attachment 14267 [details]
change-url-0.1.patch

After discussion with Holger and Alp, it seems this change is probably not the best way to fix the bug.

[Christian Dywan]

This bug was indirectly solved by bug #14678, so it should be marked as "fixed".

[Mark Rowe (bdash)]

This is "fixed" to some extent: the URL will update in response to the title-changed signal.  That is not really the correct time to update the URL, so for this to be considered "fixed" I would think this would need to be hooked up to a more appropriate signal.  I think a more appropriate time may be in response to FrameLoaderClient::dispatchDidStartProvisionalLoad, but this is not exposed in any fashion at the Gtk API level.

[Luca Bruno]

Created attachment 17928 [details]
fix and add safe API

I followed a bit how Qt handled this and found this solution which seems to be safe, and most of all remove the ugly URI passing trough title-changed.

[Luca Bruno]

Created attachment 17936 [details]
new patch

In the previous patch i duplicated the _frame_get_location (mine was _frame_get_uri, would be this more gtk-y?).
Also i didn't add any uri-related API to the webview, as you can see in the GtkLauncher the location is retrieved from the main frame.
Didn't add any property because other variables don't have any property.
Notice how get_location won't return a const string anymore.

[Maciej Stachowiak]

Most of these changes look correct. I think the end result is not entirely the right thing though.

What should happen (IMO) is that a signal should be emitted from FrameLoaderClient::dispatchDidCommitLoad and that gdklauncher should listen to this signal and update the URL at that point. Once the load is committed, the URL should update.

I'll r+ this for now because it seems like a good improvement pending availability of the right signals in the API.

[Maciej Stachowiak]

Comment on attachment 17936 [details]
new patch

r=me

[Maciej Stachowiak]

Comment on attachment 17936 [details]
new patch

Switching back to review?

alp points out that normal Gtk+ API practice is to return a "const gchar*" and require the caller to g_strdup the string if it wants to save a copy. In this case, that would mean caching the value from converting prettyURL() in a variable in the private struct, as was done before.

[Alp Toker]

Comment on attachment 17936 [details]
new patch

>Index: WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp
>===================================================================
>--- WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp	(revision 28768)
>+++ WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp	(working copy)
>@@ -503,7 +503,11 @@
> 
> void FrameLoaderClient::dispatchDidReceiveTitle(const String& title)
> {
>-    notImplemented();
>+    WebKitWebView* page = getViewFromFrame(m_frame);
>+
>+    g_signal_emit_by_name(m_frame, "title-changed", title.utf8().data());
>+    if (m_frame == webkit_web_view_get_main_frame(page))
>+        g_signal_emit_by_name(page, "title-changed", title.utf8().data());
> }

Looks good.

> 
> 
>@@ -633,14 +637,10 @@
> 
> void FrameLoaderClient::setTitle(const String& title, const KURL& url)
> {
>-    WebKitWebView* page = getViewFromFrame(m_frame);
>-
>-    CString titleString = title.utf8();
>-    DeprecatedCString urlString = url.prettyURL().utf8();
>-    g_signal_emit_by_name(m_frame, "title-changed", titleString.data(), urlString.data());
>-
>-    if (m_frame == webkit_web_view_get_main_frame(page))
>-        g_signal_emit_by_name(page, "title-changed", titleString.data(), urlString.data());
>+    WebKitWebFramePrivate* frameData = WEBKIT_WEB_FRAME_GET_PRIVATE(m_frame);
>+    if (frameData->title)

No need for this check. g_free() is successful on NULLs.

>+        g_free(frameData->title);
>+    frameData->title = g_strdup(title.utf8().data());
> }
> 
> void FrameLoaderClient::dispatchDidReceiveContentLength(DocumentLoader*, unsigned long identifier, int lengthReceived)
>Index: WebKit/gtk/WebView/webkitwebframe.cpp
>===================================================================
>--- WebKit/gtk/WebView/webkitwebframe.cpp	(revision 28768)
>+++ WebKit/gtk/WebView/webkitwebframe.cpp	(working copy)
>@@ -55,8 +55,6 @@
> 
> static guint webkit_web_frame_signals[LAST_SIGNAL] = { 0, };
> 
>-static void webkit_web_frame_real_title_changed(WebKitWebFrame* frame, gchar* title, gchar* location);
>-
> G_DEFINE_TYPE(WebKitWebFrame, webkit_web_frame, G_TYPE_OBJECT)
> 
> static void webkit_web_frame_finalize(GObject* object)
>@@ -65,7 +63,6 @@
>     privateData->frame->loader()->cancelAndClear();
>     g_free(privateData->name);
>     g_free(privateData->title);
>-    g_free(privateData->location);
>     delete privateData->frame;
> 
>     G_OBJECT_CLASS(webkit_web_frame_parent_class)->finalize(object);
>@@ -100,12 +97,12 @@
>     webkit_web_frame_signals[TITLE_CHANGED] = g_signal_new("title-changed",
>             G_TYPE_FROM_CLASS(frameClass),
>             (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
>-            G_STRUCT_OFFSET(WebKitWebFrameClass, title_changed),
>+            0,                                               
>             NULL,
>             NULL,
>-            webkit_marshal_VOID__STRING_STRING,
>-            G_TYPE_NONE, 2,
>-            G_TYPE_STRING, G_TYPE_STRING);
>+            webkit_marshal_VOID__STRING,
>+            G_TYPE_NONE, 1,
>+            G_TYPE_STRING);
> 
>     webkit_web_frame_signals[HOVERING_OVER_LINK] = g_signal_new("hovering-over-link",
>             G_TYPE_FROM_CLASS(frameClass),
>@@ -117,23 +114,12 @@
>             G_TYPE_NONE, 2,
>             G_TYPE_STRING, G_TYPE_STRING);
> 
>-    frameClass->title_changed = webkit_web_frame_real_title_changed;
>-
>     /*
>      * implementations of virtual methods
>      */
>     G_OBJECT_CLASS(frameClass)->finalize = webkit_web_frame_finalize;
> }
> 
>-static void webkit_web_frame_real_title_changed(WebKitWebFrame* frame, gchar* title, gchar* location)
>-{
>-    WebKitWebFramePrivate* frameData = WEBKIT_WEB_FRAME_GET_PRIVATE(frame);
>-    g_free(frameData->title);
>-    g_free(frameData->location);
>-    frameData->title = g_strdup(title);
>-    frameData->location = g_strdup(location);
>-}
>-
> static void webkit_web_frame_init(WebKitWebFrame* frame)
> {
>     // TODO: Move constructor code here.
>@@ -167,7 +153,6 @@
>     frameData->webView = webView;
>     frameData->name = 0;
>     frameData->title = 0;
>-    frameData->location = 0;
> 
>     return frame;
> }
>@@ -199,12 +184,15 @@
>     return frameData->title;
> }
> 
>-const gchar* webkit_web_frame_get_location(WebKitWebFrame* frame)
>+gchar* webkit_web_frame_get_location(WebKitWebFrame* frame)
> {
>     g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);
> 
>-    WebKitWebFramePrivate* frameData = WEBKIT_WEB_FRAME_GET_PRIVATE(frame);
>-    return frameData->location;
>+    KURL url = core(frame)->loader()->url();
>+    if (url.isNull() || url.isEmpty())
>+        return NULL;
>+    else
>+        return g_strdup(url.prettyURL().utf8().data());
> }
> 
> /**
>@@ -418,7 +406,6 @@
>     return g_strdup(string.utf8().data());
> }

This is bad. We really do have to return a const gchar*, even if that means caching the gchar* string in the class. The caller can't be expected to free the string.

The fix is good though, just needs to address these issues.

[Luca Bruno]

Ok, right.

Another question, location means stripping *:// from the URI in terms of files. I think location must be renamed to uri.

[Luca Bruno]

Created attachment 17960 [details]
new signal load-committed and constant uri

Hi this patch updates the URI once first data is received in the frame, and at the same time emits load-committed.

[Alp Toker]

Comment on attachment 17960 [details]
new signal load-committed and constant uri

OK with the parameter fix we discussed for main.c

[Alp Toker]

Landed in r28817.