Saving content will prefer to retrieve the data out of the HTTP cache…

… for GETs. For example, if the current tab is navigated to a JPEG and the user chooses "Save Page As" from the wrench menu, the data will be retrieved from cache without validation even if Cache-Control: no-cache is specified. BUG=32246,55551,94574 TEST=DownloadTest.SavePageNonHTMLViaGet Review URL: http://codereview.chromium.org/9178029 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@119538 0039d316-1c4b-4281-b951-d872f2087c98
cbchan · Jan 28, 2012 · 0d4e30c · 0d4e30c
1 parent 59d12de
commit 0d4e30c
Show file tree

Hide file tree

Showing 19 changed files with 130 additions and 94 deletions.
diff --git a/chrome/browser/chromeos/imageburner/burn_manager.cc b/chrome/browser/chromeos/imageburner/burn_manager.cc
@@ -447,10 +447,11 @@ void Downloader::OnFileStreamCreatedOnUIThread(const GURL& url,
 
     download_util::RecordDownloadCount(
         download_util::INITIATED_BY_IMAGE_BURNER_COUNT);
-    download_manager->DownloadUrlToFile(
+    download_manager->DownloadUrl(
         url,
         web_contents->GetURL(),
         web_contents->GetEncoding(),
+        false,
         save_info,
         web_contents);
   } else {

diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -1697,8 +1697,10 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadUrl) {
         DownloadItem::COMPLETE,  // Really done
         false,                   // Ignore select file.
         DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
+  DownloadSaveInfo save_info;
+  save_info.prompt_for_save_location = true;
   DownloadManagerForBrowser(browser())->DownloadUrl(
-      url, GURL(""), "", web_contents);
+      url, GURL(""), "", false, save_info, web_contents);
   observer->WaitForFinished();
   EXPECT_TRUE(observer->select_file_dialog_seen());
 
@@ -1708,7 +1710,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadUrl) {
   CheckDownloadUI(browser(), true, true, file);
 }
 
-IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadUrlToFile) {
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadUrlToPath) {
   ASSERT_TRUE(InitialSetup(false));
   FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
   GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
@@ -1724,8 +1726,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadUrlToFile) {
   save_info.file_path = target_file_full_path;
 
   DownloadTestObserver* observer(CreateWaiter(browser(), 1));
-  DownloadManagerForBrowser(browser())->DownloadUrlToFile(
-      url, GURL(""), "", save_info, web_contents);
+  DownloadManagerForBrowser(browser())->DownloadUrl(
+      url, GURL(""), "", false, save_info, web_contents);
   observer->WaitForFinished();
 
   // Check state.
@@ -1737,3 +1739,37 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadUrlToFile) {
   // Temporary downloads won't be visible.
   CheckDownloadUI(browser(), false, false, file);
 }
+
+IN_PROC_BROWSER_TEST_F(DownloadTest, SavePageNonHTMLViaGet) {
+  // Do initial setup.
+  ASSERT_TRUE(InitialSetup(false));
+  ASSERT_TRUE(test_server()->Start());
+  NullSelectFile(browser());
+  std::vector<DownloadItem*> download_items;
+  GetDownloads(browser(), &download_items);
+  ASSERT_TRUE(download_items.empty());
+
+  // Navigate to a non-HTML resource. The resource also has
+  // Cache-Control: no-cache set, which normally requires revalidation
+  // each time.
+  GURL url = test_server()->GetURL("files/downloads/image.jpg");
+  ASSERT_TRUE(url.is_valid());
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // Stop the test server, and then try to save the page. If cache validation
+  // is not bypassed then this will fail since the server is no longer
+  // reachable.
+  ASSERT_TRUE(test_server()->Stop());
+  scoped_ptr<DownloadTestObserver> waiter(
+      new DownloadTestObserver(
+          DownloadManagerForBrowser(browser()), 1, DownloadItem::COMPLETE,
+          false, DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
+  browser()->SavePage();
+  waiter->WaitForFinished();
+
+  // Validate that the correct file was downloaded.
+  GetDownloads(browser(), &download_items);
+  EXPECT_TRUE(waiter->select_file_dialog_seen());
+  ASSERT_EQ(1u, download_items.size());
+  ASSERT_EQ(url, download_items[0]->GetOriginalUrl());
+}
diff --git a/chrome/browser/download/download_extension_api.cc b/chrome/browser/download/download_extension_api.cc
@@ -442,6 +442,8 @@ void DownloadsDownloadFunction::BeginDownloadOnIOThread() {
   // TODO(benjhayden) Ensure that this filename is interpreted as a path
   // relative to the default downloads directory without allowing '..'.
   save_info.suggested_name = iodata_->filename;
+  save_info.prompt_for_save_location = iodata_->save_as;
+
   scoped_ptr<net::URLRequest> request(
       new net::URLRequest(iodata_->url, iodata_->rdh));
   request->set_method(iodata_->method);
@@ -461,8 +463,8 @@ void DownloadsDownloadFunction::BeginDownloadOnIOThread() {
   }
   net::Error error = iodata_->rdh->BeginDownload(
       request.Pass(),
+      false,  // prefer_cache
       save_info,
-      iodata_->save_as,
       base::Bind(&DownloadsDownloadFunction::OnStarted, this),
       iodata_->render_process_host_id,
       iodata_->render_view_host_routing_id,

diff --git a/chrome/browser/extensions/webstore_installer.cc b/chrome/browser/extensions/webstore_installer.cc
@@ -215,8 +215,9 @@ void WebstoreInstaller::StartDownload(FilePath file) {
   // download system will then pass the crx to the CrxInstaller.
   download_util::RecordDownloadCount(
         download_util::INITIATED_BY_WEBSTORE_INSTALLER_COUNT);
-  profile_->GetDownloadManager()->DownloadUrlToFile(
-      download_url_, referrer, "", save_info, controller_->GetWebContents());
+  profile_->GetDownloadManager()->DownloadUrl(
+      download_url_, referrer, "",
+      false, save_info, controller_->GetWebContents());
 }
 
 void WebstoreInstaller::ReportFailure(const std::string& error) {

diff --git a/chrome/browser/tab_contents/render_view_context_menu.cc b/chrome/browser/tab_contents/render_view_context_menu.cc
@@ -61,6 +61,7 @@
 #include "chrome/common/spellcheck_messages.h"
 #include "chrome/common/url_constants.h"
 #include "content/browser/child_process_security_policy.h"
+#include "content/browser/download/download_types.h"
 #include "content/browser/renderer_host/render_view_host.h"
 #include "content/browser/renderer_host/render_widget_host_view.h"
 #include "content/browser/speech/speech_input_preferences.h"
@@ -1495,8 +1496,15 @@ void RenderViewContextMenu::ExecuteCommand(int id, int event_flags) {
                                                   params_.src_url);
       DownloadManager* dlm =
           DownloadServiceFactory::GetForProfile(profile_)->GetDownloadManager();
+      // For images and AV "Save As" context menu commands, save the cached
+      // data even if it is no longer valid. This helps ensure that the content
+      // that is visible on the page is what is saved. For links, this behavior
+      // is not desired since the content being linked to is not visible.
+      bool prefer_cache = (id != IDC_CONTENT_CONTEXT_SAVELINKAS);
+      DownloadSaveInfo save_info;
+      save_info.prompt_for_save_location = true;
       dlm->DownloadUrl(url, referrer, params_.frame_charset,
-                       source_web_contents_);
+                       prefer_cache, save_info, source_web_contents_);
       break;
     }
 

diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
@@ -70,6 +70,7 @@ struct RenderParams {
 };
 
 void BeginDownload(const URLParams& url_params,
+                   bool prefer_cache,
                    const DownloadSaveInfo& save_info,
                    ResourceDispatcherHost* resource_dispatcher_host,
                    const RenderParams& render_params,
@@ -78,7 +79,7 @@ void BeginDownload(const URLParams& url_params,
       new net::URLRequest(url_params.url_, resource_dispatcher_host));
   request->set_referrer(url_params.referrer_.spec());
   resource_dispatcher_host->BeginDownload(
-      request.Pass(), save_info, true,
+      request.Pass(), prefer_cache, save_info,
       DownloadResourceHandler::OnStartedCallback(),
       render_params.render_process_id_,
       render_params.render_view_id_,
@@ -811,19 +812,13 @@ int DownloadManagerImpl::RemoveAllDownloads() {
 // Initiate a download of a specific URL. We send the request to the
 // ResourceDispatcherHost, and let it send us responses like a regular
 // download.
-void DownloadManagerImpl::DownloadUrl(const GURL& url,
-                                      const GURL& referrer,
-                                      const std::string& referrer_charset,
-                                      WebContents* web_contents) {
-  DownloadUrlToFile(url, referrer, referrer_charset, DownloadSaveInfo(),
-                    web_contents);
-}
-
-void DownloadManagerImpl::DownloadUrlToFile(const GURL& url,
-                                            const GURL& referrer,
-                                            const std::string& referrer_charset,
-                                            const DownloadSaveInfo& save_info,
-                                            WebContents* web_contents) {
+void DownloadManagerImpl::DownloadUrl(
+    const GURL& url,
+    const GURL& referrer,
+    const std::string& referrer_charset,
+    bool prefer_cache,
+    const DownloadSaveInfo& save_info,
+    WebContents* web_contents) {
   ResourceDispatcherHost* resource_dispatcher_host =
       ResourceDispatcherHost::Get();
 
@@ -832,8 +827,12 @@ void DownloadManagerImpl::DownloadUrlToFile(const GURL& url,
   // base::Bind can't handle 7 args, so we use URLParams and RenderParams.
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::Bind(&BeginDownload,
-          URLParams(url, referrer), save_info, resource_dispatcher_host,
+      base::Bind(
+          &BeginDownload,
+          URLParams(url, referrer),
+          prefer_cache,
+          save_info,
+          resource_dispatcher_host,
           RenderParams(web_contents->GetRenderProcessHost()->GetID(),
                        web_contents->GetRenderViewHost()->routing_id()),
           &web_contents->GetBrowserContext()->GetResourceContext()));

diff --git a/content/browser/download/download_manager_impl.h b/content/browser/download/download_manager_impl.h
@@ -69,12 +69,9 @@ class CONTENT_EXPORT DownloadManagerImpl
   virtual void DownloadUrl(const GURL& url,
                            const GURL& referrer,
                            const std::string& referrer_encoding,
+                           bool prefer_cache,
+                           const DownloadSaveInfo& save_info,
                            content::WebContents* web_contents) OVERRIDE;
-  virtual void DownloadUrlToFile(const GURL& url,
-                                 const GURL& referrer,
-                                 const std::string& referrer_encoding,
-                                 const DownloadSaveInfo& save_info,
-                                 content::WebContents* web_contents) OVERRIDE;
   virtual void AddObserver(Observer* observer) OVERRIDE;
   virtual void RemoveObserver(Observer* observer) OVERRIDE;
   virtual void OnPersistentStoreQueryComplete(

diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -42,7 +42,6 @@ DownloadResourceHandler::DownloadResourceHandler(
     const GURL& url,
     DownloadFileManager* download_file_manager,
     net::URLRequest* request,
-    bool save_as,
     const DownloadResourceHandler::OnStartedCallback& started_cb,
     const DownloadSaveInfo& save_info)
     : download_id_(DownloadId::Invalid()),
@@ -51,7 +50,6 @@ DownloadResourceHandler::DownloadResourceHandler(
       content_length_(0),
       download_file_manager_(download_file_manager),
       request_(request),
-      save_as_(save_as),
       started_cb_(started_cb),
       save_info_(save_info),
       buffer_(new content::DownloadBuffer),
@@ -141,7 +139,7 @@ bool DownloadResourceHandler::OnResponseStarted(
   }
 
   info->prompt_user_for_save_location =
-      save_as_ && save_info_.file_path.empty();
+      save_info_.prompt_for_save_location && save_info_.file_path.empty();
   info->referrer_charset = request_->context()->referrer_charset();
   info->save_info = save_info_;
 

diff --git a/content/browser/download/download_resource_handler.h b/content/browser/download/download_resource_handler.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -46,7 +46,6 @@ class DownloadResourceHandler : public ResourceHandler {
                           const GURL& url,
                           DownloadFileManager* download_file_manager,
                           net::URLRequest* request,
-                          bool save_as,
                           const OnStartedCallback& started_cb,
                           const DownloadSaveInfo& save_info);
 
@@ -113,7 +112,6 @@ class DownloadResourceHandler : public ResourceHandler {
   int64 content_length_;
   DownloadFileManager* download_file_manager_;
   net::URLRequest* request_;
-  bool save_as_;  // Request was initiated via "Save As" by the user.
   // This is used only on the UI thread.
   OnStartedCallback started_cb_;
   DownloadSaveInfo save_info_;

diff --git a/content/browser/download/download_types.cc b/content/browser/download/download_types.cc
@@ -1,29 +1,13 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
 #include "content/browser/download/download_types.h"
 
 DownloadSaveInfo::DownloadSaveInfo()
-    : offset(0) {
-}
-
-DownloadSaveInfo::DownloadSaveInfo(const DownloadSaveInfo& info)
-    : file_path(info.file_path),
-      file_stream(info.file_stream),
-      suggested_name(info.suggested_name),
-      offset(info.offset),
-      hash_state(info.hash_state) {
+    : offset(0), prompt_for_save_location(false) {
 }
 
 DownloadSaveInfo::~DownloadSaveInfo() {
 }
 
-DownloadSaveInfo& DownloadSaveInfo::operator=(const DownloadSaveInfo& info) {
-  file_path = info.file_path;
-  file_stream = info.file_stream;
-  suggested_name = info.suggested_name;
-  offset = info.offset;
-  suggested_name = info.suggested_name;
-  return *this;
-}
diff --git a/content/browser/download/download_types.h b/content/browser/download/download_types.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -17,9 +17,7 @@
 // hold the state of the hash algorithm where we left off.
 struct CONTENT_EXPORT DownloadSaveInfo {
   DownloadSaveInfo();
-  DownloadSaveInfo(const DownloadSaveInfo& info);
   ~DownloadSaveInfo();
-  DownloadSaveInfo& operator=(const DownloadSaveInfo& info);
 
   // This is usually the tentative final name, but not during resumption
   // where it will be the intermediate file name.
@@ -34,6 +32,13 @@ struct CONTENT_EXPORT DownloadSaveInfo {
 
   // The state of the hash at the start of the download.  May be empty.
   std::string hash_state;
+
+  // If |prompt_for_save_location| is true, and |file_path| is empty, then
+  // the user will be prompted for a location to save the download. Otherwise,
+  // the location will be determined automatically using |file_path| as a
+  // basis if |file_path| is not empty.
+  // |prompt_for_save_location| defaults to false.
+  bool prompt_for_save_location;
 };
 
 #endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_TYPES_H_
diff --git a/content/browser/download/drag_download_file.cc b/content/browser/download/drag_download_file.cc
@@ -132,11 +132,13 @@ void DragDownloadFile::InitiateDownload() {
   DownloadSaveInfo save_info;
   save_info.file_path = file_path_;
   save_info.file_stream = file_stream_;
-  download_manager_->DownloadUrlToFile(url_,
-                                       referrer_,
-                                       referrer_encoding_,
-                                       save_info,
-                                       web_contents_);
+
+  download_manager_->DownloadUrl(url_,
+                                 referrer_,
+                                 referrer_encoding_,
+                                 false,
+                                 save_info,
+                                 web_contents_);
   download_stats::RecordDownloadCount(
       download_stats::INITIATED_BY_DRAG_N_DROP_COUNT);
 }

diff --git a/content/browser/download/mock_download_manager.h b/content/browser/download/mock_download_manager.h
@@ -52,15 +52,12 @@ class MockDownloadManager : public content::DownloadManager {
                                            base::Time remove_end));
   MOCK_METHOD1(RemoveDownloads, int(base::Time remove_begin));
   MOCK_METHOD0(RemoveAllDownloads, int());
-  MOCK_METHOD4(DownloadUrl, void(const GURL& url,
+  MOCK_METHOD6(DownloadUrl, void(const GURL& url,
                                  const GURL& referrer,
                                  const std::string& referrer_encoding,
+                                 bool prefer_cache,
+                                 const DownloadSaveInfo& save_info,
                                  content::WebContents* web_contents));
-  MOCK_METHOD5(DownloadUrlToFile, void(const GURL& url,
-                                       const GURL& referrer,
-                                       const std::string& referrer_encoding,
-                                       const DownloadSaveInfo& save_info,
-                                       content::WebContents* web_contents));
   MOCK_METHOD1(AddObserver, void(Observer* observer));
   MOCK_METHOD1(RemoveObserver, void(Observer* observer));
   MOCK_METHOD1(OnPersistentStoreQueryComplete, void(

diff --git a/content/browser/renderer_host/buffered_resource_handler.cc b/content/browser/renderer_host/buffered_resource_handler.cc
@@ -310,7 +310,6 @@ bool BufferedResourceHandler::CompleteResponseStarted(int request_id) {
                                   request_->url(),
                                   host_->download_file_manager(),
                                   request_,
-                                  false,
                                   DownloadResourceHandler::OnStartedCallback(),
                                   DownloadSaveInfo()));
 

diff --git a/content/browser/renderer_host/render_message_filter.cc b/content/browser/renderer_host/render_message_filter.cc
@@ -683,17 +683,15 @@ void RenderMessageFilter::OnDownloadUrl(const IPC::Message& message,
                                         const GURL& url,
                                         const GURL& referrer,
                                         const string16& suggested_name) {
-  // Don't show "Save As" UI.
-  bool prompt_for_save_location = false;
   DownloadSaveInfo save_info;
   save_info.suggested_name = suggested_name;
   scoped_ptr<net::URLRequest> request(
       new net::URLRequest(url, resource_dispatcher_host_));
   request->set_referrer(referrer.spec());
   resource_dispatcher_host_->BeginDownload(
       request.Pass(),
+      false,
       save_info,
-      prompt_for_save_location,
       DownloadResourceHandler::OnStartedCallback(),
       render_process_id_,
       message.routing_id(),