Bug 13542 - gdklauncher doesnt change URL in adress GTKEntry
Summary: gdklauncher doesnt change URL in adress GTKEntry
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2007-04-29 09:11 PDT by Kulyk Nazar
Modified: 2007-12-17 12:55 PST (History)
0 users

See Also:


Attachments
change-url-0.1.patch (1.41 KB, patch)
2007-04-29 09:13 PDT, Kulyk Nazar
mjs: review-
Details | Formatted Diff | Diff
fix and add safe API (9.18 KB, patch)
2007-12-16 03:12 PST, Luca Bruno
no flags Details | Formatted Diff | Diff
new patch (10.19 KB, patch)
2007-12-16 10:39 PST, Luca Bruno
alp: review-
Details | Formatted Diff | Diff
new signal load-committed and constant uri (13.92 KB, patch)
2007-12-17 04:35 PST, Luca Bruno
alp: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kulyk Nazar 2007-04-29 09:11:57 PDT
gdklauncher doesnt change adress ind adress line on startup, after input new URL and after clicking on link befor new page is loaded.
Comment 1 Kulyk Nazar 2007-04-29 09:13:20 PDT
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.
Comment 2 Maciej Stachowiak 2007-05-11 11:47:13 PDT
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.
Comment 3 Holger Freyther 2007-05-29 10:37:16 PDT
Using .ascii() feels wrong.
Comment 4 Maciej Stachowiak 2007-05-29 11:47:51 PDT
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.
Comment 5 Christian Dywan 2007-07-24 17:58:52 PDT
This bug was indirectly solved by bug #14678, so it should be marked as "fixed".
Comment 6 Mark Rowe (bdash) 2007-09-24 06:04:53 PDT
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.
Comment 7 Luca Bruno 2007-12-16 03:12:25 PST
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.
Comment 8 Luca Bruno 2007-12-16 10:39:54 PST
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.
Comment 9 Maciej Stachowiak 2007-12-16 16:49:46 PST
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.
Comment 10 Maciej Stachowiak 2007-12-16 16:50:08 PST
Comment on attachment 17936 [details]
new patch

r=me
Comment 11 Maciej Stachowiak 2007-12-16 17:54:31 PST
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.
Comment 12 Alp Toker 2007-12-16 18:06:07 PST
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.
Comment 13 Luca Bruno 2007-12-17 02:39:03 PST
Ok, right.

Another question, location means stripping *:// from the URI in terms of files. I think location must be renamed to uri.
Comment 14 Luca Bruno 2007-12-17 04:35:18 PST
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.
Comment 15 Alp Toker 2007-12-17 12:50:55 PST
Comment on attachment 17960 [details]
new signal load-committed and constant uri

OK with the parameter fix we discussed for main.c
Comment 16 Alp Toker 2007-12-17 12:55:49 PST
Landed in r28817.