Wires up sorting of bookmarks to the 'organize menu' in the bookmark...

Wires up sorting of bookmarks to the 'organize menu' in the bookmark
manager (Glen says no context menus for now). All
BookmarkModelObservers have been updated appropriately.

BUG=1750
TEST=bring up the bookmark manager and try the 'Reorder by title' menu
  item, make sure it works and I didn't screw up anything around it.


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

[ncarter (slow), 2009-02-27 21:22:34]

LGTM unless any of the following comments cause you to make more than cosmetic
changes.

http://codereview.chromium.org/27262/diff/1/12
File chrome/browser/bookmarks/bookmark_context_menu.cc (right):

http://codereview.chromium.org/27262/diff/1/12#newcode515
Line 515: return parent_ && parent_->GetParent();
If the GetParent() check is to protect against root node children, it might be
clearer to check (parent_ != model_->root_node()) like we do earlier in the
function.

http://codereview.chromium.org/27262/diff/1/16
File chrome/browser/bookmarks/bookmark_folder_tree_model.cc (right):

http://codereview.chromium.org/27262/diff/1/16#newcode144
Line 144: bn_to_folder[folder_node->GetChild(i)->value] =
folder_node->GetChild(i);
If this map always existed (for all FolderNodes), what would be the downside? 
It could be used to trivially implement GetFolderNodeForBookmarkNode.

That being said, I am also OK with the way things are here.

http://codereview.chromium.org/27262/diff/1/7
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/27262/diff/1/7#newcode1333
Line 1333: BookmarkNode* node) {
Please confirm that it's OK to not worry about the throbbing state here (because
the bubble disappears when it loses focus?)

[sky, 2009-02-27 21:42:06]

http://codereview.chromium.org/27262/diff/1/12
File chrome/browser/bookmarks/bookmark_context_menu.cc (right):

http://codereview.chromium.org/27262/diff/1/12#newcode515
Line 515: return parent_ && parent_->GetParent();
On 2009/02/27 21:22:34, nick wrote:
> If the GetParent() check is to protect against root node children, it might be
> clearer to check (parent_ != model_->root_node()) like we do earlier in the
> function.

Done.

http://codereview.chromium.org/27262/diff/1/16
File chrome/browser/bookmarks/bookmark_folder_tree_model.cc (right):

http://codereview.chromium.org/27262/diff/1/16#newcode144
Line 144: bn_to_folder[folder_node->GetChild(i)->value] =
folder_node->GetChild(i);
On 2009/02/27 21:22:34, nick wrote:
> If this map always existed (for all FolderNodes), what would be the downside? 
> It could be used to trivially implement GetFolderNodeForBookmarkNode.
> 
> That being said, I am also OK with the way things are here.

That's a good idea. I'm going to hold off at this time though so that we don't
have to go another round.

http://codereview.chromium.org/27262/diff/1/7
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/27262/diff/1/7#newcode1333
Line 1333: BookmarkNode* node) {
On 2009/02/27 21:22:34, nick wrote:
> Please confirm that it's OK to not worry about the throbbing state here
(because
> the bubble disappears when it loses focus?)

I added a stop, but no start with a description of why.