Implement back/forward toolbar menus on Mac....

Implement back/forward toolbar menus on Mac (bug 13203).

Note: The drop-down menu is actually a drag-down (activating on
click-hold or on drag), working much like Safari's (and other Apple
apps, such as Dictionary). This can be changed to a pop-down if that's
what's desired.

[The previously included fix to bug 17990 has been split off to CL
159864.]

TODO: Show keyboard shortcut for "Show Full History". (No wait -- there
*is* no keyboard shortcut for that on the Mac.)

BUG=13203
TEST=Navigate around, check out and use the menus; do so in multiple tabs and windows.

[Avi (use Gerrit), 2009-08-04 15:04:35]

http://codereview.chromium.org/160496/diff/1023/1029
File chrome/browser/cocoa/back_forward_menu_controller.h (right):

http://codereview.chromium.org/160496/diff/1023/1029#newcode40
Line 40: @end  // @interface something
something?

http://codereview.chromium.org/160496/diff/1023/1035
File chrome/browser/cocoa/back_forward_menu_controller.mm (right):

http://codereview.chromium.org/160496/diff/1023/1035#newcode54
Line 54: // 0-th item must be blank.
Why?

http://codereview.chromium.org/160496/diff/1023/1035#newcode90
Line 90: //[theMenu insertItemWithTitle:@"Beep" action:@selector(beep:)
keyEquivalent:@"" atIndex:0];
pull this before commit

http://codereview.chromium.org/160496/diff/1023/1027
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/160496/diff/1023/1027#newcode332
Line 332: return;
I think someone else saw this bug too. This isn't part of the toolbar menu fix;
can you split this out for separate commit?

http://codereview.chromium.org/160496/diff/1023/1034
File chrome/browser/cocoa/delayedmenu_button.h (right):

http://codereview.chromium.org/160496/diff/1023/1034#newcode18
Line 18: // that the 0-th entry should be blank (and won't be displayed).
Another comment on the 0-th entry. Why?

http://codereview.chromium.org/160496/diff/1023/1031
File chrome/browser/cocoa/delayedmenu_button.mm (right):

http://codereview.chromium.org/160496/diff/1023/1031#newcode93
Line 93: // FIXME(viettrungluu@gmail.com): Don't ask me. I don't know what's
going on.
Let me guess; the 0-th menu item thing is because of the futzing you have to do
using an NSPopUpButtonCell to show the menu? Since you keep the command values
in the tags, how about dropping the whole "your menu must start at 1" thing and
putting in an extra menu item here? Keep the interface clean for your callers
and deal with the mess at the last moment.

Also, please put in a comment that this is done because there's no current way
to do a popup menu, and note that once we can drop 10.5 support we should switch
to -[NSMenu popUpMenuPositioningItem:atLocation:inView:].

[viettrungluu-gmail, 2009-08-04 19:17:15]

Thanks, Avi.

I split off the part requested into CL 159864.

See my big comment about the ugly 0-th menu item issue. It's really all Apple's
fault. We could kind of make it cleaner, but not without a cost (lots of hacky
code complexity, or disallowing the use of NSMenu's delegate and providing our
own delegate methods); I'm not sure either is a real improvement, but let me
know what you think.

I'll upload my current version (with the part from CL 159864 removed) in a
second (Patch Set 4, I guess).

http://codereview.chromium.org/160496/diff/1023/1029
File chrome/browser/cocoa/back_forward_menu_controller.h (right):

http://codereview.chromium.org/160496/diff/1023/1029#newcode40
Line 40: @end  // @interface something
On 2009/08/04 15:04:35, Avi wrote:
> something?

It's a, uh, wildcard. Right. Fixed.

http://codereview.chromium.org/160496/diff/1023/1035
File chrome/browser/cocoa/back_forward_menu_controller.mm (right):

http://codereview.chromium.org/160496/diff/1023/1035#newcode54
Line 54: // 0-th item must be blank.
On 2009/08/04 15:04:35, Avi wrote:
> Why?

It's because it's currently a pulldown, and for pulldown lists Cocoa wants to
store the title (which would be displayed in the button) in the 0-th position
(even if you're supposed to you're supposed to change it with |-setTitle:|; see
<http://developer.apple.com/documentation/Cocoa/Conceptual/MenuList/Articles/M...>).
It's horrible, but it's Apple's (or probably even NeXT's) fault.

I added an explanatory comment, though possibly I should deal with this
elsewhere to keep the interface clean (but that messes up the interface in other
ways). See my big reply below.

http://codereview.chromium.org/160496/diff/1023/1035#newcode90
Line 90: //[theMenu insertItemWithTitle:@"Beep" action:@selector(beep:)
keyEquivalent:@"" atIndex:0];
On 2009/08/04 15:04:35, Avi wrote:
> pull this before commit

D'oh. I keep on accidentally leaving cruft in. Gone.

http://codereview.chromium.org/160496/diff/1023/1027
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/160496/diff/1023/1027#newcode332
Line 332: return;
On 2009/08/04 15:04:35, Avi wrote:
> I think someone else saw this bug too. This isn't part of the toolbar menu
fix;
> can you split this out for separate commit?

Done. It's now CL 159864 (why aren't CL numbers increasing?). As punishment,
I'll set you as a reviewer. >:D

http://codereview.chromium.org/160496/diff/1023/1028
File chrome/browser/cocoa/clickhold_button_cell.mm (right):

http://codereview.chromium.org/160496/diff/1023/1028#newcode96
Line 96: //NSLog(@"reason for stopping: %d", state);
I guess I should cut this out too....

http://codereview.chromium.org/160496/diff/1023/1034
File chrome/browser/cocoa/delayedmenu_button.h (right):

http://codereview.chromium.org/160496/diff/1023/1034#newcode18
Line 18: // that the 0-th entry should be blank (and won't be displayed).
On 2009/08/04 15:04:35, Avi wrote:
> Another comment on the 0-th entry. Why?

See my previous reply.... I added an explanatory comment here too.

http://codereview.chromium.org/160496/diff/1023/1031
File chrome/browser/cocoa/delayedmenu_button.mm (right):

http://codereview.chromium.org/160496/diff/1023/1031#newcode93
Line 93: // FIXME(viettrungluu@gmail.com): Don't ask me. I don't know what's
going on.
On 2009/08/04 15:04:35, Avi wrote:
> Let me guess; the 0-th menu item thing is because of the futzing you have to
do
> using an NSPopUpButtonCell to show the menu? Since you keep the command values
> in the tags, how about dropping the whole "your menu must start at 1" thing
and
> putting in an extra menu item here? Keep the interface clean for your callers
> and deal with the mess at the last moment.

See below about keeping the interface clean (at the bottom).

As I said in previous comments, the 0-th menu item thing is because I'm using a
pulldown (using NSPopUpButtonCell -- a horrible, horrible hack, but the official
Apple-recommended one). The good news about using a dragdown NSPopUpButtonCell
is that Cocoa automatically flips it/moves it very appropriately to keep it on
the screen.

What's really strange is that the y-coordinate is in our superview's coordinate
system (this is correct, as demonstrated by proper "flipping" of the menu to the
top of the button, when you move the window so that the button's close to the
bottom of the screen), but the x-coordinate is in our coordinate system (as
demonstrated by the fact that the menus for both the forward and back buttons
end up placed properly). The "- 2.0" factor is a hack to line things up (which I
don't really understand either).

As I said, I understand nothing. :-(

> Also, please put in a comment that this is done because there's no current way
> to do a popup menu, and note that once we can drop 10.5 support we should
switch
> to -[NSMenu popUpMenuPositioningItem:atLocation:inView:].

Right (it's good to hear that such a thing is coming), though then we might have
to deal with "flipping" the menu and placing it correctly (when flipped, or when
at a left/right screen-edge) ourselves, which isn't the most pleasant thing in
the world. (Actually, unless you can tell or specify which way -- up or down --
the menu will be displayed, it's not obvious how to specify the location
correctly.)


Interface cleanliness:

You're right about keeping the interface clean (ugh -- if we decide that a
pop-up menu is more appropriate than a pulldown, then this 0-th item business
wouldn't apply), but it's a lose-lose situation. If the user (in our case,
BackForwardMenuController) of this control expects the menu as-is (a quite
reasonable assumption), then the user would expect NSMenu's delegate to function
normally. In particular, the user would expect |-menuNeedsUpdate:| delegate
method to work normally -- which is something BackForwardMenuController
currently uses (it makes itself its menu's delegate).

To keep the 0-th menu item business fully hidden from the user -- so that
NSMenu's delegate can be used normally also -- would require a huge amount of
hacky code (have DelayedMenuButton insert itself as an "intermediary
delegate"?). Alternatively, one could say that the NSMenu's delegates won't work
with DelayedMenuButton, and have DelayedMenuButton provide its own delegate
methods. Ugh.

[Avi (use Gerrit), 2009-08-04 19:29:34]

http://codereview.chromium.org/160496/diff/1023/1034
File chrome/browser/cocoa/delayedmenu_button.h (right):

http://codereview.chromium.org/160496/diff/1023/1034#newcode18
Line 18: // that the 0-th entry should be blank (and won't be displayed).
On 2009/08/04 19:17:16, viettrungluu wrote:
> I added an explanatory comment here too.

I see no added comment.

http://codereview.chromium.org/160496/diff/1023/1031
File chrome/browser/cocoa/delayedmenu_button.mm (right):

http://codereview.chromium.org/160496/diff/1023/1031#newcode93
Line 93: // FIXME(viettrungluu@gmail.com): Don't ask me. I don't know what's
going on.
Oh. Yeah, if it's that much trouble, then don't worry about it. As long as you
comment it enough that people can follow, you're fine. Why do you think I write
50-line comments? :)

[viettrungluu-gmail, 2009-08-04 19:54:18]

http://codereview.chromium.org/160496/diff/1023/1034
File chrome/browser/cocoa/delayedmenu_button.h (right):

http://codereview.chromium.org/160496/diff/1023/1034#newcode18
Line 18: // that the 0-th entry should be blank (and won't be displayed).
On 2009/08/04 19:29:34, Avi wrote:
> I see no added comment.

You weren't squinting hard enough. ;-) Fixed.

http://codereview.chromium.org/160496/diff/1023/1031
File chrome/browser/cocoa/delayedmenu_button.mm (right):

http://codereview.chromium.org/160496/diff/1023/1031#newcode93
Line 93: // FIXME(viettrungluu@gmail.com): Don't ask me. I don't know what's
going on.
On 2009/08/04 19:29:34, Avi wrote:
> Oh. Yeah, if it's that much trouble, then don't worry about it. As long as you
> comment it enough that people can follow, you're fine. Why do you think I
write
> 50-line comments? :)

Okay, following your lead, I added hopefully-sufficient comments.

[Avi (use Gerrit), 2009-08-04 19:55:45]

LG

[viettrungluu-gmail, 2009-08-05 06:24:19]

I updated the patch (in particular chrome.gyp) versus the current ToT, and it
still works. (Probably your update of the patch is the same....) Hopefully today
will be a better day for commits (and for the tree) than yesterday. Thanks!

[Avi (use Gerrit), 2009-08-05 15:13:33]

Committed r22478.

[Avi (use Gerrit), 2009-08-05 16:54:22]

On 2009/08/05 15:13:33, Avi wrote:
> Committed r22478.

Valgrind found some leaks. Investigating to see if it's an easy fix.

[Avi (use Gerrit), 2009-08-05 18:02:30]

Valgrind filed as http://code.google.com/p/chromium/issues/detail?id=18532

[Avi (use Gerrit), 2009-08-05 18:11:31]

On 2009/08/05 18:02:30, Avi wrote:
> Valgrind filed as http://code.google.com/p/chromium/issues/detail?id=18532

Reverting due to the valgrind leak:
http://build.chromium.org/buildbot/waterfall/builders/Chromium%20Mac%20(valgr...

[viettrungluu-gmail, 2009-08-05 21:47:06]

Patch Set 7 fixes the leak.

Patch Set 8 (to be uploaded in a second) merges with rohitrao's view resizing
changes (<http://codereview.chromium.org/159776>) ... which has since been
reverted. To apply it, rohitrao's patch has to go back in!

[Avi (use Gerrit), 2009-08-07 19:53:26]

Patched in set 8, but had to recreate the xib since it conflicted. Hope I got it
right :) but it worked for me.

Committed r22740; seems to pass all the tests.