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

--- Comment #1 from Sebastian Dröge (slomo) <slomo at coaxion.net> 2014-01-08 09:19:31 UTC ---
Review of attachment 265630:
 --> (https://bugzilla.gnome.org/review?bug=721764&attachment=265630)

::: ext/soup/gstsouphttpsrc.c
@@ +256,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_uint ("http-log-level", "http-log-level",

Use the SoupLoggerLogLevel enum here and make it an enum property

@@ +381,3 @@
+      break;
+  }
+  return SOUP_LOGGER_LOG_NONE;

No need to translate levels then :)

@@ +401,3 @@
+    default:
+      c = '?';
+      g_assert_not_reached ();

Let the default be the number maybe and don't assert. Who knows what libsoup
will add here later

@@ +415,3 @@
+  gchar c;
+  c = gst_soup_http_log_make_level_tag (level);
+  GST_DEBUG ("HTTP_SESSION(%c): %c %s", c, direction, data);

MEMDUMP or TRACE level IMHO, not DEBUG

@@ +419,3 @@
+
+static void
+gst_soup_http_log_setup (GstSoupHTTPSrc * src)

This function looks weird in general... see next comment

@@ +826,3 @@
+      soup_session_remove_feature (src->session,
+          SOUP_SESSION_FEATURE (src->http_logger));
+      src->http_logger = NULL;

Shouldn't this just be done at the place where the session object is destroyed?
Add the logger feature once when the session is created, and destroy once when
it's destroyed

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