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

Issue 8508056: Split a part of code managing accelerators from views::FocusManager. (Closed)

Created:
9 years, 1 month ago by mazda
Modified:
9 years, 1 month ago
CC:
chromium-reviews, tfarina, dhollowa
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Split a part of code managing accelerators from views::FocusManager. This is the second preliminary change for the support of global keyboard shortcut. This CL depends on http://codereview.chromium.org/8508055/ BUG=97255 TEST=None Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110020

Patch Set 1 #

Patch Set 2 : Rebase #

Total comments: 4

Patch Set 3 : Address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+160 lines, -45 lines) Patch
A ui/base/accelerator_manager.h View 1 2 1 chunk +65 lines, -0 lines 0 comments Download
A ui/base/accelerator_manager.cc View 1 chunk +77 lines, -0 lines 0 comments Download
M ui/ui.gyp View 1 1 chunk +2 lines, -0 lines 0 comments Download
M views/focus/focus_manager.h View 1 4 chunks +9 lines, -5 lines 0 comments Download
M views/focus/focus_manager.cc View 1 2 3 chunks +7 lines, -40 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
mazda
9 years, 1 month ago (2011-11-10 14:37:49 UTC) #1
Ben Goodger (Google)
LGTM
9 years, 1 month ago (2011-11-10 22:04:16 UTC) #2
tfarina
http://codereview.chromium.org/8508056/diff/4001/ui/base/accelerator_manager.h File ui/base/accelerator_manager.h (right): http://codereview.chromium.org/8508056/diff/4001/ui/base/accelerator_manager.h#newcode58 ui/base/accelerator_manager.h:58: }; nit: add DISALLOW_COPY_AND_ASSIGN here? if yes, don't forget ...
9 years, 1 month ago (2011-11-14 15:57:49 UTC) #3
tfarina
http://codereview.chromium.org/8508056/diff/4001/views/focus/focus_manager.cc File views/focus/focus_manager.cc (right): http://codereview.chromium.org/8508056/diff/4001/views/focus/focus_manager.cc#newcode14 views/focus/focus_manager.cc:14: #include "ui/base/accelerator_manager.h" nit: sort alphabetical.
9 years, 1 month ago (2011-11-14 15:59:54 UTC) #4
mazda
Thank you for the comments. http://codereview.chromium.org/8508056/diff/4001/ui/base/accelerator_manager.h File ui/base/accelerator_manager.h (right): http://codereview.chromium.org/8508056/diff/4001/ui/base/accelerator_manager.h#newcode58 ui/base/accelerator_manager.h:58: }; On 2011/11/14 15:57:50, ...
9 years, 1 month ago (2011-11-14 19:16:54 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mazda@chromium.org/8508056/7002
9 years, 1 month ago (2011-11-15 01:11:53 UTC) #6
commit-bot: I haz the power
9 years, 1 month ago (2011-11-15 02:27:27 UTC) #7
Change committed as 110020

Powered by Google App Engine
This is Rietveld 408576698