Bug 80800 – [BlackBerry]Cookies shouldn't be set into each of webcore's request and platform's request. And this makes a regression.

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Jason Liu]

If cookies have been set into resource request of webcore, they shouldn't be
set into platform's request again.

This makes a regression of PR130055(UTF chars printed back from cookie through php shows as ???).
And there is also a performance issue to read cookies from DB two times.

[Jason Liu]

Created attachment 131278 [details]
Patch

[Jason Liu]

Created attachment 131300 [details]
Patch

[Konrad Piascik]

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?

[Rob Buis]

Comment on attachment 131300 [details]
Patch

R- based on Konrad's concerns.

[Jason Liu]

>> 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.

[Konrad Piascik]

(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.

[Jason Liu]

Created attachment 131811 [details]
Patch

[Jason Liu]

Created attachment 131827 [details]
Patch

[Joe Mason]

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!

[Jason Liu]

Created attachment 132001 [details]
Patch

[Jason Liu]

(In reply to comment #10)
> Created an attachment (id=132001) [details]
> Patch

Please help me to review the new patch.

Thanks a lot.

[Joe Mason]

(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)

[Joe Mason]

I've filed bug 81356 to fix the pre-existing layering violation in this patch.

[WebKit Review Bot]

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

[Jason Liu]

Created attachment 132764 [details]
Patch

[Jason Liu]

Created attachment 132765 [details]
Patch

[Jason Liu]

Created attachment 132766 [details]
Patch

[Jason Liu]

Created attachment 132772 [details]
Patch

[Leo Yang]

Comment on attachment 132772 [details]
Patch

Sending the reabsed patch to cq ...

[WebKit Review Bot]

Comment on attachment 132772 [details]
Patch

Clearing flags on attachment: 132772

Committed r111370: <http://trac.webkit.org/changeset/111370>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.