Bug 193429 - Split headerValueForVary into specialized functions for NetworkProcess and WebProcess/WebKitLegacy
Summary: Split headerValueForVary into specialized functions for NetworkProcess and We...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-14 20:45 PST by Alex Christensen
Modified: 2019-01-14 21:17 PST (History)
7 users (show)

See Also:


Attachments
Patch (15.99 KB, patch)
2019-01-14 20:52 PST, Alex Christensen
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-01-14 20:45:14 PST
Split headerValueForVary into specialized functions for NetworkProcess and WebProcess/WebKitLegacy
Comment 1 Alex Christensen 2019-01-14 20:52:27 PST
Created attachment 359127 [details]
Patch
Comment 2 Joseph Pecoraro 2019-01-14 21:08:24 PST
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());
Comment 3 Alex Christensen 2019-01-14 21:10:43 PST
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.
Comment 4 Alex Christensen 2019-01-14 21:16:12 PST
http://trac.webkit.org/r239974
Comment 5 Radar WebKit Bug Importer 2019-01-14 21:17:29 PST
<rdar://problem/47275515>