[webkit-reviews] review denied: [Bug 110141] [WK2] Make Gtk and Efl build and run with ENABLE_NETWORK_PROCESS : [Attachment 205789] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Kwang Yul Seo
<skyul at company100.net>'s request for review:
Bug 110141: [WK2] Make Gtk and Efl build and run with ENABLE_NETWORK_PROCESS
https://bugs.webkit.org/show_bug.cgi?id=110141

Attachment 205789: Patch
https://bugs.webkit.org/attachment.cgi?id=205789&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=205789&action=review


Not quite ready.  Enough questions.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:221
> +#if PLATFORM(MAC)
>     
m_suggestedRequestForWillSendRequest.updateFromDelegatePreservingOldHTTPBody(ne
wRequest.nsURLRequest(DoNotUpdateHTTPBody));
> +#else
> +   
m_suggestedRequestForWillSendRequest.updateFromDelegatePreservingOldHTTPBody(ne
wRequest);
> +#endif

I wish we could find a sensible way to avoid this.

> Source/WebKit2/Shared/WebResourceBuffer.h:-34
> +#include "ShareableResource.h"
>  #include <WebCore/ResourceBuffer.h>
>  
>  namespace WebKit {
>  
> -class ShareableResource;
> -

The ChangeLog indicates this undesirable change is because of GCC requirements.


Why does GCC have this deficiency?

Was the deficiency in GCC fixed in a later version?

There's been a vocal and public push to move the compiler dependencies in the
project forward, and WebKit2 has traditionally been the flagship of this
effort.  Is it truly unavoidable to have to make this type of change?

> Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:32
>  #include "AuthenticationChallengeProxy.h"
> -#include "CustomProtocolManagerProxyMessages.h"
>  #include "DownloadProxyMessages.h"

I think the fact that CustomProtocolManagerProxyMessages.h is in the
ENABLE(NETWORK_PROCESS) block means that...

> Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:45
> +#if ENABLE(CUSTOM_PROTOCOLS)
> +#include "CustomProtocolManagerProxyMessages.h"
> +#endif

...this block should also be in the ENABLE(NETWORK_PROCESS) block.

In other words, there's no such thing as enabling custom protocol support
without enabling network process support, and the code should reflect this.

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:111
> +#if USE(CFNETWORK)
>      responseCopy.setCertificateChain(certificateInfo.certificateChain());
> +#endif

I understand this is (currently) necessary because of code in WebCore, which is
too bad.

We should push "PlatformCertificateInfo" to WebCore and make the
ResourceRequest/Response primitives work in terms of that platform agnostic
object instead of CF-specific types.

> Source/WebKit2/WebProcess/WebCoreSupport/efl/WebErrorsEfl.cpp:78
> +WebCore::ResourceError internalError(const WebCore::KURL& url)
> +{
> +    return ResourceError(WebError::webKitErrorDomain(), kWKErrorInternal,
url.string(), ASCIILiteral("Internal error"));
> +}

It's too bad that this code...

> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebErrorsGtk.cpp:80
> +WebCore::ResourceError internalError(const WebCore::KURL& url)
> +{
> +    return ResourceError(WebError::webKitErrorDomain(), kWKErrorInternal,
url.string(), ASCIILiteral("Internal error"));
> +}

...and this code are direct copies of each other.

But I suppose EFL and GTK could theoretically diverge here, as they are already
divergent from Mac...

which leads me to this question - Is there any reason why these are divergent
from Mac?  Seems like the section of errors in WebErrorsMac.mm labelled "//
Otherwise, fallback to our own errors."  *could* be 100% cross platform.