Bug 137797 – ResourceRequest deserialization unnecessarily calls partitionName() on encoded cache partition

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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>.

[Chris Dumez]

Created attachment 239984 [details]
Patch

[Vicki Pfau]

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.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 240039 [details]
Patch

[Alexey Proskuryakov]

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.

[Chris Dumez]

Created attachment 240157 [details]
Patch

[Chris Dumez]

New proposal including refactoring of the existing code, after discussing this over with Alexey and Jeffrey.

[WebKit Commit Bot]

Comment on attachment 240157 [details]
Patch

Clearing flags on attachment: 240157

Committed r174921: <http://trac.webkit.org/changeset/174921>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

(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.

[Chris Dumez]

(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