Bug 126208 – Define WebProcess::usesNetworkProcess unconditionally

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 219961 [details]
Patch

[Sergio Villar Senin]

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

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 220071 [details]
Updated patch

Just rebased to apply on current git master.

[Alexey Proskuryakov]

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.

[Carlos Garcia Campos]

(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.

[Alexey Proskuryakov]

> 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.

[Carlos Garcia Campos]

(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.

[Alexey Proskuryakov]

> 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.

[Carlos Garcia Campos]

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.

[Alexey Proskuryakov]

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.

[Martin Robinson]

Comment on attachment 220310 [details]
Updated patch

Soup and GTK+ changes look good to me.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Committed r161272: <http://trac.webkit.org/changeset/161272>

[Jae Hyun Park]

*** Bug 127026 has been marked as a duplicate of this bug. ***