[BlackBerry]Cookies shouldn't be set into each of webcore's request and platform...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 20 Mar 2012 08:24:37 +0000 (08:24 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 20 Mar 2012 08:24:37 +0000 (08:24 +0000)
https://bugs.webkit.org/show_bug.cgi?id=80800

Source/WebCore:

FrameLoaderClientBlackBerry::dispatchWillSendRequest sets cookies to resourceRequest's header
list to show cookies in inspector. And NetworkManager::startJob will set cookies to platformRequest's
m_cookieData again. So cookies are set two times. This causes performance issue.

Moreover, platformRequest will copy cookies from the resourceRequest. And then platformRequest has
the same cookies in its header list and m_cookieData. Network will use header list's cookies which
are output as latin1 only. This causes the regression of https://bugs.webkit.org/show_bug.cgi?id=80307.

Now, set cookies in initializePlatformRequest to ensure setting cookies only once.

Patch by Jason Liu <jason.liu@torchmobile.com.cn> on 2012-03-20
Reviewed by George Staikos.

Test: http/tests/cookies/resources/setUtf8Cookies.php

* platform/network/blackberry/NetworkManager.cpp:
(WebCore::NetworkManager::startJob):
* platform/network/blackberry/ResourceRequest.h:
(ResourceRequest):
* platform/network/blackberry/ResourceRequestBlackBerry.cpp:
(WebCore::ResourceRequest::initializePlatformRequest):

Source/WebKit/blackberry:

Patch by Jason Liu <jason.liu@torchmobile.com.cn> on 2012-03-20
Reviewed by George Staikos.

* WebCoreSupport/FrameLoaderClientBlackBerry.cpp:
(WebCore::FrameLoaderClientBlackBerry::dispatchDecidePolicyForNavigationAction):
(WebCore::FrameLoaderClientBlackBerry::dispatchWillSendRequest):
(WebCore::FrameLoaderClientBlackBerry::decidePolicyForExternalLoad):

LayoutTests:

Patch by Jason Liu <jason.liu@torchmobile.com.cn> on 2012-03-20
Reviewed by George Staikos.

* http/tests/cookies/resources/setUtf8Cookies-expected.txt: Added.
* http/tests/cookies/resources/setUtf8Cookies-result.php: Added.
* http/tests/cookies/resources/setUtf8Cookies.php: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@111370 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/http/tests/cookies/resources/setUtf8Cookies-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/cookies/resources/setUtf8Cookies-result.php [new file with mode: 0644]
LayoutTests/http/tests/cookies/resources/setUtf8Cookies.php [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/network/blackberry/NetworkManager.cpp
Source/WebCore/platform/network/blackberry/ResourceRequest.h
Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp
Source/WebKit/blackberry/ChangeLog
Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp

index 84138ddf284e4b2278ab6dc1adbb198c1c6ab0be..b6cbf5158a451190d96e2d590e74aa9cc08680c4 100644 (file)
@@ -1,3 +1,14 @@
+2012-03-20  Jason Liu  <jason.liu@torchmobile.com.cn>
+
+        [BlackBerry]Cookies shouldn't be set into each of webcore's request and platform's request. And this makes a regression.
+        https://bugs.webkit.org/show_bug.cgi?id=80800
+
+        Reviewed by George Staikos.
+
+        * http/tests/cookies/resources/setUtf8Cookies-expected.txt: Added.
+        * http/tests/cookies/resources/setUtf8Cookies-result.php: Added.
+        * http/tests/cookies/resources/setUtf8Cookies.php: Added.
+
 2012-03-20  Keishi Hattori  <keishi@webkit.org>
 
         [chromium] Marking listbox-clear-restore.html on Linux as flaky.
diff --git a/LayoutTests/http/tests/cookies/resources/setUtf8Cookies-expected.txt b/LayoutTests/http/tests/cookies/resources/setUtf8Cookies-expected.txt
new file mode 100644 (file)
index 0000000..9b31fa1
--- /dev/null
@@ -0,0 +1,5 @@
+some utf8 characters: UTF-8 æøå 中国
+
+php_cookie: UTF-8 æøå 中国
+
+cookies read through js: php_cookie=UTF-8+%C3%A6%C3%B8%C3%A5+%E4%B8%AD%E5%9B%BD; js_cookie=UTF-8 æøå 中国
diff --git a/LayoutTests/http/tests/cookies/resources/setUtf8Cookies-result.php b/LayoutTests/http/tests/cookies/resources/setUtf8Cookies-result.php
new file mode 100644 (file)
index 0000000..e332b4f
--- /dev/null
@@ -0,0 +1,16 @@
+<?php header('Content-Type: text/html; charset=UTF-8'); ?>
+<html>
+<body>
+<p>
+<?php
+    echo "<p>some utf8 characters: UTF-8 æøå 中国</p>";
+    echo "<p>php_cookie: ".$_COOKIE["php_cookie"]."</p>";
+?>
+<script>
+    if (window.layoutTestController)
+        layoutTestController.dumpAsText();
+    document.cookie="js_cookie=UTF-8 æøå 中国; expires=Monday, 04-Apr-2020 05:00:00 GMT";
+    document.write("cookies read through js: "+document.cookie);
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/cookies/resources/setUtf8Cookies.php b/LayoutTests/http/tests/cookies/resources/setUtf8Cookies.php
new file mode 100644 (file)
index 0000000..bc714fd
--- /dev/null
@@ -0,0 +1,5 @@
+<?php
+    setcookie("php_cookie", "UTF-8 æøå 中国");
+    header('Content-Type: text/html; charset=UTF-8');
+    header('Location: setUtf8Cookies-result.php');
+?>
index 471753360ce28cbbf376238f52c4192a68af18ab..0f6a0159ab7511fd7288add97d33f214527fb0a7 100644 (file)
@@ -1,3 +1,29 @@
+2012-03-20  Jason Liu  <jason.liu@torchmobile.com.cn>
+
+        [BlackBerry]Cookies shouldn't be set into each of webcore's request and platform's request. And this makes a regression.
+        https://bugs.webkit.org/show_bug.cgi?id=80800
+
+        FrameLoaderClientBlackBerry::dispatchWillSendRequest sets cookies to resourceRequest's header
+        list to show cookies in inspector. And NetworkManager::startJob will set cookies to platformRequest's
+        m_cookieData again. So cookies are set two times. This causes performance issue.
+
+        Moreover, platformRequest will copy cookies from the resourceRequest. And then platformRequest has
+        the same cookies in its header list and m_cookieData. Network will use header list's cookies which
+        are output as latin1 only. This causes the regression of https://bugs.webkit.org/show_bug.cgi?id=80307.
+
+        Now, set cookies in initializePlatformRequest to ensure setting cookies only once.
+
+        Reviewed by George Staikos.
+
+        Test: http/tests/cookies/resources/setUtf8Cookies.php
+
+        * platform/network/blackberry/NetworkManager.cpp:
+        (WebCore::NetworkManager::startJob):
+        * platform/network/blackberry/ResourceRequest.h:
+        (ResourceRequest):
+        * platform/network/blackberry/ResourceRequestBlackBerry.cpp:
+        (WebCore::ResourceRequest::initializePlatformRequest):
+
 2012-03-19  Benjamin Poulain  <bpoulain@apple.com>
 
         Build fix for Debug build after r111358
index 7c628f0057a7dad0642080f2f04945a49d08f959..e3832e933f930af868dae565fd769ac0fd6da97b 100644 (file)
@@ -20,7 +20,6 @@
 #include "NetworkManager.h"
 
 #include "Chrome.h"
-#include "CookieManager.h"
 #include "CredentialStorage.h"
 #include "Frame.h"
 #include "FrameLoaderClientBlackBerry.h"
@@ -73,7 +72,7 @@ bool NetworkManager::startJob(int playerId, const String& pageGroupName, PassRef
         m_initialURL = KURL();
 
     BlackBerry::Platform::NetworkRequest platformRequest;
-    request.initializePlatformRequest(platformRequest, isInitial);
+    request.initializePlatformRequest(platformRequest, frame.loader() && frame.loader()->client() && static_cast<FrameLoaderClientBlackBerry*>(frame.loader()->client())->cookiesEnabled(), isInitial, redirectCount);
 
     // Attach any applicable auth credentials to the NetworkRequest.
     AuthenticationChallenge& challenge = guardJob->getInternal()->m_currentWebChallenge;
@@ -117,16 +116,6 @@ bool NetworkManager::startJob(int playerId, const String& pageGroupName, PassRef
             platformRequest.setCredentials(credential.user().utf8().data(), credential.password().utf8().data(), BlackBerry::Platform::NetworkRequest::AuthHTTPBasic);
     }
 
-    if ((&frame) && (&frame)->loader() && (&frame)->loader()->client()
-        && static_cast<FrameLoaderClientBlackBerry*>((&frame)->loader()->client())->cookiesEnabled()) {
-        // Prepare a cookie header if there are cookies related to this url.
-        String cookiePairs = cookieManager().getCookie(url, WithHttpOnlyCookies);
-        if (!cookiePairs.isEmpty()) {
-            // We need to check the encoding and encode the cookie header data using latin1 or utf8 to support unicode characters.
-            platformRequest.setCookieData(cookiePairs.containsOnlyLatin1() ? cookiePairs.latin1().data() : cookiePairs.utf8().data());
-        }
-    }
-
     if (!request.overrideContentType().isEmpty())
         platformRequest.setOverrideContentType(request.overrideContentType().latin1().data());
 
index 7072eb1f35134adc3641bd1541b16cdac18017ab..ad191ff8423e378af00dc8e7ec0457bf7f6c08ae 100644 (file)
@@ -110,7 +110,7 @@ public:
     void setMustHandleInternally(bool mustHandleInternally) { m_mustHandleInternally = mustHandleInternally; }
     bool mustHandleInternally() const { return m_mustHandleInternally; }
 
-    void initializePlatformRequest(BlackBerry::Platform::NetworkRequest&, bool isInitial = false) const;
+    void initializePlatformRequest(BlackBerry::Platform::NetworkRequest&, bool cookiesEnabled, bool isInitial = false, bool isRedirect = false) const;
     void setForceDownload(bool forceDownload) { m_forceDownload = true; }
     bool forceDownload() const { return m_forceDownload; }
 
index 5b6792fa7f1226cd098be9ee26b82bd7f3cdbdfd..6567e95803139668e1cd783108f65d8c4ab5c9f5 100644 (file)
@@ -20,6 +20,7 @@
 #include "ResourceRequest.h"
 
 #include "BlobRegistryImpl.h"
+#include "CookieManager.h"
 #include <BlackBerryPlatformClient.h>
 #include <network/NetworkRequest.h>
 #include <wtf/HashMap.h>
@@ -126,7 +127,7 @@ ResourceRequest::TargetType ResourceRequest::targetTypeFromMimeType(const String
     return iter->second;
 }
 
-void ResourceRequest::initializePlatformRequest(NetworkRequest& platformRequest, bool isInitial) const
+void ResourceRequest::initializePlatformRequest(NetworkRequest& platformRequest, bool cookiesEnabled, bool isInitial, bool isRedirect) const
 {
     // If this is the initial load, skip the request body and headers.
     if (isInitial)
@@ -177,8 +178,22 @@ void ResourceRequest::initializePlatformRequest(NetworkRequest& platformRequest,
         for (HTTPHeaderMap::const_iterator it = httpHeaderFields().begin(); it != httpHeaderFields().end(); ++it) {
             String key = it->first;
             String value = it->second;
-            if (!key.isEmpty() && !value.isEmpty())
-                platformRequest.addHeader(key.latin1().data(), value.latin1().data());
+            if (!key.isEmpty() && !value.isEmpty()) {
+                // We need to check the encoding and encode the cookie's value using latin1 or utf8 to support unicode characters.
+                if (equalIgnoringCase(key, "Cookie"))
+                    platformRequest.addHeader(key.latin1().data(), value.containsOnlyLatin1() ? value.latin1().data() : value.utf8().data());
+                else
+                    platformRequest.addHeader(key.latin1().data(), value.latin1().data());
+            }
+        }
+       
+        // Redirection's response may contain new cookies, so add cookies again.
+        // If there aren't cookies in the header list, we need trying to add cookies.
+        if (cookiesEnabled && (isRedirect || !httpHeaderFields().contains("Cookie"))) {
+            // Prepare a cookie header if there are cookies related to this url.
+            String cookiePairs = cookieManager().getCookie(url(), WithHttpOnlyCookies);
+            if (!cookiePairs.isEmpty())
+                platformRequest.addHeader("Cookie", cookiePairs.containsOnlyLatin1() ? cookiePairs.latin1().data() : cookiePairs.utf8().data());
         }
 
         // Locale has the form "en-US". Construct accept language like "en-US, en;q=0.8".
index 2740218249805189311f7b6bbd1c8dec7ae28bb9..5b45aac397a8c965bc293137fdebebe3b40ce886 100644 (file)
@@ -1,3 +1,15 @@
+2012-03-20  Jason Liu  <jason.liu@torchmobile.com.cn>
+
+        [BlackBerry]Cookies shouldn't be set into each of webcore's request and platform's request. And this makes a regression.
+        https://bugs.webkit.org/show_bug.cgi?id=80800
+
+        Reviewed by George Staikos.
+
+        * WebCoreSupport/FrameLoaderClientBlackBerry.cpp:
+        (WebCore::FrameLoaderClientBlackBerry::dispatchDecidePolicyForNavigationAction):
+        (WebCore::FrameLoaderClientBlackBerry::dispatchWillSendRequest):
+        (WebCore::FrameLoaderClientBlackBerry::decidePolicyForExternalLoad):
+
 2012-03-19  Adam Barth  <abarth@webkit.org>
 
         Remove support for "magic" iframe
index 7db35ab76420c6a46dd04daa8d8bf3f3c5c22d7d..088f89ff67809582f54c8c7261ecf473d94cd078 100644 (file)
@@ -207,7 +207,7 @@ void FrameLoaderClientBlackBerry::dispatchDecidePolicyForNavigationAction(FrameP
     // Let the client have a chance to say whether this navigation should
     // be ignored or not.
     BlackBerry::Platform::NetworkRequest platformRequest;
-    request.initializePlatformRequest(platformRequest, false /*isInitial*/);
+    request.initializePlatformRequest(platformRequest, cookiesEnabled());
     if (isMainFrame() && !m_webPagePrivate->m_client->acceptNavigationRequest(
         platformRequest, BlackBerry::Platform::NavigationType(action.type()))) {
         if (action.type() == NavigationTypeFormSubmitted
@@ -909,20 +909,13 @@ void FrameLoaderClientBlackBerry::dispatchWillSendRequest(DocumentLoader* docLoa
 
     // Any processing which is done for all loads (both main and subresource) should go here.
     BlackBerry::Platform::NetworkRequest platformRequest;
-    request.initializePlatformRequest(platformRequest, false /*isInitial*/);
+    request.initializePlatformRequest(platformRequest, cookiesEnabled());
     m_webPagePrivate->m_client->populateCustomHeaders(platformRequest);
     const BlackBerry::Platform::NetworkRequest::HeaderList& headerLists = platformRequest.getHeaderListRef();
     for (BlackBerry::Platform::NetworkRequest::HeaderList::const_iterator it = headerLists.begin(); it != headerLists.end(); ++it) {
         std::string headerString = it->first;
         std::string headerValueString = it->second;
-        request.setHTTPHeaderField(String(headerString.c_str()), String(headerValueString.c_str()));
-    }
-    if (cookiesEnabled()) {
-        String cookiePairs = cookieManager().getCookie(request.url(), WithHttpOnlyCookies);
-        if (!cookiePairs.isEmpty()) {
-            // We only modify the WebCore request to make the cookies visible in inspector.
-            request.setHTTPHeaderField(String("Cookie"), cookiePairs);
-        }
+        request.setHTTPHeaderField(String::fromUTF8WithLatin1Fallback(headerString.data(), headerString.length()), String::fromUTF8WithLatin1Fallback(headerValueString.data(), headerValueString.length()));
     }
     if (!isMainResourceLoad) {
         // Do nothing for now.
@@ -1068,7 +1061,7 @@ PolicyAction FrameLoaderClientBlackBerry::decidePolicyForExternalLoad(const Reso
             && !request.mustHandleInternally()
             && !isFragmentScroll) {
         BlackBerry::Platform::NetworkRequest platformRequest;
-        request.initializePlatformRequest(platformRequest);
+        request.initializePlatformRequest(platformRequest, cookiesEnabled());
         m_webPagePrivate->m_client->handleExternalLink(platformRequest, request.anchorText().characters(), request.anchorText().length(), m_clientRedirectIsPending);
         return PolicyIgnore;
     }