[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