Bug 82081 – [SOUP] Implement WebFrameNetworkingContext for soup in WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

It's currently unimplemented, WebFrameNetworkingContext::create() always returns 0 and it's in gtk dir while it's actually soup specific.

[Carlos Garcia Campos]

Created attachment 133543 [details]
Patch

[Martin Robinson]

Comment on attachment 133543 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133543&action=review

> Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.cpp:29
> +
> +#include <WebCore/ResourceHandle.h>

Extra newline here.

> Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.cpp:38
> +SoupSession* WebFrameNetworkingContext::soupSession() const
> +{
> +    return ResourceHandle::defaultSession();
> +}

This seems unused. I would prefer adding this when it's first used instead of introducing dead code now.

[Carlos Garcia Campos]

This is not dead code. The frame loader in FrameLoader::init() creates the networking context with 

m_networkingContext = m_client->createNetworkingContext();

In WebKit2 that calls WebFrameLoaderClient::createNetworkingContext() in Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

That currently uses Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebFrameNetworkingContext.h that always returns NULL. 

This patch moves the implementation to Source/WebKit2/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.cpp/h since the implementation is common to all ports using soup, which creates and returns a WebFrameNetworkingContext instead of NULL.

The Networking context is used by CookieJarSoup to get the jar feature of the current soup session. And its also used by ResourceHandleSoup.

So the difference is that the context is not NULL anymore for WebKit2, but a valid one.

[Martin Robinson]

(In reply to comment #3)
> This is not dead code. The frame loader in FrameLoader::init() creates the networking context with 

If the ::soupSession getter is not used in this patch, I think it should be added in the first patch that uses it.

[Carlos Garcia Campos]

(In reply to comment #4)
> (In reply to comment #3)
> > This is not dead code. The frame loader in FrameLoader::init() creates the networking context with 
> 
> If the ::soupSession getter is not used in this patch, I think it should be added in the first patch that uses it.

soupSession is used by the current code, it's protected by checking whether network context is NULL or not, so currently it's not called because the network context is always NULL in wk2. Patch attached to bug #82082 doesn't introduce a new soupSession call and it doesn't work without this one, that's why I made this bug depend on bug #82082. You can add a printf there if you want to check this code is not dead code.

[Martin Robinson]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > This is not dead code. The frame loader in FrameLoader::init() creates the networking context with 
> > 
> > If the ::soupSession getter is not used in this patch, I think it should be added in the first patch that uses it.
> 
> soupSession is used by the current code, it's protected by checking whether network context is NULL or not, so currently it's not called because the network context is always NULL in wk2. Patch attached to bug #82082 doesn't introduce a new soupSession call and it doesn't work without this one, that's why I made this bug depend on bug #82082. You can add a printf there if you want to check this code is not dead code.

There were a couple pieces of information missing here: 

1. FrameNetworkingContext is an abstract base class with an abstract method ::soupSession. 
2. The old implementation of WebFrameNetworkingContext, Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebFrameNetworkingContext.h, did not implement that method. Normally it's a compilation error immediately after you try to instantiate a non-concrete implementation of an abstract class. As Carlos said in comment #1 though, the factory method was always returning null.

Without knowing those two things outright, this patch looked very mysterious.

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > This is not dead code. The frame loader in FrameLoader::init() creates the networking context with 
> > > 
> > > If the ::soupSession getter is not used in this patch, I think it should be added in the first patch that uses it.
> > 
> > soupSession is used by the current code, it's protected by checking whether network context is NULL or not, so currently it's not called because the network context is always NULL in wk2. Patch attached to bug #82082 doesn't introduce a new soupSession call and it doesn't work without this one, that's why I made this bug depend on bug #82082. You can add a printf there if you want to check this code is not dead code.
> 
> There were a couple pieces of information missing here: 
> 
> 1. FrameNetworkingContext is an abstract base class with an abstract method ::soupSession. 
> 2. The old implementation of WebFrameNetworkingContext, Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebFrameNetworkingContext.h, did not implement that method. Normally it's a compilation error immediately after you try to instantiate a non-concrete implementation of an abstract class. As Carlos said in comment #1 though, the factory method was always returning null.
> 

Current code never tries to instantiate a concrete implementation because the static factory method always returns NULL.

[Carlos Garcia Campos]

I've just landed patch attached to bug #82082, but it requires this patch to work in WebKit2.

[Martin Robinson]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (In reply to comment #3)
> > > > > This is not dead code. The frame loader in FrameLoader::init() creates the networking context with 
> > > > 
> > > > If the ::soupSession getter is not used in this patch, I think it should be added in the first patch that uses it.
> > > 
> > > soupSession is used by the current code, it's protected by checking whether network context is NULL or not, so currently it's not called because the network context is always NULL in wk2. Patch attached to bug #82082 doesn't introduce a new soupSession call and it doesn't work without this one, that's why I made this bug depend on bug #82082. You can add a printf there if you want to check this code is not dead code.
> > 
> > There were a couple pieces of information missing here: 
> > 
> > 1. FrameNetworkingContext is an abstract base class with an abstract method ::soupSession. 
> > 2. The old implementation of WebFrameNetworkingContext, Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebFrameNetworkingContext.h, did not implement that method. Normally it's a compilation error immediately after you try to instantiate a non-concrete implementation of an abstract class. As Carlos said in comment #1 though, the factory method was always returning null.
> > 
> 
> Current code never tries to instantiate a concrete implementation because the static factory method always returns NULL.

Yeah, I just didn't realize both 1 and 2 until later on. Perhaps some abbreviated form of that could go into the ChangeLog.

[Carlos Garcia Campos]

Committed r112273: <http://trac.webkit.org/changeset/112273>