ResourceRequest deserialization unnecessarily calls partitionName() on encoded cache partition. In the deserialization case, we already know the cache partition is a valid partition name so we should bypass the call to partitionName() for performance. This is likely a regression from <http://trac.webkit.org/changeset/145161>.
Created attachment 239984 [details] Patch
Not exactly a regression. The deserialization of the cache partition was added in <http://trac.webkit.org/changeset/146544>, after that commit. I'm a little leery of adding an argument to setCachePartition, but it doesn't look like there's much way around that. Also, isn't there a better place in the class to place the enum definition than between two function definitions? Other than that minor nitpick, this looks good.
(In reply to comment #2) > Not exactly a regression. The deserialization of the cache partition was > added in <http://trac.webkit.org/changeset/146544>, after that commit. Right, sorry about that. I did not look much further, just saw that the patch made the argument processing mandatory by moving it from the callers to the method implementation. > > I'm a little leery of adding an argument to setCachePartition, but it > doesn't look like there's much way around that. Also, isn't there a better > place in the class to place the enum definition than between two function > definitions? I see 3 possibilities: 1. Add an enum / bool argument as I did 2. Add a new setter without the argument processing. I initially wanted to do this but did not know how to call it. 3. Move the partitionName() to calls sites but I assumed we did not want to do that since it was this way and we changed it. I am honestly OK with 1 or 2 if someone has a good name for the new setter.
Created attachment 240039 [details] Patch
Comment on attachment 240039 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240039&action=review > Source/WebCore/platform/network/cf/ResourceRequest.h:-102 > + enum CachePartitionNeedsValidationOrNot { CachePartitionDoesNotNeedValidation, CachePartitionNeedsValidation }; > + void setCachePartition(const String& cachePartition, CachePartitionNeedsValidationOrNot needsValidation = CachePartitionNeedsValidation) > + { > + m_cachePartition = needsValidation ? partitionName(cachePartition) : cachePartition; > + } > const String& cachePartition() const { return m_cachePartition.isNull() ? emptyString() : m_cachePartition; } > - void setCachePartition(const String& cachePartition) { m_cachePartition = partitionName(cachePartition); } The existing code is semantically wrong. This function doesn't take a partition name, it takes a domain name. Both happen to be String objects and to follow a domain name format, but this is accidental, and only confuses matters. Domains and partition names are different things, and should never be confused. So it's not about validation, it's about making a partition name from a domain name. Talking about a better design on IRC now - I think that we should have setCachePartition be a simple setter (perhaps with as ASSERT), and callers would create a partition name from domain name.
Created attachment 240157 [details] Patch
New proposal including refactoring of the existing code, after discussing this over with Alexey and Jeffrey.
Comment on attachment 240157 [details] Patch Clearing flags on attachment: 240157 Committed r174921: <http://trac.webkit.org/changeset/174921>
All reviewed patches have been landed. Closing bug.
(In reply to comment #10) > All reviewed patches have been landed. Closing bug. This caused a link failure due to the renaming of cachePartition. WebCore.exp.in needs to be updated to match the new name.
(In reply to comment #11) > (In reply to comment #10) > > All reviewed patches have been landed. Closing bug. > > This caused a link failure due to the renaming of cachePartition. > WebCore.exp.in needs to be updated to match the new name. I fixed that a while ago: https://trac.webkit.org/r174994 https://trac.webkit.org/r174995