[webkit-reviews] review denied: [Bug 126208] Define WebProcess::usesNetworkProcess unconditionally : [Attachment 220071] Updated patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 126208: Define WebProcess::usesNetworkProcess unconditionally
https://bugs.webkit.org/show_bug.cgi?id=126208

Attachment 220071: Updated patch
https://bugs.webkit.org/attachment.cgi?id=220071&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.