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

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

6 files changed, 59 insertions, 103 deletions

diff --git a/content/renderer/fetchers/alt_error_page_resource_fetcher.cc b/content/renderer/fetchers/alt_error_page_resource_fetcher.cc
index cbcab09..fa06b56 100644
--- a/content/renderer/fetchers/alt_error_page_resource_fetcher.cc
+++ b/content/renderer/fetchers/alt_error_page_resource_fetcher.cc
@@ -29,19 +29,16 @@ AltErrorPageResourceFetcher::AltErrorPageResourceFetcher(
       callback_(callback),
       original_request_(original_request),
       original_error_(original_error) {
-  fetcher_.reset(new ResourceFetcherWithTimeout(
-      url, frame, WebURLRequest::TargetIsMainFrame, kDownloadTimeoutSec,
+  fetcher_.reset(new ResourceFetcher(
+      url, frame, WebURLRequest::TargetIsMainFrame,
       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 6d82857..eb42b89 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 ResourceFetcherWithTimeout;
+class ResourceFetcher;
 
 // Used for downloading alternate dns error pages. Once downloading is done
 // (or fails), the webview delegate is notified.
@@ -39,15 +39,12 @@ 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<ResourceFetcherWithTimeout> fetcher_;
+  scoped_ptr<ResourceFetcher> 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 0ce0d4e..4262326 100644
--- a/content/renderer/fetchers/image_resource_fetcher.cc
+++ b/content/renderer/fetchers/image_resource_fetcher.cc
@@ -39,8 +39,6 @@ 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 304abe8..c44f099 100644
--- a/content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
+++ b/content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
@@ -34,8 +34,6 @@ 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 455f59e..2da2725 100644
--- a/content/renderer/fetchers/resource_fetcher.cc
+++ b/content/renderer/fetchers/resource_fetcher.cc
@@ -26,14 +26,12 @@ namespace content {
 ResourceFetcher::ResourceFetcher(const GURL& url, WebFrame* frame,
                                  WebURLRequest::TargetType target_type,
                                  const Callback& callback)
-    : url_(url),
-      target_type_(target_type),
-      completed_(false),
+    : 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(frame);
+  Start(url, frame, target_type);
 }
 
 ResourceFetcher::~ResourceFetcher() {
@@ -41,16 +39,17 @@ ResourceFetcher::~ResourceFetcher() {
     loader_->cancel();
 }
 
-void ResourceFetcher::Cancel() {
-  if (!completed_) {
-    loader_->cancel();
-    completed_ = true;
-  }
+void ResourceFetcher::SetTimeout(base::TimeDelta timeout) {
+  DCHECK(loader_);
+  DCHECK(!completed_);
+  timeout_timer_.Start(FROM_HERE, timeout, this,
+                       &ResourceFetcher::TimeoutFired);
 }
 
-void ResourceFetcher::Start(WebFrame* frame) {
-  WebURLRequest request(url_);
-  request.setTargetType(target_type_);
+void ResourceFetcher::Start(const GURL& url, WebFrame* frame,
+                            WebURLRequest::TargetType target_type) {
+  WebURLRequest request(url);
+  request.setTargetType(target_type);
   request.setFirstPartyForCookies(frame->document().firstPartyForCookies());
   frame->dispatchWillSendRequest(request);
 
@@ -60,6 +59,8 @@ void ResourceFetcher::Start(WebFrame* frame) {
 
 void ResourceFetcher::RunCallback(const WebURLResponse& response,
                                   const std::string& data) {
+  completed_ = true;
+  timeout_timer_.Stop();
   if (callback_.is_null())
     return;
 
@@ -69,6 +70,12 @@ void ResourceFetcher::RunCallback(const WebURLResponse& response,
   callback.Run(response, data);
 }
 
+void ResourceFetcher::TimeoutFired() {
+  DCHECK(!completed_);
+  loader_->cancel();
+  RunCallback(WebURLResponse(), data_);
+}
+
 /////////////////////////////////////////////////////////////////////////////
 // WebURLLoaderClient methods
 
@@ -108,38 +115,15 @@ 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 5900973..fa71ae6 100644
--- a/content/renderer/fetchers/resource_fetcher.h
+++ b/content/renderer/fetchers/resource_fetcher.h
@@ -2,12 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
-// A wrapper around ResourceHandle and ResourceHandleClient that simplifies
-// the download of an HTTP object.  The interface is modeled after URLFetcher
-// in the /chrome/browser.
+// A wrapper that simplifies the download of an HTTP object.  The interface is
+// modeled after URLFetcher in /net/url_request/.
 //
-// ResourceFetcher::Delegate::OnURLFetchComplete will be called async after
-// the ResourceFetcher object is created.
+// The callback passed to the constructor will be called async after the
+// resource is retrieved.
 
 #ifndef CONTENT_RENDERER_FETCHERS_RESOURCE_FETCHER_H_
 #define CONTENT_RENDERER_FETCHERS_RESOURCE_FETCHER_H_
@@ -18,12 +17,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;
 
@@ -40,23 +39,36 @@ 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 URLFetcher instance is destroyed.
-  typedef base::Callback<void(const WebKit::WebURLResponse&,
-                              const std::string&)> Callback;
+  // and |data| are both valid until the ResourceFetcher instance is destroyed.
+  typedef base::Callback<void(const WebKit::WebURLResponse& response,
+                              const std::string& data)> Callback;
 
-  // We need a frame to make requests.
+  // A frame is needed 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();
 
-  // Stop the request and don't call the callback.
-  void Cancel();
+  // 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);
 
-  bool completed() const { return completed_; }
+  // 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();
 
- protected:
   // WebURLLoaderClient methods:
   virtual void willSendRequest(
       WebKit::WebURLLoader* loader, WebKit::WebURLRequest& new_request,
@@ -79,55 +91,25 @@ class CONTENT_EXPORT ResourceFetcher
 
   scoped_ptr<WebKit::WebURLLoader> loader_;
 
-  // URL we're fetching
-  GURL url_;
-
-  // Target type
-  WebKit::WebURLRequest::TargetType target_type_;
-
-  // A copy of the original resource response
-  WebKit::WebURLResponse response_;
-
-  // Set to true once the request is compelte.
+  // Set to true once the request is complete.
   bool completed_;
 
- private:
-  // Start the actual download.
-  void Start(WebKit::WebFrame* frame);
+  // Buffer to hold the content from the server.
+  std::string data_;
 
-  void RunCallback(const WebKit::WebURLResponse& response,
-                   const std::string& data);
+  // A copy of the original resource response.
+  WebKit::WebURLResponse response_;
 
-  // Callback when we're done
+  // 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();
 
- 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();
+  // Limit how long to wait for the server.
+  base::OneShotTimer<ResourceFetcher> timeout_timer_;
 
-  // Limit how long we wait for the alternate error page server.
-  base::OneShotTimer<ResourceFetcherWithTimeout> timeout_timer_;
+  DISALLOW_COPY_AND_ASSIGN(ResourceFetcher);
 };
 
 }  // namespace content