Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9722035:
Support cycling through the window by F5. (Closed)
|
Created:
8 years, 9 months ago by Jun Mukai Modified:
8 years, 9 months ago CC:
chromium-reviews, dhollowa+watch_chromium.org, sadrul, ben+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport cycling through the window by F5.
BUG=118132
TEST=unittest passed locally
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=128165
Patch Set 1 #Patch Set 2 : Remove an unnecessary declaration #
Total comments: 4
Patch Set 3 : fix #
Total comments: 2
Patch Set 4 : Use LauncherModel #Patch Set 5 : cleanup #Patch Set 6 : use ItemClicked #Patch Set 7 : fix #
Total comments: 12
Patch Set 8 : fix #Patch Set 9 : Use ItemClicked instead #Patch Set 10 : fix #
Total comments: 4
Patch Set 11 : fix #Patch Set 12 : add todo comment #
Messages
Total messages: 24 (0 generated)
https://chromiumcodereview.appspot.com/9722035/diff/7/ash/wm/window_cycle_con... File ash/wm/window_cycle_controller.cc (right): https://chromiumcodereview.appspot.com/9722035/diff/7/ash/wm/window_cycle_con... ash/wm/window_cycle_controller.cc:195: scoped_ptr<WindowListObserver> observer(new WindowListObserver); Why bother creating this here? Instead created it in the if around 199ish. https://chromiumcodereview.appspot.com/9722035/diff/7/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://chromiumcodereview.appspot.com/9722035/diff/7/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:75: if (current_index_ >= removed_index) { What if rmeoved_index == 0?
On 2012/03/19 15:26:51, sky wrote: > https://chromiumcodereview.appspot.com/9722035/diff/7/ash/wm/window_cycle_con... > File ash/wm/window_cycle_controller.cc (right): > > https://chromiumcodereview.appspot.com/9722035/diff/7/ash/wm/window_cycle_con... > ash/wm/window_cycle_controller.cc:195: scoped_ptr<WindowListObserver> > observer(new WindowListObserver); > Why bother creating this here? Instead created it in the if around 199ish. > > https://chromiumcodereview.appspot.com/9722035/diff/7/ash/wm/window_cycle_lis... > File ash/wm/window_cycle_list.cc (right): > > https://chromiumcodereview.appspot.com/9722035/diff/7/ash/wm/window_cycle_lis... > ash/wm/window_cycle_list.cc:75: if (current_index_ >= removed_index) { > What if rmeoved_index == 0? I commented on the bug. If we want to go back to support a "linear" window cycling order, I would suggest reverting parts of my removal of compact window mode (or I can do it), see crrev.com/125625. I thought we always wanted most-recently-used window cycling now that we have visible windows and removed linear cycling as part of the removal of compact window mode.
On 2012/03/19 15:55:33, James Cook (Chromium) wrote: > On 2012/03/19 15:26:51, sky wrote: > > > https://chromiumcodereview.appspot.com/9722035/diff/7/ash/wm/window_cycle_con... > > File ash/wm/window_cycle_controller.cc (right): > > > > > https://chromiumcodereview.appspot.com/9722035/diff/7/ash/wm/window_cycle_con... > > ash/wm/window_cycle_controller.cc:195: scoped_ptr<WindowListObserver> > > observer(new WindowListObserver); > > Why bother creating this here? Instead created it in the if around 199ish. > > > > > https://chromiumcodereview.appspot.com/9722035/diff/7/ash/wm/window_cycle_lis... > > File ash/wm/window_cycle_list.cc (right): > > > > > https://chromiumcodereview.appspot.com/9722035/diff/7/ash/wm/window_cycle_lis... > > ash/wm/window_cycle_list.cc:75: if (current_index_ >= removed_index) { > > What if rmeoved_index == 0? > > I commented on the bug. If we want to go back to support a "linear" window > cycling order, I would suggest reverting parts of my removal of compact window > mode (or I can do it), see crrev.com/125625. I thought we always wanted > most-recently-used window cycling now that we have visible windows and removed > linear cycling as part of the removal of compact window mode. Apparently I am wrong and we want MRU-cycling for alt-tab and linear cycling for F5. Would you like me to put back the parts of compact mode that have this behavior, or would you prefer to fix up and land this CL?
http://codereview.chromium.org/9722035/diff/7/ash/wm/window_cycle_controller.cc File ash/wm/window_cycle_controller.cc (right): http://codereview.chromium.org/9722035/diff/7/ash/wm/window_cycle_controller.... ash/wm/window_cycle_controller.cc:195: scoped_ptr<WindowListObserver> observer(new WindowListObserver); On 2012/03/19 15:26:51, sky wrote: > Why bother creating this here? Instead created it in the if around 199ish. Done. http://codereview.chromium.org/9722035/diff/7/ash/wm/window_cycle_list.cc File ash/wm/window_cycle_list.cc (right): http://codereview.chromium.org/9722035/diff/7/ash/wm/window_cycle_list.cc#new... ash/wm/window_cycle_list.cc:75: if (current_index_ >= removed_index) { On 2012/03/19 15:26:51, sky wrote: > What if rmeoved_index == 0? Add the code to taking care of the case. Thanks for heads up.
LGTM with optional nits. http://codereview.chromium.org/9722035/diff/6001/ash/wm/window_cycle_controll... File ash/wm/window_cycle_controller.cc (right): http://codereview.chromium.org/9722035/diff/6001/ash/wm/window_cycle_controll... ash/wm/window_cycle_controller.cc:195: window_list_observer_.reset(new WindowListObserver); nit: I think the code would be simpler if you constructed the WindowListObserver in the WindowCycleController() constructor. I know the event filter doesn't work this way, but then you could avoid some null checks. Likewise, I don't think you need to check |container| for NULL -- if we don't have a default container in the Shell, something is very wrong. http://codereview.chromium.org/9722035/diff/6001/ash/wm/window_cycle_controll... File ash/wm/window_cycle_controller.h (right): http://codereview.chromium.org/9722035/diff/6001/ash/wm/window_cycle_controll... ash/wm/window_cycle_controller.h:41: // interprets this call as the start of a multi-step cycle sequence and nit: Is this comment still accurate? Does passing |is_alt_down| false start a cycle?
I'm a bit confused now. John, could you clarify what behavior you are aiming for here? In the bug Alex says launcher order. If you're going for that I would say you should be talking to the launcher, as that way app tabs are honored. Stefan has been working on supporting accelerators for selecting items in the launcher. There is overlap with that and making f5 cycle through launcher items.
Yes, the change is in. (http://codereview.chromium.org/9677046) It should be easy to find out which index you have and then simply call the function (which is already in the accelerator module) and switch to the next. That way the order should be maintained.
On 2012/03/20 23:35:43, Mr4D wrote: > Yes, the change is in. (http://codereview.chromium.org/9677046) > > It should be easy to find out which index you have and then simply call the > function (which is already in the accelerator module) and switch to the next. > That way the order should be maintained. Oh, should it be same as launcher's order? I just skipped that part in the bug item. Hmm. I'm feeling that any order would be okay and we should choose simpler design. Let me look into that idea.
On 2012/03/21 01:26:32, Jun Mukai wrote: > On 2012/03/20 23:35:43, Mr4D wrote: > > Yes, the change is in. (http://codereview.chromium.org/9677046) > > > > It should be easy to find out which index you have and then simply call the > > function (which is already in the accelerator module) and switch to the next. > > That way the order should be maintained. > > Oh, should it be same as launcher's order? I just skipped that part in the bug > item. Hmm. > I'm feeling that any order would be okay and we should choose simpler design. > Let me look into that idea. Ah, by the way, if you are saying to use Stefan's SwitchToWindow() to this change, that does not sound a good idea. Because I'm trying to introduce cycling, I need to add "focused_index_" or something to accelerator, and accelerator needs to care about the focus changes by mouse clicks. That would lead to a mess. Certainly there is overlap, but not applicable to this feature. My previous comment is to reuse "LauncherWindowCycler" implementation.
On 2012/03/21 01:45:41, Jun Mukai wrote: > On 2012/03/21 01:26:32, Jun Mukai wrote: > > On 2012/03/20 23:35:43, Mr4D wrote: > > > Yes, the change is in. (http://codereview.chromium.org/9677046) > > > > > > It should be easy to find out which index you have and then simply call the > > > function (which is already in the accelerator module) and switch to the > next. > > > That way the order should be maintained. > > > > Oh, should it be same as launcher's order? I just skipped that part in the > bug > > item. Hmm. > > I'm feeling that any order would be okay and we should choose simpler design. > > Let me look into that idea. > > Ah, by the way, if you are saying to use Stefan's SwitchToWindow() to this > change, that does not sound a good idea. Because I'm trying to introduce > cycling, I need to add "focused_index_" or something to accelerator, and > accelerator needs to care about the focus changes by mouse clicks. That would > lead to a mess. Certainly there is overlap, but not applicable to this feature. > > My previous comment is to reuse "LauncherWindowCycler" implementation. LauncherWindowCycler is tightly connected to files in Launcher, so it's not handy for the use of F5-key. FYI: https://chromiumcodereview.appspot.com/9794017 is a patch I wrote for it. So I think this patch is the best approach so far, so I want to proceed the code review again. Any other ideas?
I'm still confused then as to behavior you are trying to introduce. Could you expand on that? -Scott On Tue, Mar 20, 2012 at 9:17 PM, <mukai@chromium.org> wrote: > On 2012/03/21 01:45:41, Jun Mukai wrote: >> >> On 2012/03/21 01:26:32, Jun Mukai wrote: >> > On 2012/03/20 23:35:43, Mr4D wrote: >> > > Yes, the change is in. (http://codereview.chromium.org/9677046) >> > > >> > > It should be easy to find out which index you have and then simply >> > > call > > the >> >> > > function (which is already in the accelerator module) and switch to >> > > the >> next. >> > > That way the order should be maintained. >> > >> > Oh, should it be same as launcher's order? I just skipped that part in >> > the >> bug >> > item. Hmm. >> > I'm feeling that any order would be okay and we should choose simpler > > design. >> >> > Let me look into that idea. > > >> Ah, by the way, if you are saying to use Stefan's SwitchToWindow() to this >> change, that does not sound a good idea. Because I'm trying to introduce >> cycling, I need to add "focused_index_" or something to accelerator, and >> accelerator needs to care about the focus changes by mouse clicks. That >> would >> lead to a mess. Certainly there is overlap, but not applicable to this > > feature. > >> My previous comment is to reuse "LauncherWindowCycler" implementation. > > > LauncherWindowCycler is tightly connected to files in Launcher, so it's not > handy for the use of F5-key. FYI: > https://chromiumcodereview.appspot.com/9794017 is a patch I wrote for it. > > So I think this patch is the best approach so far, so I want to proceed the > code > review again. Any other ideas? > > http://codereview.chromium.org/9722035/
As I chatted with sky, I uploaded a new patch to introduce this behavior using LauncherModel. Please take another look at this. I noticed a mysterious behavior here if we mix apps icon and tabs icon, but it is independent from this patch. See crbug.com/119255 FYI.
I would like to see some tests for this, but you can do that later if you're crunched for time. Also, I think this should be moved out into standalone functions for easier testing. But that too can be done later. http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:385: if (item.status == STATUS_RUNNING && first_running) Shouldn't this be first_running == -1? http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:408: // Only TYPE_TABBED item is interested because we do not want to change tab TYPE_APP is used for both app tabs and apps. So, you shouldn't skip it. http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:420: if (next_index > 0) nit: move to 416 and instead of breaking, return.
I defer to Sky on the overall approach. I had a couple of optional suggestions. http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:371: void AcceleratorController::CycleWindow( Can this be a static function in the anonymous namespace in this file? Then you wouldn't need to #include window_cycle_controller.h in accelerator_controller.h. http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... File ash/accelerators/accelerator_table.h (right): http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... ash/accelerators/accelerator_table.h:16: CYCLE_BACKWARD, Maybe change to CYCLE_BACKWARD_MRU? http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... ash/accelerators/accelerator_table.h:18: CYCLE_BACKWARD_THROUGH_LIST, Maybe change to CYCLE_BACKWARD_LINEAR?
Not adding tests yet. To test the code, I'm feeling to move the logic somewhere else. Probably to LauncherModel, but not so sure. Do some of you have ideas? http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:371: void AcceleratorController::CycleWindow( On 2012/03/21 15:32:27, James Cook (Chromium) wrote: > Can this be a static function in the anonymous namespace in this file? Then you > wouldn't need to #include window_cycle_controller.h in accelerator_controller.h. Done. http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:385: if (item.status == STATUS_RUNNING && first_running) On 2012/03/21 14:34:32, sky wrote: > Shouldn't this be first_running == -1? Done. http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:408: // Only TYPE_TABBED item is interested because we do not want to change tab On 2012/03/21 14:34:32, sky wrote: > TYPE_APP is used for both app tabs and apps. So, you shouldn't skip it. Done. http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:420: if (next_index > 0) On 2012/03/21 14:34:32, sky wrote: > nit: move to 416 and instead of breaking, return. Done. http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... File ash/accelerators/accelerator_table.h (right): http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... ash/accelerators/accelerator_table.h:16: CYCLE_BACKWARD, On 2012/03/21 15:32:27, James Cook (Chromium) wrote: > Maybe change to CYCLE_BACKWARD_MRU? Done. http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... ash/accelerators/accelerator_table.h:18: CYCLE_BACKWARD_THROUGH_LIST, On 2012/03/21 15:32:27, James Cook (Chromium) wrote: > Maybe change to CYCLE_BACKWARD_LINEAR? Done.
On 2012/03/21 16:16:25, Jun Mukai wrote: > Not adding tests yet. To test the code, I'm feeling to move the logic somewhere > else. Probably to LauncherModel, but not so sure. Do some of you have ideas? > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > File ash/accelerators/accelerator_controller.cc (right): > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > ash/accelerators/accelerator_controller.cc:371: void > AcceleratorController::CycleWindow( > On 2012/03/21 15:32:27, James Cook (Chromium) wrote: > > Can this be a static function in the anonymous namespace in this file? Then > you > > wouldn't need to #include window_cycle_controller.h in > accelerator_controller.h. > > Done. > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > ash/accelerators/accelerator_controller.cc:385: if (item.status == > STATUS_RUNNING && first_running) > On 2012/03/21 14:34:32, sky wrote: > > Shouldn't this be first_running == -1? > > Done. > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > ash/accelerators/accelerator_controller.cc:408: // Only TYPE_TABBED item is > interested because we do not want to change tab > On 2012/03/21 14:34:32, sky wrote: > > TYPE_APP is used for both app tabs and apps. So, you shouldn't skip it. > > Done. > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > ash/accelerators/accelerator_controller.cc:420: if (next_index > 0) > On 2012/03/21 14:34:32, sky wrote: > > nit: move to 416 and instead of breaking, return. > > Done. > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > File ash/accelerators/accelerator_table.h (right): > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > ash/accelerators/accelerator_table.h:16: CYCLE_BACKWARD, > On 2012/03/21 15:32:27, James Cook (Chromium) wrote: > > Maybe change to CYCLE_BACKWARD_MRU? > > Done. > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > ash/accelerators/accelerator_table.h:18: CYCLE_BACKWARD_THROUGH_LIST, > On 2012/03/21 15:32:27, James Cook (Chromium) wrote: > > Maybe change to CYCLE_BACKWARD_LINEAR? > > Done. Sorry, patchset8 won't work correctly in some case of apps. Sigh... I should have written tests beforehand.
I wouldn't put this in the model. Maybe launcher_navigator or something that is just a set of functions in a namespace. They would take the LauncherModel to operate on. -Scott On Wed, Mar 21, 2012 at 9:26 AM, <mukai@chromium.org> wrote: > On 2012/03/21 16:16:25, Jun Mukai wrote: >> >> Not adding tests yet. To test the code, I'm feeling to move the logic > > somewhere >> >> else. Probably to LauncherModel, but not so sure. Do some of you have >> ideas? > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... >> >> File ash/accelerators/accelerator_controller.cc (right): > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... >> >> ash/accelerators/accelerator_controller.cc:371: void >> AcceleratorController::CycleWindow( >> On 2012/03/21 15:32:27, James Cook (Chromium) wrote: >> > Can this be a static function in the anonymous namespace in this file? >> > Then >> you >> > wouldn't need to #include window_cycle_controller.h in >> accelerator_controller.h. > > >> Done. > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... >> >> ash/accelerators/accelerator_controller.cc:385: if (item.status == >> STATUS_RUNNING && first_running) >> On 2012/03/21 14:34:32, sky wrote: >> > Shouldn't this be first_running == -1? > > >> Done. > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... >> >> ash/accelerators/accelerator_controller.cc:408: // Only TYPE_TABBED item >> is >> interested because we do not want to change tab >> On 2012/03/21 14:34:32, sky wrote: >> > TYPE_APP is used for both app tabs and apps. So, you shouldn't skip it. > > >> Done. > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... >> >> ash/accelerators/accelerator_controller.cc:420: if (next_index > 0) >> On 2012/03/21 14:34:32, sky wrote: >> > nit: move to 416 and instead of breaking, return. > > >> Done. > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... >> >> File ash/accelerators/accelerator_table.h (right): > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... >> >> ash/accelerators/accelerator_table.h:16: CYCLE_BACKWARD, >> On 2012/03/21 15:32:27, James Cook (Chromium) wrote: >> > Maybe change to CYCLE_BACKWARD_MRU? > > >> Done. > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... >> >> ash/accelerators/accelerator_table.h:18: CYCLE_BACKWARD_THROUGH_LIST, >> On 2012/03/21 15:32:27, James Cook (Chromium) wrote: >> > Maybe change to CYCLE_BACKWARD_LINEAR? > > >> Done. > > > Sorry, patchset8 won't work correctly in some case of apps. Sigh... I > should > have written tests beforehand. > > http://codereview.chromium.org/9722035/
Thanks for the suggestion. For tests, I'm now thinking it's better to write them in M20. Left comments and I'll file a bug as a reminder. I fixed the bug which I mentioned by introducing ItemClicked() instead of Set(). Could you take a look at this? On 2012/03/21 16:31:57, sky wrote: > I wouldn't put this in the model. Maybe launcher_navigator or > something that is just a set of functions in a namespace. They would > take the LauncherModel to operate on. > > -Scott > > On Wed, Mar 21, 2012 at 9:26 AM, <mailto:mukai@chromium.org> wrote: > > On 2012/03/21 16:16:25, Jun Mukai wrote: > >> > >> Not adding tests yet. To test the code, I'm feeling to move the logic > > > > somewhere > >> > >> else. Probably to LauncherModel, but not so sure. Do some of you have > >> ideas? > > > > > > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > >> > >> File ash/accelerators/accelerator_controller.cc (right): > > > > > > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > >> > >> ash/accelerators/accelerator_controller.cc:371: void > >> AcceleratorController::CycleWindow( > >> On 2012/03/21 15:32:27, James Cook (Chromium) wrote: > >> > Can this be a static function in the anonymous namespace in this file? > >> > Then > >> you > >> > wouldn't need to #include window_cycle_controller.h in > >> accelerator_controller.h. > > > > > >> Done. > > > > > > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > >> > >> ash/accelerators/accelerator_controller.cc:385: if (item.status == > >> STATUS_RUNNING && first_running) > >> On 2012/03/21 14:34:32, sky wrote: > >> > Shouldn't this be first_running == -1? > > > > > >> Done. > > > > > > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > >> > >> ash/accelerators/accelerator_controller.cc:408: // Only TYPE_TABBED item > >> is > >> interested because we do not want to change tab > >> On 2012/03/21 14:34:32, sky wrote: > >> > TYPE_APP is used for both app tabs and apps. So, you shouldn't skip it. > > > > > >> Done. > > > > > > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > >> > >> ash/accelerators/accelerator_controller.cc:420: if (next_index > 0) > >> On 2012/03/21 14:34:32, sky wrote: > >> > nit: move to 416 and instead of breaking, return. > > > > > >> Done. > > > > > > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > >> > >> File ash/accelerators/accelerator_table.h (right): > > > > > > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > >> > >> ash/accelerators/accelerator_table.h:16: CYCLE_BACKWARD, > >> On 2012/03/21 15:32:27, James Cook (Chromium) wrote: > >> > Maybe change to CYCLE_BACKWARD_MRU? > > > > > >> Done. > > > > > > > > > http://codereview.chromium.org/9722035/diff/21001/ash/accelerators/accelerato... > >> > >> ash/accelerators/accelerator_table.h:18: CYCLE_BACKWARD_THROUGH_LIST, > >> On 2012/03/21 15:32:27, James Cook (Chromium) wrote: > >> > Maybe change to CYCLE_BACKWARD_LINEAR? > > > > > >> Done. > > > > > > Sorry, patchset8 won't work correctly in some case of apps. Sigh... I > > should > > have written tests beforehand. > > > > http://codereview.chromium.org/9722035/
LGTM with optional suggestion. http://codereview.chromium.org/9722035/diff/20003/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/9722035/diff/20003/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:39: window_cycle_controller()->HandleCycleWindow(direction, is_alt_down); This is not a bug in your code, but I notice on ToT that alt-tab does not select app windows. :-( Probably the MRU implementation will eventually need to share access to the same launcher data structure as your code. http://codereview.chromium.org/9722035/diff/20003/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:44: void ActivateWindow(int index) { You might want to call this something else, like ActivateLauncherItem(), to distinguish it from ash::wm::ActivateWindow() in window_utils.h.
http://codereview.chromium.org/9722035/diff/20003/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/9722035/diff/20003/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:39: window_cycle_controller()->HandleCycleWindow(direction, is_alt_down); On 2012/03/22 03:05:27, James Cook (Chromium) wrote: > This is not a bug in your code, but I notice on ToT that alt-tab does not select > app windows. :-( Probably the MRU implementation will eventually need to share > access to the same launcher data structure as your code. Let's file a bug for it. Should I leave TODO comment here? http://codereview.chromium.org/9722035/diff/20003/ash/accelerators/accelerato... ash/accelerators/accelerator_controller.cc:44: void ActivateWindow(int index) { On 2012/03/22 03:05:27, James Cook (Chromium) wrote: > You might want to call this something else, like ActivateLauncherItem(), to > distinguish it from ash::wm::ActivateWindow() in window_utils.h. Done.
Sure, leave a TODO and open a bug.
LGTM
On 2012/03/22 03:32:09, sky wrote: > LGTM Added the TODO comment. will submit soon. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
