Fix nits

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

4 files changed, 77 insertions, 75 deletions

diff --git a/chrome/browser/views/accessible_toolbar_view.cc b/chrome/browser/views/accessible_toolbar_view.cc
index 4bf537a..f18baf1 100644
--- a/chrome/browser/views/accessible_toolbar_view.cc
+++ b/chrome/browser/views/accessible_toolbar_view.cc
@@ -3,8 +3,8 @@
 // found in the LICENSE file.
 
 #include "base/logging.h"
-#include "chrome/browser/views/frame/browser_view.h"
 #include "chrome/browser/view_ids.h"
+#include "chrome/browser/views/frame/browser_view.h"
 #include "chrome/browser/views/accessible_toolbar_view.h"
 #include "views/controls/button/menu_button.h"
 #include "views/focus/view_storage.h"
@@ -13,7 +13,7 @@
 #include "views/widget/widget.h"
 
 AccessibleToolbarView::AccessibleToolbarView()
-    : acc_focused_view_(NULL),
+    : selected_focused_view_(NULL),
       last_focused_view_storage_id_(
           views::ViewStorage::GetSharedInstance()->CreateStorageID()) {
 }
@@ -21,6 +21,21 @@ AccessibleToolbarView::AccessibleToolbarView()
 AccessibleToolbarView::~AccessibleToolbarView() {
 }
 
+void AccessibleToolbarView::InitiateTraversal() {
+  // We only traverse if accessibility is active.
+  if (selected_focused_view_ != NULL)
+    return;
+
+  // Save the last focused view so that when the user presses ESC, it will
+  // return back to the last focus.
+  views::ViewStorage* view_storage = views::ViewStorage::GetSharedInstance();
+  view_storage->StoreView(last_focused_view_storage_id_,
+                          GetRootView()->GetFocusedView());
+
+  // Request focus to the toolbar.
+  RequestFocus();
+}
+
 int AccessibleToolbarView::GetNextAccessibleViewIndex(int view_index,
                                                       bool forward) {
   int modifier = forward ? 1 : -1;
@@ -38,7 +53,8 @@ int AccessibleToolbarView::GetNextAccessibleViewIndex(int view_index,
     current_view_index += modifier;
   }
 
-  // No button is available in the specified direction, remains where it was.
+  // No button is available in the specified direction, the accessible view
+  // remains in |view_index| position.
   return view_index;
 }
 
@@ -49,9 +65,9 @@ bool AccessibleToolbarView::IsAccessibleViewTraversable(views::View* view) {
 void AccessibleToolbarView::DidGainFocus() {
   // Check to see if the accessible focus should be restored to previously
   // focused button. The must be enabled and visible in the toolbar.
-  if (!acc_focused_view_ ||
-      !acc_focused_view_->IsEnabled() ||
-      !acc_focused_view_->IsVisible()) {
+  if (!selected_focused_view_ ||
+      !selected_focused_view_->IsEnabled() ||
+      !selected_focused_view_->IsVisible()) {
     // Find first accessible child (-1 to start search at parent).
     int first_acc_child = GetNextAccessibleViewIndex(-1, true);
 
@@ -59,7 +75,7 @@ void AccessibleToolbarView::DidGainFocus() {
     if (first_acc_child == -1)
       return;
 
-    set_acc_focused_view(GetChildViewAt(first_acc_child));
+    selected_focused_view_ = GetChildViewAt(first_acc_child);
   }
 
   // Set the focus to the current accessible view.
@@ -73,9 +89,9 @@ void AccessibleToolbarView::WillLoseFocus() {
 
   // Removes the child accessibility view's focus and the view from the
   // ViewStorage, when toolbar loses focus.
-  if (acc_focused_view_) {
-    acc_focused_view_->SetHotTracked(false);
-    acc_focused_view_ = NULL;
+  if (selected_focused_view_) {
+    selected_focused_view_->SetHotTracked(false);
+    selected_focused_view_ = NULL;
     views::ViewStorage* view_storage = views::ViewStorage::GetSharedInstance();
     view_storage->RemoveView(last_focused_view_storage_id_);
   }
@@ -83,25 +99,15 @@ void AccessibleToolbarView::WillLoseFocus() {
 
 void AccessibleToolbarView::ShowContextMenu(int x, int y,
                                             bool is_mouse_gesture) {
-  if (acc_focused_view_)
-    acc_focused_view_->ShowContextMenu(x, y, is_mouse_gesture);
+  if (selected_focused_view_)
+    selected_focused_view_->ShowContextMenu(x, y, is_mouse_gesture);
 }
 
 void AccessibleToolbarView::RequestFocus() {
-  // We only traverse if accessibility is active.
-  if (acc_focused_view_ != NULL)
-    return;
-
-  // Save the last focused view so that when the user presses ESC, it will
-  // return back to the last focus.
-  views::ViewStorage* view_storage = views::ViewStorage::GetSharedInstance();
-  view_storage->StoreView(last_focused_view_storage_id_,
-                          GetRootView()->GetFocusedView());
-
   // When the toolbar needs to request focus, the default implementation of
   // View::RequestFocus requires the View to be focusable. Since ToolbarView is
   // not technically focused, we need to temporarily set and remove focus so
-  // that it can focus back to its focused state. |acc_focused_view_| is
+  // that it can focus back to its focused state. |selected_focused_view_| is
   // not necessarily set since it can be null if this view has already lost
   // focus, such as traversing through the context menu.
   SetFocusable(true);
@@ -111,10 +117,10 @@ void AccessibleToolbarView::RequestFocus() {
 
 bool AccessibleToolbarView::OnKeyPressed(const views::KeyEvent& e) {
   // Paranoia check, button should be initialized upon toolbar gaining focus.
-  if (!acc_focused_view_)
+  if (!selected_focused_view_)
     return View::OnKeyPressed(e);
 
-  int focused_view = GetChildIndex(acc_focused_view_);
+  int focused_view = GetChildIndex(selected_focused_view_);
   int next_view = focused_view;
 
   switch (e.GetKeyCode()) {
@@ -126,25 +132,27 @@ bool AccessibleToolbarView::OnKeyPressed(const views::KeyEvent& e) {
       break;
     case base::VKEY_DOWN:
     case base::VKEY_RETURN:
-      if (GetAccFocusedChildView()->GetClassName() ==
+      if (selected_focused_view_->GetClassName() ==
           views::MenuButton::kViewClassName) {
         // If a menu button is activated and its menu is displayed, then the
         // active tooltip should be hidden.
         if (GetWidget()->GetTooltipManager())
           GetWidget()->GetTooltipManager()->HideKeyboardTooltip();
+
         // Safe to cast, given to above check.
-        static_cast<views::MenuButton*>(GetAccFocusedChildView())->Activate();
-        if (!GetAccFocusedChildView()) {
-         // Activate triggered a focus change, don't try to change focus.
-          return true;
+        static_cast<views::MenuButton*>(selected_focused_view_)->Activate();
+
+        // If activate did not trigger a focus change, the menu button should
+        // remain hot tracked since the view is still focused.
+        if (selected_focused_view_) {
+          // Re-enable hot-tracking, as Activate() will disable it.
+          selected_focused_view_->SetHotTracked(true);
         }
-        // Re-enable hot-tracking, as Activate() will disable it.
-        GetAccFocusedChildView()->SetHotTracked(true);
-        break;
+        return true;
       }
     default:
       // If key is not handled explicitly, pass it on to view.
-      return acc_focused_view_->OnKeyPressed(e);
+      return selected_focused_view_->OnKeyPressed(e);
   }
 
   // No buttons enabled, visible, or focus hasn't moved.
@@ -152,10 +160,10 @@ bool AccessibleToolbarView::OnKeyPressed(const views::KeyEvent& e) {
     return false;
 
   // Remove hot-tracking from old focused button.
-  acc_focused_view_->SetHotTracked(false);
+  selected_focused_view_->SetHotTracked(false);
 
   // All is well, update the focused child member variable.
-  acc_focused_view_ = GetChildViewAt(next_view);
+  selected_focused_view_ = GetChildViewAt(next_view);
 
   // Set the focus to the current accessible view.
   SetFocusToAccessibleView();
@@ -164,18 +172,18 @@ bool AccessibleToolbarView::OnKeyPressed(const views::KeyEvent& e) {
 
 bool AccessibleToolbarView::OnKeyReleased(const views::KeyEvent& e) {
   // Paranoia check, button should be initialized upon toolbar gaining focus.
-  if (!acc_focused_view_)
+  if (!selected_focused_view_)
     return false;
 
   // Have keys be handled by the views themselves.
-  return acc_focused_view_->OnKeyReleased(e);
+  return selected_focused_view_->OnKeyReleased(e);
 }
 
 bool AccessibleToolbarView::SkipDefaultKeyEventProcessing(
     const views::KeyEvent& e) {
   // Accessibility focus must be present in order to handle ESC and TAB related
   // key events.
-  if (!acc_focused_view_)
+  if (!selected_focused_view_)
     return false;
 
   // The ancestor *must* be a BrowserView.
@@ -191,7 +199,7 @@ bool AccessibleToolbarView::SkipDefaultKeyEventProcessing(
     case base::VKEY_ESCAPE: {
       // Retrieve the focused view from the storage so we can request focus back
       // to it. If |focus_view| is null, we place focus on the default view.
-      // |acc_focused_view_| doesn't need to be reset here since it will be
+      // |selected_focused_view_| doesn't need to be reset here since it will be
       // dealt within the WillLoseFocus method.
       views::ViewStorage* view_storage =
           views::ViewStorage::GetSharedInstance();
@@ -235,17 +243,18 @@ void AccessibleToolbarView::SetAccessibleName(const std::wstring& name) {
 
 void AccessibleToolbarView::SetFocusToAccessibleView() {
   // Hot-track new focused button.
-  acc_focused_view_->SetHotTracked(true);
+  selected_focused_view_->SetHotTracked(true);
 
   // Show the tooltip for the view that got the focus.
   if (GetWidget()->GetTooltipManager()) {
-    GetWidget()->GetTooltipManager()->ShowKeyboardTooltip(acc_focused_view_);
+    GetWidget()->GetTooltipManager()->ShowKeyboardTooltip(
+        selected_focused_view_);
   }
 
 #if defined(OS_WIN)
   // Retrieve information to generate an accessible focus event.
   gfx::NativeView wnd = GetWidget()->GetNativeView();
-  int view_id = acc_focused_view_->GetID();
+  int view_id = selected_focused_view_->GetID();
   // Notify Access Technology that there was a change in keyboard focus.
   ::NotifyWinEvent(EVENT_OBJECT_FOCUS, wnd, OBJID_CLIENT,
                    static_cast<LONG>(view_id));
diff --git a/chrome/browser/views/accessible_toolbar_view.h b/chrome/browser/views/accessible_toolbar_view.h
index 293ebeb..07550e7 100644
--- a/chrome/browser/views/accessible_toolbar_view.h
+++ b/chrome/browser/views/accessible_toolbar_view.h
@@ -9,36 +9,16 @@
 
 // This class provides keyboard access to any view that extends it by intiating
 // ALT+SHIFT+T. And once you press TAB or SHIFT-TAB, it will traverse all the
-// toolbars within Chrome. It traverses child views in the order of adding them,
-// not necessarily the visual order.
-//
-// If a toolbar needs to be traversal then it needs to be focusable. But we do
-// not want the toolbar to have any focus, so some hacking needs to be done to
-// tell the focus manager that this view is focusable while it's not. The main
-// purpose of this class is to provide keyboard access to any View that extends
-// this. It will properly hot track and add the tooltip to the first level
-// children if visited.
+// toolbars within Chrome. Child views are traversed in the order they were
+// added.
 
 class AccessibleToolbarView : public views::View {
  public:
   AccessibleToolbarView();
   virtual ~AccessibleToolbarView();
 
-  // Returns the index of the next view of the toolbar, starting from the given
-  // view index, in the given navigation direction, |forward| when true means it
-  // will navigate from left to right, and vice versa when false. When
-  // |view_index| is -1, it finds the first accessible child.
-  int GetNextAccessibleViewIndex(int view_index, bool forward);
-
-  // Invoked when you press left and right arrows while traversing in the view.
-  // The default implementation is true, where every child in the view is
-  // traversable.
-  virtual bool IsAccessibleViewTraversable(views::View* view);
-
-  // Sets the accessible focused view.
-  void set_acc_focused_view(views::View* acc_focused_view) {
-    acc_focused_view_ = acc_focused_view;
-  }
+  // Initiate the traversal on the toolbar.
+  void InitiateTraversal();
 
   // Overridden from views::View:
   virtual void DidGainFocus();
@@ -51,7 +31,19 @@ class AccessibleToolbarView : public views::View {
   virtual bool GetAccessibleName(std::wstring* name);
   virtual bool GetAccessibleRole(AccessibilityTypes::Role* role);
   virtual void SetAccessibleName(const std::wstring& name);
-  virtual View* GetAccFocusedChildView() { return acc_focused_view_; }
+  virtual View* GetAccFocusedChildView() { return selected_focused_view_; }
+
+ protected:
+  // Returns the index of the next view of the toolbar, starting from the given
+  // view index. |forward| when true means it will navigate from left to right,
+  // and vice versa when false. If |view_index| is -1 the first accessible child

+  // is returned.
+  int GetNextAccessibleViewIndex(int view_index, bool forward);
+
+  // Invoked from GetNextAccessibleViewIndex to determine if |view| can be

+  // traversed to. Default implementation returns true, override to return false

+  // for views you don't want reachable.
+  virtual bool IsAccessibleViewTraversable(views::View* view);
 
  private:
   // Sets the focus to the currently |acc_focused_view_| view.
@@ -60,8 +52,8 @@ class AccessibleToolbarView : public views::View {
   // Storage of strings needed for accessibility.
   std::wstring accessible_name_;
 
-  // Child view currently having accessibility focus.
-  views::View* acc_focused_view_;
+  // Selected child view currently having accessibility focus.
+  views::View* selected_focused_view_;
 
   // Last focused view that issued this traversal.
   int last_focused_view_storage_id_;
diff --git a/chrome/browser/views/bookmark_bar_view.cc b/chrome/browser/views/bookmark_bar_view.cc
index 40a8f26..c6a69b6 100644
--- a/chrome/browser/views/bookmark_bar_view.cc
+++ b/chrome/browser/views/bookmark_bar_view.cc
@@ -692,8 +692,8 @@ int BookmarkBarView::OnPerformDrop(const DropTargetEvent& event) {
                                              index);
 }
 
-bool BookmarkBarView::IsAccessibleViewTraversable(views::View* view) {

-  return view != bookmarks_separator_view_ && view != instructions_;

+bool BookmarkBarView::IsAccessibleViewTraversable(views::View* view) {
+  return view != bookmarks_separator_view_ && view != instructions_;
 }
 
 void BookmarkBarView::OnStateChanged() {
@@ -853,6 +853,7 @@ void BookmarkBarView::Init() {
   if (!kDefaultFavIcon)
     kDefaultFavIcon = rb.GetBitmapNamed(IDR_DEFAULT_FAVICON);
 
+  // Order matters here. Child views are traversed in the order they were added.
   sync_error_button_ = CreateSyncErrorButton();
   AddChildView(sync_error_button_);
 
diff --git a/chrome/browser/views/frame/browser_view.cc b/chrome/browser/views/frame/browser_view.cc
index e71acc5..5dd601d 100644
--- a/chrome/browser/views/frame/browser_view.cc
+++ b/chrome/browser/views/frame/browser_view.cc
@@ -661,10 +661,10 @@ void BrowserView::TraverseNextAccessibleToolbar(bool forward) {
   // TODO(mohamed) This needs to be smart, that applies to all toolbars.
   //               Currently it just traverses between bookmarks and toolbar.
   if (forward && toolbar_->IsVisible() && toolbar_->IsEnabled()) {
-    toolbar_->RequestFocus();
+    toolbar_->InitiateTraversal();
   } else if (!forward && bookmark_bar_view_->IsVisible() &&
              bookmark_bar_view_->IsEnabled()) {
-    bookmark_bar_view_->RequestFocus();
+    bookmark_bar_view_->InitiateTraversal();
   }
 }
 
@@ -2086,7 +2086,7 @@ bool BrowserView::UpdateChildViewAndLayout(views::View* new_view,
     new_view->SetBounds((*old_view)->bounds());
     new_view->SchedulePaint();
   } else if (new_view) {
-    DCHECK_EQ(new_height, 0);
+    DCHECK_EQ(0, new_height);
     // The heights are the same, but the old view is null. This only happens
     // when the height is zero. Zero out the bounds.
     new_view->SetBounds(0, 0, 0, 0);