Merge lp:~ivaldi/midori/drop-notify-uri into lp:midori

[André Stösel]

Drop redundant notify::uri handler

midori_tab_set_uri is called twice on every uri change

first in "midori_view_load_committed", then in "webkit_web_view_notify_uri_cb"

since there is one special case handled in "midori_view_load_committed" I would recommend removing "webkit_web_view_notify_uri_cb"

[gue5t gue5t (gue5t)]

I like the idea of removing duplicate code (and the patch itself is fine mechanically) but this makes me wary. I'd like to see either some testing (e.g. logging and examining exactly when the different codepaths get called in the course of browsing), history-digging (when and why did we end up with two codepaths for the same thing?), or webkit source code justifications to ensure that this doesn't change behavior in a subtle but buggy way. I realize I sound pretty harsh here but in my opinion details of when tab uris are set is an area in which I think discipline is warranted.

Or hopefully I'm not misunderstanding the description: did you already test before/after this a fair bit, and find load_committed to suffice to provide the same behavior even in cases where pages are interrupted (user hits stop), take ages to time out, or fail to load?

Sorry to rain on any parades, this is just the kind of thing where webkit docs aren't always clear and we would do really well to make sure we have it right. :)

review: Needs Information

[Cris Dywan (kalikiana)]

My first thought was: isn't this to ensure that either anchors or scripts using # update the page - it does seem to still work.
Example http://webkitgtk.org/reference/webkit2gtk/unstable/WebKitWebView.html

[André Stösel (ivaldi)]

Yeah, I tested it (I always do). In my tests "midori_view_load_committed" was always called before "webkit_web_view_notify_uri_cb".

I'm pretty sure it works without "webkit_web_view_notify_uri_cb", but feel free to test it yourself.

@ Christian
Anchors were the first things I tested ;)

PS: the other option is to drop midori_tab_set_uri from midori_view_load_committed

[gue5t gue5t (gue5t)]

Consulting webkit source has put all my concerns to rest. I put gdb breakpoints on the two entrypoints we have now and saw in the callstack that they were both from nearby in the webkit library, as the backtrace exerpts below indicate (note the shared frame #9):

notify_uri:
#7 0x00007fe372fb75c3 in g_object_thaw_notify () from /usr/lib/libgobject-2.0.so.0
#8 0x00007fe36e345b62 in ?? () from /usr/lib/libwebkitgtk-1.0.so.0
#9 0x00007fe36e8fe04d in ?? () from /usr/lib/libwebkitgtk-1.0.so.0

load_committed:
#7 0x00007fe372fb740b in g_object_notify () from /usr/lib/libgobject-2.0.so.0
#8 0x00007fe36e345aac in ?? () from /usr/lib/libwebkitgtk-1.0.so.0
#9 0x00007fe36e8fe04d in ?? () from /usr/lib/libwebkitgtk-1.0.so.0

So I dug into the webkit source to make sure they were in the same function:

void FrameLoaderClient::dispatchDidCommitLoad()
{
    if (m_loadingErrorPage)
        return;

    /* Update the URI once first data has been received.
     * This means the URI is valid and successfully identify the page that's going to be loaded.
     */
    g_object_freeze_notify(G_OBJECT(m_frame));

    WebKitWebFramePrivate* priv = m_frame->priv;
    g_free(priv->uri);
    priv->uri = g_strdup(core(m_frame)->loader()->activeDocumentLoader()->url().string().utf8().data());
    g_free(priv->title);
    priv->title = NULL;
    g_object_notify(G_OBJECT(m_frame), "uri");
    g_object_notify(G_OBJECT(m_frame), "title");

    g_signal_emit_by_name(m_frame, "load-committed");
    notifyStatus(m_frame, WEBKIT_LOAD_COMMITTED);

    WebKitWebView* webView = getViewFromFrame(m_frame);
    if (m_frame == webkit_web_view_get_main_frame(webView)) {
        g_object_freeze_notify(G_OBJECT(webView));
        g_object_notify(G_OBJECT(webView), "uri");
        g_object_notify(G_OBJECT(webView), "title");
        g_object_thaw_notify(G_OBJECT(webView));
        g_signal_emit_by_name(webView, "load-committed", m_frame);
    }

    g_object_thaw_notify(G_OBJECT(m_frame));
}

So we should be fine picking either and ignoring the other.

review: Approve

[Cris Dywan (kalikiana)]

review: Approve

=== modified file 'midori/midori-view.c'
--- midori/midori-view.c	2013-08-02 23:45:16 +0000
+++ midori/midori-view.c	2013-08-03 20:15:33 +0000
@@ -1471,14 +1471,6 @@
 }
 
 static void
-webkit_web_view_notify_uri_cb (WebKitWebView* web_view,
-                               GParamSpec*    pspec,
-                               MidoriView*    view)
-{
-    midori_tab_set_uri (MIDORI_TAB (view), webkit_web_view_get_uri (web_view));
-}
-
-static void
 webkit_web_view_notify_title_cb (WebKitWebView* web_view,
                                  GParamSpec*    pspec,
                                  MidoriView*    view)
@@ -3500,8 +3492,6 @@
                       midori_view_download_requested_cb, view,
                       #endif
 
-                      "signal::notify::uri",
-                      webkit_web_view_notify_uri_cb, view,
                       "signal::notify::title",
                       webkit_web_view_notify_title_cb, view,
                       "signal::leave-notify-event",