Do cookie checks in NetworkDelegate instead of the URLRequest::Delegate.

content/public/common/url_fetcher.h

Index: content/public/common/url_fetcher.h
diff --git a/content/public/common/url_fetcher.h b/content/public/common/url_fetcher.h
index 571a34bc85624b09e67fca77634229174a9a999f..e6e7fac84bb0af0642ba0c3944f4f9328e4afd45 100644
--- a/content/public/common/url_fetcher.h
+++ b/content/public/common/url_fetcher.h
@@ -32,6 +32,7 @@ typedef std::vector<std::string> ResponseCookies;
 
 namespace content {
 
+class ContentURLRequestUserData;
 class URLFetcherDelegate;
 
 // To use this class, create an instance with the desired URL and a pointer to
@@ -156,6 +157,11 @@ class CONTENT_EXPORT URLFetcher {
   virtual void SetRequestContext(
       net::URLRequestContextGetter* request_context_getter) = 0;
 
+  // Stash the |user_data| object into the request. The request will take
+  // ownership of |user_data|.  Must be called before the request is started.
+  virtual void SetContentURLRequestUserData(
+      content::ContentURLRequestUserData* user_data) = 0;

[jam, 2012/03/01 19:30:42]

nit: content:: not needed

why do you have two ways of passing this data (i.e. in
SetContentURLRequestUserData and StartWithRequestContextGetterAndUserData)? it
seems one is enough, and i think the first is the one to pick since not every
request will be associated with a render view


(after looking at this some more)
why even expose ContentURLRequestUserData to embedders? just make this method be
virtual void AssociateWithRenderView(int render_process_id, int render_view_id)
= 0;

and say it's optional

this makes

fetcher_->SetContentURLRequestUserData(
    new content::ContentURLRequestUserData(
        web_contents->GetRenderProcessHost()->GetID(), 
        web_contents->GetRenderViewHost()->routing_id()));

become


fetcher_->AssociateWithRenderView(
    web_contents->GetRenderProcessHost()->GetID(), 
    web_contents->GetRenderViewHost()->routing_id());

which is easier on the eyes :)

[jochen (gone - plz use gerrit), 2012/03/01 19:39:11]

I'll do that.

However, I'll still need to expose this to embedders, as the
ChromeNetworkDelegate reads the user data to do the content settings stuff

+
   // If |retry| is false, 5xx responses will be propagated to the observer,
   // if it is true URLFetcher will automatically re-execute the request,
   // after backoff_delay() elapses. URLFetcher has it set to true by default.
@@ -192,9 +198,11 @@ class CONTENT_EXPORT URLFetcher {
   // settings.
   virtual void Start() = 0;
 
-  // Restarts the URLFetcher with a new URLRequestContextGetter.
-  virtual void StartWithRequestContextGetter(
-      net::URLRequestContextGetter* request_context_getter) = 0;
+  // Restarts the URLFetcher with a new URLRequestContextGetter and
+  // ContentURLRequestUserData.
+  virtual void StartWithRequestContextGetterAndUserData(

[jam, 2012/03/01 19:30:42]

nit: it seems redundant to put the parameter names in the function name.. maybe
just keep it the same name?

[jochen (gone - plz use gerrit), 2012/03/01 19:39:11]

I think we could just get rid of this function entirely.

All it does is invoke SetRequestContext() and then Start()

+      net::URLRequestContextGetter* request_context_getter,
+      content::ContentURLRequestUserData* user_data) = 0;

[jam, 2012/03/01 19:30:42]

nit: content:: is not necessary

 
   // Return the URL that we were asked to fetch.
   virtual const GURL& GetOriginalURL() const = 0;