cbuiv: clean up BookmarkEditorView a bit

The main change here is to remove most of the logic from Accept().
There was code in here to handle when Accept() was called with the
accept button disabled, which is actually impossible*. This change
also cleans up some other overrides of WidgetDelegate methods in
BookmarkEditorView.

*: This change also adds a DCHECK to guarantee that AcceptDialog()
   is never called with the accept button disabled. While this
   can't happen in production, a misguided test might reasonably call
   AcceptDialog() without first ensuring that the button is enabled,
   so enforce the invariant redundantly here.

Bug: 1075649
Change-Id: Ib421da682ef3e8d8dc47be8b22d3339229d8068e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2380255Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807116}

chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc

@@ -72,6 +72,11 @@ BookmarkEditorView::BookmarkEditorView(
   DCHECK(bb_model_);
   DCHECK(bb_model_->client()->CanBeEditedByUser(parent));
   SetCanResize(true);
+  SetModalType(ui::MODAL_TYPE_WINDOW);
+  SetShowCloseButton(false);
+  SetAcceptCallback(base::BindOnce(&BookmarkEditorView::ApplyEdits,
+                                   base::Unretained(this), nullptr));
+  SetTitle(details_.GetWindowTitleId());
   SetButtonLabel(ui::DIALOG_BUTTON_OK, l10n_util::GetStringUTF16(IDS_SAVE));
   if (show_tree_) {
     new_folder_button_ =
@@ -102,32 +107,6 @@ bool BookmarkEditorView::IsDialogButtonEnabled(ui::DialogButton button) const {
   return true;
 }
 
-ui::ModalType BookmarkEditorView::GetModalType() const {
-  return ui::MODAL_TYPE_WINDOW;
-}
-
-bool BookmarkEditorView::ShouldShowCloseButton() const {
-  return false;
-}
-
-base::string16 BookmarkEditorView::GetWindowTitle() const {
-  return l10n_util::GetStringUTF16(details_.GetWindowTitleId());
-}
-
-bool BookmarkEditorView::Accept() {
-  if (!IsDialogButtonEnabled(ui::DIALOG_BUTTON_OK)) {
-    if (details_.GetNodeType() != BookmarkNode::FOLDER) {
-      // The url is invalid, focus the url field.
-      url_tf_->SelectAll(true);
-      url_tf_->RequestFocus();
-    }
-    return false;
-  }
-  // Otherwise save changes and close the dialog box.
-  ApplyEdits();
-  return true;
-}
-
 gfx::Size BookmarkEditorView::CalculatePreferredSize() const {
   if (!show_tree_)
     return views::View::CalculatePreferredSize();
@@ -523,23 +502,18 @@ BookmarkEditorView::EditorNode* BookmarkEditorView::FindNodeWithID(
   return nullptr;
 }
 
-void BookmarkEditorView::ApplyEdits() {
+void BookmarkEditorView::ApplyEdits(EditorNode* parent) {
   DCHECK(bb_model_->loaded());
 
-  if (tree_view_)
-    tree_view_->CommitEdit();
+  if (!parent) {
+    if (tree_view_)
+      tree_view_->CommitEdit();
 
-  EditorNode* parent =
-      show_tree_ ? tree_model_->AsNode(tree_view_->GetSelectedNode()) : nullptr;
-  if (show_tree_ && !parent) {
-    NOTREACHED();
-    return;
+    if (show_tree_) {
+      parent = tree_model_->AsNode(tree_view_->GetSelectedNode());
+      DCHECK(parent);
+    }
   }
-  ApplyEdits(parent);
-}
-
-void BookmarkEditorView::ApplyEdits(EditorNode* parent) {
-  DCHECK(!show_tree_ || parent);
 
   // We're going to apply edits to the bookmark bar model, which will call us
   // back. Normally when a structural edit occurs we reset the tree model.

chrome/browser/ui/views/bookmarks/bookmark_editor_view.h

@@ -80,10 +80,6 @@ class BookmarkEditorView : public BookmarkEditor,
 
   // views::DialogDelegateView:
   bool IsDialogButtonEnabled(ui::DialogButton button) const override;
-  ui::ModalType GetModalType() const override;
-  bool ShouldShowCloseButton() const override;
-  base::string16 GetWindowTitle() const override;
-  bool Accept() override;
 
   // views::View:
   gfx::Size CalculatePreferredSize() const override;
@@ -175,12 +171,10 @@ class BookmarkEditorView : public BookmarkEditor,
   // Returns the node with the specified id, or NULL if one can't be found.
   EditorNode* FindNodeWithID(BookmarkEditorView::EditorNode* node, int64_t id);
 
-  // Invokes ApplyEdits with the selected node.
-  void ApplyEdits();
-
   // Applies the edits done by the user. |parent| gives the parent of the URL
-  // being edited.
-  void ApplyEdits(EditorNode* parent);
+  // being edited. If |parent| is null, the selected node from the treeview's
+  // parent is used.
+  void ApplyEdits(EditorNode* parent = nullptr);
 
   // Recursively adds newly created folders and sets the title of nodes to
   // match the user edited title.

chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc

@@ -205,9 +205,12 @@ class ExtensionInstallDialogViewTest
 
 IN_PROC_BROWSER_TEST_F(ExtensionInstallDialogViewTest, NotifyDelegate) {
   {
-    // User presses install.
+    // User presses install. Note that we have to wait for the 0ms delay for
+    // the install button to become enabled, hence the RunLoop later.
+    ExtensionInstallDialogView::SetInstallButtonDelayForTesting(0);
     ExtensionInstallPromptTestHelper helper;
     ExtensionInstallDialogView* delegate_view = CreateAndShowPrompt(&helper);
+    base::RunLoop().RunUntilIdle();
     delegate_view->AcceptDialog();
     EXPECT_EQ(ExtensionInstallPrompt::Result::ACCEPTED, helper.result());
   }

chrome/browser/ui/views/extensions/extension_install_dialog_view_supervised_browsertest.cc

@@ -150,8 +150,10 @@ IN_PROC_BROWSER_TEST_F(ExtensionInstallDialogViewTestSupervised, AskAParent) {
   task_runner->FastForwardBy(duration);
 
   // Supervised user presses "Ask a parent".
+  ExtensionInstallDialogView::SetInstallButtonDelayForTesting(0);
   ExtensionInstallDialogView* delegate_view =
       CreateAndShowPrompt(&helper, install_prompt.GetPromptForTesting());
+  base::RunLoop().RunUntilIdle();
   delegate_view->AcceptDialog();
   EXPECT_EQ(ExtensionInstallPrompt::Result::ACCEPTED, helper.result());
 

ui/views/window/dialog_delegate.cc

@@ -381,6 +381,7 @@ void DialogDelegate::SetButtonRowInsets(const gfx::Insets& insets) {
 }
 
 void DialogDelegate::AcceptDialog() {
+  DCHECK(IsDialogButtonEnabled(ui::DIALOG_BUTTON_OK));
   if (already_started_close_ || !Accept())
     return;
 
@@ -390,6 +391,9 @@ void DialogDelegate::AcceptDialog() {
 }
 
 void DialogDelegate::CancelDialog() {
+  // Note: don't DCHECK(IsDialogButtonEnabled(ui::DIALOG_BUTTON_CANCEL)) here;
+  // CancelDialog() is *always* reachable via Esc closing the dialog, even if
+  // the cancel button is disabled or there is no cancel button at all.
   if (already_started_close_ || !Cancel())
     return;