Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
601333002:
ESC accelerator and consistent overview mode for Athena homecard (Closed)
|
Created:
6 years, 2 months ago by Greg Levin Modified:
6 years, 2 months ago CC:
chromium-reviews, oshima Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionESC accelerator and consistent overview mode for Athena homecard
BUG=394895, 412547
Committed: https://crrev.com/5c1a9983eac59cf7df8f97d13153781dfdb8de51
Cr-Commit-Position: refs/heads/master@{#299789}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address reviews, update unit tests #
Total comments: 6
Patch Set 3 : Remove ToggleOverview() #
Total comments: 7
Patch Set 4 : Fix merge (OVERRIDE->override) #Patch Set 5 : Merge with refactoring CL #
Total comments: 4
Patch Set 6 : Address review comments #
Messages
Total messages: 32 (7 generated)
glevin@chromium.org changed reviewers: + oshima@chromium.org
Here's a working draft. Please have a look. https://codereview.chromium.org/601333002/diff/1/athena/input/accelerator_man... File athena/input/accelerator_manager_impl.cc (right): https://codereview.chromium.org/601333002/diff/1/athena/input/accelerator_man... athena/input/accelerator_manager_impl.cc:346: : ui::ET_KEY_RELEASED); Fix indenting https://codereview.chromium.org/601333002/diff/1/athena/input/public/accelera... File athena/input/public/accelerator_manager.h (right): https://codereview.chromium.org/601333002/diff/1/athena/input/public/accelera... athena/input/public/accelerator_manager.h:89: Fix indenting
oshima@chromium.org changed reviewers: + mukai@chromium.org
redirecting to mukai who knows home card better. https://codereview.chromium.org/601333002/diff/1/athena/input/accelerator_man... File athena/input/accelerator_manager_impl.h (right): https://codereview.chromium.org/601333002/diff/1/athena/input/accelerator_man... athena/input/accelerator_manager_impl.h:73: AcceleratorHandler* handler) OVERRIDE; can you run "git cl format" ?
https://codereview.chromium.org/601333002/diff/1/athena/home/home_card_impl.cc File athena/home/home_card_impl.cc (right): https://codereview.chromium.org/601333002/diff/1/athena/home/home_card_impl.c... athena/home/home_card_impl.cc:308: void HomeCardImpl::SetStateAndOverview(HomeCard::State state) { This method name is unclear. SetStateAndToggleOverview? But it doesn't toggle overview state at some time. Based on my comments below, I believe this function is simply two line of: SetState(state); WindowManager::Get()->ToggleOverview(); https://codereview.chromium.org/601333002/diff/1/athena/home/home_card_impl.c... athena/home/home_card_impl.cc:434: SetState(overview_state_); I am not sure why overview_state_ is necessary. It's used here, right? I once introduced similar field, and some reviewer (oshima?) pointed out that it's okay to run SetState() -> ToggleOverview() in this order. The if-clause of the previous line was introduced for this purpose -- it prevents setting state back to BOTTOM from CENTERED. I believe you can go with the same way. https://codereview.chromium.org/601333002/diff/1/athena/wm/window_manager_imp... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/601333002/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:307: bool WindowManagerImpl::IsCommandEnabled(int command_id) const { Isn't it good enough to make this returning false when it's not in overview mode rather than registering/unregistering the accelerator?
https://codereview.chromium.org/601333002/diff/1/athena/wm/window_manager_imp... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/601333002/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:307: bool WindowManagerImpl::IsCommandEnabled(int command_id) const { On 2014/09/25 21:53:41, Jun Mukai wrote: > Isn't it good enough to make this returning false when it's not in overview mode > rather than registering/unregistering the accelerator? FYI: if you want to keep register/unregister structure, I'd suggest to change WindowOverviewMode as an accelerator handler, and it registers in ctor and unregisters in dtor. That looks more natural to me. # also, it could be great to use a WeakPtr for AcceleratorHandler in AcceleratorManager so that manager recognizes the deletion of a handler and automatically unregisters the accelerators on destruction.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
I've address comments and updated unit tests. Please have another look! https://codereview.chromium.org/601333002/diff/1/athena/home/home_card_impl.cc File athena/home/home_card_impl.cc (right): https://codereview.chromium.org/601333002/diff/1/athena/home/home_card_impl.c... athena/home/home_card_impl.cc:308: void HomeCardImpl::SetStateAndOverview(HomeCard::State state) { On 2014/09/25 21:53:41, Jun Mukai wrote: > This method name is unclear. SetStateAndToggleOverview? But it doesn't toggle > overview state at some time. > > Based on my comments below, I believe this function is simply two line of: > SetState(state); > WindowManager::Get()->ToggleOverview(); Done (mostly... see below) https://codereview.chromium.org/601333002/diff/1/athena/home/home_card_impl.c... athena/home/home_card_impl.cc:434: SetState(overview_state_); On 2014/09/25 21:53:41, Jun Mukai wrote: > I am not sure why overview_state_ is necessary. It's used here, right? > > I once introduced similar field, and some reviewer (oshima?) pointed out that > it's okay to run SetState() -> ToggleOverview() in this order. The if-clause of > the previous line was introduced for this purpose -- it prevents setting state > back to BOTTOM from CENTERED. > > I believe you can go with the same way. ToggleOverview() was a little imprecise for use here, so I created WindowManager::ActivateOverview(), and am using SetState() -> ActivateOverview() instead. https://codereview.chromium.org/601333002/diff/1/athena/input/accelerator_man... File athena/input/accelerator_manager_impl.cc (right): https://codereview.chromium.org/601333002/diff/1/athena/input/accelerator_man... athena/input/accelerator_manager_impl.cc:346: : ui::ET_KEY_RELEASED); On 2014/09/25 18:38:50, Greg Levin wrote: > Fix indenting Done. https://codereview.chromium.org/601333002/diff/1/athena/input/accelerator_man... File athena/input/accelerator_manager_impl.h (right): https://codereview.chromium.org/601333002/diff/1/athena/input/accelerator_man... athena/input/accelerator_manager_impl.h:73: AcceleratorHandler* handler) OVERRIDE; On 2014/09/25 21:39:43, oshima wrote: > can you run "git cl format" ? Done, and thanks for the advice! https://codereview.chromium.org/601333002/diff/1/athena/input/public/accelera... File athena/input/public/accelerator_manager.h (right): https://codereview.chromium.org/601333002/diff/1/athena/input/public/accelera... athena/input/public/accelerator_manager.h:89: On 2014/09/25 18:38:50, Greg Levin wrote: > Fix indenting Done. https://codereview.chromium.org/601333002/diff/1/athena/wm/window_manager_imp... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/601333002/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:307: bool WindowManagerImpl::IsCommandEnabled(int command_id) const { On 2014/09/25 21:53:41, Jun Mukai wrote: > Isn't it good enough to make this returning false when it's not in overview mode > rather than registering/unregistering the accelerator? Leaving as is based on our conversation with Oshima https://codereview.chromium.org/601333002/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:307: bool WindowManagerImpl::IsCommandEnabled(int command_id) const { On 2014/09/25 22:37:45, Jun Mukai wrote: > FYI: if you want to keep register/unregister structure, I'd suggest to change > WindowOverviewMode as an accelerator handler, and it registers in ctor and > unregisters in dtor. That looks more natural to me. Based on our conversation, I'm leaving this alone. I agree that having WindowOverviewMode (un)register it is more natural in one way. But since WindowManagerImpl is already an AcceleratorHandler, and is already handling one accelerator to (de)activate overview mode, letting it handle the other accelerator to deactivate overview mode also seems natural. Let me know if you decide you'd like this changed after all. > # also, it could be great to use a WeakPtr for AcceleratorHandler in > AcceleratorManager so that manager recognizes the deletion of a handler and > automatically unregisters the accelerators on destruction. Not implemented, based on our conversation.
https://codereview.chromium.org/601333002/diff/60001/athena/wm/public/window_... File athena/wm/public/window_manager.h (right): https://codereview.chromium.org/601333002/diff/60001/athena/wm/public/window_... athena/wm/public/window_manager.h:26: virtual void ToggleOverview() = 0; Can you remove ToggleOverview? It can be simply rewritten as SetInOverview(!IsOverviewModeActive()) https://codereview.chromium.org/601333002/diff/60001/athena/wm/public/window_... athena/wm/public/window_manager.h:27: virtual void ActivateOverview(bool activate) = 0; WindowManagerImpl already has SetInOverview(). Why not simply reusing this name? https://codereview.chromium.org/601333002/diff/60001/athena/wm/window_manager... File athena/wm/window_manager_impl.h (right): https://codereview.chromium.org/601333002/diff/60001/athena/wm/window_manager... athena/wm/window_manager_impl.h:60: void SetInOverview(bool active); Remove this method.
Please have another look. Thanks! https://codereview.chromium.org/601333002/diff/60001/athena/wm/public/window_... File athena/wm/public/window_manager.h (right): https://codereview.chromium.org/601333002/diff/60001/athena/wm/public/window_... athena/wm/public/window_manager.h:26: virtual void ToggleOverview() = 0; On 2014/10/03 02:26:06, Jun Mukai wrote: > Can you remove ToggleOverview? > It can be simply rewritten as SetInOverview(!IsOverviewModeActive()) Done. https://codereview.chromium.org/601333002/diff/60001/athena/wm/public/window_... athena/wm/public/window_manager.h:27: virtual void ActivateOverview(bool activate) = 0; On 2014/10/03 02:26:07, Jun Mukai wrote: > WindowManagerImpl already has SetInOverview(). Why not > simply reusing this name? SetInOverview() (private) only deals with internal state. ActivateOverview(false) will additionally show the window which was active prior to overview mode. Some code is only calling SetInOverview(), and so presumably doesn't want the previous window shown. I assume this is why ToggleOverview() and SetInOverview() were previously separate functions, and so I've left this separation. https://codereview.chromium.org/601333002/diff/60001/athena/wm/window_manager... File athena/wm/window_manager_impl.h (right): https://codereview.chromium.org/601333002/diff/60001/athena/wm/window_manager... athena/wm/window_manager_impl.h:60: void SetInOverview(bool active); On 2014/10/03 02:26:07, Jun Mukai wrote: > Remove this method. See comment in window_manager.h
cc: pkotwicz to notify the interface change of WindowManager. Also a nit comment. https://codereview.chromium.org/601333002/diff/80001/athena/wm/window_manager... File athena/wm/window_manager_impl.h (right): https://codereview.chromium.org/601333002/diff/80001/athena/wm/window_manager... athena/wm/window_manager_impl.h:45: virtual void ActivateOverview(bool activate) OVERRIDE; Please keep the lowercase override. That's the new C++11 style (FYI: http://chromium-cpp.appspot.com/)
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
+oshima https://codereview.chromium.org/601333002/diff/80001/athena/wm/window_manager... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/601333002/diff/80001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:221: void WindowManagerImpl::SetInOverview(bool active) { +oshima for his opinion. I remember that the architecture of this was controversial How about splitting WindowManagerImpl::SetInOverview() into WindowManagerImpl::EnterOverview() and WindowManagerImpl::ExitOverview(aura::Window* to_activate) WindowManagerImpl::ExitOverview() would have special behavior when NULL is passed in. Please wait for Oshima's opinion before proceeding
https://codereview.chromium.org/601333002/diff/80001/athena/wm/window_manager... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/601333002/diff/80001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:221: void WindowManagerImpl::SetInOverview(bool active) { On 2014/10/07 01:30:26, pkotwicz wrote: > +oshima for his opinion. I remember that the architecture of this was > controversial > > How about splitting WindowManagerImpl::SetInOverview() into > WindowManagerImpl::EnterOverview() and > WindowManagerImpl::ExitOverview(aura::Window* to_activate) > > WindowManagerImpl::ExitOverview() would have special behavior when NULL is > passed in. > > Please wait for Oshima's opinion before proceeding Yes, that'd be better, although this CL is unrelated to that change. If you can work on it in separate CL, that'd be great.
Fixed the OVERRIDE merge problem, saving the SetInOverview() refactor for subsequent CL. Please have another look. https://codereview.chromium.org/601333002/diff/80001/athena/wm/window_manager... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/601333002/diff/80001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:221: void WindowManagerImpl::SetInOverview(bool active) { On 2014/10/07 01:30:26, pkotwicz wrote: > +oshima for his opinion. I remember that the architecture of this was > controversial > > How about splitting WindowManagerImpl::SetInOverview() into > WindowManagerImpl::EnterOverview() and > WindowManagerImpl::ExitOverview(aura::Window* to_activate) > > WindowManagerImpl::ExitOverview() would have special behavior when NULL is > passed in. > > Please wait for Oshima's opinion before proceeding Acknowledged. https://codereview.chromium.org/601333002/diff/80001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:221: void WindowManagerImpl::SetInOverview(bool active) { On 2014/10/08 00:45:45, oshima wrote: > On 2014/10/07 01:30:26, pkotwicz wrote: > > +oshima for his opinion. I remember that the architecture of this was > > controversial > > > > How about splitting WindowManagerImpl::SetInOverview() into > > WindowManagerImpl::EnterOverview() and > > WindowManagerImpl::ExitOverview(aura::Window* to_activate) > > > > WindowManagerImpl::ExitOverview() would have special behavior when NULL is > > passed in. > > > > Please wait for Oshima's opinion before proceeding > > Yes, that'd be better, although this CL is unrelated to that change. If you can > work on it in separate CL, that'd be great. As per Oshima's suggestion, I'll do this refactor in a subsequent CL https://codereview.chromium.org/601333002/diff/80001/athena/wm/window_manager... File athena/wm/window_manager_impl.h (right): https://codereview.chromium.org/601333002/diff/80001/athena/wm/window_manager... athena/wm/window_manager_impl.h:45: virtual void ActivateOverview(bool activate) OVERRIDE; On 2014/10/06 21:23:49, Jun Mukai wrote: > Please keep the lowercase override. That's the new C++11 style (FYI: > http://chromium-cpp.appspot.com/) Done (!&@#$ merge tool)
lgtm
https://codereview.chromium.org/601333002/diff/80001/athena/wm/window_manager... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/601333002/diff/80001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:221: void WindowManagerImpl::SetInOverview(bool active) { I would recommend either: a) Splitting WindowManagerImpl::SetInOverview() into two functions as a CL prior to this one. This would avoid you having to change most of the files that you changed in this CL in the subsequent CL as well b) Not changing the athena::WindowManager API in this CL (and doing so in the subsequent CL)
Given that you have mukai@'s approval, I am fine with a subsequent CL
On 2014/10/08 19:55:17, pkotwicz wrote: > Given that you have mukai@'s approval, I am fine with a subsequent CL Sorry for putting approval early. I would appreciate if Greg addresses Peter's comment, but I may also think that doing everything in a subsequent CL would be in acceptable size. It's up to Greg.
Peter: > How about splitting WindowManagerImpl::SetInOverview() into > WindowManagerImpl::EnterOverview() and > WindowManagerImpl::ExitOverview(aura::Window* to_activate) > > WindowManagerImpl::ExitOverview() would have special behavior when NULL is > passed in. There's (i) only one place in the code where ExitOverview(some_window) would be called, and (ii) seven where ExitOverview(NULL) would be (in order to reactivate the previously active window). There is, additionally, (iii) once place where the calling routine doesn't want any window activated when exiting overview. I'm thinking instead to have (i) ExitOverviewNoActivate(); some_window->Show(); wm::ActivateWindow(some_window); (ii) ExitOverview() (iii) ExitOverviewNoActivate(); How does this sound?
glevin@, your proposal sounds good to me
Now that https://codereview.chromium.org/635223004 has refactored ToggleOverview() and SetInOverview() into Enter/ExitOverview(), here are the remaining changes which fix the bugs at hand. Please have another look!
LGTM with nits https://codereview.chromium.org/601333002/diff/140001/athena/input/accelerato... File athena/input/accelerator_manager_impl.cc (right): https://codereview.chromium.org/601333002/diff/140001/athena/input/accelerato... athena/input/accelerator_manager_impl.cc:351: accelerators_.erase(iter); Can you just do accelerators_.erase(accelerator) and remove lines 348 and 349 https://codereview.chromium.org/601333002/diff/140001/athena/input/accelerato... File athena/input/accelerator_manager_impl.h (right): https://codereview.chromium.org/601333002/diff/140001/athena/input/accelerato... athena/input/accelerator_manager_impl.h:66: virtual void SetDebugAcceleratorsEnabled(bool enabled) override; Nit: Group all of the accelerators which AcceleratorManager overrides together. (My preference is in the same order as defined in AcceleratorManager too)
https://codereview.chromium.org/601333002/diff/140001/athena/input/accelerato... File athena/input/accelerator_manager_impl.cc (right): https://codereview.chromium.org/601333002/diff/140001/athena/input/accelerato... athena/input/accelerator_manager_impl.cc:351: accelerators_.erase(iter); On 2014/10/15 17:00:33, pkotwicz wrote: > Can you just do > accelerators_.erase(accelerator) > and remove lines 348 and 349 Done. https://codereview.chromium.org/601333002/diff/140001/athena/input/accelerato... File athena/input/accelerator_manager_impl.h (right): https://codereview.chromium.org/601333002/diff/140001/athena/input/accelerato... athena/input/accelerator_manager_impl.h:66: virtual void SetDebugAcceleratorsEnabled(bool enabled) override; On 2014/10/15 17:00:33, pkotwicz wrote: > Nit: Group all of the accelerators which AcceleratorManager overrides together. > (My preference is in the same order as defined in AcceleratorManager too) Done.
skuhne@chromium.org changed reviewers: + skuhne@chromium.org
lgtm
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/601333002/160001
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5c1a9983eac59cf7df8f97d13153781dfdb8de51 Cr-Commit-Position: refs/heads/master@{#299789} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
