Use cocoa controls for HTML select popups in test_shell

First stage of implementing HTML select popup menus for
the Mac test_shell. Once the changes to
WebKit/WebCore/platform/chromium/PopupMenuChromium* have
been upstreamed (then landed in our tree), we can enable
them in test_shell.

The WebKit part is contained in this bug:
https://bugs.webkit.org/show_bug.cgi?id=24692

BUG=5095
(http://crbug.com/5095)


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

[pink (ping after 24hrs), 2009-03-19 13:45:45]

http://codereview.chromium.org/48149/diff/1028/64
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/48149/diff/1028/64#newcode563
Line 563: // TODO(paulg): Implement this for Mac HTML select menus.
do you want to put the bug number here for reference?

http://codereview.chromium.org/48149/diff/1028/63
File chrome/renderer/render_widget.h (right):

http://codereview.chromium.org/48149/diff/1028/63#newcode83
Line 83: 
omit the blank line since this is also from WebWidgetDelegate.

http://codereview.chromium.org/48149/diff/1028/55
File webkit/glue/chrome_client_impl.cc (right):

http://codereview.chromium.org/48149/diff/1028/55#newcode521
Line 521: WebViewDelegate* d = webview_->delegate();
should probably call the variable |delegate|. C++ style guide doesn't like
abbreviating like this.

http://codereview.chromium.org/48149/diff/1028/55#newcode524
Line 524: static_cast<WebWidgetImpl*>(d->CreatePopupWidget(webview_,
indent 4 spaces from previous line?

http://codereview.chromium.org/48149/diff/1028/53
File webkit/glue/webwidget_delegate.h (right):

http://codereview.chromium.org/48149/diff/1028/53#newcode23
Line 23: typedef struct MenuItem {
i don't think you need the typedef in c++. I believe you can just declare
|struct MenuItem { .. };|

http://codereview.chromium.org/48149/diff/1028/53#newcode34
Line 34: std::wstring label;
can we make this string16 from the start?

http://codereview.chromium.org/48149/diff/1028/53#newcode58
Line 58: // Used for popup windows.
Expand on the comment. What do the parameters mean? Why, for example, is
|item_height| important?

http://codereview.chromium.org/48149/diff/1028/60
File webkit/tools/test_shell/mac/test_webview_delegate.mm (right):

http://codereview.chromium.org/48149/diff/1028/60#newcode22
Line 22: bool menu_dismissed_;
make this @private

http://codereview.chromium.org/48149/diff/1028/60#newcode22
Line 22: bool menu_dismissed_;
should probably be a BOOL since it's obj-c. We tend to use obj-c types in obj-c
objects where we can. Additionally, it should be named |menuDismisssed_| as it's
in an obj-c object and you need to follow the obj-c naming guidelines.

http://codereview.chromium.org/48149/diff/1028/60#newcode40
Line 40: menu_dismissed_ = false;
this seems bakwards to me. After the menu item has been selected, shouldn't
"menu dismissed" be true, since the menu has been dismissed? What am i missing?

http://codereview.chromium.org/48149/diff/1028/60#newcode50
Line 50: [NSString stringWithUTF8String:WideToUTF8(item.label).c_str()];
you can use SysWideToNSString() instead here.

http://codereview.chromium.org/48149/diff/1028/60#newcode109
Line 109: void TestWebViewDelegate::ShowWithItems(
this code is pretty complex, how are we going to share it with the chrome
implementation of this method?

http://codereview.chromium.org/48149/diff/1028/60#newcode116
Line 116: NSMenu* menu = [[NSMenu alloc] initWithTitle:@""];
doesn't this leak? I don't see it being released.

http://codereview.chromium.org/48149/diff/1028/60#newcode173
Line 173: eventNumber:0  // This is clearly wrong.
if you're really concerned, you can try calling |NSApp currentEvent|, get its
event number, and increment by 1? I'm not sure if this would throw off the
global count, but it could work. 0 is probably fine for these purposes though.

http://codereview.chromium.org/48149/diff/1028/60#newcode196
Line 196: // TODO(paulg): Better to use autorelease on these objects?
yes, they should be autoreleased per the style guide. If someone inserts an
early return (eg, to bail early in an error condition), they'll leak.

http://codereview.chromium.org/48149/diff/1028/61
File webkit/tools/test_shell/test_webview_delegate.h (right):

http://codereview.chromium.org/48149/diff/1028/61#newcode222
Line 222: 
remove extra blank lines? they weren't there before.

[Paul Godavari, 2009-03-19 23:20:04]

http://codereview.chromium.org/48149/diff/1028/64
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/48149/diff/1028/64#newcode563
Line 563: // TODO(paulg): Implement this for Mac HTML select menus.
On 2009/03/19 13:45:45, pink wrote:
> do you want to put the bug number here for reference?

Done.

http://codereview.chromium.org/48149/diff/1028/63
File chrome/renderer/render_widget.h (right):

http://codereview.chromium.org/48149/diff/1028/63#newcode83
Line 83: 
On 2009/03/19 13:45:45, pink wrote:
> omit the blank line since this is also from WebWidgetDelegate.

Done.

http://codereview.chromium.org/48149/diff/1028/55
File webkit/glue/chrome_client_impl.cc (right):

http://codereview.chromium.org/48149/diff/1028/55#newcode521
Line 521: WebViewDelegate* d = webview_->delegate();
On 2009/03/19 13:45:45, pink wrote:
> should probably call the variable |delegate|. C++ style guide doesn't like
> abbreviating like this.

Changed (throughout the file).

http://codereview.chromium.org/48149/diff/1028/55#newcode524
Line 524: static_cast<WebWidgetImpl*>(d->CreatePopupWidget(webview_,
On 2009/03/19 13:45:45, pink wrote:
> indent 4 spaces from previous line?

Done.

http://codereview.chromium.org/48149/diff/1028/53
File webkit/glue/webwidget_delegate.h (right):

http://codereview.chromium.org/48149/diff/1028/53#newcode23
Line 23: typedef struct MenuItem {
On 2009/03/19 13:45:45, pink wrote:
> i don't think you need the typedef in c++. I believe you can just declare
> |struct MenuItem { .. };|

Changed to struct.

http://codereview.chromium.org/48149/diff/1028/53#newcode34
Line 34: std::wstring label;
On 2009/03/19 13:45:45, pink wrote:
> can we make this string16 from the start?

Done.

http://codereview.chromium.org/48149/diff/1028/53#newcode58
Line 58: // Used for popup windows.
On 2009/03/19 13:45:45, pink wrote:
> Expand on the comment. What do the parameters mean? Why, for example, is
> |item_height| important?

Done.

http://codereview.chromium.org/48149/diff/1028/60
File webkit/tools/test_shell/mac/test_webview_delegate.mm (right):

http://codereview.chromium.org/48149/diff/1028/60#newcode22
Line 22: bool menu_dismissed_;
On 2009/03/19 13:45:45, pink wrote:
> make this @private

Done.

http://codereview.chromium.org/48149/diff/1028/60#newcode22
Line 22: bool menu_dismissed_;
On 2009/03/19 13:45:45, pink wrote:
> should probably be a BOOL since it's obj-c. We tend to use obj-c types in
obj-c
> objects where we can. Additionally, it should be named |menuDismisssed_| as
it's
> in an obj-c object and you need to follow the obj-c naming guidelines.

Done.

http://codereview.chromium.org/48149/diff/1028/60#newcode40
Line 40: menu_dismissed_ = false;
On 2009/03/19 13:45:45, pink wrote:
> this seems bakwards to me. After the menu item has been selected, shouldn't
> "menu dismissed" be true, since the menu has been dismissed? What am i
missing?

By 'dimissed' I mean that the menu was dismissed without any entry being chosen
(for example, the user clicked outside the bounds of the menu, or hit the escape
key). I want to distinguish between an item being selected, and the menu being
dismissed without selection so I could forward the appropriate event to WebKit.

http://codereview.chromium.org/48149/diff/1028/60#newcode50
Line 50: [NSString stringWithUTF8String:WideToUTF8(item.label).c_str()];
On 2009/03/19 13:45:45, pink wrote:
> you can use SysWideToNSString() instead here.

Since |label| is now a string16, should I use base::SysUTF16ToNSString()?

http://codereview.chromium.org/48149/diff/1028/60#newcode109
Line 109: void TestWebViewDelegate::ShowWithItems(
On 2009/03/19 13:45:45, pink wrote:
> this code is pretty complex, how are we going to share it with the chrome
> implementation of this method?

My plan was to get this working properly in test_shell by leaving the code here,
and pulling out into a separate class  once I get to that part of the Chromium
implementation. I'll put a TODO here to make that clear.

http://codereview.chromium.org/48149/diff/1028/60#newcode116
Line 116: NSMenu* menu = [[NSMenu alloc] initWithTitle:@""];
On 2009/03/19 13:45:45, pink wrote:
> doesn't this leak? I don't see it being released.

Fixed by releasing after setting it to be the NSPopUpButtonCell's menu.

http://codereview.chromium.org/48149/diff/1028/60#newcode173
Line 173: eventNumber:0  // This is clearly wrong.
On 2009/03/19 13:45:45, pink wrote:
> if you're really concerned, you can try calling |NSApp currentEvent|, get its
> event number, and increment by 1? I'm not sure if this would throw off the
> global count, but it could work. 0 is probably fine for these purposes though.

I'll just remove the comment and leave it at 0, since we don't forward the event
to anything that needs the count.

http://codereview.chromium.org/48149/diff/1028/60#newcode196
Line 196: // TODO(paulg): Better to use autorelease on these objects?
On 2009/03/19 13:45:45, pink wrote:
> yes, they should be autoreleased per the style guide. If someone inserts an
> early return (eg, to bail early in an error condition), they'll leak.

Fixed.

http://codereview.chromium.org/48149/diff/1028/61
File webkit/tools/test_shell/test_webview_delegate.h (right):

http://codereview.chromium.org/48149/diff/1028/61#newcode222
Line 222: 
On 2009/03/19 13:45:45, pink wrote:
> remove extra blank lines? they weren't there before.

Done.

[pink (ping after 24hrs), 2009-03-20 15:21:58]

Almost there.

http://codereview.chromium.org/48149/diff/2006/1034
File webkit/glue/chrome_client_impl.cc (right):

http://codereview.chromium.org/48149/diff/2006/1034#newcode526
Line 526: if (delegate) {
rather than wrapping the entire method in an |if (delegate)| why not just return
at the top if |!delegate|?

http://codereview.chromium.org/48149/diff/2006/1032
File webkit/glue/webwidget_delegate.h (right):

http://codereview.chromium.org/48149/diff/2006/1032#newcode61
Line 61: // the screen of the button that will display the menu. It will be
used, along
in what coordinate system?

http://codereview.chromium.org/48149/diff/2006/1039
File webkit/tools/test_shell/mac/test_webview_delegate.mm (right):

http://codereview.chromium.org/48149/diff/2006/1039#newcode42
Line 42: menuDismissed_ = NO;
I think you need to comment what the values of |menuDismissed| mean. It's not
immediately obvious w/out reading the code.

http://codereview.chromium.org/48149/diff/2006/1039#newcode45
Line 45: - (void)addItem:(const MenuItem&)item toMenu:(NSMenu*)menu {
this api allows you to add items to multiple menus if you want to mis-use it. Is
that ok? Probably not since menuDimssed_ will be wrong. I'd say make the init be
|-initWithMenu:| or even |-initWithItems:forMenu:| and have all the
initialization code in one place.

http://codereview.chromium.org/48149/diff/2006/1039#newcode132
Line 132: [menu release];  // |button| manages |menu| from here on.
just autorelease it when you alloc it.

[Paul Godavari, 2009-03-20 20:06:05]

http://codereview.chromium.org/48149/diff/2006/1034
File webkit/glue/chrome_client_impl.cc (right):

http://codereview.chromium.org/48149/diff/2006/1034#newcode526
Line 526: if (delegate) {
On 2009/03/20 15:21:58, pink wrote:
> rather than wrapping the entire method in an |if (delegate)| why not just
return
> at the top if |!delegate|?

Done.

http://codereview.chromium.org/48149/diff/2006/1032
File webkit/glue/webwidget_delegate.h (right):

http://codereview.chromium.org/48149/diff/2006/1032#newcode61
Line 61: // the screen of the button that will display the menu. It will be
used, along
On 2009/03/20 15:21:58, pink wrote:
> in what coordinate system?

Updated comment. Input comes from WebKit coordinates (origin at top left corner)
and is drawn in NSView coordinates (origin at the bottom left corner).

http://codereview.chromium.org/48149/diff/2006/1039
File webkit/tools/test_shell/mac/test_webview_delegate.mm (right):

http://codereview.chromium.org/48149/diff/2006/1039#newcode42
Line 42: menuDismissed_ = NO;
On 2009/03/20 15:21:58, pink wrote:
> I think you need to comment what the values of |menuDismissed| mean. It's not
> immediately obvious w/out reading the code. 

I've changed the name to make it more clear.

http://codereview.chromium.org/48149/diff/2006/1039#newcode45
Line 45: - (void)addItem:(const MenuItem&)item toMenu:(NSMenu*)menu {
On 2009/03/20 15:21:58, pink wrote:
> this api allows you to add items to multiple menus if you want to mis-use it.
Is
> that ok? Probably not since menuDimssed_ will be wrong. I'd say make the init
be
> |-initWithMenu:| or even |-initWithItems:forMenu:| and have all the
> initialization code in one place.

Changed to: -initWithItems:forMenu:

[pink (ping after 24hrs), 2009-03-23 19:25:38]

LGTM

http://codereview.chromium.org/48149/diff/1084/2064
File webkit/tools/test_shell/mac/test_webview_delegate.mm (right):

http://codereview.chromium.org/48149/diff/1084/2064#newcode45
Line 45: - (id)initWithItems:(const std::vector<MenuItem>&)items
forMenu:(NSMenu*)menu {
move this to be first in @implementation. we generally put init/dealloc first so
they're easy to find.

[Paul Godavari, 2009-03-23 19:31:22]

http://codereview.chromium.org/48149/diff/1084/2064
File webkit/tools/test_shell/mac/test_webview_delegate.mm (right):

http://codereview.chromium.org/48149/diff/1084/2064#newcode45
Line 45: - (id)initWithItems:(const std::vector<MenuItem>&)items
forMenu:(NSMenu*)menu {
On 2009/03/23 19:25:38, pink wrote:
> move this to be first in @implementation. we generally put init/dealloc first
so
> they're easy to find.

I initially had it first, but then got a build error since it didn't see the
addItem method. Is it generally preferable to have all the methods declared in
the i@interface section for a private class such as this (I was following the
lead of another private class in test_shell_mac.mm)?

[pink (ping after 24hrs), 2009-03-23 20:56:47]

http://codereview.chromium.org/48149/diff/1084/2064
File webkit/tools/test_shell/mac/test_webview_delegate.mm (right):

http://codereview.chromium.org/48149/diff/1084/2064#newcode45
Line 45: - (id)initWithItems:(const std::vector<MenuItem>&)items
forMenu:(NSMenu*)menu {
you can put addItem: in a separate, private, category w/in the .mm file if it
needs to be referenced before something else, eg:

@interface MenuDelegate(PrivateMethods)
- (void)addItem:...
@end

Categories are wonderful and are your friend. Use them for everything :D

[darin (slow to review), 2009-04-01 18:02:50]

http://codereview.chromium.org/48149/diff/5002/5003
File webkit/glue/webwidget_delegate.h (right):

http://codereview.chromium.org/48149/diff/5002/5003#newcode24
Line 24: struct MenuItem {
sorry for the after-the-fact review...

MenuItem should be named WebMenuItem to be consistent with the webkit/glue API. 
it is easy to imagine name conflicts with other structures named MenuItem
elsewhere in the code base that have nothing to do with WebKit.

also, our convention in webkit/glue is to declare structures like this as
separate files.

http://codereview.chromium.org/48149/diff/5002/5003#newcode59
Line 59: // Used for displaying HTML select elements as popup menus on Mac OS X
(other
as mentioned in the bugs.webkit.org review, this kind of comment is kind of
unnecessarily specific.  this doesn't have to be limited to HTML select
elements.

comparing this ShowWithItems method to the Show method above, it is confusing
that this one does not have a WindowOpenDisposition parameter.  maybe
ShowWithItems should be named ShowAsPopupWithItems?  that way the "AsPopup"
indicates the disposition?  i'm not sure...

http://codereview.chromium.org/48149/diff/5002/5006
File webkit/glue/webwidget_impl.cc (right):

http://codereview.chromium.org/48149/diff/5002/5006#newcode74
Line 74: delegate_->SetWindowRect(this, bounds);
it seems kind of strange to have to pass bounds twice to the delegate.  what is
the reason for that?

http://codereview.chromium.org/48149/diff/5002/5012
File webkit/tools/test_shell/test_webview_delegate_win.cc (right):

http://codereview.chromium.org/48149/diff/5002/5012#newcode86
Line 86: NOTIMPLEMENTED();
this macro suggests that this method should one day be implemented, but is that
really what you intend?  perhaps NOTREACHED is better since we never intend to
implement this method, right?  same for the GTK case.

[darin (slow to review), 2009-04-01 18:23:42]

http://codereview.chromium.org/48149/diff/5002/5013
File chrome/renderer/render_widget.h (right):

http://codereview.chromium.org/48149/diff/5002/5013#newcode83
Line 83: virtual void ShowWithItems(WebWidget* webwidget,
nit: please list the methods in the order in which they were declared.