[webkit-reviews] review denied: [Bug 13542] gdklauncher doesnt change URL in adress GTKEntry : [Attachment 17936] new patch

[bugzilla-daemon at webkit.org]

Alp Toker <alp at atoker.com> has denied Maciej Stachowiak <mjs at apple.com>'s
request for review:
Bug 13542: gdklauncher doesnt change URL in adress GTKEntry
http://bugs.webkit.org/show_bug.cgi?id=13542

Attachment 17936: new patch
http://bugs.webkit.org/attachment.cgi?id=17936&action=edit

------- Additional Comments from Alp Toker <alp at atoker.com>
>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.