ExternalTabContainer should subclass WidgetWin rather than Widget and ATL CWi...

chrome/browser/external_tab_container.cc

Index: chrome/browser/external_tab_container.cc
===================================================================
--- chrome/browser/external_tab_container.cc	(revision 16974)
+++ chrome/browser/external_tab_container.cc	(working copy)
@@ -18,8 +18,6 @@
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/notification_service.h"
 #include "chrome/test/automation/automation_messages.h"
-// Included for SetRootViewForHWND.
-#include "views/widget/widget_win.h"
 
 static const wchar_t kWindowObjectKey[] = L"ChromeWindowObject";
 
@@ -29,7 +27,6 @@
 ExternalTabContainer::ExternalTabContainer(
     AutomationProvider* automation)
     : automation_(automation),
-      root_view_(this),
       tab_contents_(NULL),
       external_accel_table_(NULL),
       external_accel_entry_count_(0),
@@ -39,34 +36,27 @@
 }
 
 ExternalTabContainer::~ExternalTabContainer() {
-  Uninitialize(m_hWnd);
+  Uninitialize(GetNativeView());
 }
 
-bool ExternalTabContainer::Init(Profile* profile, HWND parent,
-                                const gfx::Rect& dimensions,
-                                unsigned int style) {
+bool ExternalTabContainer::Init(Profile* profile,
+                                HWND parent,
+                                const gfx::Rect& bounds,
+                                DWORD style) {
   if (IsWindow()) {
     NOTREACHED();
     return false;
   }
 
-  // First create the container window
-  if (!Create(NULL, dimensions.ToRECT())) {
-    NOTREACHED();
-    return false;
-  }
+  set_window_style(WS_POPUP);
+  views::WidgetWin::Init(parent, bounds, true);

[tommi (sloooow) - chröme, 2009/05/27 20:50:29]

this changes the semantics from before where NULL was the initial parent.  Also,
there's no error handling.

 
   // We don't ever remove the prop because the lifetime of this object
   // is the same as the lifetime of the window
-  SetProp(*this, kWindowObjectKey, this);
+  SetProp(GetNativeView(), kWindowObjectKey, this);
 
-  views::SetRootViewForHWND(m_hWnd, &root_view_);
-  // CreateFocusManager will subclass this window and delete the FocusManager
-  // instance when this window goes away.
   views::FocusManager* focus_manager =
-      views::FocusManager::CreateFocusManager(m_hWnd, GetRootView());
-
-  DCHECK(focus_manager);
+      views::FocusManager::GetFocusManager(GetNativeView());
   focus_manager->AddKeystrokeListener(this);
 
   tab_contents_ = new TabContents(profile, NULL, MSG_ROUTING_NONE, NULL);
@@ -76,7 +66,8 @@
   // Create a TabContentsContainer to handle focus cycling using Tab and
   // Shift-Tab.
   tab_contents_container_ = new TabContentsContainer;
-  root_view_.AddChildView(tab_contents_container_);
+  SetContentsView(tab_contents_container_);
+
   // Note that SetTabContents must be called after AddChildView is called
   tab_contents_container_->ChangeTabContents(tab_contents_);
 
@@ -98,58 +89,62 @@
   // Note that it's important to do this before we call SetParent since
   // during the SetParent call we will otherwise get a WA_ACTIVATE call
   // that causes us to steal the current focus.
-  ModifyStyle(WS_POPUP, style, 0);
+  SetWindowLong(GWL_STYLE, (GetWindowLong(GWL_STYLE) & ~WS_POPUP) | style);

[ananta, 2009/05/27 13:46:24]

Should this be GetWindowLong(GWL_STYLE) | WS_POPUP | style?

[tommi (sloooow) - chröme, 2009/05/27 20:50:29]

I think this is how it should be.  We want to remove the WS_POPUP style at this
point.

 
   // Now apply the parenting and style
   if (parent)
-    SetParent(parent);
+    SetParent(GetNativeView(), parent);
 
   ::ShowWindow(tab_contents_->GetNativeView(), SW_SHOWNA);
   return true;
 }
 
-bool ExternalTabContainer::Uninitialize(HWND window) {
-  if (::IsWindow(window)) {
-    views::FocusManager* focus_manager =
-        views::FocusManager::GetFocusManager(window);
-    if (focus_manager) {
-      focus_manager->RemoveKeystrokeListener(this);
-    }
-  }
+void ExternalTabContainer::SetAccelerators(HACCEL accel_table,
+                                           int accel_table_entry_count) {
+  external_accel_table_ = accel_table;
+  external_accel_entry_count_ = accel_table_entry_count;
+}
 
-  root_view_.RemoveAllChildViews(true);
-  if (tab_contents_) {
-    NotificationService::current()->Notify(
-        NotificationType::EXTERNAL_TAB_CLOSED,
-        Source<NavigationController>(&tab_contents_->controller()),
-        Details<ExternalTabContainer>(this));
+void ExternalTabContainer::ProcessUnhandledAccelerator(const MSG& msg) {
+  // We just received an accelerator key that we had sent to external host
+  // back. Since the external host was not interested in handling this, we
+  // need to dispatch this message as if we had just peeked this out. (we
+  // also need to call TranslateMessage to generate a WM_CHAR if needed).
+  TranslateMessage(&msg);
+  DispatchMessage(&msg);
+}
 
-    delete tab_contents_;
-    tab_contents_ = NULL;
+void ExternalTabContainer::SetInitialFocus(bool reverse) {
+  DCHECK(tab_contents_);
+  if (tab_contents_) {
+    static_cast<TabContents*>(tab_contents_)->Focus();
+    static_cast<TabContents*>(tab_contents_)->SetInitialFocus(reverse);
   }
-
-  return true;
 }
 
-void ExternalTabContainer::OnFinalMessage(HWND window) {
-  delete this;
+// static
+bool ExternalTabContainer::IsExternalTabContainer(HWND window) {
+  std::wstring class_name = win_util::GetClassName(window);
+  return _wcsicmp(class_name.c_str(), chrome::kExternalTabWindowClass) == 0;

[tommi (sloooow) - chröme, 2009/05/27 20:50:29]

maybe
return class_name.compare(chrome::kExternalTabWindowClass) == 0;

I think it's safe to be case sensitive here.

 }
 
-LRESULT ExternalTabContainer::OnSize(UINT, WPARAM, LPARAM, BOOL& handled) {
-  if (tab_contents_) {
-    RECT client_rect = {0};
-    GetClientRect(&client_rect);
-    ::SetWindowPos(tab_contents_->GetNativeView(), NULL, client_rect.left,
-                   client_rect.top, client_rect.right - client_rect.left,
-                   client_rect.bottom - client_rect.top, SWP_NOZORDER);
+// static
+ExternalTabContainer* ExternalTabContainer::GetContainerForTab(
+    HWND tab_window) {
+  HWND parent_window = ::GetParent(tab_window);
+  if (!::IsWindow(parent_window)) {
+    return NULL;
   }
-  return 0;
+  if (!IsExternalTabContainer(parent_window)) {
+    return NULL;
+  }
+  ExternalTabContainer* container = reinterpret_cast<ExternalTabContainer*>(
+      GetProp(parent_window, kWindowObjectKey));
+  return container;
 }
 
-LRESULT ExternalTabContainer::OnDestroy(UINT, WPARAM, LPARAM, BOOL& handled) {
-  Uninitialize(m_hWnd);
-  return 0;
-}
+////////////////////////////////////////////////////////////////////////////////
+// ExternalTabContainer, TabContentsDelegate implementation:
 
 void ExternalTabContainer::OpenURLFromTab(TabContents* source,
                            const GURL& url,
@@ -241,6 +236,12 @@
   }
 }
 
+ExtensionFunctionDispatcher* ExternalTabContainer::
+    CreateExtensionFunctionDispatcher(RenderViewHost* render_view_host,
+                                      const std::string& extension_id) {
+  return new ExtensionFunctionDispatcher(render_view_host, NULL, extension_id);
+}
+
 bool ExternalTabContainer::TakeFocus(bool reverse) {
   if (automation_) {
     views::FocusManager* focus_manager =
@@ -256,6 +257,9 @@
   return true;
 }
 
+////////////////////////////////////////////////////////////////////////////////
+// ExternalTabContainer, NotificationObserver implementation:
+
 void ExternalTabContainer::Observe(NotificationType type,
                                    const NotificationSource& source,
                                    const NotificationDetails& details) {
@@ -316,65 +320,17 @@
   }
 }
 
-void ExternalTabContainer::GetBounds(gfx::Rect* out,
-                                     bool including_frame) const {
-  CRect crect;
-  GetWindowRect(&crect);
-  *out = gfx::Rect(crect);
-}
+////////////////////////////////////////////////////////////////////////////////
+// ExternalTabContainer, views::WidgetWin overrides:
 
-void ExternalTabContainer::SetBounds(const gfx::Rect& bounds) {
-  SetBounds(bounds, NULL);
+void ExternalTabContainer::OnDestroy() {
+  Uninitialize(GetNativeView());
+  WidgetWin::OnDestroy();
 }
 
-void ExternalTabContainer::SetBounds(const gfx::Rect& bounds,
-                                     gfx::NativeView other_window) {
-  NOTIMPLEMENTED();
-}
+////////////////////////////////////////////////////////////////////////////////
+// ExternalTabContainer, views::KeystrokeListener implementation:
 
-void ExternalTabContainer::Close() {
-  NOTIMPLEMENTED();
-}
-
-void ExternalTabContainer::CloseNow() {
-  NOTIMPLEMENTED();
-}
-
-void ExternalTabContainer::Show() {
-  NOTIMPLEMENTED();
-}
-
-void ExternalTabContainer::Hide() {
-  NOTIMPLEMENTED();
-}
-
-gfx::NativeView ExternalTabContainer::GetNativeView() const {
-  return m_hWnd;
-}
-
-void ExternalTabContainer::PaintNow(const gfx::Rect& update_rect) {
-  RECT native_update_rect = update_rect.ToRECT();
-  RedrawWindow(&native_update_rect,
-               NULL,
-               RDW_INVALIDATE | RDW_ALLCHILDREN | RDW_NOERASE);
-}
-
-void ExternalTabContainer::SetOpacity(unsigned char opacity) {
-  NOTIMPLEMENTED();
-}
-
-views::RootView* ExternalTabContainer::GetRootView() {
-  return const_cast<views::RootView*>(&root_view_);
-}
-
-bool ExternalTabContainer::IsVisible() const {
-  return !!::IsWindowVisible(*this);
-}
-
-bool ExternalTabContainer::IsActive() const {
-  return win_util::IsWindowActive(*this);
-}
-
 bool ExternalTabContainer::ProcessKeyStroke(HWND window, UINT message,
                                             WPARAM wparam, LPARAM lparam) {
   if (!automation_) {
@@ -412,52 +368,25 @@
   return false;
 }
 
-void ExternalTabContainer::SetAccelerators(HACCEL accel_table,
-                                           int accel_table_entry_count) {
-  external_accel_table_ = accel_table;
-  external_accel_entry_count_ = accel_table_entry_count;
-}
+////////////////////////////////////////////////////////////////////////////////
+// ExternalTabContainer, private:
 
-void ExternalTabContainer::ProcessUnhandledAccelerator(const MSG& msg) {
-  // We just received an accelerator key that we had sent to external host
-  // back. Since the external host was not interested in handling this, we
-  // need to dispatch this message as if we had just peeked this out. (we
-  // also need to call TranslateMessage to generate a WM_CHAR if needed).
-  TranslateMessage(&msg);
-  DispatchMessage(&msg);
-}
+void ExternalTabContainer::Uninitialize(HWND window) {
+  if (::IsWindow(window)) {
+    views::FocusManager* focus_manager =
+        views::FocusManager::GetFocusManager(window);
+    if (focus_manager)
+      focus_manager->RemoveKeystrokeListener(this);
+  }
 
-void ExternalTabContainer::SetInitialFocus(bool reverse) {
-  DCHECK(tab_contents_);
   if (tab_contents_) {
-    static_cast<TabContents*>(tab_contents_)->Focus();
-    static_cast<TabContents*>(tab_contents_)->SetInitialFocus(reverse);
-  }
-}
+    NotificationService::current()->Notify(
+        NotificationType::EXTERNAL_TAB_CLOSED,
+        Source<NavigationController>(&tab_contents_->controller()),
+        Details<ExternalTabContainer>(this));
 
-// static
-bool ExternalTabContainer::IsExternalTabContainer(HWND window) {
-  std::wstring class_name = win_util::GetClassName(window);
-  return _wcsicmp(class_name.c_str(), chrome::kExternalTabWindowClass) == 0;
-}
-
-ExtensionFunctionDispatcher* ExternalTabContainer::
-    CreateExtensionFunctionDispatcher(RenderViewHost* render_view_host,
-                                      const std::string& extension_id) {
-  return new ExtensionFunctionDispatcher(render_view_host, NULL, extension_id);
-}
-
-// static
-ExternalTabContainer* ExternalTabContainer::GetContainerForTab(
-    HWND tab_window) {
-  HWND parent_window = ::GetParent(tab_window);
-  if (!::IsWindow(parent_window)) {
-    return NULL;
+    delete tab_contents_;
+    tab_contents_ = NULL;
   }
-  if (!IsExternalTabContainer(parent_window)) {
-    return NULL;
-  }
-  ExternalTabContainer* container = reinterpret_cast<ExternalTabContainer*>(
-      GetProp(parent_window, kWindowObjectKey));
-  return container;
 }
+