Moved ios popup opening code from Tab to BlockedPopupTabHelper.

ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm

Index: ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm
diff --git a/ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm b/ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm
index 2e9050c663637f7808fff55a71ef8699a9417b44..8fe09f8c8c89e0fcb69983f0746f0d97f810225c 100644
--- a/ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm
+++ b/ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm
@@ -15,20 +15,22 @@
 #import "ios/chrome/browser/web/chrome_web_test.h"
 #import "ios/web/public/test/fakes/test_navigation_manager.h"
 #import "ios/web/public/test/fakes/test_web_state.h"
+#import "ios/web/public/test/fakes/test_web_state_delegate.h"
 #include "ios/web/web_state/blocked_popup_info.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
+using web::WebState;
+
 // Test fixture for BlockedPopupTabHelper class.
 class BlockedPopupTabHelperTest : public ChromeWebTest {
  protected:
-  BlockedPopupTabHelperTest() {
-    web_state_.SetBrowserState(GetBrowserState());
-    web_state_.SetNavigationManager(
-        base::MakeUnique<web::TestNavigationManager>());
-
-    BlockedPopupTabHelper::CreateForWebState(&web_state_);
-    InfoBarManagerImpl::CreateForWebState(&web_state_);
+  BlockedPopupTabHelperTest()
+      : web_state_(
+            WebState::Create(WebState::CreateParams(GetBrowserState()))) {

[rohitrao (ping after 24h), 2017/02/28 22:51:57]

Why do we need a full WebState for this test now?

[Eugene But (OOO till 7-30), 2017/02/28 23:50:22]

To test that OpenURLFromWebState callback is called on WebStateDelegate.
Actually ChromeWebTest already creates a real WebState, so I will just use it.

+    web_state_->SetDelegate(&web_state_delegate_);
+    BlockedPopupTabHelper::CreateForWebState(web_state_.get());
+    InfoBarManagerImpl::CreateForWebState(web_state_.get());
   }
 
   // Returns true if InfoBarManager is being observed.
@@ -38,14 +40,16 @@ class BlockedPopupTabHelperTest : public ChromeWebTest {
 
   // Returns BlockedPopupTabHelper that is being tested.
   BlockedPopupTabHelper* GetBlockedPopupTabHelper() {
-    return BlockedPopupTabHelper::FromWebState(&web_state_);
+    return BlockedPopupTabHelper::FromWebState(web_state_.get());
   }
+
   // Returns InfoBarManager attached to |web_state_|.
   infobars::InfoBarManager* GetInfobarManager() {
-    return InfoBarManagerImpl::FromWebState(&web_state_);
+    return InfoBarManagerImpl::FromWebState(web_state_.get());
   }
 
-  web::TestWebState web_state_;
+  web::TestWebStateDelegate web_state_delegate_;
+  std::unique_ptr<WebState> web_state_;
 };
 
 // Tests ShouldBlockPopup method. This test changes content settings without
@@ -76,8 +80,8 @@ TEST_F(BlockedPopupTabHelperTest, ShouldBlockPopup) {
   EXPECT_FALSE(GetBlockedPopupTabHelper()->ShouldBlockPopup(source_url2));
 }
 
-// Tests that allowing blocked popup calls |show_popup_handler| and allows
-// future popups for the source url.
+// Tests that allowing blocked popup opens a child window and allows future
+// popups for the source url.
 TEST_F(BlockedPopupTabHelperTest, AllowBlockedPopup) {
   const GURL source_url("https://source-url");
   ASSERT_TRUE(GetBlockedPopupTabHelper()->ShouldBlockPopup(source_url));
@@ -85,10 +89,7 @@ TEST_F(BlockedPopupTabHelperTest, AllowBlockedPopup) {
   // Block popup.
   const GURL target_url("https://target-url");
   web::Referrer referrer(source_url, web::ReferrerPolicyDefault);
-  __block bool show_popup_handler_was_called = false;
-  web::BlockedPopupInfo popup_info(target_url, referrer, nil, ^{
-    show_popup_handler_was_called = true;
-  });
+  web::BlockedPopupInfo popup_info(target_url, referrer);
   GetBlockedPopupTabHelper()->HandlePopup(popup_info);
 
   // Allow blocked popup.
@@ -96,18 +97,31 @@ TEST_F(BlockedPopupTabHelperTest, AllowBlockedPopup) {
   infobars::InfoBar* infobar = GetInfobarManager()->infobar_at(0);
   auto delegate = infobar->delegate()->AsConfirmInfoBarDelegate();
   ASSERT_TRUE(delegate);
+  ASSERT_FALSE(web_state_delegate_.last_open_url_request());
   delegate->Accept();
 
-  // Verify that handler was called and popups are allowed for |test_url|.
-  EXPECT_TRUE(show_popup_handler_was_called);
+  // Verify that popups are allowed for |test_url|.
   EXPECT_FALSE(GetBlockedPopupTabHelper()->ShouldBlockPopup(source_url));
+
+  // Verify that child window was open.
+  auto open_url_request = web_state_delegate_.last_open_url_request();
+  ASSERT_TRUE(open_url_request);
+  EXPECT_EQ(web_state_.get(), open_url_request->web_state);
+  WebState::OpenURLParams params = open_url_request->params;
+  EXPECT_EQ(target_url, params.url);
+  EXPECT_EQ(source_url, params.referrer.url);
+  EXPECT_EQ(web::ReferrerPolicyDefault, params.referrer.policy);
+  EXPECT_EQ(WindowOpenDisposition::NEW_POPUP, params.disposition);
+  EXPECT_TRUE(
+      PageTransitionCoreTypeIs(params.transition, ui::PAGE_TRANSITION_LINK));
+  EXPECT_TRUE(params.is_renderer_initiated);
 }
 
 // Tests that an infobar is added to the infobar manager when
 // BlockedPopupTabHelper::HandlePopup() is called.
 TEST_F(BlockedPopupTabHelperTest, ShowAndDismissInfoBar) {

[rohitrao (ping after 24h), 2017/02/28 22:51:57]

Is there value in having a test that adds the infobar and then destroys the
webstate without choosing an infobar button?

[Eugene But (OOO till 7-30), 2017/02/28 23:50:22]

I think you added this test because you wanted to test IsObservingSources()
call. I would be happy to remove this test because I'm not a fan of testing
private API. Sounds good?

[rohitrao (ping after 24h), 2017/03/01 15:01:16]

Sorry, I was asking if we should add a new test that creates an infobar and then
destroys the webstate without first dismissing the infobar.  That might not be
necessary if we're confident that we can't have a dangling web_state_ pointer.

I'd like to keep this ShowAndDismissInfoBar test, because I think it's testing
something useful.  Let's grab baxley and chat about this sometime this week.

[Eugene But (OOO till 7-30), 2017/03/01 16:30:25]

Oh, that would be a useful test. Added.

Concerting testing via public v.s. private API. I do believe that testing
private API has more drawbacks then benefits. Those tests are not very useful
for catching regressions (they often fail during refactorings), they require
more maintenance and they rarely catch important things (important things are
usually states verifiable via public API). TabTest is a good example: it almost
always fail during my refactorings, I have to spend time fixing them every time
I touch Tab, those tests never caught a regression after my changes.
go/unit-test-practices?cl=head#public-apis has a good guidance on testing via
private API.

   const GURL test_url("https://popups.example.com");
-  web::BlockedPopupInfo popup_info(test_url, web::Referrer(), nil, nil);
+  web::BlockedPopupInfo popup_info(test_url, web::Referrer());
 
   // Check that there are no infobars showing and no registered observers.
   EXPECT_EQ(0U, GetInfobarManager()->infobar_count());