DialogClientView: Fix regression in Chrome Task Manager focusing.

ui/views/window/dialog_client_view.cc

Index: ui/views/window/dialog_client_view.cc
diff --git a/ui/views/window/dialog_client_view.cc b/ui/views/window/dialog_client_view.cc
index 5b5db8b88a4ca586a446d74ad93a0a0006e5981f..132b87f88eb19c591e7b2af84eba77ff50dfe86a 100644
--- a/ui/views/window/dialog_client_view.cc
+++ b/ui/views/window/dialog_client_view.cc
@@ -92,7 +92,7 @@ void DialogClientView::UpdateDialogButtons() {
   if (buttons & ui::DIALOG_BUTTON_OK) {
     if (!ok_button_) {
       ok_button_ = CreateDialogButton(ui::DIALOG_BUTTON_OK);
-      AddChildView(ok_button_);
+      AddOKButton(ok_button_);
     }
 
     UpdateButton(ok_button_, ui::DIALOG_BUTTON_OK);
@@ -104,7 +104,7 @@ void DialogClientView::UpdateDialogButtons() {
   if (buttons & ui::DIALOG_BUTTON_CANCEL) {
     if (!cancel_button_) {
       cancel_button_ = CreateDialogButton(ui::DIALOG_BUTTON_CANCEL);
-      AddChildView(cancel_button_);
+      AddCancelButton(cancel_button_);
     }
 
     UpdateButton(cancel_button_, ui::DIALOG_BUTTON_CANCEL);
@@ -233,8 +233,6 @@ void DialogClientView::ViewHierarchyChanged(
     else if (details.child == extra_view_)
       extra_view_ = nullptr;
   }
-
-  SetupFocusChain();
 }
 
 void DialogClientView::OnNativeThemeChanged(const ui::NativeTheme* theme) {
@@ -285,7 +283,13 @@ void DialogClientView::CreateExtraView() {
   extra_view_ = GetDialogDelegate()->CreateExtraView();
   if (extra_view_) {
     extra_view_->SetGroup(kButtonGroup);
-    AddChildView(extra_view_);
+    // Add |extra_view_| after contents_view, if it exists in the view hierarchy
+    // to ensure the correct focus order.
+    int content_view_index = GetIndexOf(contents_view());
+    // The content_view should be the first child, if it exists in the view
+    // hierarchy.
+    DCHECK(content_view_index == -1 || content_view_index == 0);
+    AddChildViewAt(extra_view_, content_view_index + 1);
   }
 }
 
@@ -320,6 +324,38 @@ LabelButton* DialogClientView::CreateDialogButton(ui::DialogButton type) {
   return button;
 }
 
+void DialogClientView::AddOKButton(LabelButton* ok_button) {
+  // Find the view which should precede |ok_button| in focus order.
+  View* previous_view = nullptr;
+  if (!kIsOkButtonOnLeftSide && cancel_button_)
+    previous_view = cancel_button_;
+  else if (extra_view_)
+    previous_view = extra_view_;
+  else
+    previous_view = contents_view();
+  int previous_index = GetIndexOf(previous_view);
+
+  // previous_index may be -1. This may happen when the contents_view() is null
+  // or hasn't been added to the view hierarchy.
+  AddChildViewAt(ok_button, previous_index + 1);
+}
+
+void DialogClientView::AddCancelButton(LabelButton* cancel_button) {
+  // Find the view which should precede |cancel_button| in focus order.
+  View* previous_view = nullptr;
+  if (kIsOkButtonOnLeftSide && ok_button_)
+    previous_view = ok_button_;
+  else if (extra_view_)
+    previous_view = extra_view_;
+  else
+    previous_view = contents_view();
+  int previous_index = GetIndexOf(previous_view);
+
+  // previous_index may be -1. This may happen when the contents_view() is null
+  // or hasn't been added to the view hierarchy.
+  AddChildViewAt(cancel_button, previous_index + 1);
+}
+
 void DialogClientView::UpdateButton(LabelButton* button,
                                     ui::DialogButton type) {
   DialogDelegate* dialog = GetDialogDelegate();
@@ -342,29 +378,4 @@ gfx::Insets DialogClientView::GetButtonRowInsets() const {
                                                 : button_row_insets_;
 }
 
-void DialogClientView::SetupFocusChain() {

[sky, 2016/03/22 16:19:23]

This code seems simpler to me. Is there a reason you can't add extra_view_ to
child_views?
An alternative to SetNextFocusableView is to reorder the views using
ReorderChildView.

[karandeepb, 2016/04/06 23:06:53]

extra_view_ is being currently added to child views. The regression occurred
since other clients may add views to the DialogClientView, but SetupFocusChain
did not take that into account. The changes in dialog_client_view_unittest.cc
illustrate this.

ReorderChildView is a good suggestion. However I have to modify the
SetupFocusChain call site since it's not safe to call ReorderChildView while a
view is being removed. For example the code at
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&s...
assumes that the call to PropagateRemoveNotifications does not alter the
position of the view being removed.

-  // Create a vector of child views in the order of intended focus.
-  std::vector<View*> child_views;
-  child_views.push_back(contents_view());
-  child_views.push_back(extra_view_);
-  if (kIsOkButtonOnLeftSide) {
-    child_views.push_back(ok_button_);
-    child_views.push_back(cancel_button_);
-  } else {
-    child_views.push_back(cancel_button_);
-    child_views.push_back(ok_button_);
-  }
-
-  // Remove all null views from the vector.
-  child_views.erase(
-      std::remove(child_views.begin(), child_views.end(), nullptr),
-      child_views.end());
-
-  // Setup focus.
-  for (size_t i = 0; i < child_views.size(); i++) {
-    child_views[i]->SetNextFocusableView(
-        i + 1 != child_views.size() ? child_views[i + 1] : nullptr);
-  }
-}
-
 }  // namespace views