Linux accelerators cleanup

Linux accelerators cleanup:
- Give renderer a chance to handle accelerators before browser does.
- Handle browser accelerators that aren't attached to any particular UI element in BrowserWindowGtk rather than in BrowserToolbarGtk
- Use Browser::ExecuteCommand() to handle accelerator activation
- Switch a random void* to gfx::NativeWindow
- Enable three browser commands on linux : Focus Location, Focus Search, Open file

This fully enables ctrl-l, ctrl-k, and ctrl-o. This fixes copy-pasta in the omnibox. This fixes the problem Dean described with <http://www.quirksmode.org/js/keys.html>;.

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

[Evan Martin, 2009-03-13 21:54:40]

+dean, who was talking about how these should work

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

http://codereview.chromium.org/42190/diff/1/5#newcode901
Line 901: #if !defined(OS_MACOSX)
I think I've been asked in the past to make this "if win || linux", since that
is more clearly what we're trying to do here (and elsewhere).  I don't
especially care too much -- just wanted to point it out if you weren't sure of
which way to go for these.

http://codereview.chromium.org/42190/diff/1/5#newcode902
Line 902: void Browser::OpenFile() {
I kinda wonder if we'd be better off not reordering functions and adding a soup
of ifdefs, if only to preserve the file change history.

http://codereview.chromium.org/42190/diff/1/5#newcode1235
Line 1235: case IDC_FOCUS_LOCATION:        FocusLocationBar();            break;
Here again, I think it was better to have all the FOCUS commands together in the
switch statement...

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

http://codereview.chromium.org/42190/diff/1/9#newcode84
Line 84: // gtk_widget_set_size_request).
One reason people use these (which isn't at all clear from the code) is that the
spacing becomes font-relative.

However, we have pixel-sized widgets so I guess we should use an absolute number
of pixels here...

[Elliot Glaysher, 2009-03-13 22:15:52]

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

http://codereview.chromium.org/42190/diff/1/5#newcode902
Line 902: void Browser::OpenFile() {
+1 to both of these requests. Unless this is fixing alphabetization or other
order of function problems, I don't think they should be reordered.

http://codereview.chromium.org/42190/diff/1/2
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/42190/diff/1/2#newcode493
Line 493: gtk_accel_group_query(accel_group, keyval, modifier, &handlers);
Is this necessary? It seems like handlers will never be 0 because we're called
in response to keypresses and we are the handler...

[Evan Stade, 2009-03-13 23:11:42]

comments addressed. Should be a bit easier to review now.

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

http://codereview.chromium.org/42190/diff/1/5#newcode901
Line 901: #if !defined(OS_MACOSX)
On 2009/03/13 21:54:40, Evan Martin wrote:
> I think I've been asked in the past to make this "if win || linux", since that
> is more clearly what we're trying to do here (and elsewhere).  I don't
> especially care too much -- just wanted to point it out if you weren't sure of
> which way to go for these.

k

http://codereview.chromium.org/42190/diff/1/5#newcode902
Line 902: void Browser::OpenFile() {
On 2009/03/13 21:54:40, Evan Martin wrote:
> I kinda wonder if we'd be better off not reordering functions and adding a
soup
> of ifdefs, if only to preserve the file change history.

I could go either way. IFDEF soup it is.

http://codereview.chromium.org/42190/diff/1/5#newcode1235
Line 1235: case IDC_FOCUS_LOCATION:        FocusLocationBar();            break;
On 2009/03/13 21:54:40, Evan Martin wrote:
> Here again, I think it was better to have all the FOCUS commands together in
the
> switch statement...

yea. The idea was they'd be reunited when the rest of the stuff was un-ifdefed.
But I'll go back and change it.

http://codereview.chromium.org/42190/diff/1/2
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/42190/diff/1/2#newcode493
Line 493: gtk_accel_group_query(accel_group, keyval, modifier, &handlers);
On 2009/03/13 22:15:53, Elliot Glaysher wrote:
> Is this necessary? It seems like handlers will never be 0 because we're called
> in response to keypresses and we are the handler...

oh, I guess you're right. From the docs I thought this might be called whether
or not we were the handler. silly me.

[Elliot Glaysher, 2009-03-14 01:26:02]

> http://codereview.chromium.org/42190/diff/1/2#newcode493
> Line 493: gtk_accel_group_query(accel_group, keyval, modifier, &handlers);
> On 2009/03/13 22:15:53, Elliot Glaysher wrote:
> > Is this necessary? It seems like handlers will never be 0 because we're
called
> > in response to keypresses and we are the handler...
> 
> oh, I guess you're right. From the docs I thought this might be called whether
> or not we were the handler. silly me.

LGTM

[Evan Stade, 2009-03-14 23:45:32]

have another look everybody (added a fix for Dean's bug)

[Dean McNamee, 2009-03-16 12:00:48]

Will patch in your change today and play around with it.

http://codereview.chromium.org/42190/diff/29/1043
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/42190/diff/29/1043#newcode911
Line 911: window_->SetFocusToLocationBar();
I don't understand this change.

http://codereview.chromium.org/42190/diff/29/1044
File chrome/browser/browser.h (right):

http://codereview.chromium.org/42190/diff/29/1044#newcode313
Line 313: void FocusToolbar();
Can we not do FocusToolbar too?

http://codereview.chromium.org/42190/diff/29/1047
File chrome/browser/gtk/browser_toolbar_gtk.cc (right):

http://codereview.chromium.org/42190/diff/29/1047#newcode54
Line 54: GtkWindow* top_level_window) {
indent this to match up with Profile

http://codereview.chromium.org/42190/diff/29/1047#newcode66
Line 66: accel_group_ = gtk_accel_group_new();
btw, it looks like we leak accel_group_ ?

http://codereview.chromium.org/42190/diff/29/1047#newcode82
Line 82: // gtk_widget_set_size_request).
While I agree, this is sort of unrelated to your change.  If you want to keep
it, I think something like:

TODO(someone): Remove use of labels are padding.

I also don't think TODO(port) is appropriate, it has nothing to do with porting
:)

http://codereview.chromium.org/42190/diff/29/1040
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/42190/diff/29/1040#newcode104
Line 104: gboolean OnKeyPress(GtkWindow* window, GdkEventKey* event, void*) {
you should name the last parameter even if we don't use it.  Also, I think it
should be gpointer, so something like gpointer userdata would be good.

http://codereview.chromium.org/42190/diff/29/1040#newcode170
Line 170: G_CALLBACK(OnKeyPress), NULL);
We don't actually need any of these G_OBJECT calls, g_signal_connect is a macro
to handle that for you.

http://codereview.chromium.org/42190/diff/29/1040#newcode455
Line 455: GtkAccelGroup* accel_group = gtk_accel_group_new();
do we leak this here?

http://codereview.chromium.org/42190/diff/29/1040#newcode502
Line 502: BrowserWindowGtk* browser_window) {
I usually make the last parameter gpointer userdata, and do a reinterpret_cast. 
I somehow feel it's more honest, but I am fine with this code too.

http://codereview.chromium.org/42190/diff/29/1041
File chrome/browser/gtk/browser_window_gtk.h (right):

http://codereview.chromium.org/42190/diff/29/1041#newcode93
Line 93: // Connect keyboard shortcuts that don't have UI equivalents (like
ctrl-o,
Connect keyboard accelerators that aren't handled by menu accelerators.  (I am
surprised we don't have a menu item for ctrl-o).

http://codereview.chromium.org/42190/diff/29/1039
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/42190/diff/29/1039#newcode81
Line 81: return TRUE;
Maybe a comment about why we don't want this to propagate?  It is not obvious to
me right now.

http://codereview.chromium.org/42190/diff/29/1052
File chrome/browser/tab_contents/web_contents_view_gtk.cc (right):

http://codereview.chromium.org/42190/diff/29/1052#newcode182
Line 182: GtkWindow* window = GetTopLevelNativeView();
Why does GetTopLeveNativeView return a NativeWindow and not a NativeView?

[Evan Stade, 2009-03-16 18:16:17]

http://codereview.chromium.org/42190/diff/29/1043
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/42190/diff/29/1043#newcode911
Line 911: window_->SetFocusToLocationBar();
On 2009/03/16 12:00:48, Dean McNamee wrote:
> I don't understand this change.

there's no difference, SetFocusToLocationBar() does
GetLocationBar()->FocusLocation(). If you got it, use it; that's what I say.

http://codereview.chromium.org/42190/diff/29/1044
File chrome/browser/browser.h (right):

http://codereview.chromium.org/42190/diff/29/1044#newcode313
Line 313: void FocusToolbar();
On 2009/03/16 12:00:48, Dean McNamee wrote:
> Can we not do FocusToolbar too?

I guess we probably can, but the purpose of this patch wasn't to find all the
things in Browser that we can port. Originally I just wanted to implement
OpenFile so I could test dialogs, and look how far it's already ballooned.

http://codereview.chromium.org/42190/diff/29/1047
File chrome/browser/gtk/browser_toolbar_gtk.cc (right):

http://codereview.chromium.org/42190/diff/29/1047#newcode54
Line 54: GtkWindow* top_level_window) {
On 2009/03/16 12:00:48, Dean McNamee wrote:
> indent this to match up with Profile

Done.

http://codereview.chromium.org/42190/diff/29/1047#newcode66
Line 66: accel_group_ = gtk_accel_group_new();
On 2009/03/16 12:00:48, Dean McNamee wrote:
> btw, it looks like we leak accel_group_ ?

yea I was thinking about that (also in BrowserWindowGtk). So  we'd have to call
g_object_ref_sink() here and then g_object_unref() later. Right?

http://codereview.chromium.org/42190/diff/29/1047#newcode82
Line 82: // gtk_widget_set_size_request).
On 2009/03/16 12:00:48, Dean McNamee wrote:
> While I agree, this is sort of unrelated to your change.  If you want to keep
> it, I think something like:
> 
> TODO(someone): Remove use of labels are padding.
> 
> I also don't think TODO(port) is appropriate, it has nothing to do with
porting
> :)

Done.

http://codereview.chromium.org/42190/diff/29/1040
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/42190/diff/29/1040#newcode104
Line 104: gboolean OnKeyPress(GtkWindow* window, GdkEventKey* event, void*) {
On 2009/03/16 12:00:48, Dean McNamee wrote:
> you should name the last parameter even if we don't use it.  Also, I think it
> should be gpointer, so something like gpointer userdata would be good.

Done.

http://codereview.chromium.org/42190/diff/29/1040#newcode170
Line 170: G_CALLBACK(OnKeyPress), NULL);
On 2009/03/16 12:00:48, Dean McNamee wrote:
> We don't actually need any of these G_OBJECT calls, g_signal_connect is a
macro
> to handle that for you.

Done.

http://codereview.chromium.org/42190/diff/29/1040#newcode502
Line 502: BrowserWindowGtk* browser_window) {
On 2009/03/16 12:00:48, Dean McNamee wrote:
> I usually make the last parameter gpointer userdata, and do a
reinterpret_cast. 
> I somehow feel it's more honest, but I am fine with this code too.

I think we've used this style more frequently.

http://codereview.chromium.org/42190/diff/29/1041
File chrome/browser/gtk/browser_window_gtk.h (right):

http://codereview.chromium.org/42190/diff/29/1041#newcode93
Line 93: // Connect keyboard shortcuts that don't have UI equivalents (like
ctrl-o,
On 2009/03/16 12:00:48, Dean McNamee wrote:
> Connect keyboard accelerators that aren't handled by menu accelerators.  (I am
> surprised we don't have a menu item for ctrl-o).

Done.

http://codereview.chromium.org/42190/diff/29/1039
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/42190/diff/29/1039#newcode81
Line 81: return TRUE;
On 2009/03/16 12:00:48, Dean McNamee wrote:
> Maybe a comment about why we don't want this to propagate?  It is not obvious
to
> me right now.

Done.

http://codereview.chromium.org/42190/diff/29/1052
File chrome/browser/tab_contents/web_contents_view_gtk.cc (right):

http://codereview.chromium.org/42190/diff/29/1052#newcode182
Line 182: GtkWindow* window = GetTopLevelNativeView();
On 2009/03/16 12:00:48, Dean McNamee wrote:
> Why does GetTopLeveNativeView return a NativeWindow and not a NativeView?

bad function name, guess I should change it.

[Dean McNamee, 2009-03-16 19:19:01]

LG with these changes, and if it valgrinds / etc.

http://codereview.chromium.org/42190/diff/35/2048
File chrome/browser/gtk/browser_toolbar_gtk.cc (right):

http://codereview.chromium.org/42190/diff/35/2048#newcode68
Line 68: // The window now owns |accel_group_|.
this makes it sound like add_accel took ownership.  Say like // Drop our
reference and let the window own.

http://codereview.chromium.org/42190/diff/35/2041
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/42190/diff/35/2041#newcode82
Line 82: static const guint kOpenFileKey = GDK_o;
there are in a namespace (so shouldn't be static)

http://codereview.chromium.org/42190/diff/35/2041#newcode457
Line 457: // The window now owns |accel_group|.
Same here, it's confusing.

http://codereview.chromium.org/42190/diff/35/2040
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/42190/diff/35/2040#newcode82
Line 82: // can't handle the event, we'll deal with that later.
we deal with that in XXX.  This makes it sound like it's an open bug "yeah,
we'll fix that later"

http://codereview.chromium.org/42190/diff/35/2053
File chrome/browser/tab_contents/web_contents_view_gtk.cc (right):

http://codereview.chromium.org/42190/diff/35/2053#newcode182
Line 182: GtkWindow* window = GetTopLevelNativeView();
GetTopLevelNativeView returning a Window still seems wrong... Can you fix it?

[Evan Stade, 2009-03-16 19:25:49]

thanks for speedy reviews

http://codereview.chromium.org/42190/diff/35/2053
File chrome/browser/tab_contents/web_contents_view_gtk.cc (right):

http://codereview.chromium.org/42190/diff/35/2053#newcode182
Line 182: GtkWindow* window = GetTopLevelNativeView();
On 2009/03/16 19:19:02, Dean McNamee wrote:
> GetTopLevelNativeView returning a Window still seems wrong... Can you fix it?

will change the name of that function in a follow up patch as it touches a
numebr of files.