Summary: | gdklauncher doesnt change URL in adress GTKEntry | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kulyk Nazar <schamane> | ||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | Keywords: | Gtk | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 523.x (Safari 3) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
Kulyk Nazar
2007-04-29 09:11:57 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.
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. Using .ascii() feels wrong. 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.
This bug was indirectly solved by bug #14678, so it should be marked as "fixed". 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. 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.
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.
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 on attachment 17936 [details]
new patch
r=me
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 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. Ok, right. Another question, location means stripping *:// from the URI in terms of files. I think location must be renamed to uri. 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 on attachment 17960 [details]
new signal load-committed and constant uri
OK with the parameter fix we discussed for main.c
Landed in r28817. |