[webkit-reviews] review denied: [Bug 22624] [SOUP][GTK] Need API to get SoupSession from WebKit. : [Attachment 27790] getsession.patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 23 00:33:22 PST 2009


Alexey Proskuryakov <ap at webkit.org> has denied Xan Lopez
<xan.lopez at gmail.com>'s request for review:
Bug 22624: [SOUP][GTK] Need API to get SoupSession from WebKit.
https://bugs.webkit.org/show_bug.cgi?id=22624

Attachment 27790: getsession.patch
https://bugs.webkit.org/attachment.cgi?id=27790&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
>	  WARNING: NO TEST CASES ADDED OR CHANGED

This line is just a reminder to the author of the patch, please remove it
(perhaps explaining why test cases aren't needed or possible).

> +#if USE(SOUP)
> +#include <libsoup/soup.h>
> +#endif

Could you just forward declare SoupSession?

> + *  Copyright (C) 2009 Igalia S.L., Author: Xan Lopez <xlopez at igalia.com>

This isn't a usual form of copyright line. I don't know if it's a real problem,
but it certainly makes me nervous.

> +static void ensureSession(SoupSession* session)

This name doesn't seem descriptive enough to me. What does this function do
with the session?

> +    if (soup_session_get_feature(session, SOUP_TYPE_LOGGER) == NULL

Per coding style, this should be "if (!soup_session_get_feature(session,
SOUP_TYPE_LOGGER)"

> +bool ResourceHandle::startHttp(String urlString)
> +{
> +    if (!session)
> +	   createSoupSession();

This is what getSession() does - could you just call it? In this case, the
static variable could just move to getSession().

> +SoupSession* ResourceHandle::getSession()

The names "getSession" and "getCookieJar" are misleading, and don't follow out
usual style. First, a getXXX function is one that returns its result in an
argument, a getter would be just "cookieJar", for example. More importantly,
these functions aren't getters, as they can create objects. I think that the
usual naming would be something like "sharedSession", "commonSession",
"defaultSession" etc.

> - *  Copyright (C) 2007, 2008 Christian Dywan <christian at imendio.com>
> + *  Copyright (C) 2007-2009 Christian Dywan <christian at imendio.com>

Again, I don't know why, but we list all years comma-separated in copyright
lines.

Looks like this needs another round of review.


More information about the webkit-reviews mailing list