[Bug 721764] souphttpsrc: Add ability to do HTTP session logging

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=721764
  GStreamer | gst-plugins-good | git

Sebastian Dröge (slomo) <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #265810|none                        |needs-work
             status|                            |

--- Comment #4 from Sebastian Dröge (slomo) <slomo at coaxion.net> 2014-01-09 16:07:58 UTC ---
Review of attachment 265810:
 --> (https://bugzilla.gnome.org/review?bug=721764&attachment=265810)

::: ext/soup/gstsouphttpsrc.c
@@ +715,3 @@
+
+  /* Setup logging */
+  gst_soup_util_log_setup (src->logger, src->log_level, src->session, src);

This does not make sense. You pass a logger here (which is never set to
anything), but in that function create one and set it on the session. Maybe
just make that
> gst_soup_util_log_setup(session, level, src)
if removing is not necessary (I don't think it is, the session will free the
object when it goes aways).

Or
> src->logger = gst_soup_util_create_logger (level, src);
> soup_session_add_feature(session, src->logger);

::: ext/soup/gstsouputils.c
@@ +31,3 @@
+gst_soup_util_init (void)
+{
+  GST_DEBUG_CATEGORY_INIT (gst_soup_utils, "gstsouputils", 0, "Soup utils");

Or just do that in plugin_init() and make the debug category extern. No need
for another function here

@@ +50,3 @@
+      break;
+    default:
+      GST_WARNING ("Don't recognize soup logger level: %d", level);

GST_WARNING_OBJECT(), but not needed at all imho. Just replace the ? with the
level number :)

@@ +84,3 @@
+
+  if (logger) {
+    GST_WARNING ("We already have a logger. Not attaching another one");

All these checks should be a g_return_if_fail() IMHO. They are programming
errors

@@ +88,3 @@
+  }
+
+  if (gst_debug_category_get_threshold (GST_CAT_DEFAULT)

No, check for the category you're using here

@@ +97,3 @@
+  /* Create a new logger and set body_size_limit to -1 (no limit) */
+  logger = soup_logger_new (level, -1);
+  soup_logger_set_printer (logger, gst_soup_util_log_printer_cb, element,
NULL);

..., gst_object_ref(element), (GDestroyNotify) gst_object_unref);

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.