[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

--- Comment #5 from Reynaldo H. Verdejo Pinochet <reynaldo at opendot.cl> 2014-01-09 17:16:19 UTC ---
(In reply to comment #4)
> Review of attachment 265810 [details]:
> 
> [..] 
> ::: 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

Do I understand correctly that would require the calling code to
be aware of souputil's debug category name? What are we gaining
really?. I'm OK with taking it out though.

> 
> @@ +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 :)

Agreed, thanks.

> 
> @@ +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

Code then will misbehave if G_DISABLE_CHECKS
was set for glib?. Now (!level) is being read from
a writable user facing prop, I'm guessing you didn't
meant to include that one in your "they"?

About the checking for GST_CAT_DEFAULT, that macro is
expanding to gst_soup_utils on that file already. What's
wrong with it?

Will post an updated patch once these questions are resolved.

Thanks a lot for your time and comments Sebastian.

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