Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
582143004:
Introduce "Preferred" accelerators (Closed)
|
Created:
6 years, 3 months ago by oshima Modified:
6 years, 3 months ago Reviewers:
James Cook CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, derat+watch_chromium.org, mazda+watch_chromium.org, kalyank, ben+ash_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionIntroduce "Preferred" accelerators, which may be processed by fullscreen, but will otherwise be processed first.
BUG=398905
TEST=covered by unit tests
Committed: https://crrev.com/555766488cb19bb5ad8f9411472551791bcaaef2
Cr-Commit-Position: refs/heads/master@{#295823}
Patch Set 1 : #
Total comments: 22
Patch Set 2 : addressed comments #Patch Set 3 : fixed typo #
Messages
Total messages: 27 (13 generated)
Patchset #1 (id:1) has been deleted
oshima@chromium.org changed reviewers: + jamescook@chromium.org
oshima@chromium.org changed reviewers: - jamescook@chromium.org
oshima@chromium.org changed reviewers: + jamescook@chromium.org
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
LGTM with nits Nice patch. https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... File ash/accelerators/accelerator_controller.h (right): https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:82: // the window is in fullscreen state. nice comments https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:86: // is always handled and will never passedto an window/web contents. nit: "never be passed to" https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:173: // Reserved actions. See accelerator_table.h for details. nit: Reserved -> Preferred https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller_unittest.cc:1087: ui::Accelerator(ui::VKEY_A, ui::EF_NONE))); How about an EXPECT_FALSE( IsPreferred() ) here also? https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller_unittest.cc:1094: PowerAcceleratorTest() { nit: match {} style with line below https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller_unittest.cc:1109: } // namespace nit: two spaces after } https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller_unittest.cc:1111: TEST_F(PowerAcceleratorTest, AcceleratorsWithFullsccreen) { nit: Fullsccreen -> Fullscreen https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller_unittest.cc:1130: // A fullscreen can consume ALT-TAB (preferred). nit: "A fullscreen" -> "A fullscreen window" https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller_unittest.cc:1133: ASSERT_EQ(active, w1); nit: This might be a little clearer if you did something like: EXPECT_EQ(active, w1); generator.PressKey(ui::VKEY_TAB, ui::EF_ALT_DOWN); EXPECT_EQ(active, w1); just to emphasize that what you're testing is that the active window doesn't change. (Yes, I know there's an assert at the top of the function, but it's 15 lines away :-) https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller_unittest.cc:1149: } nice test. You always write excellent tests. https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... File ash/accelerators/accelerator_table.h (right): https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_table.h:15: // There are four classes of accelerators in Ash: Can you update this comment? https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_table.h:171: // Actions that are always handled in Ash nit: end with .
I disabled part of the test on non chromeos platforms because VKEY_POWER is cros only. https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... File ash/accelerators/accelerator_controller.h (right): https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:86: // is always handled and will never passedto an window/web contents. On 2014/09/19 16:13:15, James Cook wrote: > nit: "never be passed to" Done. https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.h:173: // Reserved actions. See accelerator_table.h for details. On 2014/09/19 16:13:14, James Cook wrote: > nit: Reserved -> Preferred Done. https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller_unittest.cc:1087: ui::Accelerator(ui::VKEY_A, ui::EF_NONE))); On 2014/09/19 16:13:15, James Cook wrote: > How about an EXPECT_FALSE( IsPreferred() ) here also? Done. I was going to add them but apparently forgot them. Thank you for the catch! https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller_unittest.cc:1094: PowerAcceleratorTest() { On 2014/09/19 16:13:15, James Cook wrote: > nit: match {} style with line below Done. https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller_unittest.cc:1109: } // namespace On 2014/09/19 16:13:15, James Cook wrote: > nit: two spaces after } Done. https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller_unittest.cc:1111: TEST_F(PowerAcceleratorTest, AcceleratorsWithFullsccreen) { On 2014/09/19 16:13:15, James Cook wrote: > nit: Fullsccreen -> Fullscreen Done. https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller_unittest.cc:1130: // A fullscreen can consume ALT-TAB (preferred). On 2014/09/19 16:13:15, James Cook wrote: > nit: "A fullscreen" -> "A fullscreen window" Done. https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_controller_unittest.cc:1133: ASSERT_EQ(active, w1); On 2014/09/19 16:13:15, James Cook wrote: > nit: This might be a little clearer if you did something like: > > EXPECT_EQ(active, w1); > generator.PressKey(ui::VKEY_TAB, ui::EF_ALT_DOWN); > EXPECT_EQ(active, w1); > > just to emphasize that what you're testing is that the active window doesn't > change. (Yes, I know there's an assert at the top of the function, but it's 15 > lines away :-) Done. https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... File ash/accelerators/accelerator_table.h (right): https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_table.h:15: // There are four classes of accelerators in Ash: On 2014/09/19 16:13:15, James Cook wrote: > Can you update this comment? Done. https://codereview.chromium.org/582143004/diff/60001/ash/accelerators/acceler... ash/accelerators/accelerator_table.h:171: // Actions that are always handled in Ash On 2014/09/19 16:13:15, James Cook wrote: > nit: end with . Done.
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/582143004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/582143004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/582143004/100001
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/582143004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/582143004/100001
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as c25e4a53fd1f6a66ccb9265819bf9fbe17836404
Message was sent while issue was closed.
Failed to apply patch for ash/accelerators/accelerator_delegate.cc:
While running git apply --index -p1;
fatal: Unable to create
'/b/infra_internal/commit_queue/workdir/chromium/.git/index.lock': File exists.
If no other git process is currently running, this probably means a
git process crashed in this repository earlier. Make sure no other git
process is running and remove the file manually to continue.
Patch: ash/accelerators/accelerator_delegate.cc
Index: ash/accelerators/accelerator_delegate.cc
diff --git a/ash/accelerators/accelerator_delegate.cc
b/ash/accelerators/accelerator_delegate.cc
index
9dbd41035a902ef5a8a748ed18de49ab59e19b9b..bcdc75e0774d051e870bd530dc61319582f2cab3
100644
--- a/ash/accelerators/accelerator_delegate.cc
+++ b/ash/accelerators/accelerator_delegate.cc
@@ -59,26 +59,28 @@ bool AcceleratorDelegate::ShouldProcessAcceleratorNow(
root_windows.end())
return true;
- // A full screen window should be able to handle all key events including the
- // reserved ones.
aura::Window* top_level = ::wm::GetToplevelWindow(target);
+ Shell* shell = Shell::GetInstance();
+
+ // Reserved accelerators (such as Power button) always have a prority.
+ if (shell->accelerator_controller()->IsReserved(accelerator))
+ return true;
+ // A full screen window has a right to handle all key events including the
+ // reserved ones.
if (top_level && wm::GetWindowState(top_level)->IsFullscreen()) {
- // TODO(yusukes): On Chrome OS, only browser and flash windows can be full
- // screen. Launching an app in "open full-screen" mode is not supported
yet.
- // That makes the IsWindowFullscreen() check above almost meaningless
- // because a browser and flash window do handle Ash accelerators anyway
- // before they're passed to a page or flash content.
+ // On ChromeOS, fullscreen windows are either browser or apps, which
+ // send key events to a web content first, then will process keys
+ // if the web content didn't consume them.
return false;
}
- if (Shell::GetInstance()->GetAppListTargetVisibility())
+ // Handle preferred accelerators (such as ALT-TAB) before sending
+ // to the target.
+ if (shell->accelerator_controller()->IsPreferred(accelerator))
return true;
- // Unless |target| is in the full screen state, handle reserved accelerators
- // such as Alt+Tab now.
- return Shell::GetInstance()->accelerator_controller()->IsReservedAccelerator(
- accelerator);
+ return shell->GetAppListTargetVisibility();
}
} // namespace ash
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/555766488cb19bb5ad8f9411472551791bcaaef2 Cr-Commit-Position: refs/heads/master@{#295823} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
