[webkit-reviews] review denied: [Bug 110115] [WK2][Soup] Add platform specific stubs for NetworkProcess : [Attachment 206005] 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 110115: [WK2][Soup] Add platform specific stubs for NetworkProcess
https://bugs.webkit.org/show_bug.cgi?id=110115

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

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


r- because of the WebCore change.

> Source/WebCore/platform/network/NetworkStorageSession.h:-57
> +    bool isPrivateBrowsingSession() const
> +    {
> +#if PLATFORM(MAC) || USE(CFNETWORK)
> +	   return m_isPrivate;
> +#else
> +	   return false;
> +#endif
> +    }
> +
>  #if PLATFORM(MAC) || USE(CFNETWORK)
>      // May be null, in which case a Foundation default should be used.
>      CFURLStorageSessionRef platformSession() { return
m_platformSession.get(); }
>      RetainPtr<CFHTTPCookieStorageRef> cookieStorage() const;
> -    bool isPrivateBrowsingSession() const { return m_isPrivate; }

No.  This is silly.

Private browsing is a cross-platform concept that's been exposed to all of
WebCore for years.  It is going to be getting *more* important going forward,
not less.  It would be wise for every port to start handling it properly.

And we can't change WebKit1-style clients (present or future) based on one
platform's WK2-specific needs.

If this behavior is truly required (for now?), make the storage session always
have m_isPrivate == false in platform-specific code.

> Source/WebKit2/NetworkProcess/soup/NetworkResourceLoadSchedulerSoup.cpp:41
> +    // Soup has its own queue control; it wants to have all requests
> +    // given to it, so that it is able to look ahead, and schedule
> +    // them in a good way.
> +    // See also ResourceRequestSoup.cpp
> +    static const unsigned unlimitedConnectionCount = 10000;
> +    m_maxRequestsInFlightPerHost = unlimitedConnectionCount;

I would recommend against this design decision.

Even if the networking library handles per-host scheduling correctly (and I'm
sure libsoup does), there's plenty of Web-platform specific knowledge that can
help prioritization better than a networking library alone can do.

There will be work in this area in the medium term that you won't get the
benefit of if you go forward with the unlimited queue approach.

Additionally, we've seen IPC race conditions related to synchronous
XMLHTTPRequests where we've learned you can get into serious trouble endlessly
servicing asynchronous requests while a WebProcess is waiting on a sync XHR.