Bug 136370 – Remove NetworkResourceLoaderClient and subclasses.

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

This code is needlessly abstract. It can be replaced with a few ifs in NetworkResourceLoader.

[Antti Koivisto]

Created attachment 237349 [details]
patch

[Antti Koivisto]

Created attachment 237350 [details]
fix cmake build

[Darin Adler]

Comment on attachment 237350 [details]
fix cmake build

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:388
> +#endif // __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090

I don’t think this comment is helpful. It’s gotten out of sync with the #if, which is one of the main reasons I don’t like comments like this, and it’s only a few lines away from the #if.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:174
> +    struct SynchronousLoadData {
> +        SynchronousLoadData(WebCore::ResourceRequest& request, PassRefPtr<Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::DelayedReply> reply)
> +            : m_originalRequest(request)
> +            , m_delayedReply(reply)
> +        {
> +            ASSERT(m_delayedReply);
> +        }
> +        WebCore::ResourceRequest m_originalRequest;
> +        WebCore::ResourceRequest m_currentRequest;
> +        RefPtr<Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::DelayedReply> m_delayedReply;
> +        WebCore::ResourceResponse m_response;
> +        WebCore::ResourceError m_error;
> +    };

I don’t think we need to put the definition of this struct in the header. We can compile references to this struct and unique_ptr to this struct without having the entire class in the header. And it would be nice to not have to include ResourceError.h and ResourceResponse.h here. Can we forward-declare the struct here and put the definition in the .cpp file? I believe this can be done even for a nested struct.

[Alexey Proskuryakov]

Comment on attachment 237350 [details]
fix cmake build

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

Makes sense to me as well, the abstraction did cause some confusion in the past.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:264
> +    if (isSynchronous()) {

It's slightly annoying that the unusual code path goes first here and elsewhere.

[Antti Koivisto]

Comment on attachment 237350 [details]
fix cmake build

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

>> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:264
>> +    if (isSynchronous()) {
> 
> It's slightly annoying that the unusual code path goes first here and elsewhere.

I agree with if...else cases but here specifically we are doing an early return. I think the favored style is that main body of the function is the common case while early returns are exceptions.

[Antti Koivisto]

https://trac.webkit.org/r173115