Window.open calls issued by pages within ChromeFrame which use the NEW_WINDOW/NEW_POPUP

dispositions open up as regular Chrome windows, which results in them not using the host network stack,
which means that HTTP sessions/cookies, etc established by the main page will not work for the popup.

Fix is to ensure that these dispositions also get routed back to the host browser where a new tab
would be created which would attach itself to the newly created ExternalTabContainer instance.

Fixes bug http://code.google.com/p/chromium/issues/detail?id=33324

Bug=33324
Test=Covered by ChromeFrame unit test.

Review URL: http://codereview.chromium.org/549183

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@37425 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 70 insertions, 64 deletions

diff --git a/chrome/browser/external_tab_container.cc b/chrome/browser/external_tab_container.cc
index 8ec811c..bb3f603 100644
--- a/chrome/browser/external_tab_container.cc
+++ b/chrome/browser/external_tab_container.cc
@@ -311,53 +311,35 @@ void ExternalTabContainer::AddNewContents(TabContents* source,
                             WindowOpenDisposition disposition,
                             const gfx::Rect& initial_pos,
                             bool user_gesture) {
-  switch (disposition) {
-    case NEW_POPUP:
-    case NEW_WINDOW: {
-      Browser::BuildPopupWindowHelper(source, new_contents, initial_pos,
-                                      Browser::TYPE_POPUP,
-                                      tab_contents_->profile(), true);
-      break;
-    }
-
-    case NEW_FOREGROUND_TAB:
-    case NEW_BACKGROUND_TAB: {
-      DCHECK(automation_ != NULL);
-
-      scoped_refptr<ExternalTabContainer> new_container =
-          new ExternalTabContainer(NULL, NULL);
-
-      // Make sure that ExternalTabContainer instance is initialized with
-      // an unwrapped Profile.
-      bool result = new_container->Init(
-          new_contents->profile()->GetOriginalProfile(),
-          NULL,
-          initial_pos,
-          WS_CHILD,
-          load_requests_via_automation_,
-          handle_top_level_requests_,
-          new_contents,
-          GURL(),
-          GURL());
-
-      if (result) {
-        pending_tabs_[reinterpret_cast<intptr_t>(new_container.get())] =
-            new_container;
-
-        automation_->Send(new AutomationMsg_AttachExternalTab(
-            0,
-            tab_handle_,
-            reinterpret_cast<intptr_t>(new_container.get()),
-            disposition));
-      } else {
-        NOTREACHED();
-      }
-      break;
-    }
-
-    default:
-      NOTREACHED();
-      break;
+  DCHECK(automation_ != NULL);
+
+  scoped_refptr<ExternalTabContainer> new_container =
+      new ExternalTabContainer(NULL, NULL);
+
+  // Make sure that ExternalTabContainer instance is initialized with
+  // an unwrapped Profile.
+  bool result = new_container->Init(
+      new_contents->profile()->GetOriginalProfile(),
+      NULL,
+      initial_pos,
+      WS_CHILD,
+      load_requests_via_automation_,
+      handle_top_level_requests_,
+      new_contents,
+      GURL(),
+      GURL());
+
+  if (result) {
+    pending_tabs_[reinterpret_cast<intptr_t>(new_container.get())] =
+        new_container;
+
+    automation_->Send(new AutomationMsg_AttachExternalTab(
+        0,
+        tab_handle_,
+        reinterpret_cast<intptr_t>(new_container.get()),
+        disposition));
+  } else {
+    NOTREACHED();
   }
 }
 
diff --git a/chrome_frame/chrome_active_document.cc b/chrome_frame/chrome_active_document.cc
index e92aa0b..553b46f 100644
--- a/chrome_frame/chrome_active_document.cc
+++ b/chrome_frame/chrome_active_document.cc
@@ -578,9 +578,18 @@ void ChromeActiveDocument::UpdateNavigationState(
   // that only happen within Chrome such as anchor navigations) we need to
   // update IE's internal state after the fact. In the case of internal
   // navigations, we notify the BHOs but ignore the should_cancel flag.
-  bool is_internal_navigation = (new_navigation_info.navigation_index > 0) &&
+
+  // Another case where we need to issue BeforeNavigate2 calls is as below:-
+  // We get notified after the fact, when navigations are initiated within
+  // Chrome via window.open calls. These navigations are handled by creating
+  // an external tab container within chrome and then connecting to it from IE.
+  // We still want to update the address bar/history, etc, to ensure that
+  // the special URL used by Chrome to indicate this is updated correctly.
+  bool is_internal_navigation = ((new_navigation_info.navigation_index > 0) &&
       (new_navigation_info.navigation_index !=
-       navigation_info_.navigation_index);
+       navigation_info_.navigation_index)) ||
+       StartsWith(static_cast<BSTR>(url_), kChromeAttachExternalTabPrefix,
+                  false);
 
   if (new_navigation_info.url.is_valid()) {
     url_.Allocate(UTF8ToWide(new_navigation_info.url.spec()).c_str());
@@ -830,6 +839,8 @@ bool ChromeActiveDocument::ParseUrl(const std::wstring& url,
 
 bool ChromeActiveDocument::LaunchUrl(const std::wstring& url,
                                      bool is_new_navigation) {
+  url_.Reset(::SysAllocString(url.c_str()));
+
   if (!is_new_navigation) {
     WStringTokenizer tokenizer(url, L"&");
     // Skip over kChromeAttachExternalTabPrefix
@@ -850,7 +861,6 @@ bool ChromeActiveDocument::LaunchUrl(const std::wstring& url,
   } else {
     // Initiate navigation before launching chrome so that the url will be
     // cached and sent with launch settings.
-    url_.Reset(::SysAllocString(url.c_str()));
     if (url_.Length()) {
       std::string utf8_url;
       WideToUTF8(url_, url_.Length(), &utf8_url);
diff --git a/chrome_frame/chrome_frame_activex_base.h b/chrome_frame/chrome_frame_activex_base.h
index a7fd6a0..d7abc01 100644
--- a/chrome_frame/chrome_frame_activex_base.h
+++ b/chrome_frame/chrome_frame_activex_base.h
@@ -408,7 +408,8 @@ END_MSG_MAP()
     if (ie_version == IE_6) {
       if (open_disposition == NEW_FOREGROUND_TAB ||
           open_disposition == NEW_BACKGROUND_TAB ||
-          open_disposition == NEW_WINDOW) {
+          open_disposition == NEW_WINDOW ||
+          open_disposition == NEW_POPUP) {
         V_I4(&flags) = navOpenInNewWindow;
       } else if (open_disposition != CURRENT_TAB) {
         NOTREACHED() << "Unsupported open disposition in IE6";
@@ -422,6 +423,7 @@ END_MSG_MAP()
           V_I4(&flags) = navOpenInBackgroundTab;
           break;
         case NEW_WINDOW:
+        case NEW_POPUP:
           V_I4(&flags) = navOpenInNewWindow;
           break;
         default:
diff --git a/chrome_frame/chrome_frame_npapi.cc b/chrome_frame/chrome_frame_npapi.cc
index 2dd3145..b2a8acb 100644
--- a/chrome_frame/chrome_frame_npapi.cc
+++ b/chrome_frame/chrome_frame_npapi.cc
@@ -500,6 +500,7 @@ void ChromeFrameNPAPI::OnOpenURL(int tab_handle,
       target = "_blank";
       break;
     case NEW_WINDOW:
+    case NEW_POPUP:
       target = "_new";
       break;
     default:
diff --git a/chrome_frame/test/chrome_frame_unittests.cc b/chrome_frame/test/chrome_frame_unittests.cc
index 383602c1..4d9b6c7 100644
--- a/chrome_frame/test/chrome_frame_unittests.cc
+++ b/chrome_frame/test/chrome_frame_unittests.cc
@@ -1311,7 +1311,10 @@ const wchar_t kChromeFrameFullTabWindowOpenPopupUrl[] =
 // This test checks if window.open calls issued by a full tab mode ChromeFrame
 // instance make it back to IE and then transitions back to Chrome as the
 // window.open target page is supposed to render within Chrome.
-TEST_F(ChromeFrameTestWithWebServer, DISABLED_FullTabModeIE_WindowOpen) {
+// Marking this test as FLAKY initially as it relies on getting focus and user
+// input which don't work correctly at times.
+// http://code.google.com/p/chromium/issues/detail?id=26549
+TEST_F(ChromeFrameTestWithWebServer, FLAKY_FullTabModeIE_WindowOpenInChrome) {
   chrome_frame_test::TimedMsgLoop loop;
   CComObjectStackEx<MockWebBrowserEventSink> mock;
 
@@ -1324,21 +1327,31 @@ TEST_F(ChromeFrameTestWithWebServer, DISABLED_FullTabModeIE_WindowOpen) {
       .WillOnce(testing::Return(S_OK));
   EXPECT_CALL(mock, OnNavigateComplete2(_, _))
       .WillOnce(testing::Return());
-  EXPECT_CALL(mock,
-              OnLoad(testing::StrEq(kChromeFrameFullTabWindowOpenTestUrl)))
-      .WillOnce(testing::Return());
 
   EXPECT_CALL(mock,
               OnBeforeNavigate2(
                   _, testing::Field(&VARIANT::bstrVal,
-                  testing::StrCaseEq(kChromeFrameFullTabWindowOpenPopupUrl)),
+                  testing::StrCaseEq(kChromeFrameFullTabWindowOpenTestUrl)),
                   _, _, _, _, _))
       .WillOnce(testing::Return(S_OK));
   EXPECT_CALL(mock, OnNavigateComplete2(_, _))
       .WillOnce(testing::Return());
+
   EXPECT_CALL(mock,
-              OnLoad(testing::StrEq(kChromeFrameFullTabWindowOpenPopupUrl)))
-      .WillOnce(QUIT_LOOP_SOON(loop, 2));
+              OnLoad(testing::StrEq(kChromeFrameFullTabWindowOpenTestUrl)))
+      .WillOnce(testing::DoAll(
+          testing::InvokeWithoutArgs(CreateFunctor(&mock,
+              &chrome_frame_test::WebBrowserEventSink::SetFocusToChrome)),
+          testing::InvokeWithoutArgs(CreateFunctor(&loop,
+              &chrome_frame_test::TimedMsgLoop::PostDelayedTask, FROM_HERE,
+              NewRunnableMethod(
+                  &mock,
+                  &chrome_frame_test::WebBrowserEventSink::SendInputToChrome,
+                  std::string("A")), 0))));
+
+  EXPECT_CALL(mock,
+              OnNewWindow3(_, _, _, _, _))
+      .WillOnce(QUIT_LOOP(loop));
 
   HRESULT hr = mock.LaunchIEAndNavigate(kChromeFrameFullTabWindowOpenTestUrl);
   ASSERT_HRESULT_SUCCEEDED(hr);
@@ -1349,10 +1362,9 @@ TEST_F(ChromeFrameTestWithWebServer, DISABLED_FullTabModeIE_WindowOpen) {
 
   loop.RunFor(kChromeFrameLongNavigationTimeoutInSeconds);
 
-  ASSERT_TRUE(CheckResultFile(L"ChromeFrameWindowOpenPopup", "OK"));
-
   mock.Uninitialize();
   chrome_frame_test::CloseAllIEWindows();
+  ASSERT_TRUE(CheckResultFile(L"ChromeFrameWindowOpenPopup", "OK"));
 }
 
 const wchar_t kSubFrameUrl1[] =
diff --git a/chrome_frame/test/data/chrome_frame_window_open.html b/chrome_frame/test/data/chrome_frame_window_open.html
index 8102d5c..e7194cf 100644
--- a/chrome_frame/test/data/chrome_frame_window_open.html
+++ b/chrome_frame/test/data/chrome_frame_window_open.html
@@ -12,18 +12,17 @@ function onLoad() {
   if (!TestIfRunningInChrome()) {
     onFailure("ChromeFrameWindowOpen", "Window Open failed :-(",
                 "User agent = " + navigator.userAgent.toLowerCase());
-  } else {
-    setTimeout(OpenPopup, 100);
   }
 }
 
 function OpenPopup() {
-  window.open("chrome_frame_window_open_popup.html", "_self");
+  window.open("chrome_frame_window_open_popup.html", "mywindow",
+              "left=10, top=10, height=100, width=100");
 }
 </script>
 </head>
 
-<body onload="onLoad();">
+<body onload="onLoad();" onkeypress="OpenPopup();">
 <div id="statusPanel" style="border: 1px solid red; width: 100%">
 ChromeFrame full tab mode window open test running....
 </div>