Revert 232403 "Revert 232369 "Revert 232312 "Cleanup ResourceFet..."

> Revert 232369 "Revert 232312 "Cleanup ResourceFetcher in prepara..."
> 
> Revert didn't fix failing test.
> 
> > Revert 232312 "Cleanup ResourceFetcher in preparation to add to ..."
> > 
> > Seems to cause ResourceFetcherTests.ResourceFetcherDeletedInCallback
> > to timeout on Windows and crash on Linux.
> > 
> > > Cleanup ResourceFetcher in preparation to add to public content API,
> > > in preparation to move error page logic from content to chrome:
> > > 
> > > * Merge ResourceFetcher and ResourceFetcherWithTimeout.
> > > * Remove unnecessary member variables.
> > > * Remove Cancel() (All consumers were deleting and cancelling at the
> > >     same time, anyways, and deleting already cancels).
> > > * Make definition order match declaration order.
> > > * Fix outdated comments.
> > > 
> > > BUG=308232
> > > 
> > > Review URL: https://codereview.chromium.org/33363003
> > 
> > TBR=mmenke@chromium.org
> > 
> > Review URL: https://codereview.chromium.org/55963002
> 
> TBR=csharp@chromium.org
> 
> Review URL: https://codereview.chromium.org/55943003

TBR=csharp@chromium.org

Review URL: https://codereview.chromium.org/56233002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@232417 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 103 insertions, 59 deletions

diff --git a/content/renderer/fetchers/alt_error_page_resource_fetcher.cc b/content/renderer/fetchers/alt_error_page_resource_fetcher.cc
index fa06b56..cbcab09 100644
--- a/content/renderer/fetchers/alt_error_page_resource_fetcher.cc
+++ b/content/renderer/fetchers/alt_error_page_resource_fetcher.cc
@@ -29,16 +29,19 @@ AltErrorPageResourceFetcher::AltErrorPageResourceFetcher(
       callback_(callback),
       original_request_(original_request),
       original_error_(original_error) {
-  fetcher_.reset(new ResourceFetcher(
-      url, frame, WebURLRequest::TargetIsMainFrame,
+  fetcher_.reset(new ResourceFetcherWithTimeout(
+      url, frame, WebURLRequest::TargetIsMainFrame, kDownloadTimeoutSec,
       base::Bind(&AltErrorPageResourceFetcher::OnURLFetchComplete,
                  base::Unretained(this))));
-  fetcher_->SetTimeout(base::TimeDelta::FromSeconds(kDownloadTimeoutSec));
 }
 
 AltErrorPageResourceFetcher::~AltErrorPageResourceFetcher() {
 }
 
+void AltErrorPageResourceFetcher::Cancel() {
+  fetcher_->Cancel();
+}
+
 void AltErrorPageResourceFetcher::OnURLFetchComplete(
     const WebURLResponse& response,
     const std::string& data) {
diff --git a/content/renderer/fetchers/alt_error_page_resource_fetcher.h b/content/renderer/fetchers/alt_error_page_resource_fetcher.h
index eb42b89..6d82857 100644
--- a/content/renderer/fetchers/alt_error_page_resource_fetcher.h
+++ b/content/renderer/fetchers/alt_error_page_resource_fetcher.h
@@ -17,7 +17,7 @@ class WebURLResponse;
 }
 
 namespace content {
-class ResourceFetcher;
+class ResourceFetcherWithTimeout;
 
 // Used for downloading alternate dns error pages. Once downloading is done
 // (or fails), the webview delegate is notified.
@@ -39,12 +39,15 @@ class AltErrorPageResourceFetcher {
       const Callback& callback);
   ~AltErrorPageResourceFetcher();
 
+  // Stop any pending loads.
+  void Cancel();
+
  private:
   void OnURLFetchComplete(const WebKit::WebURLResponse& response,
                           const std::string& data);
 
   // Does the actual fetching.
-  scoped_ptr<ResourceFetcher> fetcher_;
+  scoped_ptr<ResourceFetcherWithTimeout> fetcher_;
 
   WebKit::WebFrame* frame_;
   Callback callback_;
diff --git a/content/renderer/fetchers/image_resource_fetcher.cc b/content/renderer/fetchers/image_resource_fetcher.cc
index 4262326..0ce0d4e 100644
--- a/content/renderer/fetchers/image_resource_fetcher.cc
+++ b/content/renderer/fetchers/image_resource_fetcher.cc
@@ -39,6 +39,8 @@ ImageResourceFetcher::ImageResourceFetcher(
 }
 
 ImageResourceFetcher::~ImageResourceFetcher() {
+  if (!fetcher_->completed())
+    fetcher_->Cancel();
 }
 
 void ImageResourceFetcher::OnURLFetchComplete(
diff --git a/content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc b/content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
index c44f099..304abe8 100644
--- a/content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
+++ b/content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
@@ -34,6 +34,8 @@ MultiResolutionImageResourceFetcher::MultiResolutionImageResourceFetcher(
 }
 
 MultiResolutionImageResourceFetcher::~MultiResolutionImageResourceFetcher() {
+  if (!fetcher_->completed())
+    fetcher_->Cancel();
 }
 
 void MultiResolutionImageResourceFetcher::OnURLFetchComplete(
diff --git a/content/renderer/fetchers/resource_fetcher.cc b/content/renderer/fetchers/resource_fetcher.cc
index 2da2725..455f59e 100644
--- a/content/renderer/fetchers/resource_fetcher.cc
+++ b/content/renderer/fetchers/resource_fetcher.cc
@@ -26,12 +26,14 @@ namespace content {
 ResourceFetcher::ResourceFetcher(const GURL& url, WebFrame* frame,
                                  WebURLRequest::TargetType target_type,
                                  const Callback& callback)
-    : completed_(false),
+    : url_(url),
+      target_type_(target_type),
+      completed_(false),
       callback_(callback) {
   // Can't do anything without a frame.  However, delegate can be NULL (so we
   // can do a http request and ignore the results).
   DCHECK(frame);
-  Start(url, frame, target_type);
+  Start(frame);
 }
 
 ResourceFetcher::~ResourceFetcher() {
@@ -39,17 +41,16 @@ ResourceFetcher::~ResourceFetcher() {
     loader_->cancel();
 }
 
-void ResourceFetcher::SetTimeout(base::TimeDelta timeout) {
-  DCHECK(loader_);
-  DCHECK(!completed_);
-  timeout_timer_.Start(FROM_HERE, timeout, this,
-                       &ResourceFetcher::TimeoutFired);
+void ResourceFetcher::Cancel() {
+  if (!completed_) {
+    loader_->cancel();
+    completed_ = true;
+  }
 }
 
-void ResourceFetcher::Start(const GURL& url, WebFrame* frame,
-                            WebURLRequest::TargetType target_type) {
-  WebURLRequest request(url);
-  request.setTargetType(target_type);
+void ResourceFetcher::Start(WebFrame* frame) {
+  WebURLRequest request(url_);
+  request.setTargetType(target_type_);
   request.setFirstPartyForCookies(frame->document().firstPartyForCookies());
   frame->dispatchWillSendRequest(request);
 
@@ -59,8 +60,6 @@ void ResourceFetcher::Start(const GURL& url, WebFrame* frame,
 
 void ResourceFetcher::RunCallback(const WebURLResponse& response,
                                   const std::string& data) {
-  completed_ = true;
-  timeout_timer_.Stop();
   if (callback_.is_null())
     return;
 
@@ -70,12 +69,6 @@ void ResourceFetcher::RunCallback(const WebURLResponse& response,
   callback.Run(response, data);
 }
 
-void ResourceFetcher::TimeoutFired() {
-  DCHECK(!completed_);
-  loader_->cancel();
-  RunCallback(WebURLResponse(), data_);
-}
-
 /////////////////////////////////////////////////////////////////////////////
 // WebURLLoaderClient methods
 
@@ -115,15 +108,38 @@ void ResourceFetcher::didReceiveCachedMetadata(
 void ResourceFetcher::didFinishLoading(
     WebURLLoader* loader, double finishTime) {
   DCHECK(!completed_);
+  completed_ = true;
 
   RunCallback(response_, data_);
 }
 
 void ResourceFetcher::didFail(WebURLLoader* loader, const WebURLError& error) {
   DCHECK(!completed_);
+  completed_ = true;
 
   // Go ahead and tell our delegate that we're done.
   RunCallback(WebURLResponse(), std::string());
 }
 
+/////////////////////////////////////////////////////////////////////////////
+// A resource fetcher with a timeout
+
+ResourceFetcherWithTimeout::ResourceFetcherWithTimeout(
+    const GURL& url, WebFrame* frame, WebURLRequest::TargetType target_type,
+    int timeout_secs, const Callback& callback)
+    : ResourceFetcher(url, frame, target_type, callback) {
+  timeout_timer_.Start(FROM_HERE, TimeDelta::FromSeconds(timeout_secs), this,
+                       &ResourceFetcherWithTimeout::TimeoutFired);
+}
+
+ResourceFetcherWithTimeout::~ResourceFetcherWithTimeout() {
+}
+
+void ResourceFetcherWithTimeout::TimeoutFired() {
+  if (!completed_) {
+    loader_->cancel();
+    didFail(NULL, WebURLError());
+  }
+}
+
 }  // namespace content
diff --git a/content/renderer/fetchers/resource_fetcher.h b/content/renderer/fetchers/resource_fetcher.h
index fa71ae6..5900973 100644
--- a/content/renderer/fetchers/resource_fetcher.h
+++ b/content/renderer/fetchers/resource_fetcher.h
@@ -2,11 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
-// A wrapper that simplifies the download of an HTTP object.  The interface is
-// modeled after URLFetcher in /net/url_request/.
+// A wrapper around ResourceHandle and ResourceHandleClient that simplifies
+// the download of an HTTP object.  The interface is modeled after URLFetcher
+// in the /chrome/browser.
 //
-// The callback passed to the constructor will be called async after the
-// resource is retrieved.
+// ResourceFetcher::Delegate::OnURLFetchComplete will be called async after
+// the ResourceFetcher object is created.
 
 #ifndef CONTENT_RENDERER_FETCHERS_RESOURCE_FETCHER_H_
 #define CONTENT_RENDERER_FETCHERS_RESOURCE_FETCHER_H_
@@ -17,12 +18,12 @@
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "content/common/content_export.h"
 #include "third_party/WebKit/public/platform/WebURLLoaderClient.h"
 #include "third_party/WebKit/public/platform/WebURLRequest.h"
 #include "third_party/WebKit/public/platform/WebURLResponse.h"
+#include "url/gurl.h"
 
 class GURL;
 
@@ -39,36 +40,23 @@ class CONTENT_EXPORT ResourceFetcher
  public:
   // This will be called when the URL has been fetched, successfully or not.
   // If there is a failure, response and data will both be empty.  |response|
-  // and |data| are both valid until the ResourceFetcher instance is destroyed.
-  typedef base::Callback<void(const WebKit::WebURLResponse& response,
-                              const std::string& data)> Callback;
+  // and |data| are both valid until the URLFetcher instance is destroyed.
+  typedef base::Callback<void(const WebKit::WebURLResponse&,
+                              const std::string&)> Callback;
 
-  // A frame is needed to make requests.
+  // We need a frame to make requests.
   ResourceFetcher(
       const GURL& url, WebKit::WebFrame* frame,
       WebKit::WebURLRequest::TargetType target_type,
       const Callback& callback);
-
-  // Deleting a ResourceFetcher will safely cancel the request if it has not yet
-  // completed.
   virtual ~ResourceFetcher();
 
-  // Sets a timeout for the request.  Request must not yet have completed
-  // or timed out when called.
-  void SetTimeout(base::TimeDelta timeout);
-
- private:
-  // Start the actual download.
-  void Start(const GURL& url, WebKit::WebFrame* frame,
-             WebKit::WebURLRequest::TargetType target_type);
-
-  void RunCallback(const WebKit::WebURLResponse& response,
-                   const std::string& data);
+  // Stop the request and don't call the callback.
+  void Cancel();
 
-  // Callback for timer that limits how long we wait for the server.  If this
-  // timer fires and the request hasn't completed, we kill the request.
-  void TimeoutFired();
+  bool completed() const { return completed_; }
 
+ protected:
   // WebURLLoaderClient methods:
   virtual void willSendRequest(
       WebKit::WebURLLoader* loader, WebKit::WebURLRequest& new_request,
@@ -91,25 +79,55 @@ class CONTENT_EXPORT ResourceFetcher
 
   scoped_ptr<WebKit::WebURLLoader> loader_;
 
-  // Set to true once the request is complete.
-  bool completed_;
+  // URL we're fetching
+  GURL url_;
 
-  // Buffer to hold the content from the server.
-  std::string data_;
+  // Target type
+  WebKit::WebURLRequest::TargetType target_type_;
 
-  // A copy of the original resource response.
+  // A copy of the original resource response
   WebKit::WebURLResponse response_;
 
-  // Callback when we're done.
+  // Set to true once the request is compelte.
+  bool completed_;
+
+ private:
+  // Start the actual download.
+  void Start(WebKit::WebFrame* frame);
+
+  void RunCallback(const WebKit::WebURLResponse& response,
+                   const std::string& data);
+
+  // Callback when we're done
   Callback callback_;
 
+  // Buffer to hold the content from the server.
+  std::string data_;
+
   // Buffer to hold metadata from the cache.
   std::string metadata_;
+};
+
+/////////////////////////////////////////////////////////////////////////////
+// A resource fetcher with a timeout
+class CONTENT_EXPORT ResourceFetcherWithTimeout
+    : NON_EXPORTED_BASE(public ResourceFetcher) {
+ public:
+  ResourceFetcherWithTimeout(const GURL& url,
+                             WebKit::WebFrame* frame,
+                             WebKit::WebURLRequest::TargetType target_type,
+                             int timeout_secs,
+                             const Callback& callback);
+  virtual ~ResourceFetcherWithTimeout();
 
-  // Limit how long to wait for the server.
-  base::OneShotTimer<ResourceFetcher> timeout_timer_;
+ private:
+  // Callback for timer that limits how long we wait for the alternate error
+  // page server.  If this timer fires and the request hasn't completed, we
+  // kill the request.
+  void TimeoutFired();
 
-  DISALLOW_COPY_AND_ASSIGN(ResourceFetcher);
+  // Limit how long we wait for the alternate error page server.
+  base::OneShotTimer<ResourceFetcherWithTimeout> timeout_timer_;
 };
 
 }  // namespace content