Fix DialogClientView layout, buttons, accelerators; etc.

Rename Get[Extra|Footnote]View to Create*; only call once per dialog.
Clamp 'extra view' size and align left in the button row.
Simplify 'extra view' users: remove containers/layout, init on demand.

Remove unused GetSizeExtraViewHeightToButtons / IsDialogButtonVisible / GetFootnoteViewHeight.

Remove non-functional PaintSizeBox code and broken |size_box_bounds_|.
Move meaningful StyleParams distinctions to GetInsets().
(temporarily regresses new dialog style resizing and button spacing)

Get default button text in DialogDelegateView::GetDialogButtonLabel.
Return superclass GetDialogButtonLabel in OVERRIDES, not string16().

Simplify FocusChangeListener add/remove for default button wrangling.
Add TreeView return key accelerator handling to commit pending edits.
Remove AreAcceleratorsEnabled (was for committing tree view, edits)

Fix [Dialog|Widget]Example's init; add extra/footnote views.

TODO(followup): Restore dialog resizing via frame view hit testing.
TODO(followup): Fix new dialog button spacing via layout constants.

BUG=166075
TEST=No observable dialog appearance/behavior changes (except minor button spacing change for experimental --enable-new-dialog-style)
R=sky@chromium.org

Review URL: https://chromiumcodereview.appspot.com/12259021

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

13 files changed, 44 insertions, 128 deletions

diff --git a/chrome/browser/ui/views/autofill/autofill_dialog_views.cc b/chrome/browser/ui/views/autofill/autofill_dialog_views.cc
index 8604d56..53a1588 100644
--- a/chrome/browser/ui/views/autofill/autofill_dialog_views.cc
+++ b/chrome/browser/ui/views/autofill/autofill_dialog_views.cc
@@ -540,11 +540,11 @@ bool AutofillDialogViews::IsDialogButtonEnabled(ui::DialogButton button) const {
   return true;
 }
 
-views::View* AutofillDialogViews::GetExtraView() {
+views::View* AutofillDialogViews::CreateExtraView() {
   return button_strip_extra_view_;
 }
 
-views::View* AutofillDialogViews::GetFootnoteView() {
+views::View* AutofillDialogViews::CreateFootnoteView() {
   // TODO(estade): add a view to contain the terms of service.
   return NULL;
 }
diff --git a/chrome/browser/ui/views/autofill/autofill_dialog_views.h b/chrome/browser/ui/views/autofill/autofill_dialog_views.h
index d669492..9078c42 100644
--- a/chrome/browser/ui/views/autofill/autofill_dialog_views.h
+++ b/chrome/browser/ui/views/autofill/autofill_dialog_views.h
@@ -89,8 +89,8 @@ class AutofillDialogViews : public AutofillDialogView,
   virtual views::View* GetContentsView() OVERRIDE;
   virtual string16 GetDialogButtonLabel(ui::DialogButton button) const OVERRIDE;
   virtual bool IsDialogButtonEnabled(ui::DialogButton button) const OVERRIDE;
-  virtual views::View* GetExtraView() OVERRIDE;
-  virtual views::View* GetFootnoteView() OVERRIDE;
+  virtual views::View* CreateExtraView() OVERRIDE;
+  virtual views::View* CreateFootnoteView() OVERRIDE;
   virtual bool Cancel() OVERRIDE;
   virtual bool Accept() OVERRIDE;
 
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc b/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc
index 2447b07..f3bfc06 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc
@@ -90,7 +90,7 @@ string16 BookmarkEditorView::GetDialogButtonLabel(
     ui::DialogButton button) const {
   if (button == ui::DIALOG_BUTTON_OK)
     return l10n_util::GetStringUTF16(IDS_SAVE);
-  return string16();
+  return views::DialogDelegateView::GetDialogButtonLabel(button);
 }
 
 bool BookmarkEditorView::IsDialogButtonEnabled(ui::DialogButton button) const {
@@ -104,6 +104,10 @@ bool BookmarkEditorView::IsDialogButtonEnabled(ui::DialogButton button) const {
   return true;
 }
 
+views::View* BookmarkEditorView::CreateExtraView() {
+  return new_folder_button_.get();
+}
+
 ui::ModalType BookmarkEditorView::GetModalType() const {
   return ui::MODAL_TYPE_WINDOW;
 }
@@ -130,25 +134,6 @@ bool BookmarkEditorView::Accept() {
   return true;
 }
 
-bool BookmarkEditorView::AreAcceleratorsEnabled(ui::DialogButton button) {
-  return !show_tree_ || !tree_view_->GetEditingNode();
-}
-
-void BookmarkEditorView::Layout() {
-  GetLayoutManager()->Layout(this);
-
-  if (!show_tree_)
-    return;
-
-  // Manually align the New Folder button with the Save and Cancel buttons.
-  gfx::Size size = new_folder_button_->GetPreferredSize();
-  gfx::Rect parent_bounds = parent()->GetContentsBounds();
-  int x = views::DialogDelegate::UseNewStyle() ? 0 : views::kPanelHorizMargin;
-  int y = views::DialogDelegate::UseNewStyle() ? GetLocalBounds().bottom() :
-      parent_bounds.bottom() - size.height() - views::kButtonVEdgeMargin;
-  new_folder_button_->SetBounds(x, y, size.width(), size.height());
-}
-
 gfx::Size BookmarkEditorView::GetPreferredSize() {
   if (!show_tree_)
     return views::View::GetPreferredSize();
@@ -158,19 +143,6 @@ gfx::Size BookmarkEditorView::GetPreferredSize() {
       IDS_EDITBOOKMARK_DIALOG_HEIGHT_LINES));
 }
 
-void BookmarkEditorView::ViewHierarchyChanged(bool is_add,
-                                              views::View* parent,
-                                              views::View* child) {
-  if (show_tree_ && child == this) {
-    // Add and remove the New Folder button from the ClientView's hierarchy.
-    if (is_add) {
-      parent->AddChildView(new_folder_button_.get());
-    } else {
-      parent->RemoveChildView(new_folder_button_.get());
-    }
-  }
-}
-
 void BookmarkEditorView::OnTreeViewSelectionChanged(
     views::TreeView* tree_view) {
 }
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_editor_view.h b/chrome/browser/ui/views/bookmarks/bookmark_editor_view.h
index d1c4ff0..7c99e9b 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_editor_view.h
+++ b/chrome/browser/ui/views/bookmarks/bookmark_editor_view.h
@@ -80,18 +80,14 @@ class BookmarkEditorView : public BookmarkEditor,
   // views::DialogDelegateView:
   virtual string16 GetDialogButtonLabel(ui::DialogButton button) const OVERRIDE;
   virtual bool IsDialogButtonEnabled(ui::DialogButton button) const OVERRIDE;
+  virtual views::View* CreateExtraView() OVERRIDE;
   virtual ui::ModalType GetModalType() const OVERRIDE;
   virtual bool CanResize() const  OVERRIDE;
   virtual string16 GetWindowTitle() const  OVERRIDE;
   virtual bool Accept() OVERRIDE;
-  virtual bool AreAcceleratorsEnabled(ui::DialogButton button) OVERRIDE;
 
   // views::View:
-  virtual void Layout() OVERRIDE;
   virtual gfx::Size GetPreferredSize() OVERRIDE;
-  virtual void ViewHierarchyChanged(bool is_add,
-                                    views::View* parent,
-                                    views::View* child) OVERRIDE;
 
   // views::TreeViewController:
   virtual void OnTreeViewSelectionChanged(views::TreeView* tree_view) OVERRIDE;
diff --git a/chrome/browser/ui/views/confirm_bubble_views.cc b/chrome/browser/ui/views/confirm_bubble_views.cc
index 399a078..9766599 100644
--- a/chrome/browser/ui/views/confirm_bubble_views.cc
+++ b/chrome/browser/ui/views/confirm_bubble_views.cc
@@ -62,10 +62,6 @@ string16 ConfirmBubbleViews::GetDialogButtonLabel(
 }
 
 bool ConfirmBubbleViews::IsDialogButtonEnabled(ui::DialogButton button) const {
-  return IsDialogButtonVisible(button);
-}
-
-bool ConfirmBubbleViews::IsDialogButtonVisible(ui::DialogButton button) const {
   switch (button) {
     case ui::DIALOG_BUTTON_OK:
       return !!(model_->GetButtons() & ConfirmBubbleModel::BUTTON_OK);
@@ -77,7 +73,7 @@ bool ConfirmBubbleViews::IsDialogButtonVisible(ui::DialogButton button) const {
   }
 }
 
-views::View* ConfirmBubbleViews::GetExtraView() {
+views::View* ConfirmBubbleViews::CreateExtraView() {
   return link_;
 }
 
diff --git a/chrome/browser/ui/views/confirm_bubble_views.h b/chrome/browser/ui/views/confirm_bubble_views.h
index aa6e06c..17f27b7 100644
--- a/chrome/browser/ui/views/confirm_bubble_views.h
+++ b/chrome/browser/ui/views/confirm_bubble_views.h
@@ -32,8 +32,7 @@ class ConfirmBubbleViews : public views::DialogDelegateView,
   // views::DialogDelegate implementation.
   virtual string16 GetDialogButtonLabel(ui::DialogButton button) const OVERRIDE;
   virtual bool IsDialogButtonEnabled(ui::DialogButton button) const OVERRIDE;
-  virtual bool IsDialogButtonVisible(ui::DialogButton button) const OVERRIDE;
-  virtual views::View* GetExtraView() OVERRIDE;
+  virtual views::View* CreateExtraView() OVERRIDE;
   virtual bool Cancel() OVERRIDE;
   virtual bool Accept() OVERRIDE;
 
diff --git a/chrome/browser/ui/views/create_application_shortcut_view.cc b/chrome/browser/ui/views/create_application_shortcut_view.cc
index d690e33..c77e983 100644
--- a/chrome/browser/ui/views/create_application_shortcut_view.cc
+++ b/chrome/browser/ui/views/create_application_shortcut_view.cc
@@ -321,7 +321,7 @@ string16 CreateApplicationShortcutView::GetDialogButtonLabel(
     ui::DialogButton button) const {
   if (button == ui::DIALOG_BUTTON_OK)
     return l10n_util::GetStringUTF16(IDS_CREATE_SHORTCUTS_COMMIT);
-  return string16();
+  return views::DialogDelegateView::GetDialogButtonLabel(button);
 }
 
 bool CreateApplicationShortcutView::IsDialogButtonEnabled(
diff --git a/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc b/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc
index d936a0b..0570066 100644
--- a/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc
+++ b/chrome/browser/ui/views/extensions/media_galleries_dialog_views.cc
@@ -30,7 +30,7 @@ MediaGalleriesDialogViews::MediaGalleriesDialogViews(
       window_(NULL),
       contents_(new views::View()),
       checkbox_container_(NULL),
-      add_gallery_container_(NULL),
+      add_gallery_button_(NULL),
       confirm_available_(false),
       accepted_(false) {
   InitChildViews();
@@ -173,17 +173,11 @@ ui::ModalType MediaGalleriesDialogViews::GetModalType() const {
 #endif
 }
 
-views::View* MediaGalleriesDialogViews::GetExtraView() {
-  if (!add_gallery_container_) {
-    views::View* button = new views::NativeTextButton(this,
-        l10n_util::GetStringUTF16(IDS_MEDIA_GALLERIES_DIALOG_ADD_GALLERY));
-    add_gallery_container_ = new views::View();
-    add_gallery_container_->SetLayoutManager(
-        new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0));
-    add_gallery_container_->AddChildView(button);
-  }
-
-  return add_gallery_container_;
+views::View* MediaGalleriesDialogViews::CreateExtraView() {
+  DCHECK(!add_gallery_button_);
+  add_gallery_button_ = new views::NativeTextButton(this,
+      l10n_util::GetStringUTF16(IDS_MEDIA_GALLERIES_DIALOG_ADD_GALLERY));
+  return add_gallery_button_;
 }
 
 bool MediaGalleriesDialogViews::Cancel() {
@@ -200,7 +194,7 @@ void MediaGalleriesDialogViews::ButtonPressed(views::Button* sender,
   confirm_available_ = true;
   GetWidget()->client_view()->AsDialogClientView()->UpdateDialogButtons();
 
-  if (sender->parent() == add_gallery_container_) {
+  if (sender == add_gallery_button_) {
     controller_->OnAddFolderClicked();
     return;
   }
diff --git a/chrome/browser/ui/views/extensions/media_galleries_dialog_views.h b/chrome/browser/ui/views/extensions/media_galleries_dialog_views.h
index ebb9e2e..f320973 100644
--- a/chrome/browser/ui/views/extensions/media_galleries_dialog_views.h
+++ b/chrome/browser/ui/views/extensions/media_galleries_dialog_views.h
@@ -16,6 +16,7 @@ class ConstrainedWindowViews;
 
 namespace views {
 class Checkbox;
+class TextButton;
 }
 
 namespace chrome {
@@ -44,7 +45,7 @@ class MediaGalleriesDialogViews : public MediaGalleriesDialog,
   virtual string16 GetDialogButtonLabel(ui::DialogButton button) const OVERRIDE;
   virtual bool IsDialogButtonEnabled(ui::DialogButton button) const OVERRIDE;
   virtual ui::ModalType GetModalType() const OVERRIDE;
-  virtual views::View* GetExtraView() OVERRIDE;
+  virtual views::View* CreateExtraView() OVERRIDE;
   virtual bool Cancel() OVERRIDE;
   virtual bool Accept() OVERRIDE;
 
@@ -73,7 +74,7 @@ class MediaGalleriesDialogViews : public MediaGalleriesDialog,
   CheckboxMap checkbox_map_;
 
   views::View* checkbox_container_;
-  views::View* add_gallery_container_;
+  views::TextButton* add_gallery_button_;
 
   // This tracks whether the confirm button can be clicked. It starts as false
   // if no checkboxes are ticked. After there is any interaction, or some
diff --git a/chrome/browser/ui/views/hung_renderer_view.cc b/chrome/browser/ui/views/hung_renderer_view.cc
index abb92f4..b54f77e 100644
--- a/chrome/browser/ui/views/hung_renderer_view.cc
+++ b/chrome/browser/ui/views/hung_renderer_view.cc
@@ -29,6 +29,7 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas.h"
+#include "ui/views/controls/button/text_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/grid_layout.h"
@@ -221,7 +222,6 @@ void HungRendererDialogView::KillRendererProcess(
 HungRendererDialogView::HungRendererDialogView()
     : hung_pages_table_(NULL),
       kill_button_(NULL),
-      kill_button_container_(NULL),
       initialized_(false) {
   InitClass();
 }
@@ -301,11 +301,14 @@ string16 HungRendererDialogView::GetDialogButtonLabel(
     ui::DialogButton button) const {
   if (button == ui::DIALOG_BUTTON_OK)
     return l10n_util::GetStringUTF16(IDS_BROWSER_HANGMONITOR_RENDERER_WAIT);
-  return string16();
+  return views::DialogDelegateView::GetDialogButtonLabel(button);
 }
 
-views::View* HungRendererDialogView::GetExtraView() {
-  return kill_button_container_;
+views::View* HungRendererDialogView::CreateExtraView() {
+  DCHECK(!kill_button_);
+  kill_button_ = new views::NativeTextButton(this,
+      l10n_util::GetStringUTF16(IDS_BROWSER_HANGMONITOR_RENDERER_END));
+  return kill_button_;
 }
 
 bool HungRendererDialogView::Accept(bool window_closing) {
@@ -372,8 +375,6 @@ void HungRendererDialogView::Init() {
       false, true);
   hung_pages_table_->SetGrouper(hung_pages_table_model_.get());
 
-  CreateKillButtonView();
-
   using views::GridLayout;
   using views::ColumnSet;
 
@@ -406,29 +407,6 @@ void HungRendererDialogView::Init() {
   initialized_ = true;
 }
 
-void HungRendererDialogView::CreateKillButtonView() {
-  kill_button_ = new views::NativeTextButton(this,
-      l10n_util::GetStringUTF16(IDS_BROWSER_HANGMONITOR_RENDERER_END));
-
-  kill_button_container_ = new View;
-
-  using views::GridLayout;
-  using views::ColumnSet;
-
-  GridLayout* layout = new GridLayout(kill_button_container_);
-  kill_button_container_->SetLayoutManager(layout);
-
-  const int single_column_set_id = 0;
-  ColumnSet* column_set = layout->AddColumnSet(single_column_set_id);
-  column_set->AddPaddingColumn(0, frozen_icon_->width() +
-      kCentralColumnPadding);
-  column_set->AddColumn(GridLayout::LEADING, GridLayout::LEADING, 0,
-                        GridLayout::USE_PREF, 0, 0);
-
-  layout->StartRow(0, single_column_set_id);
-  layout->AddView(kill_button_);
-}
-
 gfx::Rect HungRendererDialogView::GetDisplayBounds(
     WebContents* contents) {
 #if defined(USE_AURA)
diff --git a/chrome/browser/ui/views/hung_renderer_view.h b/chrome/browser/ui/views/hung_renderer_view.h
index 753ee90..16cdd8c 100644
--- a/chrome/browser/ui/views/hung_renderer_view.h
+++ b/chrome/browser/ui/views/hung_renderer_view.h
@@ -9,7 +9,7 @@
 #include "chrome/browser/favicon/favicon_tab_helper.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "ui/base/models/table_model.h"
-#include "ui/views/controls/button/text_button.h"
+#include "ui/views/controls/button/button.h"
 #include "ui/views/controls/table/table_grouper.h"
 #include "ui/views/controls/table/table_view.h"
 #include "ui/views/window/dialog_delegate.h"
@@ -18,6 +18,10 @@ namespace content {
 class WebContents;
 }
 
+namespace views {
+class TextButton;
+}
+
 // Provides functionality to display information about a hung renderer.
 class HungPagesTableModel : public ui::TableModel, public views::TableGrouper {
  public:
@@ -120,7 +124,7 @@ class HungRendererDialogView : public views::DialogDelegateView,
   virtual void WindowClosing() OVERRIDE;
   virtual int GetDialogButtons() const OVERRIDE;
   virtual string16 GetDialogButtonLabel(ui::DialogButton button) const OVERRIDE;
-  virtual views::View* GetExtraView() OVERRIDE;
+  virtual views::View* CreateExtraView() OVERRIDE;
   virtual bool Accept(bool window_closing)  OVERRIDE;
 
   // views::ButtonListener overrides:
@@ -144,7 +148,6 @@ class HungRendererDialogView : public views::DialogDelegateView,
  private:
   // Initialize the controls in this dialog.
   void Init();
-  void CreateKillButtonView();
 
   // Returns the bounds the dialog should be displayed at to be meaningfully
   // associated with the specified WebContents.
@@ -155,11 +158,8 @@ class HungRendererDialogView : public views::DialogDelegateView,
   // Controls within the dialog box.
   views::TableView* hung_pages_table_;
 
-  // The button we insert into the ClientView to kill the errant process. This
-  // is parented to a container view that uses a grid layout to align it
-  // properly.
+  // The extra button inserted into the ClientView to kill the errant process.
   views::TextButton* kill_button_;
-  views::View* kill_button_container_;
 
   // The model that provides the contents of the table that shows a list of
   // pages affected by the hang.
diff --git a/chrome/browser/ui/views/ssl_client_certificate_selector.cc b/chrome/browser/ui/views/ssl_client_certificate_selector.cc
index a66b208..8bf49b0 100644
--- a/chrome/browser/ui/views/ssl_client_certificate_selector.cc
+++ b/chrome/browser/ui/views/ssl_client_certificate_selector.cc
@@ -98,8 +98,7 @@ SSLClientCertificateSelector::SSLClientCertificateSelector(
       web_contents_(web_contents),
       window_(NULL),
       table_(NULL),
-      view_cert_button_(NULL),
-      view_cert_button_container_(NULL) {
+      view_cert_button_(NULL) {
   DVLOG(1) << __FUNCTION__;
 }
 
@@ -137,8 +136,6 @@ void SSLClientCertificateSelector::Init() {
 
   layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
 
-  CreateViewCertButton();
-
   StartObserving();
 
   window_ = ConstrainedWindowViews::Create(web_contents_, this);
@@ -213,8 +210,11 @@ views::View* SSLClientCertificateSelector::GetInitiallyFocusedView() {
   return table_;
 }
 
-views::View* SSLClientCertificateSelector::GetExtraView() {
-  return view_cert_button_container_;
+views::View* SSLClientCertificateSelector::CreateExtraView() {
+  DCHECK(!view_cert_button_);
+  view_cert_button_ = new views::NativeTextButton(this,
+      l10n_util::GetStringUTF16(IDS_PAGEINFO_CERT_INFO_BUTTON));
+  return view_cert_button_;
 }
 
 ui::ModalType SSLClientCertificateSelector::GetModalType() const {
@@ -265,24 +265,6 @@ void SSLClientCertificateSelector::CreateCertTable() {
   table_->SetObserver(this);
 }
 
-void SSLClientCertificateSelector::CreateViewCertButton() {
-  view_cert_button_ = new views::NativeTextButton(this,
-      l10n_util::GetStringUTF16(IDS_PAGEINFO_CERT_INFO_BUTTON));
-
-  // Wrap the view cert button in a grid layout in order to left-align it.
-  view_cert_button_container_ = new views::View();
-  views::GridLayout* layout = new views::GridLayout(
-      view_cert_button_container_);
-  view_cert_button_container_->SetLayoutManager(layout);
-
-  int column_set_id = 0;
-  views::ColumnSet* column_set = layout->AddColumnSet(column_set_id);
-  column_set->AddColumn(views::GridLayout::LEADING, views::GridLayout::LEADING,
-                        0, views::GridLayout::USE_PREF, 0, 0);
-  layout->StartRow(0, column_set_id);
-  layout->AddView(view_cert_button_);
-}
-
 namespace chrome {
 
 void ShowSSLClientCertificateSelector(
diff --git a/chrome/browser/ui/views/ssl_client_certificate_selector.h b/chrome/browser/ui/views/ssl_client_certificate_selector.h
index 317adda..573d8de 100644
--- a/chrome/browser/ui/views/ssl_client_certificate_selector.h
+++ b/chrome/browser/ui/views/ssl_client_certificate_selector.h
@@ -62,7 +62,7 @@ class SSLClientCertificateSelector : public SSLClientAuthObserver,
   virtual bool Cancel() OVERRIDE;
   virtual bool Accept() OVERRIDE;
   virtual views::View* GetInitiallyFocusedView() OVERRIDE;
-  virtual views::View* GetExtraView() OVERRIDE;
+  virtual views::View* CreateExtraView() OVERRIDE;
   virtual ui::ModalType GetModalType() const OVERRIDE;
 
   // views::ButtonListener:
@@ -75,7 +75,6 @@ class SSLClientCertificateSelector : public SSLClientAuthObserver,
 
  private:
   void CreateCertTable();
-  void CreateViewCertButton();
 
   scoped_ptr<CertificateSelectorTableModel> model_;
 
@@ -84,7 +83,6 @@ class SSLClientCertificateSelector : public SSLClientAuthObserver,
   WebContentsModalDialog* window_;
   views::TableView* table_;
   views::TextButton* view_cert_button_;
-  views::View* view_cert_button_container_;
 
   DISALLOW_COPY_AND_ASSIGN(SSLClientCertificateSelector);
 };