These cookies are been set, it's only missing from the web inspector.
Created attachment 133455 [details] Patch
Comment on attachment 133455 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133455&action=review > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > + > + if (m_response.httpHeaderFields().contains("Set-Cookie")) > + return m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); m_response.setHTTPHeaderField(key, value) has been called at the end of this function. Why this is needed here? Are you trying to catenate all 'Set-Cookie' headers to one?
(In reply to comment #2) > (From update of attachment 133455 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133455&action=review > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > > + > > + if (m_response.httpHeaderFields().contains("Set-Cookie")) > > + return m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); > > m_response.setHTTPHeaderField(key, value) has been called at the end of this function. Why this is needed here? Are you trying to catenate all 'Set-Cookie' headers to one? Yes, I catenate them so that overwrite doesn't happen.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 133455 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=133455&action=review > > > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > > > + > > > + if (m_response.httpHeaderFields().contains("Set-Cookie")) > > > + return m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); > > > > m_response.setHTTPHeaderField(key, value) has been called at the end of this function. Why this is needed here? Are you trying to catenate all 'Set-Cookie' headers to one? > > Yes, I catenate them so that overwrite doesn't happen. But you are setting "Set-Cookie" header twice for the first "Set-Cookie" header, which is a waste. Why use "\r\n" as delimiter? What do the other ports use?
Created attachment 133754 [details] Patch
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 133455 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=133455&action=review > > > > > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > > > > + > > > > + if (m_response.httpHeaderFields().contains("Set-Cookie")) > > > > + return m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); > > > > > > m_response.setHTTPHeaderField(key, value) has been called at the end of this function. Why this is needed here? Are you trying to catenate all 'Set-Cookie' headers to one? > > > > Yes, I catenate them so that overwrite doesn't happen. > > But you are setting "Set-Cookie" header twice for the first "Set-Cookie" header, which is a waste. Why use "\r\n" as delimiter? What do the other ports use? Never mind, you returned. But is "\r\n" the best delimiter? How other ports deal with this case?
> Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > + return m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); why do you return a value from a void method?
Created attachment 133958 [details] Patch
(In reply to comment #7) > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > > + return m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); > > why do you return a value from a void method? Sorry, I forgot updating yesterday.
> Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > + m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); I still would like to know about the delimiter, as Leo mentioned. Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it?
(In reply to comment #10) > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > > + m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); > > I still would like to know about the delimiter, as Leo mentioned. Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it? I am compiling QT. Trying to find QT's implementation.
(In reply to comment #11) > (In reply to comment #10) > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > > > + m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); > > > > I still would like to know about the delimiter, as Leo mentioned. Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it? > > I am compiling QT. Trying to find QT's implementation. Qt uses comma(",") to separate them.
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > > > > + m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); > > > > > > I still would like to know about the delimiter, as Leo mentioned. Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it? > > > > I am compiling QT. Trying to find QT's implementation. > > Qt uses comma(",") to separate them. Where does qt porting do this? Network stack? or WebCore?
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > > > > + m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); > > > > > > I still would like to know about the delimiter, as Leo mentioned. Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it? > > > > I am compiling QT. Trying to find QT's implementation. > > Qt uses comma(",") to separate them. But Qt's inspector doesn't show ",". Qt's inspector: Set-Cookie:cookieONE=ONE; expires=Tue, 27-Mar-2012 04:11:32 GMT cookieTWO=TWO; expires=Tue, 27-Mar-2012 04:11:32 GMT cookieTHREE=THREE; expires=Tue, 27-Mar-2012 04:11:32 GMT cookieFOUR=FOUR; expires=Tue, 27-Mar-2012 04:11:32 GMT Playbook's inspector(using ","): Set-Cookie:cookieONE=ONE; expires=Tue, 27-Mar-2012 04:09:22 GMT,cookieTWO=TWO; expires=Tue, 27-Mar-2012 04:09:22 GMT,cookieTHREE=THREE; expires=Tue, 27-Mar-2012 04:09:22 GMT,cookieFOUR=FOUR; expires=Tue, 27-Mar-2012 04:09:22 GMT I think "\r\n" looks better for our code.
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > (In reply to comment #10) > > > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > > > > > + m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); > > > > > > > > I still would like to know about the delimiter, as Leo mentioned. Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it? > > > > > > I am compiling QT. Trying to find QT's implementation. > > > > Qt uses comma(",") to separate them. > > Where does qt porting do this? Network stack? or WebCore? Network stack.
(In reply to comment #15) > (In reply to comment #13) > > (In reply to comment #12) > > > (In reply to comment #11) > > > > (In reply to comment #10) > > > > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > > > > > > + m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); > > > > > > > > > > I still would like to know about the delimiter, as Leo mentioned. Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it? > > > > > > > > I am compiling QT. Trying to find QT's implementation. > > > > > > Qt uses comma(",") to separate them. > > > > Where does qt porting do this? Network stack? or WebCore? > > Network stack. "\r\n" or "," (In reply to comment #14) > (In reply to comment #12) > > (In reply to comment #11) > > > (In reply to comment #10) > > > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > > > > > + m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); > > > > > > > > I still would like to know about the delimiter, as Leo mentioned. Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it? > > > > > > I am compiling QT. Trying to find QT's implementation. > > > > Qt uses comma(",") to separate them. > > But Qt's inspector doesn't show ",". > Qt's inspector: > Set-Cookie:cookieONE=ONE; expires=Tue, 27-Mar-2012 04:11:32 GMT > cookieTWO=TWO; expires=Tue, 27-Mar-2012 04:11:32 GMT > cookieTHREE=THREE; expires=Tue, 27-Mar-2012 04:11:32 GMT > cookieFOUR=FOUR; expires=Tue, 27-Mar-2012 04:11:32 GMT > > Playbook's inspector(using ","): > Set-Cookie:cookieONE=ONE; expires=Tue, 27-Mar-2012 04:09:22 GMT,cookieTWO=TWO; expires=Tue, 27-Mar-2012 04:09:22 GMT,cookieTHREE=THREE; expires=Tue, 27-Mar-2012 04:09:22 GMT,cookieFOUR=FOUR; expires=Tue, 27-Mar-2012 04:09:22 GMT > > I think "\r\n" looks better for our code. "\r\n" or "," only affect inspector's showing.
Comment on attachment 133958 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133958&action=review > Source/WebCore/ChangeLog:6 > + We shouldn't overwrite cookies of response if there are more than one Don't you mean "one or more"? > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:318 > + if (m_response.httpHeaderFields().contains("Set-Cookie")) { Is there an AtomicString/static String for Set-Cookie? > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > + m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); Is "\r\n" used like that all the time? Any constants?
(In reply to comment #17) > (From update of attachment 133958 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133958&action=review > > > Source/WebCore/ChangeLog:6 > > + We shouldn't overwrite cookies of response if there are more than one > > Don't you mean "one or more"? > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:318 > > + if (m_response.httpHeaderFields().contains("Set-Cookie")) { > > Is there an AtomicString/static String for Set-Cookie? > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319 > > + m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value); > > Is "\r\n" used like that all the time? Any constants? 1. I mean the second cookie shouldn't overwrite the first one of response. 2. No, there isn't. 3. "\r\n" is only used in this special case.
Comment on attachment 133958 [details] Patch Looks good.
Comment on attachment 133958 [details] Patch Clearing flags on attachment: 133958 Committed r112494: <http://trac.webkit.org/changeset/112494>
All reviewed patches have been landed. Closing bug.