Classify navigations without page id in parallel to the existing classifier.

chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc

Index: chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
index 536953a5bcc56a381648b9fd31fb4e076962833b..1634a708d2b62df762a1e661a70c78588699e6ed 100644
--- a/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
@@ -120,9 +120,29 @@ class SafeBrowsingBlockingPageTest : public ChromeRenderViewHostTestHarness {
       user_response_ = CANCEL;
   }
 
-  void Navigate(const char* url, int page_id) {
+  void Navigate(const char* url,
+                int page_id,
+                int nav_entry_id,
+                bool did_create_new_entry) {
+    NavigateCrossSite(url, page_id, nav_entry_id, did_create_new_entry, false);
+  }
+
+  void NavigateCrossSite(const char* url,
+                         int page_id,
+                         int nav_entry_id,
+                         bool did_create_new_entry,
+                         bool is_cross_site) {
+    // The pending RVH should commit for cross-site navigations.
+    content::RenderFrameHost* rfh = is_cross_site ?
+        WebContentsTester::For(web_contents())->GetPendingMainFrame() :
+        web_contents()->GetMainFrame();
+
     WebContentsTester::For(web_contents())->TestDidNavigate(
-        web_contents()->GetMainFrame(), page_id, GURL(url),
+        rfh,
+        page_id,
+        nav_entry_id,
+        did_create_new_entry,
+        GURL(url),
         ui::PAGE_TRANSITION_TYPED);
   }
 
@@ -139,7 +159,9 @@ class SafeBrowsingBlockingPageTest : public ChromeRenderViewHostTestHarness {
     WebContentsTester::For(web_contents())->TestDidNavigate(
         rfh,
         entry->GetPageID(),
-        GURL(entry->GetURL()),
+        entry->GetUniqueID(),
+        false,
+        entry->GetURL(),
         ui::PAGE_TRANSITION_TYPED);
   }
 
@@ -224,8 +246,7 @@ TEST_F(SafeBrowsingBlockingPageTest, MalwarePageDontProceed) {
 
   // Simulate the load causing a safe browsing interstitial to be shown.
   ShowInterstitial(false, kBadURL);
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   base::RunLoop().RunUntilIdle();
@@ -256,11 +277,11 @@ TEST_F(SafeBrowsingBlockingPageTest, MalwarePageProceed) {
   // Start a load.
   controller().LoadURL(GURL(kBadURL), content::Referrer(),
                        ui::PAGE_TRANSITION_TYPED, std::string());
+  int pending_id = controller().GetPendingEntry()->GetUniqueID();
 
   // Simulate the load causing a safe browsing interstitial to be shown.
   ShowInterstitial(false, kBadURL);
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Simulate the user clicking "proceed".
@@ -269,7 +290,7 @@ TEST_F(SafeBrowsingBlockingPageTest, MalwarePageProceed) {
   // The interstitial is shown until the navigation commits.
   ASSERT_TRUE(InterstitialPage::GetInterstitialPage(web_contents()));
   // Commit the navigation.
-  Navigate(kBadURL, 1);
+  Navigate(kBadURL, 1, pending_id, true);
   // The interstitial should be gone now.
   ASSERT_FALSE(InterstitialPage::GetInterstitialPage(web_contents()));
 
@@ -288,16 +309,15 @@ TEST_F(SafeBrowsingBlockingPageTest, PageWithMalwareResourceDontProceed) {
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Navigate somewhere.
-  Navigate(kGoogleURL, 1);
+  Navigate(kGoogleURL, 1, 0, true);
 
   // Navigate somewhere else.
-  Navigate(kGoodURL, 2);
+  Navigate(kGoodURL, 2, 0, true);

[mattm, 2015/04/24 23:42:37]

is nav_entry_id=0 some magic value? Why is it only non-zero sometimes? Would be
nice if TestDidNavigate was documented.

[Avi (use Gerrit), 2015/04/25 19:24:51]

I documented those values on RenderFrameHostTester but failed to document them
on WebContentsTester. Done.

 
   // Simulate that page loading a bad-resource triggering an interstitial.
   ShowInterstitial(true, kBadURL);
 
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Simulate the user clicking "don't proceed".
@@ -325,13 +345,12 @@ TEST_F(SafeBrowsingBlockingPageTest, PageWithMalwareResourceProceed) {
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Navigate somewhere.
-  Navigate(kGoodURL, 1);
+  Navigate(kGoodURL, 1, 0, true);
 
   // Simulate that page loading a bad-resource triggering an interstitial.
   ShowInterstitial(true, kBadURL);
 
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Simulate the user clicking "proceed".
@@ -360,10 +379,10 @@ TEST_F(SafeBrowsingBlockingPageTest,
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Navigate somewhere.
-  Navigate(kGoogleURL, 1);
+  Navigate(kGoogleURL, 1, 0, true);
 
   // Navigate somewhere else.
-  Navigate(kGoodURL, 2);
+  Navigate(kGoodURL, 2, 0, true);
 
   // Simulate that page loading a bad-resource triggering an interstitial.
   ShowInterstitial(true, kBadURL);
@@ -373,8 +392,7 @@ TEST_F(SafeBrowsingBlockingPageTest,
   ShowInterstitial(true, kBadURL2);
   ShowInterstitial(true, kBadURL3);
 
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Simulate the user clicking "don't proceed".
@@ -403,10 +421,10 @@ TEST_F(SafeBrowsingBlockingPageTest,
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Navigate somewhere.
-  Navigate(kGoogleURL, 1);
+  Navigate(kGoogleURL, 1, 0, true);
 
   // Navigate somewhere else.
-  Navigate(kGoodURL, 2);
+  Navigate(kGoodURL, 2, 0, true);
 
   // Simulate that page loading a bad-resource triggering an interstitial.
   ShowInterstitial(true, kBadURL);
@@ -416,8 +434,7 @@ TEST_F(SafeBrowsingBlockingPageTest,
   ShowInterstitial(true, kBadURL2);
   ShowInterstitial(true, kBadURL3);
 
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Proceed through the 1st interstitial.
@@ -453,8 +470,7 @@ TEST_F(SafeBrowsingBlockingPageTest,
 
 // Tests showing a blocking page for a page that contains multiple malware
 // subresources and proceeding through the multiple interstitials.
-TEST_F(SafeBrowsingBlockingPageTest,
-    PageWithMultipleMalwareResourceProceed) {
+TEST_F(SafeBrowsingBlockingPageTest, PageWithMultipleMalwareResourceProceed) {
   // Enable malware reports.
   Profile* profile = Profile::FromBrowserContext(
       web_contents()->GetBrowserContext());
@@ -462,7 +478,7 @@ TEST_F(SafeBrowsingBlockingPageTest,
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Navigate somewhere else.
-  Navigate(kGoodURL, 1);
+  Navigate(kGoodURL, 1, 0, true);
 
   // Simulate that page loading a bad-resource triggering an interstitial.
   ShowInterstitial(true, kBadURL);
@@ -472,8 +488,7 @@ TEST_F(SafeBrowsingBlockingPageTest,
   ShowInterstitial(true, kBadURL2);
   ShowInterstitial(true, kBadURL3);
 
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Proceed through the 1st interstitial.
@@ -515,19 +530,19 @@ TEST_F(SafeBrowsingBlockingPageTest, NavigatingBackAndForth) {
       prefs::kSafeBrowsingExtendedReportingEnabled, true);
 
   // Navigate somewhere.
-  Navigate(kGoodURL, 1);
+  Navigate(kGoodURL, 1, 0, true);
 
   // Now navigate to a bad page triggerring an interstitial.
   controller().LoadURL(GURL(kBadURL), content::Referrer(),
                        ui::PAGE_TRANSITION_TYPED, std::string());
+  int pending_id = controller().GetPendingEntry()->GetUniqueID();
   ShowInterstitial(false, kBadURL);
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Proceed, then navigate back.
   ProceedThroughInterstitial(sb_interstitial);
-  Navigate(kBadURL, 2);  // Commit the navigation.
+  Navigate(kBadURL, 2, pending_id, true);  // Commit the navigation.
   GoBack(true);
 
   // We are back on the good page.
@@ -538,13 +553,15 @@ TEST_F(SafeBrowsingBlockingPageTest, NavigatingBackAndForth) {
 
   // Navigate forward to the malware URL.
   web_contents()->GetController().GoForward();
+  pending_id = controller().GetPendingEntry()->GetUniqueID();
   ShowInterstitial(false, kBadURL);
   sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   // Let's proceed and make sure everything is OK (bug 17627).
   ProceedThroughInterstitial(sb_interstitial);
-  Navigate(kBadURL, 2);  // Commit the navigation.
+  // Commit the navigation.
+  NavigateCrossSite(kBadURL, 2, pending_id, false, true);

[mattm, 2015/04/24 23:42:37]

Since NavigateCrossSite is only directly called with is_cross_site=true, maybe
it should just mean that, and there can be some internal function that both
Navigate and NavigateCrossSite call. Also it looks like did_create_new_entry is
always true when calling Navigate and always false when calling
NavigateCrossSite, is that always the case? Can it also only be exposed on the
internal function to simplify things?

[Avi (use Gerrit), 2015/04/25 19:24:51]

did_create_new_entry is false because it is a history navigation. See the new
documentation that I added on WebContentsTester. I can't hide it because only
the caller of the Navigate functions would know the correct value.

Re the code cleanup, done. (I stole this bit of the code from the
DataReductionProxyDebugBlockingPageTest, so I made these changes there too.)

   sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_FALSE(sb_interstitial);
   ASSERT_EQ(2, controller().GetEntryCount());
@@ -570,8 +587,7 @@ TEST_F(SafeBrowsingBlockingPageTest, ProceedThenDontProceed) {
 
   // Simulate the load causing a safe browsing interstitial to be shown.
   ShowInterstitial(false, kBadURL);
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   base::RunLoop().RunUntilIdle();
@@ -607,8 +623,7 @@ TEST_F(SafeBrowsingBlockingPageTest, MalwareReportsDisabled) {
 
   // Simulate the load causing a safe browsing interstitial to be shown.
   ShowInterstitial(false, kBadURL);
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   base::RunLoop().RunUntilIdle();
@@ -642,8 +657,7 @@ TEST_F(SafeBrowsingBlockingPageTest, MalwareReportsToggling) {
 
   // Simulate the load causing a safe browsing interstitial to be shown.
   ShowInterstitial(false, kBadURL);
-  SafeBrowsingBlockingPage* sb_interstitial =
-      GetSafeBrowsingBlockingPage();
+  SafeBrowsingBlockingPage* sb_interstitial = GetSafeBrowsingBlockingPage();
   ASSERT_TRUE(sb_interstitial);
 
   base::RunLoop().RunUntilIdle();