Bug 193429 – Split headerValueForVary into specialized functions for NetworkProcess and WebProcess/WebKitLegacy

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Split headerValueForVary into specialized functions for NetworkProcess and WebProcess/WebKitLegacy

[Alex Christensen]

Created attachment 359127 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 359127 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359127&action=review

r=me

> Source/WebCore/platform/network/CacheValidation.cpp:346
> +        return cookieRequestHeaderFieldValue();

I was confused for a while that this was the parameter name and not one of the static functions. Might be nice to give this a different name that doesn't clash, or maybe that was your intention.

> Source/WebCore/platform/network/CacheValidation.cpp:407
> +    return verifyVaryingRequestHeaders(varyingRequestHeaders, [&] (const String& headerName) {

Do we want to maintain the assert, and potentially change it later?

    ASSERT(sessionID == PAL::SessionID::defaultSessionID());

[Alex Christensen]

Comment on attachment 359127 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359127&action=review

>> Source/WebCore/platform/network/CacheValidation.cpp:346
>> +        return cookieRequestHeaderFieldValue();
> 
> I was confused for a while that this was the parameter name and not one of the static functions. Might be nice to give this a different name that doesn't clash, or maybe that was your intention.

Yeah, I'll add -Function and -Internal suffixes to make this less confusing.

>> Source/WebCore/platform/network/CacheValidation.cpp:407
>> +    return verifyVaryingRequestHeaders(varyingRequestHeaders, [&] (const String& headerName) {
> 
> Do we want to maintain the assert, and potentially change it later?
> 
>     ASSERT(sessionID == PAL::SessionID::defaultSessionID());

The assertion is not valid here.  It is valid in the versions that use a NetworkStorageSession&, and I'm using defaultStorageSession in this patch which accomplishes the same thing as the assertion in a better way.

[Alex Christensen]

http://trac.webkit.org/r239974

[Radar WebKit Bug Importer]

<rdar://problem/47275515>