Bug 82081 - [SOUP] Implement WebFrameNetworkingContext for soup in WebKit2
Summary: [SOUP] Implement WebFrameNetworkingContext for soup in WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, Soup
Depends on:
Blocks: 82082
  Show dependency treegraph
 
Reported: 2012-03-23 12:54 PDT by Carlos Garcia Campos
Modified: 2012-03-27 08:23 PDT (History)
3 users (show)

See Also:


Attachments
Patch (9.36 KB, patch)
2012-03-23 12:58 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-03-23 12:54:42 PDT
It's currently unimplemented, WebFrameNetworkingContext::create() always returns 0 and it's in gtk dir while it's actually soup specific.
Comment 1 Carlos Garcia Campos 2012-03-23 12:58:06 PDT
Created attachment 133543 [details]
Patch
Comment 2 Martin Robinson 2012-03-23 13:58:40 PDT
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.
Comment 3 Carlos Garcia Campos 2012-03-25 07:13:33 PDT
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.
Comment 4 Martin Robinson 2012-03-25 08:47:50 PDT
(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.
Comment 5 Carlos Garcia Campos 2012-03-25 23:22:48 PDT
(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.
Comment 6 Martin Robinson 2012-03-26 08:26:57 PDT
(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.
Comment 7 Carlos Garcia Campos 2012-03-26 08:39:21 PDT
(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.
Comment 8 Carlos Garcia Campos 2012-03-27 03:00:27 PDT
I've just landed patch attached to bug #82082, but it requires this patch to work in WebKit2.
Comment 9 Martin Robinson 2012-03-27 08:11:52 PDT
(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.
Comment 10 Carlos Garcia Campos 2012-03-27 08:23:16 PDT
Committed r112273: <http://trac.webkit.org/changeset/112273>