Summary: | [BlackBerry]Cookies shouldn't be set into each of webcore's request and platform's request. And this makes a regression. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jason Liu <jasonliuwebkit> | ||||||||||||||||||||
Component: | WebKit BlackBerry | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | charles.wei, joenotcharles, kpiascik, staikos, tonikitoo, webkit.review.bot | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Other | ||||||||||||||||||||||
OS: | Other | ||||||||||||||||||||||
Attachments: |
|
Description
Jason Liu
2012-03-11 18:44:13 PDT
Created attachment 131278 [details]
Patch
Created attachment 131300 [details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=131300&action=review > Source/WebCore/ChangeLog:3 > + [BlackBerry]Cookies shouldn't be set into each of webcore's request and platform's request. And this makes a regression. Can you clarify what the regression is in the commit message and how it is fixed. I know its in the bug but it should also be in the commit message. > Source/WebCore/ChangeLog:8 > + No new tests. There should be a UTF8 vs latin1 test you can add to test your fix. > Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:183 > + if (key.contains("Cookie")) instead of checking for key.contains("Cookie"), which you may want to verify if it's case sensitive or not, you could use String::fromUTF8WithLatin1Fallback similarly to how it's called in FrameLoaderClienBlackBerry::dispatchWillSendRequest(). This could be more efficient than a string comparison. > Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:192 > + if (cookiesEnabled && (isRedirect || !httpHeaderField("Cookie").length())) { Can you elaborate on why this could happen? > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:212 > + request.initializePlatformRequest(platformRequest, cookiesEnabled(), false /*isInitial*/); false /*isInitial*/ This doesn't need to be here since false is the default value. Either it should be taken out for all the calls to initializePlatformRequest, or you should also add "false /*isRedirect*/" to each method call in FrameLoaderClientBlackBerry for consistency. > Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:941 > + request.initializePlatformRequest(platformRequest, cookiesEnabled(), 0, false /*isInitial*/); is this a typo? Comment on attachment 131300 [details]
Patch
R- based on Konrad's concerns.
>> Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:183 >> + if (key.contains("Cookie")) >instead of checking for key.contains("Cookie"), which you may want to verify >if it's case sensitive or not, you could use >String::fromUTF8WithLatin1Fallback similarly to how it's called in >FrameLoaderClienBlackBerry::dispatchWillSendRequest(). This could be more >efficient than a string comparison. Joe Mason suggests to talk about checking all headers in another PR. So I make an if() branch to ensure only check cookies. And I don't know how to use String::fromUTF8WithLatin1Fallback instead of if(key.contains("Cookies"). Can you give me some detail information? Thanks. (In reply to comment #5) instead of if(key.contains("Cookies"). > Can you give me some detail information? > > Thanks. I think I was confused in thinking that String::fromUTF8WithLatin1Fallback could be used to set the platformRequest header data but it appears its only used as a constructor. Ignore my comment about this. Created attachment 131811 [details]
Patch
Created attachment 131827 [details]
Patch
I think key.contains("Cookie") should be more specific, since that will match headers like "X-FreshBakedCookies". It should check for the key to be exactly equal to "Cookie" (make sure to do a case-insensitive compare). Also the layout tests should include dumpAsText (see dom/html/level2/html/HTMLBaseElement01.html for an example) so that the results are easier to read. Otherwise looks good to me! Created attachment 132001 [details]
Patch
(In reply to comment #10) > Created an attachment (id=132001) [details] > Patch Please help me to review the new patch. Thanks a lot. (In reply to comment #10) > Created an attachment (id=132001) [details] > Patch r+ from me (need a real WebKit reviewer to look at it too, though) I've filed bug 81356 to fix the pre-existing layering violation in this patch. Comment on attachment 132001 [details] Patch Rejecting attachment 132001 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ing rejects to file Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp.rej patching file LayoutTests/http/tests/cookies/resources/setUtf8Cookies-expected.txt patching file LayoutTests/http/tests/cookies/resources/setUtf8Cookies-result.php patching file LayoutTests/http/tests/cookies/resources/setUtf8Cookies.php Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'George Sta..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/11967234 Created attachment 132764 [details]
Patch
Created attachment 132765 [details]
Patch
Created attachment 132766 [details]
Patch
Created attachment 132772 [details]
Patch
Comment on attachment 132772 [details]
Patch
Sending the reabsed patch to cq ...
Comment on attachment 132772 [details] Patch Clearing flags on attachment: 132772 Committed r111370: <http://trac.webkit.org/changeset/111370> All reviewed patches have been landed. Closing bug. |