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

content/public/common/url_fetcher.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_PUBLIC_COMMON_URL_FETCHER_H_
 #define CONTENT_PUBLIC_COMMON_URL_FETCHER_H_
 #pragma once
 
 #include <string>
 #include <vector>
@@ skipping 14 matching lines @@
 class HostPortPair;
 class HttpRequestHeaders;
 class HttpResponseHeaders;
 class URLRequestContextGetter;
 class URLRequestStatus;
 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
 // the object to be notified when the URL has been loaded:
 //   URLFetcher* fetcher = URLFetcher::Create("http://www.google.com",
 //                                            URLFetcher::GET, this);
 //
 // Then, optionally set properties on this object, like the request context or
 // extra headers:
 //   fetcher->set_extra_request_headers("X-Foo: bar");
@@ skipping 104 matching lines @@
   // This appends the header to the current extra request headers.
   virtual void AddExtraRequestHeader(const std::string& header_line) = 0;
 
   virtual void GetExtraRequestHeaders(net::HttpRequestHeaders* headers) = 0;
 
   // Set the net::URLRequestContext on the request.  Must be called before the
   // request is started.
   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.
   virtual void SetAutomaticallyRetryOn5xx(bool retry) = 0;
 
   virtual void SetMaxRetries(int max_retries) = 0;
   virtual int GetMaxRetries() const = 0;
 
   // Returns the back-off delay before the request will be retried,
   // when a 5xx response was received.
@@ skipping 16 matching lines @@
 
   // Returns true if the request was delivered through a proxy.  Must only
   // be called after the OnURLFetchComplete callback has run and the request
   // has not failed.
   virtual bool WasFetchedViaProxy() const = 0;
 
   // Start the request.  After this is called, you may not change any other
   // 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;
 
   // Return the URL that this fetcher is processing.
   virtual const GURL& GetURL() const = 0;
 
   // The status of the URL fetch.
   virtual const net::URLRequestStatus& GetStatus() const = 0;
 
@@ skipping 22 matching lines @@
   // true, caller takes responsibility for the temp file, and it will not
   // be removed once the URLFetcher is destroyed.  User should not take
   // ownership more than once, or call this method after taking ownership.
   virtual bool GetResponseAsFilePath(bool take_ownership,
                                      FilePath* out_response_path) const = 0;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_COMMON_URL_FETCHER_H_