Bug 126208 - Define WebProcess::usesNetworkProcess unconditionally
Summary: Define WebProcess::usesNetworkProcess unconditionally
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 127026 (view as bug list)
Depends on:
Blocks: 126130
  Show dependency treegraph
 
Reported: 2013-12-24 03:18 PST by Carlos Garcia Campos
Modified: 2014-01-14 18:12 PST (History)
4 users (show)

See Also:


Attachments
Patch (10.62 KB, patch)
2013-12-24 03:22 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (11.30 KB, patch)
2013-12-28 02:24 PST, Carlos Garcia Campos
ap: review-
Details | Formatted Diff | Diff
Updated patch (13.10 KB, patch)
2014-01-03 09:48 PST, 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 2013-12-24 03:18:17 PST
Returning false when network process is not enabled like WebContext does. This way we reduce the amount of ifdefs used when checking whether network process is in use.
Comment 1 Carlos Garcia Campos 2013-12-24 03:22:38 PST
Created attachment 219961 [details]
Patch
Comment 2 Sergio Villar Senin 2013-12-26 02:42:00 PST
Comment on attachment 219961 [details]
Patch

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

The patch looks sensible to me

> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:190
> +        return;

Is this correct? The previous code was not early returning
Comment 3 Carlos Garcia Campos 2013-12-27 00:06:22 PST
Comment on attachment 219961 [details]
Patch

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

>> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:190
>> +        return;
> 
> Is this correct? The previous code was not early returning

Yes, it's correct, we don't want to initialize network features in the web process when using network process.
Comment 4 Carlos Garcia Campos 2013-12-28 02:24:19 PST
Created attachment 220071 [details]
Updated patch

Just rebased to apply on current git master.
Comment 5 Alexey Proskuryakov 2014-01-02 11:15:21 PST
Comment on attachment 220071 [details]
Updated patch

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

It looks like this patch leaves a lot of call sites inside ifdefs. What are the main reasons that make this impossible? It's nice to be consistent.

E.g. CustomProtocolManager::initialize(), DownloadProxy::cancel(), most of WebPlatformStrategies.

> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:101
> +    if (!usesNetworkProcess()) {

I don't think that this is correct. Even when using network process, we still do some loading in WebProcess, simply because no one has finished the migration yet (that would be a great project of medium complexity to tackle).

Some of the things that are loaded in WebProcess:
- <a ping> (PingLoader).
- Application Cache.
- not sure about plug-ins.

> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:190
> +    if (usesNetworkProcess())
> +        return;

The diff is a little convoluted. I didn't look into it very closely, but it's suspicious for the same reason.

> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:217
> +    if (!usesNetworkProcess())
> +        WebCore::removeLanguageChangeObserver(this);

Ditto.

> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:222
> +    ASSERT(!usesNetworkProcess());

Ditto.

> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:228
> +    ASSERT(!usesNetworkProcess());

Ditto.
Comment 6 Carlos Garcia Campos 2014-01-02 11:47:13 PST
(In reply to comment #5)
> (From update of attachment 220071 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220071&action=review
> 
> It looks like this patch leaves a lot of call sites inside ifdefs. What are the main reasons that make this impossible? It's nice to be consistent.
> 
> E.g. CustomProtocolManager::initialize(), DownloadProxy::cancel(), most of WebPlatformStrategies.

This only allows to avoid the ifdefs when the only thing used is usesNetworkProcess, the cases you mentioned use other API only defined inside ENABLE(NETWORK_PROCESS) blocks, like  NetworkProcessProxy in DownloadProxy::cancel() or NetworkConnectionToWebProcessMessages in platform strategies.

> > Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:101
> > +    if (!usesNetworkProcess()) {
> 
> I don't think that this is correct. Even when using network process, we still do some loading in WebProcess, simply because no one has finished the migration yet (that would be a great project of medium complexity to tackle).
> 
> Some of the things that are loaded in WebProcess:
> - <a ping> (PingLoader).
> - Application Cache.
> - not sure about plug-ins.

Do we really need disk cache, or cookies for those loads? I'm not sure having both the web and network process writing to the same cookies database and disk cache storage is going to work.

> > Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:190
> > +    if (usesNetworkProcess())
> > +        return;
> 
> The diff is a little convoluted. I didn't look into it very closely, but it's suspicious for the same reason.
> 
> > Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:217
> > +    if (!usesNetworkProcess())
> > +        WebCore::removeLanguageChangeObserver(this);
> 
> Ditto.
> 
> > Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:222
> > +    ASSERT(!usesNetworkProcess());
> 
> Ditto.

This is a message sent by the ui process to the networking process, it should never be received by the web process when using network process.

> > Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:228
> > +    ASSERT(!usesNetworkProcess());
> 
> Ditto.

Ditto.
Comment 7 Alexey Proskuryakov 2014-01-02 12:17:44 PST
> This only allows to avoid the ifdefs when the only thing used is usesNetworkProcess, the cases you mentioned use other API only defined inside ENABLE(NETWORK_PROCESS) blocks, like  NetworkProcessProxy in DownloadProxy::cancel() or NetworkConnectionToWebProcessMessages in platform strategies.

Yes, I was asking whether these can be easily moved out of ifdefs too. It's really inconsistent to keep most of the ifdefs around, and I'm not sure if this patch is much of an improvement right now.

> Do we really need disk cache, or cookies for those loads?

We need cookies and http credentials for appcache for sure, otherwise it would be impossible to install an offline app from a protected site.
Comment 8 Carlos Garcia Campos 2014-01-03 00:01:19 PST
(In reply to comment #7)
> > This only allows to avoid the ifdefs when the only thing used is usesNetworkProcess, the cases you mentioned use other API only defined inside ENABLE(NETWORK_PROCESS) blocks, like  NetworkProcessProxy in DownloadProxy::cancel() or NetworkConnectionToWebProcessMessages in platform strategies.
> 
> Yes, I was asking whether these can be easily moved out of ifdefs too. It's really inconsistent to keep most of the ifdefs around, and I'm not sure if this patch is much of an improvement right now.

I don't think it's possible. Well, the patch mainly tries to avoid these kind of things:

#if ENABLE(NETWORK_PROCESS)
    if (!m_usesNetworkProcess) {
#endif
#if ENABLE(CUSTOM_PROTOCOLS)
        for (const auto& scheme : globalURLSchemesWithCustomProtocolHandlers())
            parameters.urlSchemesRegisteredForCustomProtocols.append(scheme);
#endif
#if ENABLE(NETWORK_PROCESS)
    }
#endif

I find a lot easier to read this:

if (!usesNetworkProcess) {
#if ENABLE(CUSTOM_PROTOCOLS)
        for (const auto& scheme : globalURLSchemesWithCustomProtocolHandlers())
            parameters.urlSchemesRegisteredForCustomProtocols.append(scheme);
#endif
}

If you think this doesn't improve anything, feel free to close the bug and I'll focus on soup specific changes only.

> > Do we really need disk cache, or cookies for those loads?
> 
> We need cookies and http credentials for appcache for sure, otherwise it would be impossible to install an offline app from a protected site.

Ok, thanks for clarifying it. I'll make sure this still works.
Comment 9 Alexey Proskuryakov 2014-01-03 08:41:54 PST
> I find a lot easier to read this:

OK, let's make this change. I still think that the code should make it clear that WebProcess still (unfortunately) does loading even with NetworkProcess enabled.
Comment 10 Carlos Garcia Campos 2014-01-03 09:48:24 PST
Created attachment 220310 [details]
Updated patch

Removed ifdefs in a couple places more. I'm leaving the soup changes for now, because this doesn't change the behaviour, if it's wrong it should be fixed in a different bug.
Comment 11 Alexey Proskuryakov 2014-01-03 10:15:59 PST
Comment on attachment 220310 [details]
Updated patch

I sign off as a WebKit2 owner. I didn't look at Gtk and Soup specific changes closely this time.
Comment 12 Martin Robinson 2014-01-03 10:24:31 PST
Comment on attachment 220310 [details]
Updated patch

Soup and GTK+ changes look good to me.
Comment 13 Carlos Garcia Campos 2014-01-03 10:29:42 PST
(In reply to comment #11)
> (From update of attachment 220310 [details])
> I sign off as a WebKit2 owner. I didn't look at Gtk and Soup specific changes closely this time.

Thank you.
Comment 14 Carlos Garcia Campos 2014-01-03 11:10:00 PST
Committed r161272: <http://trac.webkit.org/changeset/161272>
Comment 15 Jae Hyun Park 2014-01-14 18:12:26 PST
*** Bug 127026 has been marked as a duplicate of this bug. ***