Cleanup redundant model_ field in BookmarkBarView.

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

Index: chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc b/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
index bd8727e00af2790e9e0d66e53c539e35b71af49d..1decce54aefd613c999e82b070c47f4d9169294c 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
@@ -6,7 +6,7 @@
 
 #include <algorithm>
 #include <limits>
-#include <set>
+#include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ -16,7 +16,6 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
-#include "chrome/browser/bookmarks/chrome_bookmark_client.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/defaults.h"
@@ -439,7 +438,6 @@ static const gfx::ImageSkia& GetFolderIcon() {
 
 BookmarkBarView::BookmarkBarView(Browser* browser, BrowserView* browser_view)
     : page_navigator_(NULL),
-      model_(NULL),
       client_(NULL),
       bookmark_menu_(NULL),
       bookmark_drop_menu_(NULL),
@@ -463,8 +461,8 @@ BookmarkBarView::BookmarkBarView(Browser* browser, BrowserView* browser_view)
 }
 
 BookmarkBarView::~BookmarkBarView() {
-  if (model_)
-    model_->RemoveObserver(this);
+  if (client_)
+    model()->RemoveObserver(this);
 
   // It's possible for the menu to outlive us, reset the observer to make sure
   // it doesn't have a reference to us.
@@ -544,20 +542,20 @@ const BookmarkNode* BookmarkBarView::GetNodeForButtonAtModelIndex(
     if (!child->visible())
       break;
     if (child->bounds().Contains(adjusted_loc))
-      return model_->bookmark_bar_node()->GetChild(i);
+      return model()->bookmark_bar_node()->GetChild(i);
   }
 
   // Then the overflow button.
   if (overflow_button_->visible() &&
       overflow_button_->bounds().Contains(adjusted_loc)) {
     *model_start_index = GetFirstHiddenNodeIndex();
-    return model_->bookmark_bar_node();
+    return model()->bookmark_bar_node();
   }
 
   // And finally the other folder.
   if (other_bookmarked_button_->visible() &&
       other_bookmarked_button_->bounds().Contains(adjusted_loc)) {
-    return model_->other_node();
+    return model()->other_node();
   }
 
   return NULL;
@@ -567,11 +565,11 @@ views::MenuButton* BookmarkBarView::GetMenuButtonForNode(
     const BookmarkNode* node) {
   if (node == client_->managed_node())
     return managed_bookmarks_button_;
-  if (node == model_->other_node())
+  if (node == model()->other_node())
     return other_bookmarked_button_;
-  if (node == model_->bookmark_bar_node())
+  if (node == model()->bookmark_bar_node())
     return overflow_button_;
-  int index = model_->bookmark_bar_node()->GetIndexOf(node);
+  int index = model()->bookmark_bar_node()->GetIndexOf(node);
   if (index == -1 || !node->is_folder())
     return NULL;
   return static_cast<views::MenuButton*>(child_at(index));
@@ -811,7 +809,7 @@ void BookmarkBarView::PaintChildren(gfx::Canvas* canvas,
 bool BookmarkBarView::GetDropFormats(
       int* formats,
       std::set<ui::OSExchangeData::CustomFormat>* custom_formats) {
-  if (!model_ || !model_->loaded())
+  if (!client_ || !model()->loaded())
     return false;
   *formats = ui::OSExchangeData::URL;
   custom_formats->insert(BookmarkNodeData::GetBookmarkCustomFormat());
@@ -823,7 +821,7 @@ bool BookmarkBarView::AreDropTypesRequired() {
 }
 
 bool BookmarkBarView::CanDrop(const ui::OSExchangeData& data) {
-  if (!model_ || !model_->loaded() ||
+  if (!client_ || !model()->loaded() ||
       !browser_->profile()->GetPrefs()->GetBoolean(
           prefs::kEditBookmarksEnabled))
     return false;
@@ -880,11 +878,11 @@ int BookmarkBarView::OnDragUpdated(const DropTargetEvent& event) {
       location.button_type == DROP_OTHER_FOLDER) {
     const BookmarkNode* node;
     if (location.button_type == DROP_OTHER_FOLDER)
-      node = model_->other_node();
+      node = model()->other_node();
     else if (location.button_type == DROP_OVERFLOW)
-      node = model_->bookmark_bar_node();
+      node = model()->bookmark_bar_node();
     else
-      node = model_->bookmark_bar_node()->GetChild(location.index);
+      node = model()->bookmark_bar_node()->GetChild(location.index);
     StartShowFolderDropMenuTimer(node);
   }
 
@@ -917,7 +915,7 @@ int BookmarkBarView::OnPerformDrop(const DropTargetEvent& event) {
 
   const BookmarkNode* root =
       (drop_info_->location.button_type == DROP_OTHER_FOLDER) ?
-      model_->other_node() : model_->bookmark_bar_node();
+      model()->other_node() : model()->bookmark_bar_node();
   int index = drop_info_->location.index;
 
   if (index != -1) {
@@ -994,7 +992,7 @@ void BookmarkBarView::ShowImportDialog() {
 
 void BookmarkBarView::OnBookmarkBubbleShown(const GURL& url) {
   StopThrobbing(true);
-  const BookmarkNode* node = model_->GetMostRecentlyAddedUserNodeForURL(url);
+  const BookmarkNode* node = model()->GetMostRecentlyAddedUserNodeForURL(url);
   if (!node)
     return;  // Generally shouldn't happen.
   StartThrobbing(node, false);
@@ -1009,14 +1007,14 @@ void BookmarkBarView::BookmarkModelLoaded(BookmarkModel* model,
   // There should be no buttons. If non-zero it means Load was invoked more than
   // once, or we didn't properly clear things. Either of which shouldn't happen.
   DCHECK_EQ(0, GetBookmarkButtonCount());
-  const BookmarkNode* node = model_->bookmark_bar_node();
+  const BookmarkNode* node = model->bookmark_bar_node();
   DCHECK(node);
   // Create a button for each of the children on the bookmark bar.
   for (int i = 0, child_count = node->child_count(); i < child_count; ++i)
     AddChildViewAt(CreateBookmarkButton(node->GetChild(i)), i);
-  DCHECK(model_->other_node());
-  other_bookmarked_button_->SetAccessibleName(model_->other_node()->GetTitle());
-  other_bookmarked_button_->SetText(model_->other_node()->GetTitle());
+  DCHECK(model->other_node());
+  other_bookmarked_button_->SetAccessibleName(model->other_node()->GetTitle());
+  other_bookmarked_button_->SetText(model->other_node()->GetTitle());
   managed_bookmarks_button_->SetAccessibleName(
       client_->managed_node()->GetTitle());
   managed_bookmarks_button_->SetText(client_->managed_node()->GetTitle());
@@ -1032,8 +1030,7 @@ void BookmarkBarView::BookmarkModelLoaded(BookmarkModel* model,
 void BookmarkBarView::BookmarkModelBeingDeleted(BookmarkModel* model) {
   NOTREACHED();
   // Do minimal cleanup, presumably we'll be deleted shortly.
-  model_->RemoveObserver(this);
-  model_ = NULL;
+  model->RemoveObserver(this);
   client_ = NULL;
 }
 
@@ -1092,7 +1089,7 @@ void BookmarkBarView::BookmarkNodeChanged(BookmarkModel* model,
 
 void BookmarkBarView::BookmarkNodeChildrenReordered(BookmarkModel* model,
                                                     const BookmarkNode* node) {
-  if (node != model_->bookmark_bar_node())
+  if (node != model->bookmark_bar_node())
     return;  // We only care about reordering of the bookmark bar node.
 
   // Remove the existing buttons.
@@ -1130,7 +1127,7 @@ void BookmarkBarView::WriteDragDataForView(View* sender,
       drag_utils::SetDragImageOnDataObject(*canvas, button->size(),
                                            press_pt.OffsetFromOrigin(),
                                            data);
-      WriteBookmarkDragData(model_->bookmark_bar_node()->GetChild(i), data);
+      WriteBookmarkDragData(model()->bookmark_bar_node()->GetChild(i), data);
       return;
     }
   }
@@ -1152,7 +1149,7 @@ int BookmarkBarView::GetDragOperationsForView(View* sender,
   for (int i = 0; i < GetBookmarkButtonCount(); ++i) {
     if (sender == GetBookmarkButton(i)) {
       return chrome::GetBookmarkDragOperation(
-          browser_->profile(), model_->bookmark_bar_node()->GetChild(i));
+          browser_->profile(), model()->bookmark_bar_node()->GetChild(i));
     }
   }
   NOTREACHED();
@@ -1169,7 +1166,7 @@ bool BookmarkBarView::CanStartDragForView(views::View* sender,
   if (!View::ExceededDragThreshold(horizontal_offset) && move_offset.y() > 0) {
     for (int i = 0; i < GetBookmarkButtonCount(); ++i) {
       if (sender == GetBookmarkButton(i)) {
-        const BookmarkNode* node = model_->bookmark_bar_node()->GetChild(i);
+        const BookmarkNode* node = model()->bookmark_bar_node()->GetChild(i);
         // If the folder button was dragged, show the menu instead.
         if (node && node->is_folder()) {
           views::MenuButton* menu_button =
@@ -1190,16 +1187,16 @@ void BookmarkBarView::OnMenuButtonClicked(views::View* view,
 
   int start_index = 0;
   if (view == other_bookmarked_button_) {
-    node = model_->other_node();
+    node = model()->other_node();
   } else if (view == managed_bookmarks_button_) {
     node = client_->managed_node();
   } else if (view == overflow_button_) {
-    node = model_->bookmark_bar_node();
+    node = model()->bookmark_bar_node();
     start_index = GetFirstHiddenNodeIndex();
   } else {
     int button_index = GetIndexOf(view);
     DCHECK_NE(-1, button_index);
-    node = model_->bookmark_bar_node()->GetChild(button_index);
+    node = model()->bookmark_bar_node()->GetChild(button_index);
   }
 
   RecordBookmarkFolderOpen(GetBookmarkLaunchLocation());
@@ -1227,13 +1224,13 @@ void BookmarkBarView::ButtonPressed(views::Button* sender,
 
   const BookmarkNode* node;
   if (sender->tag() == kOtherFolderButtonTag) {
-    node = model_->other_node();
+    node = model()->other_node();
   } else if (sender->tag() == kManagedFolderButtonTag) {
     node = client_->managed_node();
   } else {
     int index = GetIndexOf(sender);
     DCHECK_NE(-1, index);
-    node = model_->bookmark_bar_node()->GetChild(index);
+    node = model()->bookmark_bar_node()->GetChild(index);
   }
   DCHECK(page_navigator_);
 
@@ -1254,7 +1251,7 @@ void BookmarkBarView::ButtonPressed(views::Button* sender,
 void BookmarkBarView::ShowContextMenuForView(views::View* source,
                                              const gfx::Point& point,
                                              ui::MenuSourceType source_type) {
-  if (!model_->loaded()) {
+  if (!model()->loaded()) {
     // Don't do anything if the model isn't loaded.
     return;
   }
@@ -1262,7 +1259,7 @@ void BookmarkBarView::ShowContextMenuForView(views::View* source,
   const BookmarkNode* parent = NULL;
   std::vector<const BookmarkNode*> nodes;
   if (source == other_bookmarked_button_) {
-    parent = model_->other_node();
+    parent = model()->other_node();
     // Do this so the user can open all bookmarks. BookmarkContextMenu makes
     // sure the user can't edit/delete the node in this case.
     nodes.push_back(parent);
@@ -1277,15 +1274,15 @@ void BookmarkBarView::ShowContextMenuForView(views::View* source,
     DCHECK(bookmark_button_index != -1 &&
            bookmark_button_index < GetBookmarkButtonCount());
     const BookmarkNode* node =
-        model_->bookmark_bar_node()->GetChild(bookmark_button_index);
+        model()->bookmark_bar_node()->GetChild(bookmark_button_index);
     nodes.push_back(node);
     parent = node->parent();
   } else {
-    parent = model_->bookmark_bar_node();
+    parent = model()->bookmark_bar_node();
     nodes.push_back(parent);
   }
   bool close_on_remove =
-      (parent == model_->other_node()) && (parent->child_count() == 1);
+      (parent == model()->other_node()) && (parent->child_count() == 1);
 
   context_menu_.reset(new BookmarkContextMenu(
       GetWidget(), browser_, browser_->profile(),
@@ -1339,11 +1336,10 @@ void BookmarkBarView::Init() {
 
   client_ = BookmarkModelFactory::GetChromeBookmarkClientForProfile(
       browser_->profile());
-  model_ = client_->model();
-  if (model_) {
-    model_->AddObserver(this);
-    if (model_->loaded())
-      BookmarkModelLoaded(model_, false);
+  if (client_) {

[sky, 2014/06/06 19:49:20]

I'm not sure this ever happens, but I suppose that can be dealt with separately.

[Joao da Silva, 2014/06/06 20:12:18]

BookmarkModelFactory::ServiceIsNULLWhileTesting() return true. I'm guessing this
checks for tests that use a TestingProfile but don't call
TestingProfile::CreateBookmarkModel().

+    model()->AddObserver(this);
+    if (model()->loaded())
+      BookmarkModelLoaded(model(), false);
     // else case: we'll receive notification back from the BookmarkModel when
     // done loading, then we'll populate the bar.
   }
@@ -1464,7 +1460,7 @@ void BookmarkBarView::ConfigureButton(const BookmarkNode* node,
   button->set_context_menu_controller(this);
   button->set_drag_controller(this);
   if (node->is_url()) {
-    const gfx::Image& favicon = model_->GetFavicon(node);
+    const gfx::Image& favicon = model()->GetFavicon(node);
     if (!favicon.IsEmpty())
       button->SetIcon(*favicon.ToImageSkia());
     else
@@ -1477,7 +1473,7 @@ void BookmarkBarView::BookmarkNodeAddedImpl(BookmarkModel* model,
                                             const BookmarkNode* parent,
                                             int index) {
   UpdateButtonsVisibility();
-  if (parent != model_->bookmark_bar_node()) {
+  if (parent != model->bookmark_bar_node()) {
     // We only care about nodes on the bookmark bar.
     return;
   }
@@ -1502,7 +1498,7 @@ void BookmarkBarView::BookmarkNodeRemovedImpl(BookmarkModel* model,
   // No need to start throbbing again as the bookmark bubble can't be up at
   // the same time as the user reorders.
 
-  if (parent != model_->bookmark_bar_node()) {
+  if (parent != model->bookmark_bar_node()) {
     // We only care about nodes on the bookmark bar.
     return;
   }
@@ -1516,11 +1512,11 @@ void BookmarkBarView::BookmarkNodeRemovedImpl(BookmarkModel* model,
 
 void BookmarkBarView::BookmarkNodeChangedImpl(BookmarkModel* model,
                                               const BookmarkNode* node) {
-  if (node->parent() != model_->bookmark_bar_node()) {
+  if (node->parent() != model->bookmark_bar_node()) {
     // We only care about nodes on the bookmark bar.
     return;
   }
-  int index = model_->bookmark_bar_node()->GetIndexOf(node);
+  int index = model->bookmark_bar_node()->GetIndexOf(node);
   DCHECK_NE(-1, index);
   views::TextButton* button = GetBookmarkButton(index);
   gfx::Size old_pref = button->GetPreferredSize();
@@ -1548,7 +1544,7 @@ void BookmarkBarView::ShowDropFolderForNode(const BookmarkNode* node) {
     return;
 
   int start_index = 0;
-  if (node == model_->bookmark_bar_node())
+  if (node == model()->bookmark_bar_node())
     start_index = GetFirstHiddenNodeIndex();
 
   drop_info_->is_menu_showing = true;
@@ -1581,8 +1577,8 @@ void BookmarkBarView::StartShowFolderDropMenuTimer(const BookmarkNode* node) {
 void BookmarkBarView::CalculateDropLocation(const DropTargetEvent& event,
                                             const BookmarkNodeData& data,
                                             DropLocation* location) {
-  DCHECK(model_);
-  DCHECK(model_->loaded());
+  DCHECK(client_);
+  DCHECK(model()->loaded());
   DCHECK(data.is_valid());
 
   *location = DropLocation();
@@ -1606,7 +1602,7 @@ void BookmarkBarView::CalculateDropLocation(const DropTargetEvent& event,
   } else if (!GetBookmarkButtonCount()) {
     // No bookmarks, accept the drop.
     location->index = 0;
-    const BookmarkNode* node = data.GetFirstNode(model_, profile->GetPath());
+    const BookmarkNode* node = data.GetFirstNode(model(), profile->GetPath());
     int ops = node && client_->CanBeEditedByUser(node) ?
         ui::DragDropTypes::DRAG_MOVE :
         ui::DragDropTypes::DRAG_COPY | ui::DragDropTypes::DRAG_LINK;
@@ -1622,7 +1618,7 @@ void BookmarkBarView::CalculateDropLocation(const DropTargetEvent& event,
     int button_w = button->width();
     if (button_x < button_w) {
       found = true;
-      const BookmarkNode* node = model_->bookmark_bar_node()->GetChild(i);
+      const BookmarkNode* node = model()->bookmark_bar_node()->GetChild(i);
       if (node->is_folder()) {
         if (button_x <= views::kDropBetweenPixels) {
           location->index = i;
@@ -1669,18 +1665,18 @@ void BookmarkBarView::CalculateDropLocation(const DropTargetEvent& event,
 
   if (location->on) {
     const BookmarkNode* parent = (location->button_type == DROP_OTHER_FOLDER) ?
-        model_->other_node() :
-        model_->bookmark_bar_node()->GetChild(location->index);
+        model()->other_node() :
+        model()->bookmark_bar_node()->GetChild(location->index);
     location->operation = chrome::GetBookmarkDropOperation(
         profile, event, data, parent, parent->child_count());
     if (!location->operation && !data.has_single_url() &&
-        data.GetFirstNode(model_, profile->GetPath()) == parent) {
+        data.GetFirstNode(model(), profile->GetPath()) == parent) {
       // Don't open a menu if the node being dragged is the menu to open.
       location->on = false;
     }
   } else {
     location->operation = chrome::GetBookmarkDropOperation(
-        profile, event, data, model_->bookmark_bar_node(), location->index);
+        profile, event, data, model()->bookmark_bar_node(), location->index);
   }
 }
 
@@ -1697,7 +1693,7 @@ void BookmarkBarView::StartThrobbing(const BookmarkNode* node,
 
   // Determine which visible button is showing the bookmark (or is an ancestor
   // of the bookmark).
-  const BookmarkNode* bbn = model_->bookmark_bar_node();
+  const BookmarkNode* bbn = model()->bookmark_bar_node();
   const BookmarkNode* parent_on_bb = node;
   while (parent_on_bb) {
     const BookmarkNode* parent = parent_on_bb->parent();
@@ -1727,7 +1723,7 @@ void BookmarkBarView::StartThrobbing(const BookmarkNode* node,
 views::CustomButton* BookmarkBarView::DetermineViewToThrobFromRemove(
     const BookmarkNode* parent,
     int old_index) {
-  const BookmarkNode* bbn = model_->bookmark_bar_node();
+  const BookmarkNode* bbn = model()->bookmark_bar_node();
   const BookmarkNode* old_node = parent;
   int old_index_on_bb = old_index;
   while (old_node && old_node != bbn) {
@@ -1767,7 +1763,7 @@ void BookmarkBarView::UpdateColors() {
 }
 
 void BookmarkBarView::UpdateButtonsVisibility() {
-  bool has_other_children = !model_->other_node()->empty();
+  bool has_other_children = !model()->other_node()->empty();
   bool update_other = has_other_children != other_bookmarked_button_->visible();
   if (update_other) {
     other_bookmarked_button_->SetVisible(has_other_children);
@@ -1851,7 +1847,7 @@ void BookmarkBarView::LayoutItems() {
   }
 
   // Then go through the bookmark buttons.
-  if (GetBookmarkButtonCount() == 0 && model_ && model_->loaded()) {
+  if (GetBookmarkButtonCount() == 0 && client_ && model()->loaded()) {
     gfx::Size pref = instructions_->GetPreferredSize();
     instructions_->SetBounds(
         x + kInstructionsPadding, y,