Make Web Intents picker in Views conform to latest mocks

chrome/browser/ui/views/constrained_window_views.cc

Index: chrome/browser/ui/views/constrained_window_views.cc
diff --git a/chrome/browser/ui/views/constrained_window_views.cc b/chrome/browser/ui/views/constrained_window_views.cc
index 2aaf122a177c2f81929b7e117daa11edd070a1e9..f3199349657aadbe03c7c074f7f4bad98a6184fb 100644
--- a/chrome/browser/ui/views/constrained_window_views.cc
+++ b/chrome/browser/ui/views/constrained_window_views.cc
@@ -23,6 +23,10 @@
 #include "chrome/common/chrome_switches.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_view.h"
+#include "content/public/browser/navigation_controller.h"

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Alphabetical order

[please use gerrit instead, 2012/10/16 00:12:23]

Done.

+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_source.h"
+#include "content/public/browser/notification_types.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
@@ -581,12 +585,14 @@ class ConstrainedWindowFrameViewAsh : public ash::CustomFrameViewAsh {
 ConstrainedWindowViews::ConstrainedWindowViews(
     content::WebContents* web_contents,
     views::WidgetDelegate* widget_delegate,
-    bool enable_chrome_style)
+    bool enable_chrome_style,
+    const gfx::Insets& client_insets)
     : WebContentsObserver(web_contents),
       web_contents_(web_contents),
+      enable_chrome_style_(enable_chrome_style),
+      client_insets_(client_insets),
       ALLOW_THIS_IN_INITIALIZER_LIST(native_constrained_window_(
-          NativeConstrainedWindow::CreateNativeConstrainedWindow(this))),
-      enable_chrome_style_(enable_chrome_style) {
+          NativeConstrainedWindow::CreateNativeConstrainedWindow(this))) {
   views::Widget::InitParams params(views::Widget::InitParams::TYPE_WINDOW);
   params.delegate = widget_delegate;
   params.native_widget = native_constrained_window_->AsNativeWidget();
@@ -594,9 +600,9 @@ ConstrainedWindowViews::ConstrainedWindowViews(
 
   if (enable_chrome_style_) {
     params.parent_widget = Widget::GetTopLevelWidgetForNativeView(
-        web_contents->GetView()->GetNativeView());
+        web_contents_->GetView()->GetNativeView());
   } else {
-    params.parent = web_contents->GetNativeView();
+    params.parent = web_contents_->GetNativeView();
   }
 
 #if defined(USE_ASH)
@@ -622,7 +628,10 @@ ConstrainedWindowViews::ConstrainedWindowViews(
       if (dialog_client_view)
         dialog_client_view->set_background(background);
     }
-    PositionChromeStyleWindow();
+    PositionChromeStyleWindow(GetRootView()->bounds().size());
+    registrar_.Add(this,
+                   content::NOTIFICATION_WEB_CONTENTS_VISIBILITY_CHANGED,
+                   content::Source<content::WebContents>(web_contents));
   }
 
   ConstrainedWindowTabHelper* constrained_window_tab_helper =
@@ -688,14 +697,22 @@ void ConstrainedWindowViews::NotifyTabHelperWillClose() {
 // ConstrainedWindowViews, views::Widget overrides:
 
 void ConstrainedWindowViews::CenterWindow(const gfx::Size& size) {
-  Widget::CenterWindow(size);
   if (enable_chrome_style_)
-    PositionChromeStyleWindow();
+    PositionChromeStyleWindow(size);
+  else
+    Widget::CenterWindow(size);
 }
 
 views::NonClientFrameView* ConstrainedWindowViews::CreateNonClientFrameView() {
   if (enable_chrome_style_) {
-    return new ConstrainedWindowFrameSimple(this);
+    ConstrainedWindowFrameSimple* frame =
+        new ConstrainedWindowFrameSimple(this);
+    frame->set_border(views::Border::CreateEmptyBorder(
+        client_insets_.top(),
+        client_insets_.left(),
+        client_insets_.bottom(),
+        client_insets_.right()));
+    return frame;
   } else {

[Peter Kasting, 2012/10/15 21:46:54]

Nit: No else after return

[please use gerrit instead, 2012/10/16 00:12:23]

Done.

 #if defined(USE_ASH)
     CommandLine* command_line = CommandLine::ForCurrentProcess();
@@ -730,21 +747,21 @@ int ConstrainedWindowViews::GetNonClientComponent(const gfx::Point& point) {
   return HTNOWHERE;
 }
 
-void ConstrainedWindowViews::PositionChromeStyleWindow() {
+void ConstrainedWindowViews::PositionChromeStyleWindow(const gfx::Size& size) {
   DCHECK(enable_chrome_style_);
-  gfx::Rect bounds = GetRootView()->bounds();
+  gfx::Rect bounds(GetRootView()->bounds());
+  bounds.set_size(size);

[Peter Kasting, 2012/10/15 21:46:54]

Nit: How about:

  gfx::rect bounds(GetRootView()->bounds().origin(), size);

[please use gerrit instead, 2012/10/16 00:12:23]

That's a cool trick, thanks! Done.

   ConstrainedWindowTabHelperDelegate* tab_helper_delegate =
       ConstrainedWindowTabHelper::FromWebContents(web_contents_)->delegate();
-
   BrowserWindow* browser_window =
       tab_helper_delegate ? tab_helper_delegate->GetBrowserWindow() : NULL;
   int top_y;

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Declare this inside the conditional

[please use gerrit instead, 2012/10/16 00:12:23]

Done.

-  if (browser_window && browser_window->GetConstrainedWindowTopY(&top_y)) {
-    bounds.set_y(top_y);
-    bounds.set_x(
-        browser_window->GetBounds().width() / 2 - bounds.width() / 2);
-    SetBounds(bounds);
+  if (browser_window) {
+    bounds.set_x(browser_window->GetBounds().width() / 2 - bounds.width() / 2);
+    if (browser_window->GetConstrainedWindowTopY(&top_y))
+      bounds.set_y(top_y);
   }
+  SetBounds(bounds);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -754,3 +771,19 @@ void ConstrainedWindowViews::WebContentsDestroyed(
     content::WebContents* web_contents) {
   web_contents_ = NULL;
 }
+
+////////////////////////////////////////////////////////////////////////////////
+// ConstrainedWindowViews, content::NotificationObserver implementation:

[Peter Kasting, 2012/10/15 21:46:54]

Nit: This practice of separating each group of overrides with its own section
header is fairly unusual... I'd remove all of these

[please use gerrit instead, 2012/10/16 00:12:23]

Removed all //////////////. Done.

+
+void ConstrainedWindowViews::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  if (enable_chrome_style_ &&
+      type == content::NOTIFICATION_WEB_CONTENTS_VISIBILITY_CHANGED) {

[Peter Kasting, 2012/10/15 21:46:54]

Nit: DCHECK_EQ that |type| is this value atop the function

[please use gerrit instead, 2012/10/16 00:12:23]

I've added DCHECK(enable_chrome_style_) and DHCECK_EQ(type, ...) on top and
removed the if-statement below. Done.

+    if (*content::Details<bool>(details).ptr())
+      Show();
+    else
+      Hide();
+  }
+}