Make Web Intents picker in Views conform to latest mocks

chrome/browser/ui/views/constrained_window_views.h

Index: chrome/browser/ui/views/constrained_window_views.h
diff --git a/chrome/browser/ui/views/constrained_window_views.h b/chrome/browser/ui/views/constrained_window_views.h
index 97d5b6049c28080bead032b941a0d4f92e7537d6..c4be984628db27f42cbeabfa02adecfacf0018ac 100644
--- a/chrome/browser/ui/views/constrained_window_views.h
+++ b/chrome/browser/ui/views/constrained_window_views.h
@@ -8,6 +8,8 @@
 #include "base/compiler_specific.h"
 #include "chrome/browser/ui/constrained_window.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/notification_registrar.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_observer.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/gfx/rect.h"
 #include "ui/views/widget/widget.h"
@@ -59,11 +61,13 @@ class NativeConstrainedWindow {
 class ConstrainedWindowViews : public views::Widget,
                                public ConstrainedWindow,
                                public NativeConstrainedWindowDelegate,
-                               public content::WebContentsObserver {
+                               public content::WebContentsObserver,
+                               public content::NotificationObserver {
  public:
   ConstrainedWindowViews(content::WebContents* web_contents,
                          views::WidgetDelegate* widget_delegate,
-                         bool enable_chrome_style);
+                         bool enable_chrome_style,
+                         const gfx::Insets& client_insets);
   virtual ~ConstrainedWindowViews();
 
   // Returns the WebContents that constrains this Constrained Window.
@@ -92,20 +96,34 @@ class ConstrainedWindowViews : public views::Widget,
   virtual int GetNonClientComponent(const gfx::Point& point) OVERRIDE;
 
   // Set the top of the window to overlap the browser chrome.
-  void PositionChromeStyleWindow();
+  void PositionChromeStyleWindow(const gfx::Size& size);

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

Nit: While here it would be nice to put this above or below all the virtual
overrides.

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

Done.

 
   // Overridden from content::WebContentsObserver:
   virtual void WebContentsDestroyed(content::WebContents* web_contents)
       OVERRIDE;
 
-  content::WebContents* web_contents_;
+  // content::NotificationObserver method override.
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
 
-  NativeConstrainedWindow* native_constrained_window_;
+  // A scoped container for notification registries.

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

Nit: I'm not sure what "scoped" here means, and I don't think this comment says
anything important anyway.

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

There're 9 instances of this exact comment above aNotificationRegistrar member
in our code :-) . That's why I used that comment. I am removing the whole
comment now.

+  content::NotificationRegistrar registrar_;
+
+  content::WebContents* web_contents_;
 
   // TODO(wittman): remove once all constrained window dialogs are moved
   // over to Chrome style.
   const bool enable_chrome_style_;
 
+  // Client insets for use when |enable_chrome_style_| is true. Empty insets
+  // provide no window decoration. The insets from
+  // ConstrainedWindow::GetDefaultInsets() provide enough insets for the title
+  // and the close button to be visible.

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

Nit: What does "Empty insets provide no window decoration" mean?  If anything it
sounds like "we won't try to lay out a title or close button", but that doesn't
happen, let alone in this class, does it?

Should we even be talking about GetDefaultInsets() here?  That seems like the
province of callers.

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

I have removed the discussion of GetDefaultInsets() as you requested.

All the callers to ConstrainedWindowViews need to specify these insets for
themselves. Because WebIntentPickerViews needs to control its own header, it
specifies the empty insets gfx::Insets(). The empty insets cause the client to
be drawn on top of the non-client frame.

All other callers use ConstrainedWindowViews::GetDefaultInsets(). These default
insets show a 20px non-client border around the client, a text header, and a
close button.

[please use gerrit instead, 2012/10/16 00:15:59]

My last statement is ambigous. Here is a better phrasing: "These default insets
show a close button, a text header, and 20px non-client border around the
client."

+  const gfx::Insets client_insets_;
+
+  NativeConstrainedWindow* native_constrained_window_;
+
   DISALLOW_COPY_AND_ASSIGN(ConstrainedWindowViews);
 };