Adopt CFNetwork storage partitioning SPI
rdar://problem/23614620
Created attachment 272744 [details] Patch
Created attachment 272747 [details] Patch
Attachment 272747 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:99: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 272747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272747&action=review > Source/WebCore/page/Settings.h:358 > + static bool gCookieStoragePartitioningEnabled; These "g" prefixes sure seem peculiar. > Source/WebCore/platform/network/NetworkStorageSession.h:84 > +String cookieStoragePartition(const URL& firstPartyForCookies, const URL&); Might want a name for the second URL to somehow say that it’s the actual resource URL or whatever. > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:126 > +String cookieStoragePartition(const URL& firstPartyForCookies, const URL& url) > +{ > + if (!Settings::cookieStoragePartitioningEnabled()) > + return emptyString(); > + > + String firstPartyHost = firstPartyForCookies.host(); > +#if ENABLE(PUBLIC_SUFFIX_LIST) > + firstPartyHost = topPrivatelyControlledDomain(firstPartyHost); > +#endif > + > + String host = url.host(); > + if (!host.endsWithIgnoringASCIICase(firstPartyHost)) > + return firstPartyHost; > + > + ASSERT(host.length() >= firstPartyHost.length()); > + unsigned suffixOffset = host.length() - firstPartyHost.length(); > + if (suffixOffset > 0 && host[suffixOffset - 1] != '.') > + return firstPartyHost; > + > + return emptyString(); > +} The combination of policy and string algorithms seems a bit unpleasant to me; hard to see that both halves of the function’s job are done correctly. Maybe the predicate that takes two host strings and does the suffix check could be a separate helper function. It would return a boolean and take two StringView, not const String&. Not sure if empty string is better than null string for the "no partition" magic value. Annoying to have to even consider that question. Optional<String> would be better, if String didn’t already have two obvious special values. > Source/WebCore/platform/network/mac/CookieJarMac.mm:67 > +#if HAVE(CFNETWORK_STORAGE_PARTITIONING) I like to use blank lines inside the #if/#endif so they don’t seem grouped with the first/last functions inside more than with the rest of what’s inside. > Source/WebCore/platform/network/mac/CookieJarMac.mm:89 > + // FIXME: Stop creating a new NSHTTPCookieStorage object each time we want to query the cookie jar. A thousand times yes. > Source/WebCore/platform/network/mac/CookieJarMac.mm:97 > + // completionHandler is called synchronously. That is a peculiar way to design a method. Is the synchrony guaranteed? The comment should use active voice for greater clarity: // The _getCookiesForURL: method calls the completionHandler synchronously. > Source/WebCore/platform/network/mac/CookieJarMac.mm:184 > + filteredCookies = applyPartitionToCookies(partition, filteredCookies.autorelease()); What’s the benefit of calling autorelease() here instead of get()? I think maybe there is no benefit. > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:179 > + [(NSMutableURLRequest *)nsRequest _setProperty:storagePartition forKey:@"__STORAGE_PARTITION_IDENTIFIER"]; Why is this cast to mutable safe? The code just above for !shouldContentSniff goes out of its way to make a mutable copy if needed. Why isn’t that kind of logic needed here?
(In reply to comment #5) > Comment on attachment 272747 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=272747&action=review > > > Source/WebCore/platform/network/NetworkStorageSession.h:84 > > +String cookieStoragePartition(const URL& firstPartyForCookies, const URL&); > > Might want a name for the second URL to somehow say that it’s the actual > resource URL or whatever. Sure. I'll call it resource. > > > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:126 > > +String cookieStoragePartition(const URL& firstPartyForCookies, const URL& url) > > +{ > > + if (!Settings::cookieStoragePartitioningEnabled()) > > + return emptyString(); > > + > > + String firstPartyHost = firstPartyForCookies.host(); > > +#if ENABLE(PUBLIC_SUFFIX_LIST) > > + firstPartyHost = topPrivatelyControlledDomain(firstPartyHost); > > +#endif > > + > > + String host = url.host(); > > + if (!host.endsWithIgnoringASCIICase(firstPartyHost)) > > + return firstPartyHost; > > + > > + ASSERT(host.length() >= firstPartyHost.length()); > > + unsigned suffixOffset = host.length() - firstPartyHost.length(); > > + if (suffixOffset > 0 && host[suffixOffset - 1] != '.') > > + return firstPartyHost; > > + > > + return emptyString(); > > +} > > The combination of policy and string algorithms seems a bit unpleasant to > me; hard to see that both halves of the function’s job are done correctly. > Maybe the predicate that takes two host strings and does the suffix check > could be a separate helper function. It would return a boolean and take two > StringView, not const String&. Agreed. I'll split this up into two functions. > > Not sure if empty string is better than null string for the "no partition" > magic value. Annoying to have to even consider that question. > Optional<String> would be better, if String didn’t already have two obvious > special values. I don't think we want to create empty partition names in the cookie jar, so I'd still have to check for empty strings if I used another value for "no partition" (for instance, I'd get an empty string if firstPartyForCookie's host was empty for some reason). > > > Source/WebCore/platform/network/mac/CookieJarMac.mm:67 > > +#if HAVE(CFNETWORK_STORAGE_PARTITIONING) > > I like to use blank lines inside the #if/#endif so they don’t seem grouped > with the first/last functions inside more than with the rest of what’s > inside. Sure. > > > Source/WebCore/platform/network/mac/CookieJarMac.mm:97 > > + // completionHandler is called synchronously. > > That is a peculiar way to design a method. Is the synchrony guaranteed? Yeah, I was also surprised. I actually don't know if it's guaranteed, so I'll add some verification logic. I think I'll also file a bug asking for the cookies array to be a return value if the method is intended to remain synchronous. > > The comment should use active voice for greater clarity: > > // The _getCookiesForURL: method calls the completionHandler > synchronously. Done. > > > Source/WebCore/platform/network/mac/CookieJarMac.mm:184 > > + filteredCookies = applyPartitionToCookies(partition, filteredCookies.autorelease()); > > What’s the benefit of calling autorelease() here instead of get()? I think > maybe there is no benefit. Yeah, no benefit. I'll use get(). > > > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:179 > > + [(NSMutableURLRequest *)nsRequest _setProperty:storagePartition forKey:@"__STORAGE_PARTITION_IDENTIFIER"]; > > Why is this cast to mutable safe? The code just above for > !shouldContentSniff goes out of its way to make a mutable copy if needed. > Why isn’t that kind of logic needed here? Yeah, I'll just follow the pattern from !shouldContentSniff. I think it was safe in this specific case, but I shouldn't rely on that. Thanks for the review!
Committed r197518: <http://trac.webkit.org/changeset/197518>