Add the result of the inclusion whitelist to the OffDomainInclusionDetector's analysis.

chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc

Index: chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc
diff --git a/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc b/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc
index c06da992174f7265c1e6e88b23b2c74e10a37b6d..475c9f4a01656a6b60276a08e56fa852cd6c1de5 100644
--- a/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc
+++ b/chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector_unittest.cc
@@ -3,7 +3,11 @@
 // found in the LICENSE file.
 
 #include "base/bind.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/safe_browsing/database_manager.h"
 #include "chrome/browser/safe_browsing/incident_reporting/off_domain_inclusion_detector.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/common/resource_type.h"
 #include "content/public/test/test_browser_thread_bundle.h"
@@ -11,9 +15,15 @@
 #include "net/base/request_priority.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_test_util.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
+using testing::_;
+using testing::Return;
+using testing::StrictMock;

[grt (UTC plus 2), 2015/01/07 14:35:48]

unused

[gab, 2015/01/07 17:52:01]

Oops, indeed, last minute change, thanks :-)!

+using testing::Values;
+
 namespace safe_browsing {
 
 using AnalysisEvent = OffDomainInclusionDetector::AnalysisEvent;
@@ -49,14 +59,50 @@ static_assert(
         arraysize(kResourceTypesIgnored) == content::RESOURCE_TYPE_LAST_TYPE,
     "Expected resource types list aren't comprehensive");
 
-class OffDomainInclusionDetectorTest : public testing::Test {
+// A set of test cases to run each parametrized test case below through.
+enum class OffDomainInclusionTestCases {
+  OFF_DOMAIN_INCLUSION_WHITELISTED,
+  OFF_DOMAIN_INCLUSION_UNKNOWN,
+};
+
+class MockSafeBrowsingDatabaseManager : public SafeBrowsingDatabaseManager {
+ public:
+  explicit MockSafeBrowsingDatabaseManager(
+      const scoped_refptr<SafeBrowsingService>& service)
+      : SafeBrowsingDatabaseManager(service) {}
+
+  MOCK_METHOD1(MatchInclusionWhitelistUrl, bool(const GURL& url));
+
+ protected:
+  virtual ~MockSafeBrowsingDatabaseManager() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockSafeBrowsingDatabaseManager);
+};
+
+class OffDomainInclusionDetectorTest
+    : public testing::TestWithParam<OffDomainInclusionTestCases> {
  public:
   OffDomainInclusionDetectorTest()
-      : off_domain_inclusion_detector_(
-            base::Bind(
-                &OffDomainInclusionDetectorTest::OnOffDomainInclusionEvent,
-                base::Unretained(this))),
-        observed_analysis_event_(AnalysisEvent::NO_EVENT) {}
+      : observed_analysis_event_(AnalysisEvent::NO_EVENT) {}
+
+  void SetUp() override {

[grt (UTC plus 2), 2015/01/07 14:35:48]

move this into the protected: block below. actually, i don't think the ctor
needs to be public, either.

[gab, 2015/01/07 17:52:01]

Done.

+    // Only used for initializing mock objects.
+    scoped_refptr<SafeBrowsingService> sb_service =
+        SafeBrowsingService::CreateSafeBrowsingService();
+    mock_safe_browsing_database_manager_ =
+        new MockSafeBrowsingDatabaseManager(sb_service);
+    ON_CALL(*mock_safe_browsing_database_manager_,

[grt (UTC plus 2), 2015/01/07 14:35:48]

is ON_CALL enough to prevent the mock from logging uninteresting calls? use
NiceMock if you don't care about methods being called on the mock
(https://code.google.com/p/googlemock/wiki/CookBook#The_Nice,_the_Strict,_and_...).
if you find that there's no logging of uninteresting calls at the moment, i
wonder if MatchInclusionWhitelistUrl is actually being called by the test.

[gab, 2015/01/07 17:52:01]

Oh interesting, didn't know about NiceMock :-)!

I went away from StrictMock as it logged uninteresting calls failures whereas in
this case I only care about mocking MatchInclusionWhitelistUrl()'s return value
without wanting to care about the implementation detail of when it is called

I hadn't realize that the default mock was still logging warnings as logs are
gobbled up and only shown on failure when using multiple test-launcher-jobs.

Along with ON_CALL this seems to be exactly what I want for this use case
(https://code.google.com/p/googlemock/wiki/CookBook#Knowing_When_to_Expect).

+            MatchInclusionWhitelistUrl(_))
+        .WillByDefault(Return(
+            GetParam() ==
+            OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_WHITELISTED));
+
+    off_domain_inclusion_detector_.reset(new OffDomainInclusionDetector(
+        mock_safe_browsing_database_manager_,
+        base::Bind(&OffDomainInclusionDetectorTest::OnOffDomainInclusionEvent,
+                   base::Unretained(this))));
+  }
 
  protected:
   AnalysisEvent GetLastEventAndReset() {
@@ -92,7 +138,16 @@ class OffDomainInclusionDetectorTest : public testing::Test {
     return url_request.Pass();
   }
 
-  OffDomainInclusionDetector off_domain_inclusion_detector_;
+  // Returns the expected AnalysisEvent produced when facing an off-domain
+  // inclusion in the current test configuration.
+  AnalysisEvent GetExpectedOffDomainInclusionEventType() {
+    return GetParam() ==
+                   OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_WHITELISTED
+               ? AnalysisEvent::OFF_DOMAIN_INCLUSION_WHITELISTED
+               : AnalysisEvent::OFF_DOMAIN_INCLUSION_SUSPICIOUS;
+  }
+
+  scoped_ptr<OffDomainInclusionDetector> off_domain_inclusion_detector_;
 
  private:
   void OnOffDomainInclusionEvent(AnalysisEvent event) {
@@ -101,13 +156,16 @@ class OffDomainInclusionDetectorTest : public testing::Test {
     observed_analysis_event_ = event;
   }
 
+  scoped_refptr<MockSafeBrowsingDatabaseManager>

[grt (UTC plus 2), 2015/01/07 14:35:48]

does this need to be a member of the test fixture? the detector will hold a ref
on the db manager.

[gab, 2015/01/07 17:52:01]

Good point, I was originally thinking it would to set up mock expectations but
since they're now set up inline in SetUp() it doesn't need to be here :-)!

+      mock_safe_browsing_database_manager_;
+
   content::TestBrowserThreadBundle thread_bundle_;
   net::TestURLRequestContext context_;
 
   AnalysisEvent observed_analysis_event_;
 };
 
-TEST_F(OffDomainInclusionDetectorTest, NoEventForIgnoredResourceTypes) {
+TEST_P(OffDomainInclusionDetectorTest, NoEventForIgnoredResourceTypes) {
   for (content::ResourceType tested_type : kResourceTypesIgnored) {
     SCOPED_TRACE(tested_type);
 
@@ -118,14 +176,14 @@ TEST_F(OffDomainInclusionDetectorTest, NoEventForIgnoredResourceTypes) {
                           true,     // is_main_frame
                           false));  // parent_is_main_frame
 
-    off_domain_inclusion_detector_.OnResourceRequest(
+    off_domain_inclusion_detector_->OnResourceRequest(
         request_for_tested_type.get());
 
     EXPECT_EQ(AnalysisEvent::NO_EVENT, GetLastEventAndReset());
   }
 }
 
-TEST_F(OffDomainInclusionDetectorTest, NoEventForSameDomainInclusions) {
+TEST_P(OffDomainInclusionDetectorTest, NoEventForSameDomainInclusions) {
   for (content::ResourceType tested_type :
        kResourceTypesObservedIfInMainFrame) {
     SCOPED_TRACE(tested_type);
@@ -137,14 +195,14 @@ TEST_F(OffDomainInclusionDetectorTest, NoEventForSameDomainInclusions) {
                           true,     // is_main_frame
                           false));  // parent_is_main_frame
 
-    off_domain_inclusion_detector_.OnResourceRequest(
+    off_domain_inclusion_detector_->OnResourceRequest(
         request_for_tested_type.get());
 
     EXPECT_EQ(AnalysisEvent::NO_EVENT, GetLastEventAndReset());
   }
 }
 
-TEST_F(OffDomainInclusionDetectorTest, OffDomainInclusionInMainFrame) {
+TEST_P(OffDomainInclusionDetectorTest, OffDomainInclusionInMainFrame) {
   for (content::ResourceType tested_type :
        kResourceTypesObservedIfInMainFrame) {
     SCOPED_TRACE(tested_type);
@@ -156,15 +214,14 @@ TEST_F(OffDomainInclusionDetectorTest, OffDomainInclusionInMainFrame) {
                           true,     // is_main_frame
                           false));  // parent_is_main_frame
 
-    off_domain_inclusion_detector_.OnResourceRequest(
+    off_domain_inclusion_detector_->OnResourceRequest(
         request_for_tested_type.get());
 
-    EXPECT_EQ(AnalysisEvent::OFF_DOMAIN_INCLUSION_DETECTED,
-              GetLastEventAndReset());
+    EXPECT_EQ(GetExpectedOffDomainInclusionEventType(), GetLastEventAndReset());
   }
 }
 
-TEST_F(OffDomainInclusionDetectorTest, HttpsOffDomainInclusionInMainFrame) {
+TEST_P(OffDomainInclusionDetectorTest, HttpsOffDomainInclusionInMainFrame) {
   for (content::ResourceType tested_type :
        kResourceTypesObservedIfInMainFrame) {
     SCOPED_TRACE(tested_type);
@@ -176,15 +233,14 @@ TEST_F(OffDomainInclusionDetectorTest, HttpsOffDomainInclusionInMainFrame) {
                           true,     // is_main_frame
                           false));  // parent_is_main_frame
 
-    off_domain_inclusion_detector_.OnResourceRequest(
+    off_domain_inclusion_detector_->OnResourceRequest(
         request_for_tested_type.get());
 
-    EXPECT_EQ(AnalysisEvent::OFF_DOMAIN_INCLUSION_DETECTED,
-              GetLastEventAndReset());
+    EXPECT_EQ(GetExpectedOffDomainInclusionEventType(), GetLastEventAndReset());
   }
 }
 
-TEST_F(OffDomainInclusionDetectorTest,
+TEST_P(OffDomainInclusionDetectorTest,
        NoEventForNonHttpOffDomainInclusionInMainFrame) {
   for (content::ResourceType tested_type :
        kResourceTypesObservedIfInMainFrame) {
@@ -197,14 +253,14 @@ TEST_F(OffDomainInclusionDetectorTest,
                           true,     // is_main_frame
                           false));  // parent_is_main_frame
 
-    off_domain_inclusion_detector_.OnResourceRequest(
+    off_domain_inclusion_detector_->OnResourceRequest(
         request_for_tested_type.get());
 
     EXPECT_EQ(AnalysisEvent::NO_EVENT, GetLastEventAndReset());
   }
 }
 
-TEST_F(OffDomainInclusionDetectorTest, NoEventForSameTopLevelDomain) {
+TEST_P(OffDomainInclusionDetectorTest, NoEventForSameTopLevelDomain) {
   for (content::ResourceType tested_type :
        kResourceTypesObservedIfInMainFrame) {
     SCOPED_TRACE(tested_type);
@@ -216,14 +272,14 @@ TEST_F(OffDomainInclusionDetectorTest, NoEventForSameTopLevelDomain) {
                           true,     // is_main_frame
                           false));  // parent_is_main_frame
 
-    off_domain_inclusion_detector_.OnResourceRequest(
+    off_domain_inclusion_detector_->OnResourceRequest(
         request_for_tested_type.get());
 
     EXPECT_EQ(AnalysisEvent::NO_EVENT, GetLastEventAndReset());
   }
 }
 
-TEST_F(OffDomainInclusionDetectorTest,
+TEST_P(OffDomainInclusionDetectorTest,
        OffDomainInclusionForSameTopLevelRegistryButDifferentDomain) {
   for (content::ResourceType tested_type :
        kResourceTypesObservedIfInMainFrame) {
@@ -236,15 +292,14 @@ TEST_F(OffDomainInclusionDetectorTest,
                           true,     // is_main_frame
                           false));  // parent_is_main_frame
 
-    off_domain_inclusion_detector_.OnResourceRequest(
+    off_domain_inclusion_detector_->OnResourceRequest(
         request_for_tested_type.get());
 
-    EXPECT_EQ(AnalysisEvent::OFF_DOMAIN_INCLUSION_DETECTED,
-              GetLastEventAndReset());
+    EXPECT_EQ(GetExpectedOffDomainInclusionEventType(), GetLastEventAndReset());
   }
 }
 
-TEST_F(OffDomainInclusionDetectorTest,
+TEST_P(OffDomainInclusionDetectorTest,
        NoEventForOffDomainRegularResourceInSubframe) {
   for (content::ResourceType tested_type :
        kResourceTypesObservedIfInMainFrame) {
@@ -257,14 +312,14 @@ TEST_F(OffDomainInclusionDetectorTest,
                           false,   // is_main_frame
                           true));  // parent_is_main_frame
 
-    off_domain_inclusion_detector_.OnResourceRequest(
+    off_domain_inclusion_detector_->OnResourceRequest(
         request_for_tested_type.get());
 
     EXPECT_EQ(AnalysisEvent::NO_EVENT, GetLastEventAndReset());
   }
 }
 
-TEST_F(OffDomainInclusionDetectorTest,
+TEST_P(OffDomainInclusionDetectorTest,
        NoEventForOffDomainSubSubFrameInclusion) {
   for (content::ResourceType tested_type :
        kResourceTypesObservedIfParentIsMainFrame) {
@@ -277,14 +332,14 @@ TEST_F(OffDomainInclusionDetectorTest,
                           false,    // is_main_frame
                           false));  // parent_is_main_frame
 
-    off_domain_inclusion_detector_.OnResourceRequest(
+    off_domain_inclusion_detector_->OnResourceRequest(
         request_for_tested_type.get());
 
     EXPECT_EQ(AnalysisEvent::NO_EVENT, GetLastEventAndReset());
   }
 }
 
-TEST_F(OffDomainInclusionDetectorTest,
+TEST_P(OffDomainInclusionDetectorTest,
        OffDomainInclusionForOffDomainResourcesObservedIfParentIsMainFrame) {
   for (content::ResourceType tested_type :
        kResourceTypesObservedIfParentIsMainFrame) {
@@ -297,15 +352,14 @@ TEST_F(OffDomainInclusionDetectorTest,
                           false,   // is_main_frame
                           true));  // parent_is_main_frame
 
-    off_domain_inclusion_detector_.OnResourceRequest(
+    off_domain_inclusion_detector_->OnResourceRequest(
         request_for_tested_type.get());
 
-    EXPECT_EQ(AnalysisEvent::OFF_DOMAIN_INCLUSION_DETECTED,
-              GetLastEventAndReset());
+    EXPECT_EQ(GetExpectedOffDomainInclusionEventType(), GetLastEventAndReset());
   }
 }
 
-TEST_F(OffDomainInclusionDetectorTest,
+TEST_P(OffDomainInclusionDetectorTest,
        EmptyEventForOffDomainInclusionWithNoReferrer) {
   for (content::ResourceType tested_type :
        kResourceTypesObservedIfInMainFrame) {
@@ -318,11 +372,17 @@ TEST_F(OffDomainInclusionDetectorTest,
                           true,     // is_main_frame
                           false));  // parent_is_main_frame
 
-    off_domain_inclusion_detector_.OnResourceRequest(
+    off_domain_inclusion_detector_->OnResourceRequest(
         request_for_tested_type.get());
 
     EXPECT_EQ(AnalysisEvent::EMPTY_MAIN_FRAME_URL, GetLastEventAndReset());
   }
 }
 
+INSTANTIATE_TEST_CASE_P(
+    OffDomainInclusionDetectorTestInstance,
+    OffDomainInclusionDetectorTest,
+    Values(OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_WHITELISTED,
+           OffDomainInclusionTestCases::OFF_DOMAIN_INCLUSION_UNKNOWN));
+
 }  // namespace safe_browsing