[webkit-reviews] review requested: [Bug 22624] [SOUP][GTK] Need API to get SoupSession from WebKit. : [Attachment 26836] API to retrieve and set soup sessions #6

[bugzilla-daemon at webkit.org]

Christian Dywan <christian at imendio.com> has asked  for review:
Bug 22624: [SOUP][GTK] Need API to get SoupSession from WebKit.
https://bugs.webkit.org/show_bug.cgi?id=22624

Attachment 26836: API to retrieve and set soup sessions #6
https://bugs.webkit.org/attachment.cgi?id=26836&action=review

------- Additional Comments from Christian Dywan <christian at imendio.com>
(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=26741) [review] [review]
> > API to retrieve and set soup sessions #5
> +SoupSession* ResourceHandle::getSession(const Frame *frame)
> +{
> +    static SoupSession* default_session = soup_session_async_new();
> +
> +    if (!frame)
> +	   return default_session;
> +    FrameLoader *loader = frame->loader();
> +    if (!loader)
> +	   return 0;
> 
> Why not return the default session here? If this is some kind of catastrophic

> error situation maybe we should assert or something...

There is no catstrohpe, but I acknowledge you might think so if you don't see
it in the full context. In fact I defined getSession(0) to return default
session. Your question lead me to separate this into a function
getDefaultSession, to avoid confusion.

I think we must not assert actually, in case the view is being used while the
object is finalized. Nothing harmful should happen in any case.

> +    SoupSession* session =
> static_cast<FrameLoaderClient*>(loader->client())->session();
> +    return session ? session : default_session;
> +}
> 
> Also, just wondering in general, if the sessions are async maybe some checks
> wold be in order when swapping them (making sure they have no pending work?).

> Maybe that's related to your error...

My error came during browsing while keeping the default session, and while
using a non-default session, so it doesn't look like swapping is the problem. I
added an if(!session) to ensureSession now, and it looks like it prevents that
wanring. I'm still unsure however under what condition startHttp would have an
empty session...

> And as an optimization, I think a flag saying the session is initialized
> instead of checking for the features every time we go through startHttp would

> be good (I think I had this in previous patches).

I wonder about that, I was thinking you intended to make it dynamic later on.
Do you think the feature test is that slow? If we agree on which we prefer and
how to document that API wise, I can change it accordingly.