Bug 58463 – Switch HTTP pipelining from user default to private setting

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

[David Kilzer (:ddkilzer)]

Created attachment 89413 [details]
Patch

Reviewed by NOBODY (OOPS!).

This replaces support for the WebKitEnableHTTPPipelining user
default with methods on the WebCore::Setting C++ class and
the WebPreference Objective-C class.  It also removes support
for the WebKitForceHTTPPipeliningPriorityHigh user default which
was not needed.

Source/WebCore:

Run these commands if you set the user defaults previously,
replacing "BUNDLE.ID" with your application's bundle ID:

defaults delete BUNDLE.ID WebKitEnableHTTPPipelining
defaults delete BUNDLE.ID WebKitForceHTTPPipeliningPriorityHigh

* WebCore.exp.in: Export Settings::httpPipeliningEnabled() and
Settings::setHTTPPipeliningEnabled().
* page/Settings.cpp:
(WebCore::Settings::gHTTPPipeliningEnabled): Added.
(WebCore::Settings::setHTTPPipeliningEnabled): Added.
(WebCore::Settings::httpPipeliningEnabled): Added.
* page/Settings.h:
(WebCore::Settings::setHTTPPipeliningEnabled): Added declaration.
(WebCore::Settings::httpPipeliningEnabled): Added declaration.
(WebCore::Settings::gHTTPPipeliningEnabled): Added declaration.
* platform/network/ResourceRequestBase.h:
(WebCore::isHTTPPipeliningEnabled): Removed declaration.
(WebCore::shouldForceHTTPPipeliningPriorityHigh): Removed declaration.
* platform/network/cf/ResourceRequestCFNet.cpp:
(WebCore::initializeMaximumHTTPConnectionCountPerHost): Switched
to using Settings::httpPipeliningEnabled().
(WebCore::readBooleanPreference): Removed.
(WebCore::isHTTPPipeliningEnabled): Removed.
(WebCore::shouldForceHTTPPipeliningPriorityHigh): Removed.
* platform/network/mac/ResourceRequestMac.mm:
(WebCore::ResourceRequest::doUpdateResourceRequest): Switched to
using Settings::httpPipeliningEnabled().  Removed check for
shouldForceHTTPPipeliningPriorityHigh().
(WebCore::ResourceRequest::doUpdatePlatformRequest): Ditto.

Source/WebKit/mac:

* WebView/WebPreferences.mm:
(+[WebPreferences httpPipeliningEnabled]): Added.
(+[WebPreferences setHTTPPipeliningEnabled:]): Added.
* WebView/WebPreferencesPrivate.h:
(+[WebPreferences httpPipeliningEnabled]): Added declaration.
(+[WebPreferences setHTTPPipeliningEnabled:]): Added declaration.

Source/WebKit2:

* UIProcess/API/C/WKPreferences.cpp:
(WKPreferencesSetHTTPPipeliningEnabled): Added.
(WKPreferencesGetHTTPPipeliningEnabled): Added.
* UIProcess/API/C/WKPreferencesPrivate.h:
(WKPreferencesSetHTTPPipeliningEnabled): Added declaration.
(WKPreferencesGetHTTPPipeliningEnabled): Added declaration.
---
 13 files changed, 133 insertions(+), 33 deletions(-)

[David Kilzer (:ddkilzer)]

<rdar://problem/9268729>

[WebKit Review Bot]

Attachment 89413 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/mac/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 89413 [details]
Patch

Even though there is precedent for static members in Settings, I don’t think it’s a good idea to add more. The same goes for class methods on WebPreferences. This sort of global setting, when absolutely necessary, should be a WebView class method. In WebKit2, this is probably something that should be set on a context.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> (From update of attachment 89413 [details])
> Even though there is precedent for static members in Settings, I don’t think it’s a good idea to add more. The same goes for class methods on WebPreferences. This sort of global setting, when absolutely necessary, should be a WebView class method. In WebKit2, this is probably something that should be set on a context.

It's easy enough to move the static methods to the WebView class, but what object should back the settings in WebCore?  ResourceRequestCFNet.cpp makes the most sense, I guess, since that's where it's used.

Is there an example of a static setting on a context in WebKit2 that I can use as an example?

[David Kilzer (:ddkilzer)]

Created attachment 89914 [details]
Patch v2

Changes since v1:

- Moved static variable and getter/setter methods from Settings to ResourceRequest.
- Moved WebKit1 methods from WebPreferences to WebView.
- Moved WebKit2 methods from WKPreferences to WebContext.

[WebKit Review Bot]

Attachment 89914 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/UIProcess/WebContext.h:175:  The parameter name "enabled" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 89914 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8436462

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> Attachment 89914 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/8436462

Fixed locally:


diff --git a/Source/WebKit2/UIProcess/WebContext.cpp b/Source/WebKit2/UIProcess/WebContext.cpp
index 2b58357..948ef14 100644
--- a/Source/WebKit2/UIProcess/WebContext.cpp
+++ b/Source/WebKit2/UIProcess/WebContext.cpp
@@ -751,12 +751,18 @@ String WebContext::localStorageDirectory() const
 
 void WebContext::setHttpPipeliningEnabled(bool enabled)
 {
+#if PLATFORM(MAC)
     ResourceRequest::setHttpPipeliningEnabled(enabled);
+#endif
 }
 
 bool WebContext::httpPipeliningEnabled()
 {
+#if PLATFORM(MAC)
     return ResourceRequest::httpPipeliningEnabled();
+#else
+    return false;
+#endif
 }
 
 } // namespace WebKit

[David Kilzer (:ddkilzer)]

Created attachment 89927 [details]
Patch v3

Fixed Qt build failure.

[WebKit Review Bot]

Attachment 89927 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/UIProcess/WebContext.h:175:  The parameter name "enabled" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 89927 [details]
Patch v3

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

Did you forget to add the WKContext API for this? r- because of naming issues.

> Source/WebCore/platform/network/cf/ResourceRequest.h:92
> +        static void setHttpPipeliningEnabled(bool flag);

“flag” is redundant. HTTP should be all-uppercase.

> Source/WebKit/mac/WebView/WebView.mm:2831
> ++ (BOOL)_httpPipeliningEnabled

HTTP should be all-uppercase

> Source/WebKit/mac/WebView/WebView.mm:2836
> ++ (void)_setHttpPipeliningEnabled:(BOOL)enabled

HTTP should be all-uppercase

> Source/WebKit2/UIProcess/WebContext.cpp:752
> +void WebContext::setHttpPipeliningEnabled(bool enabled)

HTTP should be all-uppercase.

[David Kilzer (:ddkilzer)]

(In reply to comment #11)
> (From update of attachment 89927 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89927&action=review
> 
> Did you forget to add the WKContext API for this?

What is the relationship of WKContext to WebContext in WebKit2?

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 89927 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=89927&action=review
> > 
> > Did you forget to add the WKContext API for this?
> 
> What is the relationship of WKContext to WebContext in WebKit2?

WKContext is the API form of WebContext. Clients can’t call WebContext functions, so there would need to be functions like WKContextSetHTTPPipeliningEnabled(WKContextRef contextRef, … enabled).

[David Kilzer (:ddkilzer)]

Created attachment 89981 [details]
Patch v4

Changes since Patch v3:

- Addressed issues in Comment #11 (Patch v3 review), including adding a method to WKContext.

Comment on attachment 89981 [details]
Patch v4

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

> Source/WebKit2/UIProcess/API/C/WKContextPrivate.h:69
> +WK_EXPORT void _WKContextSetHTTPPipeliningEnabled(WKContextRef, bool);

In a C API header, the parameters should be named.

[David Kilzer (:ddkilzer)]

Committed r84120: <http://trac.webkit.org/changeset/84120>