GNOME Bugzilla – Bug 702775
Use libsoup directly for HTTP GET requests
Last modified: 2013-07-26 00:23:39 UTC
See patches
Created attachment 247383 [details] [review] lib: Make use of g_clear_pointer() We are doing that already in geocode-forward to free the hashtable.
Created attachment 247384 [details] [review] lib: Use libsoup directly for HTTP GET requests We should be doing this anyways, libsoup being our direct dependency but this patch also avoids the issue of reverse geocoding testcase hanging becuase of some gvfs bug.
Review of attachment 247383 [details] [review]: ++
Review of attachment 247384 [details] [review]: ::: geocode-glib/geocode-forward.c @@ +79,3 @@ forward->priv->ht = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free); + forward->priv->soup_session = soup_session_async_new (); "soup_session_async_new is deprecated and should not be used in newly-written code. SoupSessionAsync is deprecated; use a plain SoupSession, created with soup_session_new(). See the porting guide." @@ +80,3 @@ g_free, g_free); + forward->priv->soup_session = soup_session_async_new (); + soup_session_add_feature_by_type (forward->priv->soup_session, SOUP_TYPE_PROXY_RESOLVER_DEFAULT); That's not needed: "Note that a SoupProxyResolverDefault and a SoupContentDecoder are added to the session by default (unless you are using one of the deprecated session subclasses)." @@ +332,3 @@ g_object_set_data (G_OBJECT (query), "is-search", g_object_get_data (G_OBJECT (cache), "is-search")); + g_object_ref (query); // soup_session_queue_message steals ref No C++ style comments. ::: geocode-glib/geocode-reverse.c @@ +75,3 @@ object->priv->ht = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free); + object->priv->soup_session = soup_session_async_new (); Ditto. @@ +76,3 @@ g_free, g_free); + object->priv->soup_session = soup_session_async_new (); + soup_session_add_feature_by_type (object->priv->soup_session, SOUP_TYPE_PROXY_RESOLVER_DEFAULT); Ditto.
In my defence, I just followed Alex' similar comment from Boxes repo. :)
(In reply to comment #5) > In my defence, I just followed Alex' similar comment from Boxes repo. :) comment -> commit
Created attachment 247568 [details] [review] lib: Use libsoup directly for HTTP GET requests We should be doing this anyways, libsoup being our direct dependency but this patch also avoids the issue of reverse geocoding testcase hanging becuase of some gvfs bug. At the same time, this change reveals that our reverse geocoding API is not working anymore since the Yahoo web API we rely on does not seem to be available anymore.
Review of attachment 247568 [details] [review]: ::: geocode-glib/geocode-forward.c @@ +333,3 @@ g_object_set_data (G_OBJECT (query), "is-search", g_object_get_data (G_OBJECT (cache), "is-search")); + g_object_ref (query); /* soup_session_queue_message steals ref */ And why do you need it around? The query will drop its last reference at the end of the callback function. ::: geocode-glib/geocode-glib-private.h @@ +54,3 @@ +gboolean _geocode_glib_cache_load (SoupMessage *query, + char **contents); +GHashTable *_geocode_glib_dup_hash_table (GHashTable *ht); This isn't in this patch. ::: geocode-glib/geocode-reverse.c @@ +361,2 @@ query = g_object_get_data (G_OBJECT (cache), "query"); + g_object_ref (query); /* soup_session_queue_message steals ref */ Same comment as earlier.
Comment on attachment 247383 [details] [review] lib: Make use of g_clear_pointer() Attachment 247383 [details] pushed as 201c251 - lib: Make use of g_clear_pointer()
Review of attachment 247568 [details] [review]: ::: geocode-glib/geocode-forward.c @@ +333,3 @@ g_object_set_data (G_OBJECT (query), "is-search", g_object_get_data (G_OBJECT (cache), "is-search")); + g_object_ref (query); /* soup_session_queue_message steals ref */ Because the cache GFile instance is going to be unrefed when this callback function returns and that will also unref the only ref we have of 'query' instance: g_object_set_data_full (G_OBJECT (cache), "query", query, (GDestroyNotify) g_object_unref); g_file_load_contents_async (cache, cancellable, on_cache_data_loaded, simple); g_object_unref (cache);
Created attachment 247681 [details] [review] lib: Use libsoup directly for HTTP GET requests We should be doing this anyways, libsoup being our direct dependency but this patch also avoids the issue of reverse geocoding testcase hanging becuase of some gvfs bug. At the same time, this change reveals that our reverse geocoding API is not working anymore since the Yahoo web API we rely on does not seem to be available anymore.
Review of attachment 247681 [details] [review]: Looks fine if it's valgrind clean and passes the tests.
Created attachment 249937 [details] [review] lib: Use libsoup directly for HTTP GET requests Rerunning test case pointed out to error not being set in certain cases and redundant NULL checks. Fixed those in this version. Ran the test case inside valgrind but it didn't report anything unusual and number of errors (i dont have any suppressions files for glib/gobject here) reduced actually with this patch.
Attachment 249937 [details] pushed as d00059f - lib: Use libsoup directly for HTTP GET requests Rebased on top of git master and pushed.