[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 #265869|none                        |needs-work
             status|                            |

--- Comment #13 from Sebastian Dröge (slomo) <slomo at coaxion.net> 2014-01-10 08:55:30 UTC ---
Review of attachment 265869:
 --> (https://bugzilla.gnome.org/review?bug=721764&attachment=265869)

::: ext/soup/gstsoup.c
@@ +39,3 @@
   gst_element_register (plugin, "souphttpclientsink", GST_RANK_NONE,
       GST_TYPE_SOUP_HTTP_CLIENT_SINK);
+  GST_DEBUG_CATEGORY_INIT (gst_soup_utils, "gstsouputils", 0, "Soup utils");

gstsouputils -> souputils. We don't put the gst prefix there anywhere else

::: ext/soup/gstsouphttpsrc.c
@@ +248,3 @@
           DEFAULT_IRADIO_MODE, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
+  g_object_class_install_property (gobject_class, PROP_SOUP_LOG_LEVEL,
+      g_param_spec_enum ("http-log-level", "http-log-level",

The second string should be "HTTP Log Level" or something similar

@@ +713,3 @@
+
+  /* Setup logging */
+  gst_soup_util_log_setup (src->log_level, src->session, src);

Having the session as first parameter seems more natural but ok ;)

::: ext/soup/gstsouputils.c
@@ +66,3 @@
+void
+gst_soup_util_log_setup (SoupLoggerLogLevel level, SoupSession * session,
+    gpointer element)

Make that a GstElement *

@@ +77,3 @@
+
+  if (!session) {
+    GST_WARNING_OBJECT (who, "Have no session. Can't attach a logger");

This seriously needs a g_return_if_fail() as it must not happen. Same goes for
!element

@@ +84,3 @@
+      < GST_LEVEL_TRACE) {
+    GST_WARNING ("Not setting up HTTP session logger. "
+        "Need at least GST_LEVEL_TRACE");

Not a warning

@@ +91,3 @@
+   * more than once (not meant to do so). Session
+   * will need to be checked for an already existant
+   * logger to drop it first */

Not necessarily, maybe you want different loggers with different printers.

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