bookmarks: Rename |loaded_favicon_| to |is_favicon_loaded_| so it matches with the getter accessor.

bookmarks: Rename |loaded_favicon_| to |is_favicon_loaded_| so it matches with the getter accessor.

Updates the setter as well.

Some other changes:
- Improve the comments in the BookmarkNode class.
- Reorganize the member variables of BookmarkNode class to match with the
  getter/setter accessors.
- Reorder some functions in the source file to match with the declaration in the header file.

BUG=None
TEST=None

R=sky@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91858

[tfarina, 2011-07-06 20:15:37]

http://codereview.chromium.org/7318001/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/7318001/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_model.cc:55: type_ = !url_.is_empty() ? URL :
BOOKMARK_BAR;
Scott, shouldn't this change from BOOKMARK_BAR to FOLDER?

[sky, 2011-07-07 15:52:17]

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.h (left):

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:42: // star id and type. BookmarkNodes
are returned from a BookmarkModel.
I prefer the old description, minus the 'star' part.

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:52: // Creates a new node with id of 0
and |url|.
'with id' -> 'with an id'

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:101: void set_favicon_loaded(bool
loaded) { favicon_loaded_ = loaded; }
I prefer the old, so can you rename the field to is_favicon_loaded_ and rename
the setter to match.

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:120: void InvalidateFavicon();
Move this to be with other favicon methods too.

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:128: // A helper function to
initialize some of the variables below.
Old description is better.

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:135: // changes to the URL should be
done through the BookmarkModel.
'changes changes' -> changes.
'should be' -> 'must be'

[tfarina, 2011-07-07 17:03:29]

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.h (left):

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:42: // star id and type. BookmarkNodes
are returned from a BookmarkModel.
On 2011/07/07 15:52:17, sky wrote:
> I prefer the old description, minus the 'star' part.

Done.

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:52: // Creates a new node with id of 0
and |url|.
On 2011/07/07 15:52:17, sky wrote:
> 'with id' -> 'with an id'

Done.

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:101: void set_favicon_loaded(bool
loaded) { favicon_loaded_ = loaded; }
On 2011/07/07 15:52:17, sky wrote:
> I prefer the old, so can you rename the field to is_favicon_loaded_ and rename
> the setter to match.

Done.

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:120: void InvalidateFavicon();
On 2011/07/07 15:52:17, sky wrote:
> Move this to be with other favicon methods too.

Done.

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:128: // A helper function to
initialize some of the variables below.
On 2011/07/07 15:52:17, sky wrote:
> Old description is better.

Done.

http://codereview.chromium.org/7318001/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.h:135: // changes to the URL should be
done through the BookmarkModel.
On 2011/07/07 15:52:17, sky wrote:
> 'changes changes' -> changes.
> 'should be' -> 'must be'

Done.

http://codereview.chromium.org/7318001/diff/3012/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/7318001/diff/3012/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:71: type_ = !url_.is_empty() ? URL :
BOOKMARK_BAR;
So, instead of BOOKMARK_BAR shouldn't this be FOLDER?

http://codereview.chromium.org/7318001/diff/3012/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:237:
mutable_node->set_is_favicon_loaded(true);
Should we remove the friend relationship with BookmarkModel from BookmarkNode?
It seems we aren't using it at all. Otherwise, this would have to be
mutable_node->is_favicon_loaded_ = true;

[sky, 2011-07-07 22:26:36]

http://codereview.chromium.org/7318001/diff/3012/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/7318001/diff/3012/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:71: type_ = !url_.is_empty() ? URL :
BOOKMARK_BAR;
On 2011/07/07 17:03:29, tfarina wrote:
> So, instead of BOOKMARK_BAR shouldn't this be FOLDER?

Yes, I believe everyone sets the type, so that should be fine.

http://codereview.chromium.org/7318001/diff/3012/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:237:
mutable_node->set_is_favicon_loaded(true);
On 2011/07/07 17:03:29, tfarina wrote:
> Should we remove the friend relationship with BookmarkModel from BookmarkNode?
> It seems we aren't using it at all. Otherwise, this would have to be
> mutable_node->is_favicon_loaded_ = true;
> 

If the friend is not needed it, remove it.

[tfarina, 2011-07-07 23:08:59]

http://codereview.chromium.org/7318001/diff/3012/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/7318001/diff/3012/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_model.cc:71: type_ = !url_.is_empty() ? URL :
BOOKMARK_BAR;
On 2011/07/07 22:26:36, sky wrote:
> On 2011/07/07 17:03:29, tfarina wrote:
> > So, instead of BOOKMARK_BAR shouldn't this be FOLDER?
> 
> Yes, I believe everyone sets the type, so that should be fine.

Done.

[sky, 2011-07-08 15:32:50]

LGTM