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(-)
<rdar://problem/9268729>
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.
(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?
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.
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.
Attachment 89914 [details] did not build on qt: Build output: http://queues.webkit.org/results/8436462
(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
Created attachment 89927 [details] Patch v3 Fixed Qt build failure.
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.
(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).
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.
Committed r84120: <http://trac.webkit.org/changeset/84120>