bookmarks: Don't expose an implementation detail function through BookmarkNode public API.

This function is only interesting for BookmarkModel and at this moment is just used there.
Instead of exposing it through the BookmarkNode class just do it internally in BookmarkModel.
This has the advantage of preventing further external clients of using it.
This is a responsibility of BookmarkModel, and just it knows when to call this function.
The consumers of BookmarkNode API don't know and shouldn't know where/when it's necessary to call
InvalidateIcon(), so lets the model do the work for them.

BUG=None
TEST=None

R=sky@chromium.org

Review URL: http://codereview.chromium.org/7401013

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

2 files changed, 30 insertions, 32 deletions

diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc
index 39054c7..ecdc5f8 100644
--- a/chrome/browser/bookmarks/bookmark_model.cc
+++ b/chrome/browser/bookmarks/bookmark_model.cc
@@ -55,11 +55,6 @@ BookmarkNode::BookmarkNode(int64 id, const GURL& url)
 BookmarkNode::~BookmarkNode() {
 }
 
-void BookmarkNode::InvalidateFavicon() {
-  favicon_ = SkBitmap();
-  is_favicon_loaded_ = false;
-}
-
 bool BookmarkNode::IsVisible() const {
   // The synced bookmark folder is invisible if the flag isn't set and there are
   // no bookmarks under it.
@@ -79,6 +74,11 @@ void BookmarkNode::Initialize(int64 id) {
   favicon_load_handle_ = 0;
 }
 
+void BookmarkNode::InvalidateFavicon() {
+  favicon_ = SkBitmap();
+  is_favicon_loaded_ = false;
+}
+
 // BookmarkModel --------------------------------------------------------------
 
 namespace {
@@ -310,16 +310,17 @@ void BookmarkModel::SetURL(const BookmarkNode* node, const GURL& url) {
     return;
   }
 
-  if (url == node->GetURL())
+  if (node->GetURL() == url)
     return;
 
-  AsMutable(node)->InvalidateFavicon();
-  CancelPendingFaviconLoadRequests(AsMutable(node));
+  BookmarkNode* mutable_node = AsMutable(node);
+  mutable_node->InvalidateFavicon();
+  CancelPendingFaviconLoadRequests(mutable_node);
 
   {
     base::AutoLock url_lock(url_lock_);
     NodesOrderedByURLSet::iterator i = nodes_ordered_by_url_set_.find(
-        AsMutable(node));
+        mutable_node);
     DCHECK(i != nodes_ordered_by_url_set_.end());
     // i points to the first node with the URL, advance until we find the
     // node we're removing.
@@ -327,8 +328,8 @@ void BookmarkModel::SetURL(const BookmarkNode* node, const GURL& url) {
       ++i;
     nodes_ordered_by_url_set_.erase(i);
 
-    AsMutable(node)->SetURL(url);
-    nodes_ordered_by_url_set_.insert(AsMutable(node));
+    mutable_node->SetURL(url);
+    nodes_ordered_by_url_set_.insert(mutable_node);
   }
 
   if (store_.get())
@@ -401,8 +402,7 @@ const BookmarkNode* BookmarkModel::AddFolder(const BookmarkNode* parent,
     return NULL;
   }
 
-  BookmarkNode* new_node = new BookmarkNode(generate_next_node_id(),
-                                            GURL());
+  BookmarkNode* new_node = new BookmarkNode(generate_next_node_id(), GURL());
   new_node->set_date_folder_modified(Time::Now());
   new_node->set_title(title);
   new_node->set_type(BookmarkNode::FOLDER);
@@ -530,12 +530,6 @@ bool BookmarkModel::IsBookmarkedNoLock(const GURL& url) {
           nodes_ordered_by_url_set_.end());
 }
 
-void BookmarkModel::FaviconLoaded(const BookmarkNode* node) {
-  // Send out notification to the observer.
-  FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
-                    BookmarkNodeFaviconChanged(this, node));
-}
-
 void BookmarkModel::RemoveNode(BookmarkNode* node,
                                std::set<GURL>* removed_urls) {
   if (!loaded_ || !node || is_permanent_node(node)) {
@@ -772,6 +766,11 @@ void BookmarkModel::LoadFavicon(BookmarkNode* node) {
   node->set_favicon_load_handle(handle);
 }
 
+void BookmarkModel::FaviconLoaded(const BookmarkNode* node) {
+  FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
+                    BookmarkNodeFaviconChanged(this, node));
+}
+
 void BookmarkModel::CancelPendingFaviconLoadRequests(BookmarkNode* node) {
   if (node->favicon_load_handle()) {
     FaviconService* favicon_service =
diff --git a/chrome/browser/bookmarks/bookmark_model.h b/chrome/browser/bookmarks/bookmark_model.h
index 75a6e4c..630b4cc 100644
--- a/chrome/browser/bookmarks/bookmark_model.h
+++ b/chrome/browser/bookmarks/bookmark_model.h
@@ -69,9 +69,8 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode> {
   Type type() const { return type_; }
   void set_type(Type type) { type_ = type; }
 
-  // Returns the time the bookmark/folder was added.
+  // Returns the time the node was added.
   const base::Time& date_added() const { return date_added_; }
-  // Sets the time the bookmark/folder was added.
   void set_date_added(const base::Time& date) { date_added_ = date; }
 
   // Returns the last time the folder was modified. This is only maintained
@@ -83,7 +82,7 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode> {
     date_folder_modified_ = date;
   }
 
-  // Convenience for testing if this nodes represents a folder. A folder is a
+  // Convenience for testing if this node represents a folder. A folder is a
   // node whose type is not URL.
   bool is_folder() const { return type_ != URL; }
   bool is_url() const { return type_ == URL; }
@@ -108,9 +107,6 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode> {
     favicon_load_handle_ = handle;
   }
 
-  // Called when the favicon becomes invalid.
-  void InvalidateFavicon();
-
   // Accessor method for controlling the visibility of a bookmark node/sub-tree.
   // Note that visibility is not propagated down the tree hierarchy so if a
   // parent node is marked as invisible, a child node may return "Visible". This
@@ -129,6 +125,9 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode> {
   // A helper function to initialize various fields during construction.
   void Initialize(int64 id);
 
+  // Called when the favicon becomes invalid.
+  void InvalidateFavicon();
+
   // The unique identifier for this node.
   int64 id_;
 
@@ -354,9 +353,6 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
   // obtain a lock on url_lock_.
   bool IsBookmarkedNoLock(const GURL& url);
 
-  // Overriden to notify the observer the favicon has been loaded.
-  void FaviconLoaded(const BookmarkNode* node);
-
   // Removes the node from internal maps and recurses through all children. If
   // the node is a url, its url is added to removed_urls.
   //
@@ -367,7 +363,7 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
   // BookmarkModel takes ownership of |details|.
   void DoneLoading(BookmarkLoadDetails* details);
 
-  // Populates nodes_ordered_by_url_set_ from root.
+  // Populates |nodes_ordered_by_url_set_| from root.
   void PopulateNodesByURL(BookmarkNode* node);
 
   // Removes the node from its parent, sends notification, and deletes it.
@@ -401,10 +397,13 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
   // favicon service.
   void LoadFavicon(BookmarkNode* node);
 
+  // Called to notify the observers that the favicon has been loaded.
+  void FaviconLoaded(const BookmarkNode* node);
+
   // If we're waiting on a favicon for node, the load request is canceled.
   void CancelPendingFaviconLoadRequests(BookmarkNode* node);
 
-  // NotificationObserver.
+  // NotificationObserver:
   virtual void Observe(int type,
                        const NotificationSource& source,
                        const NotificationDetails& details);
@@ -451,8 +450,8 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
 
   // Set of nodes ordered by URL. This is not a map to avoid copying the
   // urls.
-  // WARNING: nodes_ordered_by_url_set_ is accessed on multiple threads. As
-  // such, be sure and wrap all usage of it around url_lock_.
+  // WARNING: |nodes_ordered_by_url_set_| is accessed on multiple threads. As
+  // such, be sure and wrap all usage of it around |url_lock_|.
   typedef std::multiset<BookmarkNode*, NodeURLComparator> NodesOrderedByURLSet;
   NodesOrderedByURLSet nodes_ordered_by_url_set_;
   base::Lock url_lock_;