URLRequest::Interceptor enhancements...

URLRequest::Interceptor enhancements1) Allow an interceptor to change its mind and not intercept after all. This allows the decision to start or not to start to be made asynchronously.2) Allow an interceptor to intercept on error conditions if the original job fails. This is to support the FALLBACK semantics in the appcache.Info about where this is going can be found in the appcache design doc at https://docs.google.com/a/google.com/Doc?docid=agv6ghfsqr_15f749cgt3&hl=enI still have to put together test cases, so I'm not ready to submit this yet, but wanted to get some feedback at this point. 

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

[Matt Perry, 2009-04-10 01:43:40]

Here's a summary of our in-person discussion:

There's 2 ugly bits to this approach: 
1. excluded_interceptors collection, when you really only want your interceptor
to be excluded
2. fallback_job, which is created for every URLRequest the interceptor is
interested in, but which is wasted in most cases (when the request succeeds).

We can solve both.
1. Add some sort of property bag on URLRequest, so that your interceptor can
drop a "dont_intercept_me" token in the bag and later see that it's there.  This
gets rid of excluded_interceptors.
2. Add a MaybeFallback call on the interceptor interface, which is called only
when we actually fail a request.  The interceptor can decide whether to fallback
at that point.  (Or, if it previously decided, it can look it up in the property
bag in #1.)

Not sure what the property bag will look like.  Maybe it's just a map of
UserData pointers?

http://codereview.chromium.org/67019/diff/1001/1008
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/67019/diff/1001/1008#newcode268
Line 268: // Should only be called if the original job didn't make any progress.
seems like this comment should go in the header

http://codereview.chromium.org/67019/diff/1001/1003
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/67019/diff/1001/1003#newcode488
Line 488: ResponseStarted();;
extra ;

http://codereview.chromium.org/67019/diff/1001/1005
File net/url_request/url_request_job_manager.h (right):

http://codereview.chromium.org/67019/diff/1001/1005#newcode9
Line 9: #include <vector>
accidental change?

[Matt Perry, 2009-04-13 18:42:03]

cool, looking better.  might want to run it by darin to verify that the way
you're restarting jobs is kosher - I don't understand the network stack enough
to be sure.

http://codereview.chromium.org/67019/diff/1013/23
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/67019/diff/1013/23#newcode79
Line 79: std::for_each(specific_user_data_->begin(),
use STLDeleteContainerPairSecondPointers from stl_util-inl.h

http://codereview.chromium.org/67019/diff/1013/23#newcode499
Line 499: delete GetSpecificUserData(key);  // NULL check unnecessary for delete
nit: no need for the comment here

http://codereview.chromium.org/67019/diff/1013/23#newcode502
Line 502: specific_user_data_->insert(std::pair<void*,UserData*>(key, data));
how about (*specific_user_data_)[key] = data ?

http://codereview.chromium.org/67019/diff/1013/19
File net/url_request/url_request.h (right):

http://codereview.chromium.org/67019/diff/1013/19#newcode216
Line 216: static void RegisterRedirectInterceptor(Interceptor* interceptor);
Given that Interceptors default to "no intercept" for Response and Redirect,
what's the harm in just registering interceptors once, and calling the right
MaybeIntercept on all of them?

[michaeln, 2009-04-13 22:50:00]

new snapshot uploaded

http://codereview.chromium.org/67019/diff/1013/23
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/67019/diff/1013/23#newcode79
Line 79: std::for_each(specific_user_data_->begin(),
That would be perfect... unfortunately that file is under chrome (instead of
base) for some reason it can't be included in net w/o screwing up dependencies.

I've recoded things here to not need the helper function.

http://codereview.chromium.org/67019/diff/1013/23#newcode502
Line 502: specific_user_data_->insert(std::pair<void*,UserData*>(key, data));
On 2009/04/13 18:42:03, Matt Perry wrote:
> how about (*specific_user_data_)[key] = data ?

Works for me... Done.

http://codereview.chromium.org/67019/diff/1013/19
File net/url_request/url_request.h (right):

http://codereview.chromium.org/67019/diff/1013/19#newcode216
Line 216: static void RegisterRedirectInterceptor(Interceptor* interceptor);
Basic size/time tradeoff... i had come down on the side of time... the
difference is so small between the two on both dimensions... np changing it.

Done.

[Matt Perry, 2009-04-13 23:27:39]

Looks good

http://codereview.chromium.org/67019/diff/2003/31
File net/url_request/url_request.h (right):

http://codereview.chromium.org/67019/diff/2003/31#newcode91
Line 91: // called when there is no server resposne at all to allow interception
sp: resposne

http://codereview.chromium.org/67019/diff/2003/31#newcode95
Line 95: // response, instead the response produced by the intercept job will be
returned.
line length

[michaeln, 2009-04-15 19:18:09]

hi again... looking for a final review... added unit tests and removed the two
files non-NET library files from the CL

The unit tests turned up a couple of changes to make, but not much. Had to be
careful not to hijack canceled requests on at MaybeInterceptRedirect and
MaybeInterceptResponse time.

http://codereview.chromium.org/67019/diff/2003/31
File net/url_request/url_request.h (right):

http://codereview.chromium.org/67019/diff/2003/31#newcode91
Line 91: // called when there is no server resposne at all to allow interception
On 2009/04/13 23:27:39, Matt Perry wrote:
> sp: resposne

Done.

http://codereview.chromium.org/67019/diff/2003/31#newcode95
Line 95: // response, instead the response produced by the intercept job will be
returned.
On 2009/04/13 23:27:39, Matt Perry wrote:
> line length

Done.

[Matt Perry, 2009-04-15 19:53:39]

Mostly looks good, assuming all the unit tests still pass after the TestRequest
refactor.

http://codereview.chromium.org/67019/diff/3009/3016
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/67019/diff/3009/3016#newcode350
Line 350: if (job)
nit: chrome style is to use braces any time there's an else branch.

http://codereview.chromium.org/67019/diff/3009/3017
File net/url_request/url_request_test_job.cc (right):

http://codereview.chromium.org/67019/diff/3009/3017#newcode47
Line 47: return StringPrintf(
StrinPrintf means test_headers is not a simple getter - so use CamelCase naming.

http://codereview.chromium.org/67019/diff/3009/3018
File net/url_request/url_request_test_job.h (right):

http://codereview.chromium.org/67019/diff/3009/3018#newcode62
Line 62: // The headers that correspond to each of the URLs above
periods at the end of these comments

http://codereview.chromium.org/67019/diff/3009/3018#newcode77
Line 77: // the caller having to call ProcessOnePendingMessage. Auto advnce
depends
sp advnce

http://codereview.chromium.org/67019/diff/3009/3015
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/67019/diff/3009/3015#newcode1134
Line 1134: class TestInterceptor : URLRequest::Interceptor {
Hmm.. I wonder if it would be less code to just make a specific interceptor for
each test?  No need to change it if you disagree.

http://codereview.chromium.org/67019/diff/3009/3015#newcode1227
Line 1227: // Other actions we can take at MaybeInterceptRedirect time
periods at end of comments

[michaeln, 2009-04-15 21:52:26]

The try servers are happy so far. Are there tests that they don't run that I
should run locally?

I added a bit more to the unit tests to get another case and to excersize the
Get/SetSpecificiUserData methods.

http://codereview.chromium.org/67019/diff/3009/3016
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/67019/diff/3009/3016#newcode350
Line 350: if (job)
On 2009/04/15 19:53:40, Matt Perry wrote:
> nit: chrome style is to use braces any time there's an else branch.

Done.

http://codereview.chromium.org/67019/diff/3009/3017
File net/url_request/url_request_test_job.cc (right):

http://codereview.chromium.org/67019/diff/3009/3017#newcode47
Line 47: return StringPrintf(
Done by getting rid of Sprintf.
What makes GURL ctors and std::string ctors are simple gets?

http://codereview.chromium.org/67019/diff/3009/3018
File net/url_request/url_request_test_job.h (right):

http://codereview.chromium.org/67019/diff/3009/3018#newcode77
Line 77: // the caller having to call ProcessOnePendingMessage. Auto advnce
depends
On 2009/04/15 19:53:40, Matt Perry wrote:
> sp advnce

Done.

http://codereview.chromium.org/67019/diff/3009/3015
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/67019/diff/3009/3015#newcode1134
Line 1134: class TestInterceptor : URLRequest::Interceptor {
On 2009/04/15 19:53:40, Matt Perry wrote:
> Hmm.. I wonder if it would be less code to just make a specific interceptor
for
> each test?  No need to change it if you disagree.

Maybe, but i'm not optimizing for unit test codesize :)

This is in keeping with having a single TestDelegate class that is reused all
over the place. That pattern works for me.

http://codereview.chromium.org/67019/diff/3009/3015#newcode1227
Line 1227: // Other actions we can take at MaybeInterceptRedirect time
On 2009/04/15 19:53:40, Matt Perry wrote:
> periods at end of comments

ya know... i see a mix of comment styles in the codebase, since this isn't a
sentence... no period... i don't see the rhyme or reason behind the mix of
styles honestly.

[Matt Perry, 2009-04-15 22:17:26]

LGTM

http://codereview.chromium.org/67019/diff/3009/3017
File net/url_request/url_request_test_job.cc (right):

http://codereview.chromium.org/67019/diff/3009/3017#newcode47
Line 47: return StringPrintf(
On 2009/04/15 21:52:26, michaeln wrote:
> Done by getting rid of Sprintf.
> What makes GURL ctors and std::string ctors are simple gets?
> 

Good point, they're not really.  But I guess we're being a little lax for test
code :).

[darin (slow to review), 2009-04-15 23:03:59]

looks good!  just some minor tweaks:

http://codereview.chromium.org/67019/diff/5039/99
File net/url_request/url_request.h (right):

http://codereview.chromium.org/67019/diff/5039/99#newcode547
Line 547: typedef std::map<void*, UserData*> UserDataMap;
couple comments:

1- seems fine to just eliminate user_data / set_user_data in favor of the new
API

2- in that case, might as well not bother with lazy / heap-alloc of the std::map

3- maybe use linked_ptr<UserData> to simplify the code

[michaeln, 2009-04-16 01:42:02]

Thnx for taking a look.

Sure, I'll make those changes. I'll have to add a few files to the CL to switch
current user_data() and set_user_data() callers over, but there aren't many of
these. The only real consumer(s) of these methods is ResourceDispatcherHost and
related classes. Since it has a static method to retrieve the RDH's user data,
I'll use NULL as the 'key' for RDH's user data.