window.open calls issued by pages within ChromeFrame would not honor …

…the suggested dimensions and would end up

opening a default top level browser window in IE. 

ChromeFrame does receive the dimensions from the external tab container when it is notified about a popup being
opened. 

Fix is to honor these dimensions by passing them off in the specially crafted url containing other arguments.

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

This fix is currently implemented for IE full tab mode only.

Bug=42250
Test=Covered by augmenting the existing window open test to also validate the window size. Added a new unit test
     to test the ParseAttachExternalTabUrl helper function.

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

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

chrome_frame/chrome_active_document.cc

@@ -65,7 +65,8 @@ ChromeActiveDocument::ChromeActiveDocument()
     : first_navigation_(true),
       is_automation_client_reused_(false),
       popup_allowed_(false),
-      accelerator_table_(NULL) {
+      accelerator_table_(NULL),
+      is_new_navigation_(false) {
   TRACE_EVENT_BEGIN("chromeframe.createactivedocument", this, "");
 
   url_fetcher_.set_frame_busting(false);
@@ -519,6 +520,23 @@ HRESULT ChromeActiveDocument::ActiveXDocActivate(LONG verb) {
       } else {
         m_hWnd = Create(parent_window, position_rect, 0, 0, WS_EX_CLIENTEDGE);
       }
+
+      ScopedComPtr<IWebBrowser2> web_browser2;
+      DoQueryService(SID_SWebBrowserApp, m_spClientSite,
+                     web_browser2.Receive());
+      if (web_browser2) {
+        if (!dimensions_.IsEmpty()) {
+          web_browser2->put_Width(dimensions_.width());
+          web_browser2->put_Height(dimensions_.height());
+          web_browser2->put_Left(dimensions_.x());
+          web_browser2->put_Top(dimensions_.y());
+          web_browser2->put_MenuBar(VARIANT_FALSE);
+          web_browser2->put_ToolBar(VARIANT_FALSE);
+
+          dimensions_.set_height(0);
+          dimensions_.set_width(0);
+        }
+      }
     }
     SetObjectRects(&position_rect, &clip_rect);
   }
@@ -976,23 +994,24 @@ bool ChromeActiveDocument::LaunchUrl(const std::wstring& url,
   std::string utf8_url;
 
   if (!is_new_navigation) {
-    WStringTokenizer tokenizer(url, L"&");
-    // Skip over kChromeAttachExternalTabPrefix
-    tokenizer.GetNext();
-
+    int disposition = 0;
     uint64 external_tab_cookie = 0;
-    if (tokenizer.GetNext()) {
-      wchar_t* end_ptr = 0;
-      external_tab_cookie = _wcstoui64(tokenizer.token().c_str(), &end_ptr, 10);
+
+    if (!ParseAttachExternalTabUrl(url, &external_tab_cookie, &dimensions_,
+                                   &disposition)) {
+      NOTREACHED() << "Failed to parse attach tab url:" << url;
+      return false;
     }
 
     if (external_tab_cookie == 0) {
       NOTREACHED() << "invalid url for attach tab: " << url;
       return false;
     }
 
+    is_new_navigation_ = false;
     automation_client_->AttachExternalTab(external_tab_cookie);
   } else {
+    is_new_navigation_ = true;
     // Initiate navigation before launching chrome so that the url will be
     // cached and sent with launch settings.
     if (url_.Length()) {

chrome_frame/chrome_active_document.h

@@ -454,6 +454,10 @@ END_EXEC_COMMAND_MAP()
   UrlmonUrlRequestManager::PrivacyInfo::PrivacyRecords::iterator
       next_privacy_record_;
 
+  // Dimensions of the window. Used only when opening popups.
+  gfx::Rect dimensions_;
+  // Set to true if the document was loaded by a window.open in chrome.
+  bool is_new_navigation_;
  public:
   ScopedComPtr<IOleInPlaceFrame> in_place_frame_;
   OLEINPLACEFRAMEINFO frame_info_;

chrome_frame/chrome_frame_activex_base.h

@@ -444,8 +444,14 @@ END_MSG_MAP()
   virtual void OnAttachExternalTab(int tab_handle,
       const IPC::AttachExternalTabParams& params) {
     std::string url;
-    url = StringPrintf("%lsattach_external_tab&%ls&%d", kChromeProtocolPrefix,
-        Uint64ToWString(params.cookie).c_str(), params.disposition);
+    url = StringPrintf("%lsattach_external_tab&%ls&%d&%d&%d&%d&%d",
+                       kChromeProtocolPrefix,
+                       Uint64ToWString(params.cookie).c_str(),
+                       params.disposition,
+                       params.dimensions.x(),
+                       params.dimensions.y(),
+                       params.dimensions.width(),
+                       params.dimensions.height());
     HostNavigate(GURL(url), GURL(), params.disposition);
   }
 

chrome_frame/test/data/chrome_frame_window_open.html

@@ -18,8 +18,8 @@
 var new_window;
 
 function OpenPopup() {
-  new_window = window.open("chrome_frame_window_open_popup.html", "mywindow",
-                           "left=10, top=10, height=100, width=100");
+  new_window = window.open("chrome_frame_window_open_popup.html", "_blank",
+                           "left=10, top=10, height=250, width=250");
 }
 
 function OnKeyPress() {

chrome_frame/test/test_mock_with_web_server.cc

@@ -371,6 +371,7 @@ TEST_F(ChromeFrameTestWithWebServer, FLAKY_FullTabModeIE_WindowOpenInChrome) {
   EXPECT_CALL(new_window_mock, OnLoad(testing::StrCaseEq(kWindowOpenPopupUrl)))
       .WillOnce(testing::DoAll(
           VerifyAddressBarUrl(&new_window_mock),
+          ValidateWindowSize(&new_window_mock, 10, 10, 250, 250),
           CloseBrowserMock(&new_window_mock)));
 
   EXPECT_CALL(new_window_mock, OnQuit())
@@ -389,6 +390,8 @@ TEST_F(ChromeFrameTestWithWebServer, FLAKY_FullTabModeIE_WindowOpenInChrome) {
   ASSERT_TRUE(mock.web_browser2() != NULL);
 
   loop.RunFor(kChromeFrameLongNavigationTimeoutInSeconds);
+
+  ASSERT_TRUE(new_window_mock.web_browser2() != NULL);
 }
 
 const wchar_t kSubFrameUrl1[] =

chrome_frame/test/test_mock_with_web_server.h

@@ -106,6 +106,23 @@ ACTION_P4(DelaySendScanCode, loop, delay, c, mod) {
         simulate_input::SendScanCode, c, mod), delay);
 }
 
+ACTION_P5(ValidateWindowSize, mock, left, top, width, height) {
+  long actual_left = 0;
+  long actual_top = 0;
+  long actual_width = 0;
+  long actual_height = 0;
+
+  mock->web_browser2()->get_Left(&actual_left);
+  mock->web_browser2()->get_Top(&actual_top);
+  mock->web_browser2()->get_Width(&actual_width);
+  mock->web_browser2()->get_Height(&actual_height);
+
+  EXPECT_EQ(actual_left, left);
+  EXPECT_EQ(actual_top, top);
+  EXPECT_EQ(actual_width, width);
+  EXPECT_EQ(actual_height, height);
+}
+
 }  // namespace chrome_frame_test
 
 #endif  // CHROME_FRAME_TEST_MOCK_WITH_WEB_SERVER_H_

chrome_frame/test/util_unittests.cc

@@ -112,3 +112,28 @@ TEST(UtilTests, GetTempInternetFiles) {
   FilePath path = GetIETemporaryFilesFolder();
   EXPECT_FALSE(path.empty());
 }
+
+TEST(UtilTests, ParseAttachTabUrlTest) {
+  std::wstring url = L"attach_external_tab&10&1&0&0&100&100";
+
+  uint64 cookie = 0;
+  gfx::Rect dimensions;
+  int disposition = 0;
+
+  EXPECT_TRUE(ParseAttachExternalTabUrl(url, &cookie, &dimensions,
+                                        &disposition));
+  EXPECT_EQ(10, cookie);
+  EXPECT_EQ(1, disposition);
+  EXPECT_EQ(0, dimensions.x());
+  EXPECT_EQ(0, dimensions.y());
+  EXPECT_EQ(100, dimensions.width());
+  EXPECT_EQ(100, dimensions.height());
+
+  url = L"http://www.foobar.com?&10&1&0&0&100&100";
+  EXPECT_FALSE(ParseAttachExternalTabUrl(url, &cookie, &dimensions,
+                                         &disposition));
+  url = L"attach_external_tab&10&1";
+  EXPECT_FALSE(ParseAttachExternalTabUrl(url, &cookie, &dimensions,
+                                         &disposition));
+}
+

chrome_frame/utils.cc

@@ -16,6 +16,7 @@
 #include "base/scoped_bstr_win.h"
 #include "base/scoped_comptr_win.h"
 #include "base/scoped_variant_win.h"
+#include "base/string_tokenizer.h"
 #include "base/string_util.h"
 #include "base/thread_local.h"
 #include "base/utf_string_conversions.h"
@@ -1213,3 +1214,65 @@ HRESULT ReadStream(IStream* stream, size_t size, std::string* data) {
   return hr;
 }
 
+bool ParseAttachExternalTabUrl(const std::wstring& url, uint64* cookie,
+                               gfx::Rect* dimensions, int* disposition) {
+  if (!StartsWith(url, kChromeAttachExternalTabPrefix, true)) {
+    DLOG(WARNING) << "Invalid url passed in:"
+                  << url.c_str();
+    return false;
+  }
+
+  if (!cookie || !dimensions || !disposition)
+    return false;
+
+  WStringTokenizer tokenizer(url, L"&");
+  // Skip over kChromeAttachExternalTabPrefix
+  tokenizer.GetNext();
+
+  // Read the following items in order.
+  // 1. cookie
+  // 2. disposition
+  // 3. dimension.x
+  // 4. dimension.y
+  // 5. dimension.width
+  // 6. dimension.height.
+  if (tokenizer.GetNext()) {
+    wchar_t* end_ptr = 0;
+    *cookie = _wcstoui64(tokenizer.token().c_str(), &end_ptr, 10);
+  } else {
+    return false;
+  }
+
+  if (tokenizer.GetNext()) {
+    *disposition = _wtoi(tokenizer.token().c_str());
+  } else {
+    return false;
+  }
+
+  if (tokenizer.GetNext()) {
+    dimensions->set_x(_wtoi(tokenizer.token().c_str()));
+  } else {
+    return false;
+  }
+
+  if (tokenizer.GetNext()) {
+    dimensions->set_y(_wtoi(tokenizer.token().c_str()));
+  } else {
+    return false;
+  }
+
+  if (tokenizer.GetNext()) {
+    dimensions->set_width(_wtoi(tokenizer.token().c_str()));
+  } else {
+    return false;
+  }
+
+  if (tokenizer.GetNext()) {
+    dimensions->set_height(_wtoi(tokenizer.token().c_str()));
+  } else {
+    return false;
+  }
+
+  return true;
+}
+

chrome_frame/utils.h

@@ -18,7 +18,7 @@
 #include "base/lock.h"
 #include "base/logging.h"
 #include "base/thread.h"
-
+#include "gfx/rect.h"
 #include "googleurl/src/gurl.h"
 
 // utils.h : Various utility functions and classes
@@ -464,4 +464,13 @@ std::string Bscf2Str(DWORD flags);
 // Reads data from a stream into a string.
 HRESULT ReadStream(IStream* stream, size_t size, std::string* data);
 
+// Parses the attach external tab url, which comes in from Chrome in the course
+// of a window.open operation. The format of this URL is as below:-
+// gcf:attach_external_tab&n1&n2&x&y&width&height
+// n1 -> cookie, n2 -> disposition, x, y, width, height -> dimensions of the
+// window.
+// Returns true on success.
+bool ParseAttachExternalTabUrl(const std::wstring& url, uint64* cookie,
+                               gfx::Rect* dimensions, int* disposition);
+
 #endif  // CHROME_FRAME_UTILS_H_