Fix DCHECK when shutting down safe browsing DownloadMetadataManager.

chrome/browser/safe_browsing/incident_reporting/download_metadata_manager_unittest.cc

Index: chrome/browser/safe_browsing/incident_reporting/download_metadata_manager_unittest.cc
diff --git a/chrome/browser/safe_browsing/incident_reporting/download_metadata_manager_unittest.cc b/chrome/browser/safe_browsing/incident_reporting/download_metadata_manager_unittest.cc
index 1539fe025d572886a7ea71240a6b5e8b6f849439..071ec3c591ee2ce443b7a14548e189a1e5fb33aa 100644
--- a/chrome/browser/safe_browsing/incident_reporting/download_metadata_manager_unittest.cc
+++ b/chrome/browser/safe_browsing/incident_reporting/download_metadata_manager_unittest.cc
@@ -180,6 +180,14 @@ class DownloadMetadataManagerTestBase : public ::testing::Test {
   void ShutdownDownloadManager() {
     if (dm_observer_) {
       dm_observer_->ManagerGoingDown(&download_manager_);
+      // Note: these calls may result in "Uninteresting mock function call"
+      // warnings as a result of MockDownloadItem invoking observers in its
+      // dtor. This happens after the NiceMock wrapper has removed its niceness
+      // hook. These can safely be ignored, as they are entirely expected. The
+      // values specified by ON_CALL invocations in AddDownloadItems are
+      // returned as desired.

[robertshield, 2014/11/05 01:04:50]

I don't know enough gmock to know whether this is work-aroundable. Can
expectations be added for these calls? MAD claims knowledge of such things.

[grt (UTC plus 2), 2014/11/05 01:25:35]

The right thing when you don't want an expectation (something that would make
the test fail) but rather to specify the mock's behavior is to use ON_CALL
(https://code.google.com/p/googlemock/wiki/CookBook#Setting_the_Default_Action...)
as on line 200. This returns the desired value and will emit the "Uninteresting"
warning as noted in the comment. The right way to suppress that warning is to
use a NiceMock
(https://code.google.com/p/googlemock/wiki/CookBook#The_Nice,_the_Strict,_and_...),
which means "don't let me know about calls that don't have expectations." The
problem, as I perhaps poorly explained in the comment here, is that
MockDownloadItem's dtor runs callbacks which will invoke methods on the item.
These work just fine, but the NiceMock's dtor (which removes the "suppress
unintersting calls" hook for this mock) has already run at that point, so the
uninteresting calls are logged.

+      other_item_.reset();
+      test_item_.reset();
       dm_observer_ = nullptr;
     }
   }
@@ -208,13 +216,6 @@ class DownloadMetadataManagerTestBase : public ::testing::Test {
     dm_observer_->OnDownloadCreated(&download_manager_, other_item_.get());
   }
 
-  // Destroyes the DownloadItems added to the manager. Safe to call any number
-  // of times.
-  void DestroyDownloadItems() {
-    other_item_.reset();
-    test_item_.reset();
-  }
-
   content::TestBrowserThreadBundle thread_bundle_;
   NiceMock<MockDownloadMetadataManager> manager_;
   TestingProfile profile_;
@@ -329,17 +330,14 @@ TEST_P(GetDetailsTest, GetDownloadDetails) {
   // Fire in the hole!
   manager_.GetDownloadDetails(&profile_, details_getter.GetCallback());
 
-  // Destroy download items and shutdown the download manager, if relevant.
-  if (early_shutdown_) {
-    DestroyDownloadItems();
+  // Shutdown the download manager, if relevant.
+  if (early_shutdown_)
     ShutdownDownloadManager();
-  }
 
   // Allow the read task and the response callback to run.
   RunAllTasks();
 
-  // Destroy download items and shutdown the download manager, if relevant.
-  DestroyDownloadItems();
+  // Shutdown the download manager, if relevant.
   ShutdownDownloadManager();
 }
 
@@ -464,7 +462,6 @@ TEST_P(SetRequestTest, SetRequest) {
   }
   manager_.GetDownloadDetails(&profile_, details_getter.GetCallback());
 
-  DestroyDownloadItems();
   ShutdownDownloadManager();
 
   scoped_ptr<DownloadMetadata> metadata(ReadTestMetadataFile());