Moved ios popup opening code from Tab to BlockedPopupTabHelper.

ios/chrome/browser/web/blocked_popup_tab_helper_unittest.mm

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ios/chrome/browser/web/blocked_popup_tab_helper.h"
 
 #include "base/memory/ptr_util.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/infobars/core/confirm_infobar_delegate.h"
 #include "components/infobars/core/infobar.h"
 #include "components/infobars/core/infobar_manager.h"
 #include "ios/chrome/browser/browser_state/test_chrome_browser_state.h"
 #include "ios/chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "ios/chrome/browser/infobars/infobar_manager_impl.h"
 #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.
   bool IsObservingSources() {
     return GetBlockedPopupTabHelper()->scoped_observer_.IsObservingSources();
   }
 
   // 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
 // restoring them back, which is fine because changes do not persist across test
 // runs.
 TEST_F(BlockedPopupTabHelperTest, ShouldBlockPopup) {
   const GURL source_url1("https://source-url1");
   EXPECT_TRUE(GetBlockedPopupTabHelper()->ShouldBlockPopup(source_url1));
 
   // Allow popups for |source_url1|.
@@ skipping 10 matching lines @@
   EXPECT_TRUE(GetBlockedPopupTabHelper()->ShouldBlockPopup(source_url2));
 
   // Allow all popups.
   settings_map->SetDefaultContentSetting(CONTENT_SETTINGS_TYPE_POPUPS,
                                          CONTENT_SETTING_ALLOW);
 
   EXPECT_FALSE(GetBlockedPopupTabHelper()->ShouldBlockPopup(source_url1));
   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));
 
   // 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.
   ASSERT_EQ(1U, GetInfobarManager()->infobar_count());
   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());
   EXPECT_FALSE(IsObservingSources());
 
   // Call |HandlePopup| to show an infobar.
   GetBlockedPopupTabHelper()->HandlePopup(popup_info);
   ASSERT_EQ(1U, GetInfobarManager()->infobar_count());
   EXPECT_TRUE(IsObservingSources());
 
   // Dismiss the infobar and check that the tab helper no longer has any
   // registered observers.
   GetInfobarManager()->infobar_at(0)->RemoveSelf();
   EXPECT_EQ(0U, GetInfobarManager()->infobar_count());
   EXPECT_FALSE(IsObservingSources());
 }