For downloads requiring a user gesture, also require...

chrome/browser/download/download_manager.cc

Index: chrome/browser/download/download_manager.cc
===================================================================
--- chrome/browser/download/download_manager.cc	(revision 86341)
+++ chrome/browser/download/download_manager.cc	(working copy)
@@ -269,18 +269,37 @@
       NewCallback(this, &DownloadManager::CheckDownloadUrlDone));
 }
 
+void DownloadManager::CheckVisitedBeforeDone(int32 download_id,
+                                             bool visited_before) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  DownloadItem* download = GetActiveDownloadItem(download_id);
+  if (download) {
+    download->SetVisitedBefore(visited_before);
+    HistoryAndSBChecksMaybeDone(download);
+  }
+}
+
 void DownloadManager::CheckDownloadUrlDone(int32 download_id,
                                            bool is_dangerous_url) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DownloadItem* download = GetActiveDownloadItem(download_id);
-  if (!download)
-    return;
+  if (download) {

[Randy Smith (Not in Mondays), 2011/05/24 18:28:25]

It's a personal style preference (which probably means you can quote me chapter
and versus in the style guide against it :-}) but I like to have "the main path"
outside of conditionals, all other things being equal.  So my preference would
be to retain the "if (!download) return;" pattern, though if you feel strongly I
won't stop the change.

+    download->SetUrlDangerous(is_dangerous_url);
+    HistoryAndSBChecksMaybeDone(download);
+  }
+}
 
-  if (is_dangerous_url)
-    download->MarkUrlDangerous();
+void DownloadManager::HistoryAndSBChecksMaybeDone(DownloadItem* download) {

[Randy Smith (Not in Mondays), 2011/05/24 18:28:25]

One of the biggest things we've done in the past couple of months is to de-race
the download system down to the absolute minimum of races--that was a big source
of the flakiness that had the tests disabled for so long.  For that reason, I
want to keep to an absolute minium the number of places where we do things in
parallel and "join back up" later (as this function does).  My evaluation was
that the only really really important things to do in parallel were the data
download and the user interaction, since either of those could take an arbitrary
time.  So we've made just about everything linear along the user interaction
path and made the data download go in parallel with that.

So unless you have some reason to think that there's a major advantage to doing
the fork/join around the history and safe browsing checks, could we keep the
flow linear?  

(I'm hopeful that that also means you can ditch the TriState enum, since you'll
know by what code you're in whether or not you've done the check.)

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(download);
 
+  // Only proceed once both the history and safe browsing checks have completed.
   DownloadStateInfo state = download->state_info();
+  if ((state.visited_referrer_before == DownloadStateInfo::UNKNOWN) ||
+      (state.is_dangerous_url == DownloadStateInfo::UNKNOWN))
+    return;
 
   // Check whether this download is for an extension install or not.
   // Allow extensions to be explicitly saved.
@@ -343,7 +362,7 @@
       NewRunnableMethod(
           this,
           &DownloadManager::CheckIfSuggestedPathExists,
-          download_id,
+          download->id(),
           state,
           download_prefs()->download_path()));
 }
@@ -497,6 +516,9 @@
   DCHECK(!ContainsKey(active_downloads_, download_id));
   downloads_.insert(download);
   active_downloads_[download_id] = download;
+
+  download_history_->CheckVisitedReferrerBefore(download_id, info->referrer_url,
+      NewCallback(this, &DownloadManager::CheckVisitedBeforeDone));
 }
 
 void DownloadManager::ContinueDownloadWithPath(DownloadItem* download,
@@ -1048,7 +1070,8 @@
     return !(auto_open && state.has_user_gesture);
 
   if (danger_level == download_util::AllowOnUserGesture &&
-      !state.has_user_gesture)
+      (!state.has_user_gesture ||
+       state.visited_referrer_before != DownloadStateInfo::YES))
     return true;
 
   if (state.is_extension_install) {