Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10700057:
Add always on top windows to the alt+tab list (Closed)
|
Created:
8 years, 5 months ago by Zachary Kuznia Modified:
8 years, 4 months ago CC:
chromium-reviews, sadrul, ben+watch_chromium.org, kuscher1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd always on top windows to the alt+tab list
BUG=114631
TEST=Open Google Talk. Try to alt+tab away from it, and then back.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150094
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add test case. #Patch Set 3 : Fix always on top windows for other desktops #Patch Set 4 : Add a test to verify functionality with multiple desktops #
Total comments: 4
Patch Set 5 : Use most recently used windows. #
Total comments: 8
Patch Set 6 : Properly watch window events #Patch Set 7 : Fix comment #Patch Set 8 : More comments. #Patch Set 9 : Rebase #
Total comments: 18
Patch Set 10 : Code review fixes #
Total comments: 6
Patch Set 11 : Code review fixes #Patch Set 12 : Fix clang #
Total comments: 4
Patch Set 13 : Code review fixes #
Messages
Total messages: 42 (0 generated)
Hello sky, Could you review this at: https://chromiumcodereview.appspot.com/10700057 Thanks, -Zach
Hello Daniel, Could you have a look at this? Thanks, -Zach
Please also add a test for this. https://chromiumcodereview.appspot.com/10700057/diff/1/ash/wm/window_cycle_co... File ash/wm/window_cycle_controller.cc (right): https://chromiumcodereview.appspot.com/10700057/diff/1/ash/wm/window_cycle_co... ash/wm/window_cycle_controller.cc:191: windows.insert(active_location, top_windows.begin(), top_windows.end()); Is this putting always-on-top windows from other desktops ahead of non-always-on-top windows from the current desktop? If so, I'm not sure that that's desirable -- I'd expect us to cycle through all the windows on the current desktop first.
Unittest added. How should I filter out/test windows on other desktops? The controller and tests do not seem to be aware of desktops. On 2012/07/11 14:53:41, Daniel Erat wrote: > Please also add a test for this. > > https://chromiumcodereview.appspot.com/10700057/diff/1/ash/wm/window_cycle_co... > File ash/wm/window_cycle_controller.cc (right): > > https://chromiumcodereview.appspot.com/10700057/diff/1/ash/wm/window_cycle_co... > ash/wm/window_cycle_controller.cc:191: windows.insert(active_location, > top_windows.begin(), top_windows.end()); > Is this putting always-on-top windows from other desktops ahead of > non-always-on-top windows from the current desktop? If so, I'm not sure that > that's desirable -- I'd expect us to cycle through all the windows on the > current desktop first.
https://chromiumcodereview.appspot.com/10700057/diff/1/ash/wm/window_cycle_co... File ash/wm/window_cycle_controller.cc (right): https://chromiumcodereview.appspot.com/10700057/diff/1/ash/wm/window_cycle_co... ash/wm/window_cycle_controller.cc:191: windows.insert(active_location, top_windows.begin(), top_windows.end()); On 2012/07/11 14:53:41, Daniel Erat wrote: > Is this putting always-on-top windows from other desktops ahead of > non-always-on-top windows from the current desktop? If so, I'm not sure that > that's desirable -- I'd expect us to cycle through all the windows on the > current desktop first. Sorry, s/desktop/root window/g in this comment. Adding Oshima to see if he has an opinion about this.
https://chromiumcodereview.appspot.com/10700057/diff/1/ash/wm/window_cycle_co... File ash/wm/window_cycle_controller.cc (right): https://chromiumcodereview.appspot.com/10700057/diff/1/ash/wm/window_cycle_co... ash/wm/window_cycle_controller.cc:191: windows.insert(active_location, top_windows.begin(), top_windows.end()); On 2012/07/13 14:07:12, Daniel Erat wrote: > On 2012/07/11 14:53:41, Daniel Erat wrote: > > Is this putting always-on-top windows from other desktops ahead of > > non-always-on-top windows from the current desktop? If so, I'm not sure that > > that's desirable -- I'd expect us to cycle through all the windows on the > > current desktop first. > > Sorry, s/desktop/root window/g in this comment. Adding Oshima to see if he has > an opinion about this. I agree with dan. I think that's what a user would expect.
Fixed to put other desktops lower in the stack. I don't see a good way to unittest with multiple root windows, though. Do you have any suggestions? On 2012/07/13 14:53:30, oshima wrote: > https://chromiumcodereview.appspot.com/10700057/diff/1/ash/wm/window_cycle_co... > File ash/wm/window_cycle_controller.cc (right): > > https://chromiumcodereview.appspot.com/10700057/diff/1/ash/wm/window_cycle_co... > ash/wm/window_cycle_controller.cc:191: windows.insert(active_location, > top_windows.begin(), top_windows.end()); > On 2012/07/13 14:07:12, Daniel Erat wrote: > > On 2012/07/11 14:53:41, Daniel Erat wrote: > > > Is this putting always-on-top windows from other desktops ahead of > > > non-always-on-top windows from the current desktop? If so, I'm not sure > that > > > that's desirable -- I'd expect us to cycle through all the windows on the > > > current desktop first. > > > > Sorry, s/desktop/root window/g in this comment. Adding Oshima to see if he > has > > an opinion about this. > > I agree with dan. I think that's what a user would expect.
On 2012/07/17 02:40:59, Zachary Kuznia wrote: > Fixed to put other desktops lower in the stack. > > I don't see a good way to unittest with multiple root windows, though. Do you > have any suggestions?\ Please see ash/extended_desktop_unittest.cc as an example. > > On 2012/07/13 14:53:30, oshima wrote: > > > https://chromiumcodereview.appspot.com/10700057/diff/1/ash/wm/window_cycle_co... > > File ash/wm/window_cycle_controller.cc (right): > > > > > https://chromiumcodereview.appspot.com/10700057/diff/1/ash/wm/window_cycle_co... > > ash/wm/window_cycle_controller.cc:191: windows.insert(active_location, > > top_windows.begin(), top_windows.end()); > > On 2012/07/13 14:07:12, Daniel Erat wrote: > > > On 2012/07/11 14:53:41, Daniel Erat wrote: > > > > Is this putting always-on-top windows from other desktops ahead of > > > > non-always-on-top windows from the current desktop? If so, I'm not sure > > that > > > > that's desirable -- I'd expect us to cycle through all the windows on the > > > > current desktop first. > > > > > > Sorry, s/desktop/root window/g in this comment. Adding Oshima to see if he > > has > > > an opinion about this. > > > > I agree with dan. I think that's what a user would expect.
Thanks! Added a test. On 2012/07/17 07:24:31, oshima wrote: > On 2012/07/17 02:40:59, Zachary Kuznia wrote: > > Fixed to put other desktops lower in the stack. > > > > I don't see a good way to unittest with multiple root windows, though. Do you > > have any suggestions?\ > > Please see ash/extended_desktop_unittest.cc as an example. > > > > > On 2012/07/13 14:53:30, oshima wrote: > > > > > > https://chromiumcodereview.appspot.com/10700057/diff/1/ash/wm/window_cycle_co... > > > File ash/wm/window_cycle_controller.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/10700057/diff/1/ash/wm/window_cycle_co... > > > ash/wm/window_cycle_controller.cc:191: windows.insert(active_location, > > > top_windows.begin(), top_windows.end()); > > > On 2012/07/13 14:07:12, Daniel Erat wrote: > > > > On 2012/07/11 14:53:41, Daniel Erat wrote: > > > > > Is this putting always-on-top windows from other desktops ahead of > > > > > non-always-on-top windows from the current desktop? If so, I'm not sure > > > that > > > > > that's desirable -- I'd expect us to cycle through all the windows on > the > > > > > current desktop first. > > > > > > > > Sorry, s/desktop/root window/g in this comment. Adding Oshima to see if > he > > > has > > > > an opinion about this. > > > > > > I agree with dan. I think that's what a user would expect.
Just to make sure I understand, this is the order in which the windows are being returned with this change, right? (It'd probably be good to document this in a comment.) - active window, if any - always-on-top windows on current root - other windows on current root in top-to-bottom order - always-on-top windows on first non-active root - other windows on first non-active root in top-to-bottom order - always-on-top windows on second non-active root - etc. Does this mean that it becomes painful to use Alt-Tab to switch back and forth between two non-always-on-top windows? I'm wondering if this should be special-cased, e.g. maybe the order should be more like: - active window - next highest non-always-on-top window (i.e. previously-active window) - always-on-top windows But this breaks using Alt-Tab to switch back and forth between a non-always-on-top window and an always-on-top chat window, which seems like a common use case. Perhaps the per-root activation order should be tracked independently of stacking. I'm worried that doing anything more complicated than pure MRU will be baffling to users.
https://chromiumcodereview.appspot.com/10700057/diff/10002/ash/wm/window_cycl... File ash/wm/window_cycle_controller.cc (right): https://chromiumcodereview.appspot.com/10700057/diff/10002/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:143: aura::Window* always_on_top_container = Shell::GetContainer( Shouldn't always on top be added first? https://chromiumcodereview.appspot.com/10700057/diff/10002/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:144: *iter, internal::kShellWindowId_AlwaysOnTopContainer); Rather than this define an array of the ids of the containers you want to include and loop through that. This should always simplify the code below so that you don't need top_windows.
https://chromiumcodereview.appspot.com/10700057/diff/10002/ash/wm/window_cycl... File ash/wm/window_cycle_controller.cc (right): https://chromiumcodereview.appspot.com/10700057/diff/10002/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:143: aura::Window* always_on_top_container = Shell::GetContainer( It's added last because the container is reversed later. On 2012/07/17 17:43:37, sky wrote: > Shouldn't always on top be added first? https://chromiumcodereview.appspot.com/10700057/diff/10002/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:144: *iter, internal::kShellWindowId_AlwaysOnTopContainer); top_windows is handled differently, because they need to be inserted into the list after the currently active window. On 2012/07/17 17:43:37, sky wrote: > Rather than this define an array of the ids of the containers you want to > include and loop through that. This should always simplify the code below so > that you don't need top_windows.
This is correct. This should probably be refactored, as you suggest. However, I think this CL is a relatively safe solution for merging to the R21 branch. I'd be a bit concerned about merging a major change to window ordering. I'd be happy to file a separate M22 bug to myself to refactor this. On 2012/07/17 17:33:09, Daniel Erat wrote: > Just to make sure I understand, this is the order in which the windows are being > returned with this change, right? (It'd probably be good to document this in a > comment.) > > - active window, if any > - always-on-top windows on current root > - other windows on current root in top-to-bottom order > - always-on-top windows on first non-active root > - other windows on first non-active root in top-to-bottom order > - always-on-top windows on second non-active root > - etc. > > Does this mean that it becomes painful to use Alt-Tab to switch back and forth > between two non-always-on-top windows? I'm wondering if this should be > special-cased, e.g. maybe the order should be more like: > > - active window > - next highest non-always-on-top window (i.e. previously-active window) > - always-on-top windows > > But this breaks using Alt-Tab to switch back and forth between a > non-always-on-top window and an always-on-top chat window, which seems like a > common use case. > > Perhaps the per-root activation order should be tracked independently of > stacking. I'm worried that doing anything more complicated than pure MRU will > be baffling to users.
LGTM, my strong preference is to defer the refactoring to 22, as we need this fixed in 21 quickly
On 2012/07/18 02:01:55, Emmanuel Saint-loubert-Bié wrote: > LGTM, my strong preference is to defer the refactoring to 22, as we need this > fixed in 21 quickly I wasn't proposing refactoring -- I was suggesting reworking the behavior so Alt-Tab's current MRU behavior isn't broken.
The behavior should still be MRU, except when there is an always on top window that would have been unreachable before. Is the actual MRU information currently tracked? It looks to me like we'd have to add plumbing to maintain this state, which is what I worry about merging. On 2012/07/18 03:23:41, Daniel Erat wrote: > On 2012/07/18 02:01:55, Emmanuel Saint-loubert-Bié wrote: > > LGTM, my strong preference is to defer the refactoring to 22, as we need this > > fixed in 21 quickly > > I wasn't proposing refactoring -- I was suggesting reworking the behavior so > Alt-Tab's current MRU behavior isn't broken.
I agree with Dan. We should fix this the right way. -Scott On Tue, Jul 17, 2012 at 8:23 PM, <derat@chromium.org> wrote: > On 2012/07/18 02:01:55, Emmanuel Saint-loubert-Bié wrote: >> >> LGTM, my strong preference is to defer the refactoring to 22, as we need >> this >> fixed in 21 quickly > > > I wasn't proposing refactoring -- I was suggesting reworking the behavior so > Alt-Tab's current MRU behavior isn't broken. > > https://chromiumcodereview.appspot.com/10700057/
Updated to track window activation, and maintain an MRU window list. On 2012/07/18 14:49:22, sky wrote: > I agree with Dan. We should fix this the right way. > > -Scott > > On Tue, Jul 17, 2012 at 8:23 PM, <mailto:derat@chromium.org> wrote: > > On 2012/07/18 02:01:55, Emmanuel Saint-loubert-Bié wrote: > >> > >> LGTM, my strong preference is to defer the refactoring to 22, as we need > >> this > >> fixed in 21 quickly > > > > > > I wasn't proposing refactoring -- I was suggesting reworking the behavior so > > Alt-Tab's current MRU behavior isn't broken. > > > > https://chromiumcodereview.appspot.com/10700057/
High-level question: Do you need to watch for window destruction and remove windows from the list? https://chromiumcodereview.appspot.com/10700057/diff/26001/ash/wm/window_cycl... File ash/wm/window_cycle_controller.h (right): https://chromiumcodereview.appspot.com/10700057/diff/26001/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:90: std::list<aura::Window*> mru_windows_; nit: add a comment describing what this is (e.g. ordering) https://chromiumcodereview.appspot.com/10700057/diff/26001/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:92: bool mru_ignore_; nit: add a comment describing what this is
This class should become a WindowObserver for the containers it cares about. That will allow it to track the set of windows it cares about. OnWindowActivated will then shuffle the items in the list around. https://chromiumcodereview.appspot.com/10700057/diff/26001/ash/wm/window_cycl... File ash/wm/window_cycle_controller.cc (right): https://chromiumcodereview.appspot.com/10700057/diff/26001/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:226: if (active) { Only add windows that are children of the containers this class cares about. https://chromiumcodereview.appspot.com/10700057/diff/26001/ash/wm/window_cycl... File ash/wm/window_cycle_controller.h (right): https://chromiumcodereview.appspot.com/10700057/diff/26001/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:27: // may change during the gesture. Thus we maintain the state of the windows Update description to reflect changes.
I've updated the change to only monitor the container that we care about, and to ensure that windows are removed from the mru_windows_ list when they are destroyed. https://chromiumcodereview.appspot.com/10700057/diff/26001/ash/wm/window_cycl... File ash/wm/window_cycle_controller.cc (right): https://chromiumcodereview.appspot.com/10700057/diff/26001/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:226: if (active) { On 2012/07/19 16:41:15, sky wrote: > Only add windows that are children of the containers this class cares about. Done. https://chromiumcodereview.appspot.com/10700057/diff/26001/ash/wm/window_cycl... File ash/wm/window_cycle_controller.h (right): https://chromiumcodereview.appspot.com/10700057/diff/26001/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:27: // may change during the gesture. Thus we maintain the state of the windows On 2012/07/19 16:41:15, sky wrote: > Update description to reflect changes. Done. https://chromiumcodereview.appspot.com/10700057/diff/26001/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:90: std::list<aura::Window*> mru_windows_; On 2012/07/19 15:14:17, Daniel Erat wrote: > nit: add a comment describing what this is (e.g. ordering) Done. https://chromiumcodereview.appspot.com/10700057/diff/26001/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:92: bool mru_ignore_; On 2012/07/19 15:14:17, Daniel Erat wrote: > nit: add a comment describing what this is Done.
https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... File ash/wm/window_cycle_controller.cc (right): https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:98: StopCycling(); Doesn't this need to remove observers from all the root window containers? https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:119: StartCycling(); Wouldn't it make more sense to set mru_ignore_ in Start/StopCycling and do any processing there? https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:147: std::list<aura::Window*>* mru_windows) { const std::list& https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:148: remove newline https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:179: // Put the windows in the mru_windows list at the head, if it's available. Document why this uses a reverse iterator. https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:237: if (!active->parent()) This code would be easier to read if you made a function something like: IsTrackedContainer(aura::Window* window); that had this if and the loop from 241-246. That way the code becomes: if (active && !mru_ignore_ && IsTrackedContainer(window->parent()) https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:250: if (!mru_ignore_) { Move this check early on. https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... File ash/wm/window_cycle_controller.h (right): https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:107: // Indicates that we should not update the mru_windows_ list when windows are |mru_windows_| https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:109: bool mru_ignore_; I think this should be is cycling. Do you even need the boolean though? Can you kep off whether windows_ is non-NULL?
https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... File ash/wm/window_cycle_controller.cc (right): https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:98: StopCycling(); No. The containers get the OnWindowDestroying notification before this destructor. On 2012/07/24 03:53:31, sky wrote: > Doesn't this need to remove observers from all the root window containers? https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:119: StartCycling(); On 2012/07/24 03:53:31, sky wrote: > Wouldn't it make more sense to set mru_ignore_ in Start/StopCycling and do any > processing there? Done. https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:147: std::list<aura::Window*>* mru_windows) { Changed to const. It needs to be a pointer, though, so that NULL can be passed in from other callsites that don't need it to be MRU sorted. On 2012/07/24 03:53:31, sky wrote: > const std::list& https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:148: On 2012/07/24 03:53:31, sky wrote: > remove newline Done. https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:179: // Put the windows in the mru_windows list at the head, if it's available. On 2012/07/24 03:53:31, sky wrote: > Document why this uses a reverse iterator. Done. https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:237: if (!active->parent()) On 2012/07/24 03:53:31, sky wrote: > This code would be easier to read if you made a function something like: > > IsTrackedContainer(aura::Window* window); > that had this if and the loop from 241-246. That way the code becomes: > > if (active && !mru_ignore_ && IsTrackedContainer(window->parent()) Done. https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:250: if (!mru_ignore_) { On 2012/07/24 03:53:31, sky wrote: > Move this check early on. Done. https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... File ash/wm/window_cycle_controller.h (right): https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:107: // Indicates that we should not update the mru_windows_ list when windows are Removed. On 2012/07/24 03:53:31, sky wrote: > |mru_windows_| https://chromiumcodereview.appspot.com/10700057/diff/31003/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:109: bool mru_ignore_; On 2012/07/24 03:53:31, sky wrote: > I think this should be is cycling. Do you even need the boolean though? Can you > kep off whether windows_ is non-NULL? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zork@chromium.org/10700057/36001
Presubmit check for 10700057-36001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/ui
ash
chrome/browser/ui/views/ash
Presubmit checks took 1.4s to calculate.
Sorry, that was a mis-click that hit the commit queue button. :-(
https://chromiumcodereview.appspot.com/10700057/diff/36001/ash/wm/window_cycl... File ash/wm/window_cycle_controller.h (right): https://chromiumcodereview.appspot.com/10700057/diff/36001/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:64: // Setup the observers to handle window changes for the containers we care nit: s/Setup/Sets up/ https://chromiumcodereview.appspot.com/10700057/diff/36001/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:90: // Check if the window represents a container whose children we track. nit:s/Check/Checks/ https://chromiumcodereview.appspot.com/10700057/diff/36001/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:91: static bool IsTrackedWindow(aura::Window* window); nit: IsTrackedContainer()?
https://chromiumcodereview.appspot.com/10700057/diff/36001/ash/wm/window_cycl... File ash/wm/window_cycle_controller.h (right): https://chromiumcodereview.appspot.com/10700057/diff/36001/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:64: // Setup the observers to handle window changes for the containers we care On 2012/07/30 15:32:52, Daniel Erat wrote: > nit: s/Setup/Sets up/ Done. https://chromiumcodereview.appspot.com/10700057/diff/36001/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:90: // Check if the window represents a container whose children we track. On 2012/07/30 15:32:52, Daniel Erat wrote: > nit:s/Check/Checks/ Done. https://chromiumcodereview.appspot.com/10700057/diff/36001/ash/wm/window_cycl... ash/wm/window_cycle_controller.h:91: static bool IsTrackedWindow(aura::Window* window); On 2012/07/30 15:32:52, Daniel Erat wrote: > nit: IsTrackedContainer()? Done.
(I'll defer to Scott for the final looks-good since he's been reviewing this more closely than I have.)
sky: Ping? On 2012/07/31 21:47:47, Daniel Erat wrote: > (I'll defer to Scott for the final looks-good since he's been reviewing this > more closely than I have.)
LGTM https://chromiumcodereview.appspot.com/10700057/diff/48001/ash/shell.h File ash/shell.h (right): https://chromiumcodereview.appspot.com/10700057/diff/48001/ash/shell.h#newcod... ash/shell.h:256: internal::ActivationController* activation_controller() { We shouldn't expose classes in the internal namespace here. It means anyone can get a handle to them. https://chromiumcodereview.appspot.com/10700057/diff/48001/ash/wm/window_cycl... File ash/wm/window_cycle_controller.cc (left): https://chromiumcodereview.appspot.com/10700057/diff/48001/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:142: // Add windows in the active root windows last so that the topmost window Move this comment to 164ish new.
https://chromiumcodereview.appspot.com/10700057/diff/48001/ash/shell.h File ash/shell.h (right): https://chromiumcodereview.appspot.com/10700057/diff/48001/ash/shell.h#newcod... ash/shell.h:256: internal::ActivationController* activation_controller() { On 2012/08/02 21:18:00, sky wrote: > We shouldn't expose classes in the internal namespace here. It means anyone can > get a handle to them. Done. https://chromiumcodereview.appspot.com/10700057/diff/48001/ash/wm/window_cycl... File ash/wm/window_cycle_controller.cc (left): https://chromiumcodereview.appspot.com/10700057/diff/48001/ash/wm/window_cycl... ash/wm/window_cycle_controller.cc:142: // Add windows in the active root windows last so that the topmost window On 2012/08/02 21:18:00, sky wrote: > Move this comment to 164ish new. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zork@chromium.org/10700057/42003
Try job failure for 10700057-42003 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zork@chromium.org/10700057/42003
Try job failure for 10700057-42003 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zork@chromium.org/10700057/42003
Try job failure for 10700057-42003 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zork@chromium.org/10700057/42003
Try job failure for 10700057-42003 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zork@chromium.org/10700057/42003
Change committed as 150094 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
