Mac bookmark work. ...

Mac bookmark work.  

- The bookmark menu is populated dynamically with bookmarks, including
  subfolders --> submenus.  E.g. star something --> shows up in menu.
  Menu items are disabled but always present and current.
- Always Show Bookmarks" menu now live; reads from / writes to
  preference, and shows correct "toggle state".
- Bookmark bar on each tab, present if requested.  (Currently an empty box).
- Random stuff; e.g. bookmark prefs init moved to a x-plat location.

This CL does not contain Cole's views.

Bried english description of the nib file changes:
- add a new view for the bookmark bar in the tab; hook it up to the controller
- Many tag sets (e.g. View-->Always Show Bookmarks Bar now 40009)
- Remove dummy bookmark menu items


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

[John Grabowski, 2009-03-14 02:55:23]

This will probably fail the try servers (conflict with recent pink tabs changes)
but I thought I'd get the review process started.  Sorry it's big but I didn't
hit a reasonable stopping point until now.

[TVL, 2009-03-16 12:29:14]

drive by nits.  :)

http://codereview.chromium.org/46078/diff/1/3
File chrome/app/chrome_dll_resource.h (right):

http://codereview.chromium.org/46078/diff/1/3#newcode3
Line 3: // Used by chrome_dll.rc
if this is generated, why are we checking it in?

http://codereview.chromium.org/46078/diff/1/13
File chrome/browser/cocoa/bookmark_bar_state_controller.h (right):

http://codereview.chromium.org/46078/diff/1/13#newcode15
Line 15: @private
indent 1 space

http://codereview.chromium.org/46078/diff/1/23
File chrome/browser/cocoa/bookmark_bar_state_controller_unittest.mm (right):

http://codereview.chromium.org/46078/diff/1/23#newcode24
Line 24: public:
only indent 1 char

http://codereview.chromium.org/46078/diff/1/21
File chrome/browser/cocoa/bookmark_menu_bridge.h (right):

http://codereview.chromium.org/46078/diff/1/21#newcode46
Line 46: void ClearBookmarkMenu(NSMenu *menu);
nit: this uses "Type *", rest of file seems to use "Type* ", use one style?

http://codereview.chromium.org/46078/diff/1/27
File chrome/browser/cocoa/bookmark_menu_bridge.mm (right):

http://codereview.chromium.org/46078/diff/1/27#newcode38
Line 38: int new_index) {
align (and the rest of the file down)

http://codereview.chromium.org/46078/diff/1/20
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/46078/diff/1/20#newcode46
Line 46: [[TabStripController alloc] initWithView:tabStripView_
browser:browser_];
nit: 4 space intent for continuatio

http://codereview.chromium.org/46078/diff/1/26
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/46078/diff/1/26#newcode47
Line 47: IBOutlet ToolbarView* /* BookmarkView* */ bookmarkView_;
why not make it BookmarkView then?

[Avi (use Gerrit), 2009-03-16 14:49:33]

What he said, plus...

(but mostly nits; LG otherwise)

http://codereview.chromium.org/46078/diff/1/21
File chrome/browser/cocoa/bookmark_menu_bridge.h (right):

http://codereview.chromium.org/46078/diff/1/21#newcode50
Line 50: // At most |nodesleft| will be added.
What's |nodesleft|?

http://codereview.chromium.org/46078/diff/1/27
File chrome/browser/cocoa/bookmark_menu_bridge.mm (right):

http://codereview.chromium.org/46078/diff/1/27#newcode81
Line 81: [menu removeItem:item];  // Will eventually [obj release] all it's kids
its

http://codereview.chromium.org/46078/diff/1/18
File chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm (right):

http://codereview.chromium.org/46078/diff/1/18#newcode16
Line 16: // Friend hastle
hastle? what does that mean?

http://codereview.chromium.org/46078/diff/1/18#newcode103
Line 103: delete root;  // deletes all it's kids
its

http://codereview.chromium.org/46078/diff/1/26
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/46078/diff/1/26#newcode33
Line 33: // have it's own ToolbarView.  Similarly, each also has it's own
its x 2

[John Grabowski, 2009-03-17 00:27:30]

New diffs uploaded.

http://codereview.chromium.org/46078/diff/1/13
File chrome/browser/cocoa/bookmark_bar_state_controller.h (right):

http://codereview.chromium.org/46078/diff/1/13#newcode15
Line 15: @private
On 2009/03/16 12:29:14, TVL wrote:
> indent 1 space

Done.
Thwarted by Emacs than can't handle c++-mode and objc-mode at the same time. 
Curse you Stallman!!!

http://codereview.chromium.org/46078/diff/1/23
File chrome/browser/cocoa/bookmark_bar_state_controller_unittest.mm (right):

http://codereview.chromium.org/46078/diff/1/23#newcode24
Line 24: public:
On 2009/03/16 12:29:14, TVL wrote:
> only indent 1 char

fixed

http://codereview.chromium.org/46078/diff/1/21
File chrome/browser/cocoa/bookmark_menu_bridge.h (right):

http://codereview.chromium.org/46078/diff/1/21#newcode46
Line 46: void ClearBookmarkMenu(NSMenu *menu);
On 2009/03/16 12:29:14, TVL wrote:
> nit: this uses "Type *", rest of file seems to use "Type* ", use one style?

Fixed.  jgm and I used "Type *" in Keystone so it's easy for me to revert back
to familiar patterns when in objc-land.

http://codereview.chromium.org/46078/diff/1/21#newcode50
Line 50: // At most |nodesleft| will be added.
On 2009/03/16 14:49:34, Avi wrote:
> What's |nodesleft|?

I had planned on passing down a var to limit max menus but didn't get that in
this CL.  Comment removed; TODO(jrg) added.

http://codereview.chromium.org/46078/diff/1/27
File chrome/browser/cocoa/bookmark_menu_bridge.mm (right):

http://codereview.chromium.org/46078/diff/1/27#newcode38
Line 38: int new_index) {
On 2009/03/16 12:29:14, TVL wrote:
> align (and the rest of the file down)

aligned

http://codereview.chromium.org/46078/diff/1/27#newcode81
Line 81: [menu removeItem:item];  // Will eventually [obj release] all it's kids
On 2009/03/16 14:49:34, Avi wrote:
> its

sedded

http://codereview.chromium.org/46078/diff/1/18
File chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm (right):

http://codereview.chromium.org/46078/diff/1/18#newcode16
Line 16: // Friend hastle
On 2009/03/16 14:49:34, Avi wrote:
> hastle? what does that mean?

Comment clarified.

http://codereview.chromium.org/46078/diff/1/18#newcode103
Line 103: delete root;  // deletes all it's kids
On 2009/03/16 14:49:34, Avi wrote:
> its

sedded.  I can't stop myself with the it's; it's like an OCD.

http://codereview.chromium.org/46078/diff/1/20
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/46078/diff/1/20#newcode46
Line 46: [[TabStripController alloc] initWithView:tabStripView_
browser:browser_];
On 2009/03/16 12:29:14, TVL wrote:
> nit: 4 space intent for continuatio

reindented

http://codereview.chromium.org/46078/diff/1/26
File chrome/browser/cocoa/tab_contents_controller.h (right):

http://codereview.chromium.org/46078/diff/1/26#newcode33
Line 33: // have it's own ToolbarView.  Similarly, each also has it's own
On 2009/03/16 14:49:34, Avi wrote:
> its x 2

de-OCD-ified

http://codereview.chromium.org/46078/diff/1/26#newcode47
Line 47: IBOutlet ToolbarView* /* BookmarkView* */ bookmarkView_;
On 2009/03/16 12:29:14, TVL wrote:
> why not make it BookmarkView then?

Because it's not written yet!

Using ToolbarView since it has the right background color.
Will likely use ToolbarView as a base class (or refactor) to make theming
easier.

[John Grabowski, 2009-03-17 01:04:22]

tvl sez:
>> drive by nits.  :)

Drive by anytime!

[Mark Mentovai, 2009-03-17 02:52:25]

Drive-bys from me too.

This was a big job, and a nice job.  LGTM with these changes as far as I'm
concerned.

http://codereview.chromium.org/46078/diff/2005/2027
File chrome/browser/cocoa/bookmark_bar_state_controller_unittest.mm (right):

http://codereview.chromium.org/46078/diff/2005/2027#newcode35
Line 35: EXPECT_EQ(NO, c->toggled_);
The checks for c->toggled_ should be EXPECT_TRUE and EXPECT_FALSE instead of
EXPECT_EQ(YES and EXPECT_EQ(NO, right?

The old_visible vs. [c visible] checks should be adjusted similarly.

http://codereview.chromium.org/46078/diff/2005/2025
File chrome/browser/cocoa/bookmark_menu_bridge.h (right):

http://codereview.chromium.org/46078/diff/2005/2025#newcode21
Line 21: friend class BookmarkMenuBridgeTest;
This should move into the private section.

http://codereview.chromium.org/46078/diff/2005/2031
File chrome/browser/cocoa/bookmark_menu_bridge.mm (right):

http://codereview.chromium.org/46078/diff/2005/2031#newcode60
Line 60: // Nothing to do here -- no icons in the menubar menus
Yet?

http://codereview.chromium.org/46078/diff/2005/2031#newcode91
Line 91: for (int i=0; i<node->GetChildCount(); i++) {
Spaces around the <

http://codereview.chromium.org/46078/diff/2005/2022
File chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm (right):

http://codereview.chromium.org/46078/diff/2005/2022#newcode14
Line 14: public:
Unindent by 1.

http://codereview.chromium.org/46078/diff/2005/2022#newcode44
Line 44: BookmarkMenuBridge *bridge = new BookmarkMenuBridge(browser);
Try to be a little more consistent with "T* v" (previous line) and "T *v" (this
line).  Most Chromium code uses the former.  This file bounces back and forth
between styles.

http://codereview.chromium.org/46078/diff/2005/2022#newcode57
Line 57: // Make sure all IDC_BOOKMARK items removed, and all items with
Sentenceify

http://codereview.chromium.org/46078/diff/2005/2018
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/46078/diff/2005/2018#newcode14
Line 14: : browser_(browser), controller_(controller), window_(window),
Fix indent

http://codereview.chromium.org/46078/diff/2005/2018#newcode149
Line 149: return [controller_ toggleBookmarkBar];
This function and toggleBookmarkBar both return void, right?  This should just
be written without the "return."

http://codereview.chromium.org/46078/diff/2005/2023
File chrome/browser/cocoa/tab_strip_controller.h (right):

http://codereview.chromium.org/46078/diff/2005/2023#newcode80
Line 80: // Return YES or NO (ObjC bool) to say if the bookmark bar is visible.
Saying "ObjC bool" is extraneous here, since the declaration with the type
(BOOL) is right on the next line.

http://codereview.chromium.org/46078/diff/2005/2015
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/46078/diff/2005/2015#newcode659
Line 659: // Contents of these methods moved to bookmark_utils.cc:
Is this comment actually helping anyone now?  Assuming that the contents have
just plain moved, I don't think we should leave turds like this behind.  Source
code archaeologists can always dig through svn history.

http://codereview.chromium.org/46078/diff/2005/2013
File chrome/browser/views/bookmark_table_view.cc (right):

http://codereview.chromium.org/46078/diff/2005/2013#newcode53
Line 53: // Contents of BookmarkTableView::RegisterUserPrefs() moved
same

http://codereview.chromium.org/46078/diff/2005/2037
File chrome/chrome.gyp (right):

http://codereview.chromium.org/46078/diff/2005/2037#newcode483
Line 483: 'browser/cocoa/bookmark_menu_bridge.h',
bookmark < browser

http://codereview.chromium.org/46078/diff/2005/2037#newcode485
Line 485: 'browser/cocoa/bookmark_bar_state_controller.h',
bar < menu

http://codereview.chromium.org/46078/diff/2005/2037#newcode1899
Line 1899: 'browser/cocoa/browser_test_helper.h',
You should put these in the main 'sources' list and not inside a mac
conditional.  .mm files are excluded from non-Mac platforms, and nobody tries to
build headers.  And files in directories named cocoa are excluded from non-Macs
too.

[John Grabowski, 2009-03-17 17:48:21]

Applied Mark's comments, did a sync/merge, and uploaded new diffs.
If the try servers are happy this is what I'll land.
(Uh, unless someone changed chrome.gyp, in which case I'll play the game again.)

http://codereview.chromium.org/46078/diff/2005/2027
File chrome/browser/cocoa/bookmark_bar_state_controller_unittest.mm (right):

http://codereview.chromium.org/46078/diff/2005/2027#newcode35
Line 35: EXPECT_EQ(NO, c->toggled_);
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> The checks for c->toggled_ should be EXPECT_TRUE and EXPECT_FALSE instead of
> EXPECT_EQ(YES and EXPECT_EQ(NO, right?
> 
> The old_visible vs. [c visible] checks should be adjusted similarly.

fixed

http://codereview.chromium.org/46078/diff/2005/2025
File chrome/browser/cocoa/bookmark_menu_bridge.h (right):

http://codereview.chromium.org/46078/diff/2005/2025#newcode21
Line 21: friend class BookmarkMenuBridgeTest;
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> This should move into the private section.
moved

http://codereview.chromium.org/46078/diff/2005/2031
File chrome/browser/cocoa/bookmark_menu_bridge.mm (right):

http://codereview.chromium.org/46078/diff/2005/2031#newcode60
Line 60: // Nothing to do here -- no icons in the menubar menus
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> Yet?
Comment clarified.

http://codereview.chromium.org/46078/diff/2005/2031#newcode91
Line 91: for (int i=0; i<node->GetChildCount(); i++) {
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> Spaces around the <

spaced (in a few spots)

http://codereview.chromium.org/46078/diff/2005/2022
File chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm (right):

http://codereview.chromium.org/46078/diff/2005/2022#newcode14
Line 14: public:
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> Unindent by 1.

more c++-mode vs objc-mode :-(

http://codereview.chromium.org/46078/diff/2005/2022#newcode44
Line 44: BookmarkMenuBridge *bridge = new BookmarkMenuBridge(browser);
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> Try to be a little more consistent with "T* v" (previous line) and "T *v"
(this
> line).  Most Chromium code uses the former.  This file bounces back and forth
> between styles.

Consistentified.

http://codereview.chromium.org/46078/diff/2005/2022#newcode57
Line 57: // Make sure all IDC_BOOKMARK items removed, and all items with
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> Sentenceify

Sentencified.

http://codereview.chromium.org/46078/diff/2005/2018
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/46078/diff/2005/2018#newcode14
Line 14: : browser_(browser), controller_(controller), window_(window),
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> Fix indent

Emacs toys with me yet again.

http://codereview.chromium.org/46078/diff/2005/2018#newcode149
Line 149: return [controller_ toggleBookmarkBar];
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> This function and toggleBookmarkBar both return void, right?  This should just
> be written without the "return."

'return' removed.

http://codereview.chromium.org/46078/diff/2005/2023
File chrome/browser/cocoa/tab_strip_controller.h (right):

http://codereview.chromium.org/46078/diff/2005/2023#newcode80
Line 80: // Return YES or NO (ObjC bool) to say if the bookmark bar is visible.
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> Saying "ObjC bool" is extraneous here, since the declaration with the type
> (BOOL) is right on the next line.

The intent was clarity as a bool became a BOOL across the call chains.
Comment modified.

http://codereview.chromium.org/46078/diff/2005/2015
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/46078/diff/2005/2015#newcode659
Line 659: // Contents of these methods moved to bookmark_utils.cc:
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> Is this comment actually helping anyone now?  Assuming that the contents have
> just plain moved, I don't think we should leave turds like this behind. 
Source
> code archaeologists can always dig through svn history.

removed

http://codereview.chromium.org/46078/diff/2005/2013
File chrome/browser/views/bookmark_table_view.cc (right):

http://codereview.chromium.org/46078/diff/2005/2013#newcode53
Line 53: // Contents of BookmarkTableView::RegisterUserPrefs() moved
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> same

deleted

http://codereview.chromium.org/46078/diff/2005/2037
File chrome/chrome.gyp (right):

http://codereview.chromium.org/46078/diff/2005/2037#newcode483
Line 483: 'browser/cocoa/bookmark_menu_bridge.h',
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> bookmark < browser
sorted

http://codereview.chromium.org/46078/diff/2005/2037#newcode485
Line 485: 'browser/cocoa/bookmark_bar_state_controller.h',
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> bar < menu

sorted

http://codereview.chromium.org/46078/diff/2005/2037#newcode1899
Line 1899: 'browser/cocoa/browser_test_helper.h',
On 2009/03/17 02:52:26, Mark Mentovai wrote:
> You should put these in the main 'sources' list and not inside a mac
> conditional.  .mm files are excluded from non-Mac platforms, and nobody tries
to
> build headers.  And files in directories named cocoa are excluded from
non-Macs
> too.

moved

[Mark Mentovai, 2009-03-17 18:12:49]

http://codereview.chromium.org/46078/diff/1048/1070
File chrome/browser/cocoa/bookmark_bar_state_controller_unittest.mm (right):

http://codereview.chromium.org/46078/diff/1048/1070#newcode40
Line 40: EXPECT_NE(old_visible, [c visible]);
Doesn't this have the same problem of doing an |== YES| or |!= YES| comparison? 
Line 44 too.  Doing "a != b" for BOOL a, b is still a kind of shady practice.

These would probably be best if forced into a language boolean from a BOOL
boolean.  For example, one of these tricks:

EXPECT_NE(!!old_visible, !![c visible]);
EXPECT_NE(old_visible != NO, [c visible] != NO);
EXPECT_NE(old_visible ? true : false, [c visible] ? true : false);
EXPECT_NE((bool)old_visible, (bool)[c visible]);
EXPECT_NE(static_cast<bool>(old_visible), static_cast<bool>([c visible]));

It would be nice if gtest had EXPECT_NE_BOOL.

[John Grabowski, 2009-03-17 18:20:44]

http://codereview.chromium.org/46078/diff/1048/1070
File chrome/browser/cocoa/bookmark_bar_state_controller_unittest.mm (right):

http://codereview.chromium.org/46078/diff/1048/1070#newcode40
Line 40: EXPECT_NE(old_visible, [c visible]);
On 2009/03/17 18:12:49, Mark Mentovai wrote:
> Doesn't this have the same problem of doing an |== YES| or |!= YES|
comparison? 
> Line 44 too.  Doing "a != b" for BOOL a, b is still a kind of shady practice.
> 
> These would probably be best if forced into a language boolean from a BOOL
> boolean.  For example, one of these tricks:
> 
> EXPECT_NE(!!old_visible, !![c visible]);
> EXPECT_NE(old_visible != NO, [c visible] != NO);
> EXPECT_NE(old_visible ? true : false, [c visible] ? true : false);
> EXPECT_NE((bool)old_visible, (bool)[c visible]);
> EXPECT_NE(static_cast<bool>(old_visible), static_cast<bool>([c visible]));
> 
> It would be nice if gtest had EXPECT_NE_BOOL.

I do not agree that BOOL comparison is shady.  A constraint such as "can't
compare BOOLs" would make them less than useful.

Can you elaborate on your suspicion?

[Mark Mentovai, 2009-03-17 18:48:05]

http://codereview.chromium.org/46078/diff/1048/1070
File chrome/browser/cocoa/bookmark_bar_state_controller_unittest.mm (right):

http://codereview.chromium.org/46078/diff/1048/1070#newcode40
Line 40: EXPECT_NE(old_visible, [c visible]);
jrg wrote:
> I do not agree that BOOL comparison is shady.  A constraint such as "can't
> compare BOOLs" would make them less than useful.
> 
> Can you elaborate on your suspicion?

A BOOL may be neither YES nor NO, in which case it evaluates as true in a
boolean context but isn't strictly valid for comparison against another BOOL.

http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=BOOL_...

"Be careful with BOOL return values and mixing bool with BOOL. Avoid comparing
directly with YES."

which I read as meaning, by extension, "avoid directly comparing BOOL variables
that may contain YES."

There's also this: "Also be careful when interoperating with C++ and its bool
type. Don't just assume that YES and NO map directly to true and false. You can
use the ternary operator to succinctly map between them."  Now, note that in
bookmark_bar_state_controller.mm:34, you've (maybe without realizing) added
bool<->BOOL interoperation.  Although it happens to be a completely safe use
there, you wind up storing static_cast<BOOL>(true) and static_cast<BOOL>(false)
to visible_.  static_cast<BOOL>(true) may be distinct from YES.  Now, there's
nothing wrong with bookmark_bar_state_controller.mm, because a promoted bool
just winds up as 0 or 1, both of which are totally safe to stick in a BOOL.  But
there is something wrong with assuming that YES and 1 are the same, which is
what you've done on this line and on line 44.  YES is true, but true is not
necessarily YES.

Rather than relying (and forcing) NO==false and YES==true throughout the rest of
the code, it's far better to just put things into the right boolean context
whenever you need these sorts of comparisons.  This doesn't obviate other code's
responsibility for ensuring that its bool<->BOOL conversions are safe.

This isn't just mental masturbation.  I've seen it happen in the wild, fo'
sho'...