Enable and implement last 2 items in bookmark bar context menu: "Open...

Enable and implement last 2 items in bookmark bar context menu: "Open
All In New Window" and "Open All In Incognito Window"  (17616).

Bookmark Folders have the bar context menu, not the bookmark context
menu (26748).

Disable bookmark manager item on bookmark contextual menus (26944).

BUG=http://crbug.com/17616, http://crbug.com/26748, http://crbug.com/26944

TEST=Add some bookmarks.
Make sure "Open All In New Window" and "Open All In Incognito Window" work from the bar context menu.
Add a bookmark folder.
Make sure the context menu of the folder is the bar conetxt menu, not the 'mark context menu.
Make sure Bookmark Manager item is DISabled in both context menus.

nib change: hook up actions in the "Open All Blah" items just like the normal "Open All". 


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

[mrossetti, 2009-11-07 02:11:45]

LGTM

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

http://codereview.chromium.org/373022/diff/1/3#newcode565
Line 565: BookmarkNode* node =
(BookmarkNode*)bookmarkModel_->GetBookmarkBarNode();
Can we use const_cast in .mm's?  If so, one should be used here.

http://codereview.chromium.org/373022/diff/1/3#newcode577
Line 577: BookmarkNode* node =
(BookmarkNode*)bookmarkModel_->GetBookmarkBarNode();
const_cast.

[viettrungluu, 2009-11-07 02:35:02]

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

http://codereview.chromium.org/373022/diff/1/3#newcode314
Line 314: action == @selector(openAllBookmarksIncognitoWindow:)) &&
Is there a reason 2 out of 3 of the '==' conditions get an extra pair of
parentheses? Won't the third one feel left out?

http://codereview.chromium.org/373022/diff/1/3#newcode324
Line 324: BrowserList::GetLastActive()->OpenURL(url, GURL(), disposition,
Are you absolutely, 110% positive that GetLastActive() will always return the
right browser in this situation?

The advantage of the URL delegate is that this question doesn't arise; the
disadvantage is that it's another thing which can go stale. Might it be better
to grab the BrowserWindowController through the window?

http://codereview.chromium.org/373022/diff/1/7
File chrome/browser/cocoa/bookmark_bar_controller_unittest.mm (right):

http://codereview.chromium.org/373022/diff/1/7#newcode128
Line 128: resizeDelegate:resizeDelegate_.get()]);
I have to say I'm not hugely thrilled with the alignment here, though it's a
tough situation. I'd go for indenting a bit more, and lining up along the ':'.

[pink (ping after 24hrs), 2009-11-11 16:40:06]

http://codereview.chromium.org/373022/diff/1/9
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/373022/diff/1/9#newcode2390
Line 2390: #if !defined(OS_MACOSX)
TODO? bug#? This looks intentional, when clearly it's temporary. Usually we've
been handling this in the nib file by not setting the tag on the item, rather
than adding ifdefs to XP code.

http://codereview.chromium.org/373022/diff/1/3
File chrome/browser/cocoa/bookmark_bar_controller.mm (left):

http://codereview.chromium.org/373022/diff/1/3#oldcode322
Line 322: [urlDelegate_ openBookmarkURL:node->GetURL()
why did you remove this?

[pink (ping after 24hrs), 2009-11-11 16:40:29]

http://codereview.chromium.org/373022/diff/1/9
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/373022/diff/1/9#newcode2390
Line 2390: #if !defined(OS_MACOSX)
TODO? bug#? This looks intentional, when clearly it's temporary. Usually
we've been handling this in the nib file by not setting the tag on the
item, rather than adding ifdefs to XP code.

http://codereview.chromium.org/373022/diff/1/3
File chrome/browser/cocoa/bookmark_bar_controller.mm (left):

http://codereview.chromium.org/373022/diff/1/3#oldcode322
Line 322: [urlDelegate_ openBookmarkURL:node->GetURL()
why did you remove this?

http://codereview.chromium.org/373022

[John Grabowski, 2009-11-12 00:24:55]

http://codereview.chromium.org/373022/diff/1/9
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/373022/diff/1/9#newcode2390
Line 2390: #if !defined(OS_MACOSX)
On 2009/11/11 16:40:06, pink wrote:
> TODO? bug#? This looks intentional, when clearly it's temporary. Usually we've
> been handling this in the nib file by not setting the tag on the item, rather
> than adding ifdefs to XP code.

There are at least 3 nib spots which would need to be changed.  I wanted to make
it easier to find and fix all at once.  Bug # added.

http://codereview.chromium.org/373022/diff/1/3
File chrome/browser/cocoa/bookmark_bar_controller.mm (left):

http://codereview.chromium.org/373022/diff/1/3#oldcode322
Line 322: [urlDelegate_ openBookmarkURL:node->GetURL()
On 2009/11/11 16:40:06, pink wrote:
> why did you remove this?

Because "open all in new window" doesn't want to open in the old window.  I
believe the original intent of urlDelegate was to make mocking easier.

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

http://codereview.chromium.org/373022/diff/1/3#newcode314
Line 314: action == @selector(openAllBookmarksIncognitoWindow:)) &&
On 2009/11/07 02:35:02, viettrungluu wrote:
> Is there a reason 2 out of 3 of the '==' conditions get an extra pair of
> parentheses? Won't the third one feel left out?

Loneliness averted with the hug of a paren.

http://codereview.chromium.org/373022/diff/1/3#newcode324
Line 324: BrowserList::GetLastActive()->OpenURL(url, GURL(), disposition,
On 2009/11/07 02:35:02, viettrungluu wrote:
> Are you absolutely, 110% positive that GetLastActive() will always return the
> right browser in this situation?
> 
> The advantage of the URL delegate is that this question doesn't arise; the
> disadvantage is that it's another thing which can go stale. Might it be better
> to grab the BrowserWindowController through the window?

Yes.

URLDelegate can give wrong answer!  For "open all in new window", we need to
create a new window.  In that case the urlDelegate is the OLD window.

http://codereview.chromium.org/373022/diff/1/3#newcode565
Line 565: BookmarkNode* node =
(BookmarkNode*)bookmarkModel_->GetBookmarkBarNode();
On 2009/11/07 02:11:45, mrossetti wrote:
> Can we use const_cast in .mm's?  If so, one should be used here.

done

http://codereview.chromium.org/373022/diff/1/3#newcode577
Line 577: BookmarkNode* node =
(BookmarkNode*)bookmarkModel_->GetBookmarkBarNode();
On 2009/11/07 02:11:45, mrossetti wrote:
> const_cast.

done

http://codereview.chromium.org/373022/diff/1/7
File chrome/browser/cocoa/bookmark_bar_controller_unittest.mm (right):

http://codereview.chromium.org/373022/diff/1/7#newcode128
Line 128: resizeDelegate:resizeDelegate_.get()]);
On 2009/11/07 02:35:02, viettrungluu wrote:
> I have to say I'm not hugely thrilled with the alignment here, though it's a
> tough situation. I'd go for indenting a bit more, and lining up along the ':'.

Realigned.  (I probably did a M-x replace-string which messed things up).

[pink (ping after 24hrs), 2009-11-12 14:58:51]

Actually, the original purpose of the url delegate was such that the
bookmark bar didn't need to have logic to load urls (and rely on
TabContents, etc etc). BWC already knows about that stuff, but things
like the tab strip (drag&drop) and the bookmark bar shouldn't have to
know about our loading logic.

On Wed, Nov 11, 2009 at 7:24 PM,  <jrg@chromium.org> wrote:
>
> http://codereview.chromium.org/373022/diff/1/9
> File chrome/browser/browser.cc (right):
>
> http://codereview.chromium.org/373022/diff/1/9#newcode2390
> Line 2390: #if !defined(OS_MACOSX)
> On 2009/11/11 16:40:06, pink wrote:
>>
>> TODO? bug#? This looks intentional, when clearly it's temporary.
>
> Usually we've
>>
>> been handling this in the nib file by not setting the tag on the item,
>
> rather
>>
>> than adding ifdefs to XP code.
>
> There are at least 3 nib spots which would need to be changed. =A0I wante=
d
> to make it easier to find and fix all at once. =A0Bug # added.
>
> http://codereview.chromium.org/373022/diff/1/3
> File chrome/browser/cocoa/bookmark_bar_controller.mm (left):
>
> http://codereview.chromium.org/373022/diff/1/3#oldcode322
> Line 322: [urlDelegate_ openBookmarkURL:node->GetURL()
> On 2009/11/11 16:40:06, pink wrote:
>>
>> why did you remove this?
>
> Because "open all in new window" doesn't want to open in the old window.
> =A0I believe the original intent of urlDelegate was to make mocking
> easier.
>
> http://codereview.chromium.org/373022/diff/1/3
> File chrome/browser/cocoa/bookmark_bar_controller.mm (right):
>
> http://codereview.chromium.org/373022/diff/1/3#newcode314
> Line 314: action =3D=3D @selector(openAllBookmarksIncognitoWindow:)) &&
> On 2009/11/07 02:35:02, viettrungluu wrote:
>>
>> Is there a reason 2 out of 3 of the '=3D=3D' conditions get an extra pai=
r
>
> of
>>
>> parentheses? Won't the third one feel left out?
>
> Loneliness averted with the hug of a paren.
>
> http://codereview.chromium.org/373022/diff/1/3#newcode324
> Line 324: BrowserList::GetLastActive()->OpenURL(url, GURL(),
> disposition,
> On 2009/11/07 02:35:02, viettrungluu wrote:
>>
>> Are you absolutely, 110% positive that GetLastActive() will always
>
> return the
>>
>> right browser in this situation?
>
>> The advantage of the URL delegate is that this question doesn't arise;
>
> the
>>
>> disadvantage is that it's another thing which can go stale. Might it
>
> be better
>>
>> to grab the BrowserWindowController through the window?
>
> Yes.
>
> URLDelegate can give wrong answer! =A0For "open all in new window", we
> need to create a new window. =A0In that case the urlDelegate is the OLD
> window.
>
> http://codereview.chromium.org/373022/diff/1/3#newcode565
> Line 565: BookmarkNode* node =3D
> (BookmarkNode*)bookmarkModel_->GetBookmarkBarNode();
> On 2009/11/07 02:11:45, mrossetti wrote:
>>
>> Can we use const_cast in .mm's? =A0If so, one should be used here.
>
> done
>
> http://codereview.chromium.org/373022/diff/1/3#newcode577
> Line 577: BookmarkNode* node =3D
> (BookmarkNode*)bookmarkModel_->GetBookmarkBarNode();
> On 2009/11/07 02:11:45, mrossetti wrote:
>>
>> const_cast.
>
> done
>
> http://codereview.chromium.org/373022/diff/1/7
> File chrome/browser/cocoa/bookmark_bar_controller_unittest.mm (right):
>
> http://codereview.chromium.org/373022/diff/1/7#newcode128
> Line 128: resizeDelegate:resizeDelegate_.get()]);
> On 2009/11/07 02:35:02, viettrungluu wrote:
>>
>> I have to say I'm not hugely thrilled with the alignment here, though
>
> it's a
>>
>> tough situation. I'd go for indenting a bit more, and lining up along
>
> the ':'.
>
> Realigned. =A0(I probably did a M-x replace-string which messed things
> up).
>
> http://codereview.chromium.org/373022
>



--=20
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[viettrungluu, 2009-11-12 16:18:08]

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

http://codereview.chromium.org/373022/diff/1/3#newcode324
Line 324: BrowserList::GetLastActive()->OpenURL(url, GURL(), disposition,
On 2009/11/12 00:24:55, John Grabowski wrote:
> On 2009/11/07 02:35:02, viettrungluu wrote:
> > Are you absolutely, 110% positive that GetLastActive() will always return
the
> > right browser in this situation?
> > 
> > The advantage of the URL delegate is that this question doesn't arise; the
> > disadvantage is that it's another thing which can go stale. Might it be
better
> > to grab the BrowserWindowController through the window?
> 
> Yes.
> 
> URLDelegate can give wrong answer!  For "open all in new window", we need to
> create a new window.  In that case the urlDelegate is the OLD window.

Which shouldn't matter, or are you saying that the urlDelegate didn't properly
respect the opening disposition?

In general, I don't think the bookmark bar controller should be mucking with
browsers directly (that's the BWC's job). The reason I don't like
BrowserList::GetLastActive() is that (a) it's crazy global state, and (b) I'm
not sure what would happen if JavaScript popped up a window at an inopportune
moment.