Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(326)

Issue 224023: Don't send tab switching/killing/creating keyboard accelerators to pages. Th... (Closed)

Created:
11 years, 3 months ago by jam
Modified:
9 years, 7 months ago
CC:
chromium-reviews_googlegroups.com, John Grabowski, Erik does not do reviews, pam+watch_chromium.org, Paweł Hajdan Jr., darin (slow to review), tim (not reviewing)
Visibility:
Public.

Description

Don't send tab switching/killing/creating keyboard accelerators to pages. This avoids tabs maliciously preventing closing using ctrl+f4/ctrl+w/alt+f4, and also hung/slow renderers from making tab cycling sluggish. BUG=5496 TEST=added ui test Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27814

Patch Set 1 : '' #

Total comments: 3

Patch Set 2 : '' #

Total comments: 6

Patch Set 3 : '' #

Patch Set 4 : '' #

Total comments: 5

Patch Set 5 : '' #

Patch Set 6 : '' #

Patch Set 7 : add missing accelerators and fix crash with deleted RWH being used #

Patch Set 8 : no code change, just sync to head #

Patch Set 9 : switched to interactive ui test since the browser test couldn't wait for the tab to close in linux #

Patch Set 10 : call Browser:IsReservedAccelerator from tab_contents_view.cc instead of each platform file #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+198 lines, -30 lines) Patch
M chrome/browser/browser.h View 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/browser.cc View 3 4 5 6 7 8 9 1 chunk +22 lines, -0 lines 0 comments Download
M chrome/browser/browser_focus_uitest.cc View 9 2 chunks +40 lines, -0 lines 0 comments Download
M chrome/browser/browser_window.h View 3 4 5 6 7 8 9 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/main_menu.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/cocoa/browser_window_cocoa.h View 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/cocoa/browser_window_cocoa.mm View 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 1 comment Download
M chrome/browser/extensions/extension_host.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/extensions/extension_host.cc View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/gtk/browser_window_gtk.h View 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/gtk/browser_window_gtk.cc View 3 4 5 6 7 8 9 5 chunks +38 lines, -13 lines 0 comments Download
M chrome/browser/renderer_host/render_view_host.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/renderer_host/render_view_host.cc View 1 2 3 4 5 6 7 8 9 1 chunk +9 lines, -10 lines 0 comments Download
M chrome/browser/renderer_host/render_view_host_delegate.h View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/render_widget_host.h View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/render_widget_host.cc View 1 2 3 4 5 6 7 8 9 2 chunks +8 lines, -1 line 0 comments Download
M chrome/browser/renderer_host/render_widget_host_view_gtk.h View 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/renderer_host/render_widget_host_view_gtk.cc View 7 8 9 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/tab_contents/interstitial_page.cc View 1 2 3 4 5 6 7 8 9 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/tab_contents/tab_contents_delegate.h View 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/tab_contents/tab_contents_view.h View 2 chunks +1 line, -5 lines 0 comments Download
M chrome/browser/tab_contents/tab_contents_view.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/views/frame/browser_view.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/views/frame/browser_view.cc View 1 2 3 4 5 6 7 8 9 3 chunks +20 lines, -0 lines 0 comments Download
M chrome/test/test_browser_window.h View 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 41 (0 generated)
jam
11 years, 3 months ago (2009-09-24 22:43:40 UTC) #1
Peter Kasting
http://codereview.chromium.org/224023/diff/1029/1034 File chrome/browser/views/frame/browser_view.cc (right): http://codereview.chromium.org/224023/diff/1029/1034#newcode624 Line 624: command_id == IDC_CLOSE_TAB || Why not IDC_CLOSE_WINDOW? Seems ...
11 years, 3 months ago (2009-09-24 23:08:26 UTC) #2
jam
http://codereview.chromium.org/224023/diff/1029/1034 File chrome/browser/views/frame/browser_view.cc (right): http://codereview.chromium.org/224023/diff/1029/1034#newcode624 Line 624: command_id == IDC_CLOSE_TAB || On 2009/09/24 23:08:26, Peter ...
11 years, 3 months ago (2009-09-24 23:15:47 UTC) #3
Ben Goodger (Google)
http://codereview.chromium.org/224023/diff/27/47 File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right): http://codereview.chromium.org/224023/diff/27/47#newcode573 Line 573: return browser_view->IsReservedAccelerator(accelerator); I think this is incorrect. The ...
11 years, 3 months ago (2009-09-24 23:18:36 UTC) #4
Peter Kasting
http://codereview.chromium.org/224023/diff/1029/1034 File chrome/browser/views/frame/browser_view.cc (right): http://codereview.chromium.org/224023/diff/1029/1034#newcode624 Line 624: command_id == IDC_CLOSE_TAB || On 2009/09/24 23:15:47, John ...
11 years, 3 months ago (2009-09-24 23:22:03 UTC) #5
Peter Kasting
On 2009/09/24 23:22:03, Peter Kasting wrote: > IDC_CLOSE_WINDOW has one on GTK, and it's supposed ...
11 years, 3 months ago (2009-09-24 23:28:02 UTC) #6
jam
http://codereview.chromium.org/224023/diff/27/47 File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right): http://codereview.chromium.org/224023/diff/27/47#newcode573 Line 573: return browser_view->IsReservedAccelerator(accelerator); On 2009/09/24 23:18:36, Ben Goodger wrote: ...
11 years, 3 months ago (2009-09-24 23:31:03 UTC) #7
jam
On Thu, Sep 24, 2009 at 4:28 PM, <pkasting@chromium.org> wrote: > On 2009/09/24 23:22:03, Peter ...
11 years, 3 months ago (2009-09-24 23:42:45 UTC) #8
Ben Goodger (Google)
On 2009/09/24 23:31:03, John Abd-El-Malek wrote: > I see, ok I'll sync up with some ...
11 years, 3 months ago (2009-09-25 00:38:54 UTC) #9
James Su
IMHO, this change may not be appropriate. According to http://www.w3.org/TR/DOM-Level-3-Events/#event-flow-cancelation, the default action of an ...
11 years, 3 months ago (2009-09-25 23:23:23 UTC) #10
jam
On 2009/09/25 23:23:23, James Su wrote: > IMHO, this change may not be appropriate. According ...
11 years, 3 months ago (2009-09-25 23:33:03 UTC) #11
James Su
I tested these browsers on this page: http://www.quirksmode.org/js/keys.html. You can try just return false in ...
11 years, 3 months ago (2009-09-26 00:03:32 UTC) #12
jam
On Fri, Sep 25, 2009 at 4:23 PM, <suzhe@chromium.org> wrote: > IMHO, this change may ...
11 years, 2 months ago (2009-09-29 19:49:31 UTC) #13
jam
Thanks for the review comments. New patch up with suggested changes. I also switched to ...
11 years, 2 months ago (2009-09-29 19:50:19 UTC) #14
James Su
Hi, Just tested on another website: http://unixpapa.com/js/testkey.html. The keyboard events handling in browsers is a ...
11 years, 2 months ago (2009-09-30 01:26:19 UTC) #15
jam
On Tue, Sep 29, 2009 at 6:26 PM, <suzhe@chromium.org> wrote: > Hi, > Just tested ...
11 years, 2 months ago (2009-09-30 01:44:50 UTC) #16
jam
On Tue, Sep 29, 2009 at 6:44 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > ...
11 years, 2 months ago (2009-09-30 01:57:25 UTC) #17
Evan Stade
I agree with James. I don't think he was suggesting we wait till the renderer ...
11 years, 2 months ago (2009-09-30 02:32:53 UTC) #18
Evan Stade
Another way to satisfy the use cases I can think of would be to make ...
11 years, 2 months ago (2009-09-30 03:02:40 UTC) #19
James Su
Hi, I agree that we should handle some accelerator keys specially to deal with a ...
11 years, 2 months ago (2009-09-30 04:30:34 UTC) #20
Ben Goodger (Google)
No. It's not just Ctrl+F4. There are several window management controls that are commonly used, ...
11 years, 2 months ago (2009-09-30 04:32:28 UTC) #21
Ben Goodger (Google)
Let me be more specific: Any accelerator that maps to a window management command should ...
11 years, 2 months ago (2009-09-30 04:38:43 UTC) #22
James Su
Hi, I agree to filter all Tab closing accelerators. The question is: How to do ...
11 years, 2 months ago (2009-09-30 04:49:38 UTC) #23
Ben Goodger (Google)
As I saw it, John's change passes a NativeWebKeyboardEvent to a platform-specific translation function that ...
11 years, 2 months ago (2009-09-30 05:11:47 UTC) #24
James Su
On 2009/09/30 05:11:47, Ben Goodger wrote: > As I saw it, John's change passes a ...
11 years, 2 months ago (2009-09-30 05:22:56 UTC) #25
Ben Goodger (Google)
I don't know anything about these APIs so forgive me, but as I understand it ...
11 years, 2 months ago (2009-09-30 05:25:13 UTC) #26
James Su
It's possible, but it's more complicated than going down current code path. However, it would ...
11 years, 2 months ago (2009-09-30 05:43:43 UTC) #27
jam
Trying to respond to the slew of messages: estade re security, my point was that ...
11 years, 2 months ago (2009-09-30 17:07:30 UTC) #28
jam
also regarding keyboard layout, thanks for the heads up I wasn't aware of that. Doest ...
11 years, 2 months ago (2009-09-30 17:28:13 UTC) #29
James Su
On 2009/09/30 17:07:30, John Abd-El-Malek wrote: > Trying to respond to the slew of messages: ...
11 years, 2 months ago (2009-09-30 17:34:26 UTC) #30
James Su
And if you have time, please have a look at the CL I just sent ...
11 years, 2 months ago (2009-09-30 18:12:58 UTC) #31
James Su
OnGtkAccelerator is ok. It's called by gtk_window_activate_key() function, which handles the keyboard layout mess for ...
11 years, 2 months ago (2009-09-30 18:59:13 UTC) #32
Evan Stade
http://codereview.chromium.org/224023/diff/14043/9067 File chrome/browser/browser.cc (right): http://codereview.chromium.org/224023/diff/14043/9067#newcode2124 Line 2124: return command_id == IDC_CLOSE_TAB || perhaps we should ...
11 years, 2 months ago (2009-09-30 19:16:10 UTC) #33
brettw
http://codereview.chromium.org/224023/diff/14043/9067 File chrome/browser/browser.cc (right): http://codereview.chromium.org/224023/diff/14043/9067#newcode2124 Line 2124: return command_id == IDC_CLOSE_TAB || On 2009/09/30 19:16:10, ...
11 years, 2 months ago (2009-09-30 19:21:54 UTC) #34
Ben Goodger (Google)
FYI, Spreadsheets overrides Ctrl+PageUp/Dn and it's enfuriating when tab switching suddenly stops working because the ...
11 years, 2 months ago (2009-09-30 19:34:58 UTC) #35
jam
regarding ctrl+pgup/ctrl+pgdown and ctrl+1..9: I too would prefer to do them, but given that Spreadsheets ...
11 years, 2 months ago (2009-09-30 20:45:35 UTC) #36
Peter Kasting
On Wed, Sep 30, 2009 at 1:45 PM, John Abd-El-Malek <jam@chromium.org> wrote: > regarding ctrl+pgup/ctrl+pgdown ...
11 years, 2 months ago (2009-09-30 20:49:53 UTC) #37
jam
On Wed, Sep 30, 2009 at 1:49 PM, Peter Kasting <pkasting@chromium.org>wrote: > On Wed, Sep ...
11 years, 2 months ago (2009-09-30 21:04:00 UTC) #38
Ben Goodger (Google)
LGTM. -Ben On 2009/09/30 21:04:00, John Abd-El-Malek wrote: > On Wed, Sep 30, 2009 at ...
11 years, 2 months ago (2009-09-30 21:43:03 UTC) #39
James Su
Are you really sure ctrl-w works fine? How about other keys and layouts? I can't ...
11 years, 2 months ago (2009-10-01 01:54:37 UTC) #40
Nico
11 years, 2 months ago (2009-10-02 03:11:01 UTC) #41
suzhe, thanks for worrying about the mac side. I think this is a viable
approach.

http://codereview.chromium.org/224023/diff/21011/21029
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/224023/diff/21011/21029#newcode325
Line 325: // CommandForKeyboardShortcut() doesn't have the full list.
Yes, that's because in cocoa most keyboard shortcuts are bound to menu items
which in turn are bound to "actions" (basically function pointers).

suzhe: I think this is ok, we can walk the menu and for all menu items that have
target commandDispatch: get the tag (which is the command id) and the
accelerator and use that to implement this function.

Powered by Google App Engine
This is Rietveld 408576698