bookmark STAR bubble: Disambiguate folders with the same name...

bookmark STAR bubble: Disambiguate folders with the same name

BUG=http://crbug.com/19408

TEST=Create a bookmark.
Create 2 bookmark folders both with the same name, "foo".
Go to your bookmarked page.
Click STAR to get bookmark bubble.
Change parent folder to the 1st "foo".
Confirm it's there on the bar.
Click STAR to get bookmark bubble.
Change parent folder to the 2nd "foo".
Confirm it's there on the bar.
Click STAR to get bookmark bubble.
Chose "choose another folder" to be sure that logic still works.

[TVL, 2009-10-30 13:05:52]

http://codereview.chromium.org/340042/diff/1/3
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/340042/diff/1/3#newcode131
Line 131: [self addFolderNodes:model_->root_node() toComboBox:folderComboBox_];
should you make sure the array and combobox are empty?

http://codereview.chromium.org/340042/diff/1/3#newcode148
Line 148: // TODO(jrg): no distinction is made among folders with the same name.
is this still a concern?

http://codereview.chromium.org/340042/diff/1/3#newcode205
Line 205: for (NSInteger i=0; i<[folderComboBox_ numberOfItems]; i++) {
for (NSValue* possibleWrapper in parentMapping_) {
  const BookmarkNode* possible = static_cast<const
BookmarkNode*>([possibleWrapper pointerValue]);
  ...
}

the current loop won't actually dcheck if not found, it will throw an exception
because you'll use the index for "Choose another folder..." which is out of
range for the array.

[John Grabowski, 2009-11-01 20:55:53]

New diffs up.

http://codereview.chromium.org/340042/diff/1/3
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/340042/diff/1/3#newcode131
Line 131: [self addFolderNodes:model_->root_node() toComboBox:folderComboBox_];
On 2009/10/30 13:05:52, TVL wrote:
> should you make sure the array and combobox are empty?

This is only called from windowDidLoad, so only once.
DCHECKs added to insure that is always true.

http://codereview.chromium.org/340042/diff/1/3#newcode148
Line 148: // TODO(jrg): no distinction is made among folders with the same name.
On 2009/10/30 13:05:52, TVL wrote:
> is this still a concern?

Nope; comment removed.

http://codereview.chromium.org/340042/diff/1/3#newcode205
Line 205: for (NSInteger i=0; i<[folderComboBox_ numberOfItems]; i++) {
On 2009/10/30 13:05:52, TVL wrote:
> for (NSValue* possibleWrapper in parentMapping_) {
>   const BookmarkNode* possible = static_cast<const
> BookmarkNode*>([possibleWrapper pointerValue]);
>   ...
> }
> 
> the current loop won't actually dcheck if not found, it will throw an
exception
> because you'll use the index for "Choose another folder..." which is out of
> range for the array.

The goal of this function/loop is to find the index in folderComboBox_ to
select.  if I rewrite the loop to for/in instead of index++ I won't know what
value to apply to selectItemAtIndex:.
Added a DCHECK (and made the final DCHECK a NOTREACHED) to fix the problem you
addressed.

[TVL, 2009-11-02 12:18:49]

lgtm

http://codereview.chromium.org/340042/diff/5001/2004
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/340042/diff/5001/2004#newcode209
Line 209: i < (NSUInteger)[folderComboBox_ numberOfItems]-1;
just use [parentMapping_ count] so you can skip the -1 here?

[mrossetti, 2009-11-02 17:50:03]

LGTM

http://codereview.chromium.org/340042/diff/1/3
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/340042/diff/1/3#newcode148
Line 148: // TODO(jrg): no distinction is made among folders with the same name.
Stale TODO?

[pink (ping after 24hrs), 2009-11-02 18:07:18]

driveby nits.

http://codereview.chromium.org/340042/diff/5001/2004
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/340042/diff/5001/2004#newcode179
Line 179: if (selectedIndex == -1)  // no selection ever made
Capitalize, punctuation

http://codereview.chromium.org/340042/diff/5001/2004#newcode187
Line 187: [[parentMapping_ objectAtIndex:selectedIndex] pointerValue]);
indent 4 additional spaces from previous line

http://codereview.chromium.org/340042/diff/5001/2004#newcode207
Line 207: DCHECK((NSInteger)[parentMapping_ count] == [folderComboBox_
numberOfItems]-1);
80cols

http://codereview.chromium.org/340042/diff/5001/2004#newcode212
Line 212: [[parentMapping_ objectAtIndex:i] pointerValue]);
indent 4 additional

[John Grabowski, 2009-11-02 18:58:28]

Final diffs up.
Thx guys.

http://codereview.chromium.org/340042/diff/1/3
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/340042/diff/1/3#newcode148
Line 148: // TODO(jrg): no distinction is made among folders with the same name.
On 2009/11/02 17:50:03, mrossetti wrote:
> Stale TODO?

removed

http://codereview.chromium.org/340042/diff/5001/2004#newcode179
Line 179: if (selectedIndex == -1)  // no selection ever made
On 2009/11/02 18:07:18, pink wrote:
> Capitalize, punctuation

fixed

http://codereview.chromium.org/340042/diff/5001/2004#newcode187
Line 187: [[parentMapping_ objectAtIndex:selectedIndex] pointerValue]);
On 2009/11/02 18:07:18, pink wrote:
> indent 4 additional spaces from previous line

indented. TODO(jrg): fix emacs to do this for me.

http://codereview.chromium.org/340042/diff/5001/2004#newcode207
Line 207: [[parentMapping_ objectAtIndex:i] pointerValue]);
On 2009/11/02 18:07:18, pink wrote:
> 80cols

split

http://codereview.chromium.org/340042/diff/5001/2004#newcode209
Line 209: if (possible == parent) {
On 2009/11/02 12:18:49, TVL wrote:
> just use [parentMapping_ count] so you can skip the -1 here?

done

http://codereview.chromium.org/340042/diff/5001/2004#newcode212
Line 212: }
On 2009/11/02 18:07:18, pink wrote:
> indent 4 additional

fixed.  Same TODO as above.