Net module changes to support caching responses to a POST request.

Net module changes to support caching responses to a POST request.

The solution is to add a user-defined identifier to UploadData.  If that identifier is set, and if the request method is POST, then HttpCache will enable caching for the response.  (The cache key will be a composition of the identifier and the URL.)  A subsequent POST request to the same URL with the same identifier will "hit" the previously generated cache entry.  Reuse from the cache is subject to all of the standard rules.

BUG=2636
R=wtc


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12374

[wtc, 2009-03-23 23:38:35]

LGTM.

Is there a bug number for this issue?

Your CL description implies that changes outside the net
module are also required to fix this bug.  I guess that's
why you didn't document what the new identifier_ member
of UploadData means.  Without that, I can't fully understand
your two changes to http_cache.cc, but they look good to me.

http://codereview.chromium.org/52028/diff/1/6
File net/base/upload_data.h (right):

http://codereview.chromium.org/52028/diff/1/6#newcode94
Line 94: void set_identifier(int64 id) { identifier_ = id; }
Could you explain what the identifier_ is?

http://codereview.chromium.org/52028/diff/1/3
File net/http/http_cache.cc (right):

http://codereview.chromium.org/52028/diff/1/3#newcode642
Line 642: request_->upload_data && request_->upload_data->identifier())
It seems that the presence of request_->upload_data implies
a POST method?  Do we need to worry about PUT?

http://codereview.chromium.org/52028/diff/1/3#newcode1188
Line 1188: if (request->upload_data && request->upload_data->identifier())
If this new if statement the only change you made to the
GenerateCacheKey function?

I almost missed it because you moved the function.

http://codereview.chromium.org/52028/diff/1/5
File net/http/http_cache.h (left):

http://codereview.chromium.org/52028/diff/1/5#oldcode109
Line 109: // Generate a key that can be used inside the cache.
You may want to move this comment to the definition of
GenerateCacheKey in the .cc file.

http://codereview.chromium.org/52028/diff/1/2
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/52028/diff/1/2#newcode37
Line 37: // 'key' is prefixed with an identifier if it corresponds to a cached
POST.
The only description of an identifier in this CL is in the
unit test file!

http://codereview.chromium.org/52028/diff/1/2#newcode968
Line 968: const int kUploadId = 1;  // Just a dummy value.
Nit: should kUploadId be int64 instead?

[wtc, 2009-03-23 23:39:49]

I forgot to say that I'm glad to see you fixed this bug.
I use Bugzilla a lot, and am often affected by this bug.
I've figured out how to avoid it, but it's a pain when I
forget to do that.

[darin (slow to review), 2009-03-24 06:30:24]

Thanks for the review!  Sorry for the spartan CL comments.  Updated...

http://codereview.chromium.org/52028/diff/1/6
File net/base/upload_data.h (right):

http://codereview.chromium.org/52028/diff/1/6#newcode94
Line 94: void set_identifier(int64 id) { identifier_ = id; }
I'll add a descriptive comment :)

http://codereview.chromium.org/52028/diff/1/3
File net/http/http_cache.cc (right):

http://codereview.chromium.org/52028/diff/1/3#newcode642
Line 642: request_->upload_data && request_->upload_data->identifier())
I decided to only enable caching for POST requests.  PUT requests can only be
generated via XMLHttpRequest, which means that we don't have to worry about PUT
requests in the back/forward use case.

http://codereview.chromium.org/52028/diff/1/3#newcode1188
Line 1188: if (request->upload_data && request->upload_data->identifier())
yes.  sorry for including the function movement in this CL!

the change here is to create a cache key that is composed of <identifier> + "/"
+ <url>.  since no valid URL can begin with numeric values, we don't have to
worry about collisions with actual URLs.

http://codereview.chromium.org/52028/diff/1/5
File net/http/http_cache.h (left):

http://codereview.chromium.org/52028/diff/1/5#oldcode109
Line 109: // Generate a key that can be used inside the cache.
Good idea.  Done.

http://codereview.chromium.org/52028/diff/1/2
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/52028/diff/1/2#newcode968
Line 968: const int kUploadId = 1;  // Just a dummy value.
Done.

[wtc, 2009-03-24 16:52:32]

LGTM.  Just some nits and minor questions below.
Please go ahead and check this in after fixing the nits.

http://codereview.chromium.org/52028/diff/1/3
File net/http/http_cache.cc (right):

http://codereview.chromium.org/52028/diff/1/3#newcode1188
Line 1188: if (request->upload_data && request->upload_data->identifier())
On 2009/03/24 06:30:24, darin wrote:
> yes.  sorry for including the function movement in this CL!

No problem.  It's strongly recommended by the style guide.
 
> the change here is to create a cache key that is composed of <identifier> +
"/"
> + <url>.  since no valid URL can begin with numeric values, we don't have to
> worry about collisions with actual URLs.

Could you add this comment to the file?  It'll help someone
unfamiliar with the cache to understand the code.

http://codereview.chromium.org/52028/diff/1005/19
File net/base/upload_data.h (right):

http://codereview.chromium.org/52028/diff/1005/19#newcode94
Line 94: void swap_elements(std::vector<Element>& elements) {
The swap_elements method is new.  Is it required by the
other half of this fix?

Should the 'elements' in/out parameter be declared as a
pointer?

  std::vector<Element>* elements

Or are you matching the type used in the STL's swap methods?

http://codereview.chromium.org/52028/diff/1005/19#newcode99
Line 99: // formulate a cache key.  This value should be unique across browser
Did you really mean "across browser sessions", as opposed to
"within a browser session"?  I'm curious as to how you
generate these unique identifiers across browser sessions.

Do identifiers for different upload URLs also need to be
unique?  I guess not.

http://codereview.chromium.org/52028/diff/1005/16
File net/http/http_cache.cc (right):

http://codereview.chromium.org/52028/diff/1005/16#newcode1183
Line 1183: // Generate a key that can be used inside the cache.
Nit: it'd be better to put this comment before the method.

[darin (slow to review), 2009-03-24 17:13:22]

Thanks again for the review!

http://codereview.chromium.org/52028/diff/1005/19
File net/base/upload_data.h (right):

http://codereview.chromium.org/52028/diff/1005/19#newcode94
Line 94: void swap_elements(std::vector<Element>& elements) {
Yes, it is used by the other half of this change.  I was matching the STL swap
method, but I think it is better to conform to google style.  Changed.

http://codereview.chromium.org/52028/diff/1005/19#newcode99
Line 99: // formulate a cache key.  This value should be unique across browser
generated using time-of-day:  http://trac.webkit.org/changeset/41919

http://codereview.chromium.org/52028/diff/1005/16
File net/http/http_cache.cc (right):

http://codereview.chromium.org/52028/diff/1005/16#newcode1183
Line 1183: // Generate a key that can be used inside the cache.
Done.

[wtc, 2009-03-24 17:39:14]

LGTM.  Nice CL description!