Add ctrl+w accelerator to close bookmark manager for linux

Add ctrl+w accelerator to close bookmark manager for linux

Following the way that the Task Manger does it.

Comitted at http://src.chromium.org/viewvc/chrome?view=rev&revision=23160

[Joel Stanley (old), 2009-08-09 15:27:13]

Totally unsure about comment and code on line 339 and 340.

Slightly confused about the differences between esc and ctrl+w.  See
http://crbug.com/18879

[Evan Stade, 2009-08-10 18:26:14]

lgtm with nits

http://codereview.chromium.org/165220/diff/1002/1003
File chrome/browser/gtk/bookmark_manager_gtk.cc (right):

http://codereview.chromium.org/165220/diff/1002/1003#newcode1456
Line 1456: bookmark_manager) {
variable name and type should be on same line always

http://codereview.chromium.org/165220/diff/1002/1003#newcode1457
Line 1457: if (keyval == GDK_w && modifier == GDK_CONTROL_MASK) {
I think this if should be a DCHECK (or maybe just put else NOTREACHED())

[Joel Stanley (old), 2009-08-11 03:57:53]

http://codereview.chromium.org/165220/diff/1002/1003
File chrome/browser/gtk/bookmark_manager_gtk.cc (right):

http://codereview.chromium.org/165220/diff/1002/1003#newcode1457
Line 1457: if (keyval == GDK_w && modifier == GDK_CONTROL_MASK) {
On 2009/08/10 18:26:14, Evan Stade wrote:
> I think this if should be a DCHECK (or maybe just put else NOTREACHED())

I don't understand.  Doesn't this if check for what the key and modifier was?
What would putting it in a DCHECK achieve?

[Evan Stade, 2009-08-11 04:06:45]

http://codereview.chromium.org/165220/diff/1002/1003
File chrome/browser/gtk/bookmark_manager_gtk.cc (right):

http://codereview.chromium.org/165220/diff/1002/1003#newcode1457
Line 1457: if (keyval == GDK_w && modifier == GDK_CONTROL_MASK) {
On 2009/08/11 03:57:53, Joel Stanley wrote:
> On 2009/08/10 18:26:14, Evan Stade wrote:
> > I think this if should be a DCHECK (or maybe just put else NOTREACHED())
> 
> I don't understand.  Doesn't this if check for what the key and modifier was?
> What would putting it in a DCHECK achieve?

I was suggesting replacing the if with a DCHECK (or two). Also you have to &
|modifier| with gtk_accelerator_get_default_mod_mask()

[Joel Stanley (old), 2009-08-11 14:20:52]

http://codereview.chromium.org/165220/diff/1002/1003
File chrome/browser/gtk/bookmark_manager_gtk.cc (right):

http://codereview.chromium.org/165220/diff/1002/1003#newcode1456
Line 1456: bookmark_manager) {
On 2009/08/10 18:26:14, Evan Stade wrote:
> variable name and type should be on same line always

Done.

http://codereview.chromium.org/165220/diff/1002/1003#newcode1457
Line 1457: if (keyval == GDK_w && modifier == GDK_CONTROL_MASK) {
On 2009/08/11 04:06:46, Evan Stade wrote:
> On 2009/08/11 03:57:53, Joel Stanley wrote:
> > On 2009/08/10 18:26:14, Evan Stade wrote:
> > > I think this if should be a DCHECK (or maybe just put else NOTREACHED())
> > 
> > I don't understand.  Doesn't this if check for what the key and modifier
was?
> > What would putting it in a DCHECK achieve?
> 
> I was suggesting replacing the if with a DCHECK (or two). 

I've been thinking about it off and on all day and I'm still not sure what you
want the DCHECK to be checking.

The way I understand it: the function is called whenever GTK gets a keyboard
press that contains a modifier.  When the modifier is control, and the key is w,
we want to close the window.  Other cases (different key combinations) will not
trigger any action, but are not an error either.

> Also you have to &
> |modifier| with gtk_accelerator_get_default_mod_mask()

Done.

[Evan Stade, 2009-08-11 18:26:58]

On 2009/08/11 14:20:52, Joel Stanley wrote:
> http://codereview.chromium.org/165220/diff/1002/1003
> File chrome/browser/gtk/bookmark_manager_gtk.cc (right):
> 
> http://codereview.chromium.org/165220/diff/1002/1003#newcode1456
> Line 1456: bookmark_manager) {
> On 2009/08/10 18:26:14, Evan Stade wrote:
> > variable name and type should be on same line always
> 
> Done.
> 
> http://codereview.chromium.org/165220/diff/1002/1003#newcode1457
> Line 1457: if (keyval == GDK_w && modifier == GDK_CONTROL_MASK) {
> On 2009/08/11 04:06:46, Evan Stade wrote:
> > On 2009/08/11 03:57:53, Joel Stanley wrote:
> > > On 2009/08/10 18:26:14, Evan Stade wrote:
> > > > I think this if should be a DCHECK (or maybe just put else NOTREACHED())
> > > 
> > > I don't understand.  Doesn't this if check for what the key and modifier
> was?
> > > What would putting it in a DCHECK achieve?
> > 
> > I was suggesting replacing the if with a DCHECK (or two). 
> 
> I've been thinking about it off and on all day and I'm still not sure what you
> want the DCHECK to be checking.
> 
> The way I understand it: the function is called whenever GTK gets a keyboard
> press that contains a modifier.  When the modifier is control, and the key is
w,
> we want to close the window.  Other cases (different key combinations) will
not
> trigger any action, but are not an error either.

We won't get called back unless it is an accelerator we have registered for.
Otherwise, what is the point of registering for individual accelerators?

> 
> > Also you have to &
> > |modifier| with gtk_accelerator_get_default_mod_mask()
> 
> Done.

[Joel Stanley (old), 2009-08-12 03:09:20]

On 2009/08/11 18:26:58, Evan Stade wrote:
> We won't get called back unless it is an accelerator we have registered for.
> Otherwise, what is the point of registering for individual accelerators?

Okay, I failed to read the code I wrote.  Thanks for your patience.

I've uploaded a new version.

[Evan Stade, 2009-08-12 03:16:36]

lg; nit

http://codereview.chromium.org/165220/diff/1012/10
File chrome/browser/gtk/bookmark_manager_gtk.cc (right):

http://codereview.chromium.org/165220/diff/1012/10#newcode1452
Line 1452: GObject* acceleratable,
wrapped parameters have a 4 space indent

[Joel Stanley (old), 2009-08-12 03:21:22]

Please commit for me.

http://codereview.chromium.org/165220/diff/1012/10
File chrome/browser/gtk/bookmark_manager_gtk.cc (right):

http://codereview.chromium.org/165220/diff/1012/10#newcode1452
Line 1452: GObject* acceleratable,
On 2009/08/12 03:16:36, Evan Stade wrote:
> wrapped parameters have a 4 space indent

Done.