Make Web Intents picker in Views conform to latest mocks

chrome/browser/ui/views/constrained_window_views.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_UI_VIEWS_CONSTRAINED_WINDOW_VIEWS_H_
 #define CHROME_BROWSER_UI_VIEWS_CONSTRAINED_WINDOW_VIEWS_H_
 
 #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"
 
 namespace content {
 class WebContents;
 }
 namespace views {
 namespace internal {
 class NativeWidgetDelegate;
@@ skipping 31 matching lines @@
 ///////////////////////////////////////////////////////////////////////////////
 // ConstrainedWindowViews
 //
 //  A ConstrainedWindow implementation that implements a Constrained Window as
 //  a child HWND with a custom window frame. The ConstrainedWindowViews owns
 //  itself and will be deleted soon after being closed.
 //
 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.
   content::WebContents* owner() const { return web_contents_; }
 
   // Overridden from ConstrainedWindow:
   virtual void ShowConstrainedWindow() OVERRIDE;
   virtual void CloseConstrainedWindow() OVERRIDE;
   virtual void FocusConstrainedWindow() OVERRIDE;
   virtual gfx::NativeWindow GetNativeWindow() OVERRIDE;
 
   // Overridden from views::Widget:
   void CenterWindow(const gfx::Size& size);
 
  private:
   void NotifyTabHelperWillClose();
 
   // Overridden from views::Widget:
   virtual views::NonClientFrameView* CreateNonClientFrameView() OVERRIDE;
 
   // Overridden from NativeConstrainedWindowDelegate:
   virtual void OnNativeConstrainedWindowDestroyed() OVERRIDE;
   virtual void OnNativeConstrainedWindowMouseActivate() OVERRIDE;
   virtual views::internal::NativeWidgetDelegate*
       AsNativeWidgetDelegate() OVERRIDE;
   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::NotificationObserver method override.
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  // 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_;
 
-  NativeConstrainedWindow* native_constrained_window_;
-
   // 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);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_CONSTRAINED_WINDOW_VIEWS_H_