Merge lp:~evfool/midori/lp699970 into lp:midori

[Robert Roth]

This branch registers a handler to the network-changed signal of the GNetworkMonitor (which is already available, as midori already depends on GIO>=2.32.3, and GNetworkMonitor was introduced in 2.32), and in case a network is available (either connected to a network from an unconnected state, or switched to another network) reload all tabs from all browsers of midori.

This branch registers a handler to the network-changed signal of the GNetworkMonitor (which is already available, as midori already depends on GIO>=2.32.3, and GNetworkMonitor was introduced in 2.32), and in case a network is available (either connected to a network from an unconnected state, or switched to another network) reload all tabs from all browsers of midori.

[Cris Dywan (kalikiana)]

I like the idea; though I think it definitely needs restricting, unconditional reloading would be very painful.

How about adding this to midori-tab.vala
enum LoadError { NONE, DELAYED, SECURITY, CRASH, NETWORK }
Set it like
midori_tab_set_load_error (MIDORI_TAB (view), MIDORI_LOAD_ERROR_NETWORK);
in webkit_web_view_load_error_cb
and reset it in midori_view_load_committed to NONE

The other suggested values aren't required for this one, I'm mentioning them for completeness if you'd like to add them as well I wouldn't mind, but just NONE and NETWORK would do.

review: Needs Fixing

[Robert Roth (evfool)]

That is a good idea, and not hard to implement, but it will only partially cover the bug, as the description says that "This could take me away from an error page to a website, or from an outdated website to an updated one.". This change will take you away from an error page, but an outdated website won't be updated. Maybe we could set some kind of threshold here? Do we know when a page was loaded? If yes, we can see how old that page is, and reload if older than the threshold.

[Robert Roth (evfool)]

Another problem I have found with your solution is that:
* i set the load error to none in load_committed
* i set the load error to network in load_error_cb
* on a load error, the load_error_cb sets the load error status correctly, however when displaying the error.html page, loading that also invokes load_committed (due to the webkit_web_view_load_alternate_html / webkit_web_frame_load_alternate_string call from midori_view_set_html), which resets the network load error status :(

Any ideas on how to work around this?

[lp:~evfool/midori/lp699970]

6579. By Robert Roth on 2014-03-02

Only reload if load error is caused by network error

[Robert Roth (evfool)]

I have updated the branch with the changes you have requested, but it still needs fixing, as it doesn't work in this state, because of the issue mentioned in my previous comment.

[lp:~evfool/midori/lp699970]

6580. By Robert Roth on 2014-03-02

Only reset the load error status if the loaded page is not a special page (lp:699970)

[Robert Roth (evfool)]

Ok, found it, updated the branch, so that I only reset the load error status in load committed if the page uri is the requested uri (the view is not a special view), which is not the case for the error page, so now only errored out pages are reloading.

[Cris Dywan (kalikiana)]

That's looking nice. If you're okay with that it might be best to wait with this branch until the start of the next feature period since it may turn up unexpected behavior and we're going into freeze today.

review: Approve (after-feature-freeze)

[Robert Roth (evfool)]

I'm OK with that, no hurry. I will ping you after the next release to merge this.

[André Stösel (ivaldi)]

There is a little bug:
- disable network
- load some page
- enable network
- page is reloading (works fine)
- disable network again
- page is reloading <- you stuck with the error page again

(i tested it with a static html, no js -> no background requests)

review: Needs Fixing

[lp:~evfool/midori/lp699970]

6581. By Robert Roth on 2014-04-14

Merged from trunk

6582. By Robert Roth on 2014-04-14

Clear load error status on load started instead of on load committed (lp:699970)

[Robert Roth (evfool)]

Finally I have managed to check what's up with this branch :)

Thanks André Stösel (ivaldi) for the heads-up, the load error status wasn't cleared indeed in some cases, doing that in load committed didn't seem to be enough. I have moved the load error clearing to load started (a bit earlier in the process), to make sure the error status is clear when we start loading a page, and if we come back with an error, it will be set to error in the error callback, and with this change, it seems to work as expected. Could you please retest it, and update your review if it works?

[André Stösel (ivaldi)]

My last test case works fine now, thanks for the patch.

There is just one minor issue left: it still sends a new DNS request for each status change if the DNS request fails because it does not exists.

But I guess that it could actually depend on the network connection which is used (VPN or intra- vs internet,..) -> so it should be fine.

Thanks again and sorry for the delay.

review: Approve

=== modified file 'midori/midori-app.c'
--- midori/midori-app.c	2014-03-26 21:12:05 +0000
+++ midori/midori-app.c	2014-04-14 07:13:35 +0000
@@ -569,6 +569,24 @@
 }
 
 static void
+midori_app_network_changed (GNetworkMonitor* monitor,
+                            gboolean         available,
+                            MidoriApp*       app)
+{
+    if (available) 
+    {
+        MidoriBrowser *browser;
+        KATZE_ARRAY_FOREACH_ITEM (browser, app->browsers) {
+            GList* tabs = midori_browser_get_tabs (browser);
+            for (; tabs != NULL; tabs = g_list_next (tabs))
+                if (midori_tab_get_load_error (MIDORI_TAB (tabs->data)) == MIDORI_LOAD_ERROR_NETWORK)
+                    midori_view_reload (tabs->data, FALSE);
+            g_list_free (tabs);
+        }
+    }
+}
+
+static void
 midori_app_create_instance (MidoriApp* app)
 {
     if (g_application_get_is_registered (G_APPLICATION (app)))
@@ -602,6 +620,10 @@
                   "flags", G_APPLICATION_HANDLES_OPEN,
                   NULL);
     g_signal_connect (app, "startup", G_CALLBACK (midori_app_startup_cb), NULL);
+
+    g_signal_connect (g_network_monitor_get_default (), "network-changed",
+                       G_CALLBACK (midori_app_network_changed), app);
+
     GError* error = NULL;
     if (!g_application_register (G_APPLICATION (app), NULL, &error))
         midori_error (error->message);

=== modified file 'midori/midori-tab.vala'
--- midori/midori-tab.vala	2014-03-26 21:12:05 +0000
+++ midori/midori-tab.vala	2014-04-14 07:13:35 +0000
@@ -33,6 +33,15 @@
         PROVISIONAL /* A new URI was scheduled. */
     }
 
+    [CCode (cprefix = "MIDORI_LOAD_ERROR_")]
+    public enum LoadError {
+        NONE,
+        DELAYED,
+        SECURITY,
+        CRASH,
+        NETWORK
+    }
+
     public class Tab : Gtk.VBox {
         public Tab related { get; set construct; }
         public WebKit.WebView web_view { get; private set; }
@@ -56,6 +65,7 @@
         /* Since: 0.1.2 */
         public Security security { get; protected set; default = Security.NONE; }
         public LoadStatus load_status { get; protected set; default = LoadStatus.FINISHED; }
+        public LoadError load_error { get; protected set; default = LoadError.NONE; }
         public string? statusbar_text { get; protected set; default = null; }
         /* Since: 0.5.0 */
 

=== modified file 'midori/midori-view.c'
--- midori/midori-view.c	2014-04-06 16:31:06 +0000
+++ midori/midori-view.c	2014-04-14 07:13:35 +0000
@@ -683,6 +683,7 @@
 {
     midori_view_update_load_status (view, MIDORI_LOAD_PROVISIONAL);
     midori_tab_set_progress (MIDORI_TAB (view), 0.0);
+    midori_tab_set_load_error (MIDORI_TAB (view), MIDORI_LOAD_ERROR_NONE);
 }
 
 #ifdef HAVE_GCR
@@ -765,6 +766,7 @@
         midori_tab_set_security (MIDORI_TAB (view), MIDORI_SECURITY_NONE);
 
     view->find_links = -1;
+    
     midori_view_update_load_status (view, MIDORI_LOAD_COMMITTED);
 
 }
@@ -1230,6 +1232,9 @@
     result = midori_view_display_error (view, uri, "stock://dialog/network-error", title,
                                         message, error->message, g_string_free (suggestions, FALSE),
                                         _("Try Again"), web_frame);
+
+    midori_tab_set_load_error (MIDORI_TAB (view), MIDORI_LOAD_ERROR_NETWORK);
+
     g_free (message);
     g_free (title);
     return result;