Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
420603011:
Split Screen mode implementation (Closed)
|
Created:
6 years, 5 months ago by mfomitchev Modified:
6 years, 4 months ago CC:
chromium-reviews, Mr4D (OOO till 08-26) Base URL:
https://chromium.googlesource.com/chromium/src.git@split_view Project:
chromium Visibility:
Public. |
DescriptionSplit Screen mode implementation.
Implements Split Screen mode and window cycling behavior.
BUG=383421
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288593
Patch Set 1 #
Total comments: 40
Patch Set 2 : Addressing some of the review feedback. #Patch Set 3 : Rebase #Patch Set 4 : Fixing include #Patch Set 5 : FIxing the crash - removing const & #Patch Set 6 : Fix accidental check-in #Patch Set 7 : Re-submitting fix for the crash. #Patch Set 8 : Implementing review feedback. #Patch Set 9 : Fixed upstream branch #
Total comments: 14
Patch Set 10 : Addressing mukai's review feedback. #
Total comments: 38
Patch Set 11 : Addressing review feedback + Rebase. #
Total comments: 14
Patch Set 12 : Addressing review feedback. #
Total comments: 8
Patch Set 13 : Fixing nits + Rebase #Patch Set 14 : Fixing a rebase glitch #
Total comments: 16
Patch Set 15 : Addressing sadrul's feedback, adding license header. #
Messages
Total messages: 42 (0 generated)
mukai@chromium.org: Please review changes in oshima@chromium.org: Please review changes in
The implementation is functional. I just need to clean the code up a bit (remove logging, add some comments, etc). I will submit another patch cleaning this up later today, but wanted to get the review out sooner rather than later to increase the chances of this making it in in time for the demo on Tuesday.
https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.cc File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:43: gfx::Point point_in_screen(location.x(), location.y()); point_in_screen(location) would copy it. https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:47: : point_in_screen.x() - container_->GetBoundsInScreen().width(); Not the thing in this CL, but should this be GetBoundsInScreen().right() instead? And the previous line should be x() - GetBoundsInScreen().x()? The container is spanning fullscreen so this makes no difference for now, but I think that is more strict. https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:61: } remove unnecessary indents https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:121: SetState(BEZEL_GESTURE_STARTED, kScrollDeltaNone); I don't like using None all around. Can we make a struct for the state, which has the enum and the distance. Then declare two struct constructors: just receiving enum (distance is None), and receiving enum and distance. https://codereview.chromium.org/420603011/diff/1/athena/wm/coordinate_convers... File athena/wm/coordinate_conversion.h (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/coordinate_convers... athena/wm/coordinate_conversion.h:20: void ConvertPointToScreen(const aura::Window* window, This file would be great to be in ui/wm/core (or even a method of aura::Window?) https://codereview.chromium.org/420603011/diff/1/athena/wm/public/window_mana... File athena/wm/public/window_manager.h (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/public/window_mana... athena/wm/public/window_manager.h:11: class Window; remove unused declarations. https://codereview.chromium.org/420603011/diff/1/athena/wm/split_view_control... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/split_view_control... athena/wm/split_view_controller.h:20: class LayerAnimationSequence; Remove unused declaration https://codereview.chromium.org/420603011/diff/1/athena/wm/split_view_control... athena/wm/split_view_controller.h:63: virtual void OnOverviewModeExit() OVERRIDE {} Please move the implementation to .cc file even it is empty https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:37: if (overview_) { Keep in mind that pkotwicz is changing the code around |overview_|, see crbug.com/396368 for its progress (his first patch was reverted though...) https://codereview.chromium.org/420603011/diff/1/athena/wm/window_overview_mo... File athena/wm/window_overview_mode.h (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/window_overview_mo... athena/wm/window_overview_mode.h:26: aura::Window::Windows windows, const &
https://codereview.chromium.org/420603011/diff/1/athena/wm/coordinate_convers... File athena/wm/coordinate_conversion.h (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/coordinate_convers... athena/wm/coordinate_conversion.h:20: void ConvertPointToScreen(const aura::Window* window, On 2014/07/25 20:21:29, Jun Mukai wrote: > This file would be great to be in ui/wm/core (or even a method of aura::Window?) yep, and consolidate methods (that make sense) ash/wm/coordinate_conversion.h https://codereview.chromium.org/420603011/diff/1/athena/wm/split_view_control... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/split_view_control... athena/wm/split_view_controller.cc:34: base::MessageLoop::current()->DeleteSoon(FROM_HERE, this); can't you remove immediately? https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:133: aura::Window::Windows ordered_windows_; what's the reason to keep its own list?
https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.cc File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:43: gfx::Point point_in_screen(location.x(), location.y()); Doesn't look like I can pass PointF to a Point constructor... https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:47: : point_in_screen.x() - container_->GetBoundsInScreen().width(); Since this is a bezel gesture recognizer, I think point_in_screen.x() is right, but instead of container_->GetBoundsInScreen().width() we want to get the screen width is not taking up the entire screen. Does this make sense? And if so - what's a good way to get the screen width? https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:61: } On 2014/07/25 20:21:29, Jun Mukai wrote: > remove unnecessary indents Done. https://codereview.chromium.org/420603011/diff/1/athena/wm/public/window_mana... File athena/wm/public/window_manager.h (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/public/window_mana... athena/wm/public/window_manager.h:11: class Window; On 2014/07/25 20:21:29, Jun Mukai wrote: > remove unused declarations. Done. https://codereview.chromium.org/420603011/diff/1/athena/wm/split_view_control... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/split_view_control... athena/wm/split_view_controller.h:20: class LayerAnimationSequence; On 2014/07/25 20:21:29, Jun Mukai wrote: > Remove unused declaration Done. https://codereview.chromium.org/420603011/diff/1/athena/wm/split_view_control... athena/wm/split_view_controller.h:63: virtual void OnOverviewModeExit() OVERRIDE {} On 2014/07/25 20:21:29, Jun Mukai wrote: > Please move the implementation to .cc file even it is empty Done. https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:133: aura::Window::Windows ordered_windows_; The order within the container changes when you interact with the UI. When a window becomes the target of a gesture, it is stacked on top of all the other windows. https://codereview.chromium.org/420603011/diff/1/athena/wm/window_overview_mo... File athena/wm/window_overview_mode.h (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/window_overview_mo... athena/wm/window_overview_mode.h:26: aura::Window::Windows windows, On 2014/07/25 20:21:30, Jun Mukai wrote: > const & Done.
https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.cc File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:43: gfx::Point point_in_screen(location.x(), location.y()); On 2014/07/25 23:27:38, mfomitchev wrote: > Doesn't look like I can pass PointF to a Point constructor... oops, missed your point. Well, in that case, consider using a function in ui/gfx/geometry/pointer_conversions.h https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:47: : point_in_screen.x() - container_->GetBoundsInScreen().width(); On 2014/07/25 23:27:38, mfomitchev wrote: > Since this is a bezel gesture recognizer, I think point_in_screen.x() is right, > but instead of container_->GetBoundsInScreen().width() we want to get the screen > width is not taking up the entire screen. Does this make sense? And if so - > what's a good way to get the screen width? I don't object to point_in_screen.x(). I'm saying to use right() instead of width(), and use x() instead of doing nothing, about container_->GetBoundsInScreen(). That addresses the case that the container's rectangle (i.e. user's work area) is different from the display/screen's bounds. Although right now I don't hear any plan of doing so, it might happen eventually. For example, ash work area originally used the entire screen width, but at some point we introduced side shelf and docked window which shrinks work area's width, and suffered from weird corner cases caused by implicit assumptions of 'work area width must match to the screen width'. It's better to address this kind of thing earlier.
https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.cc File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:43: gfx::Point point_in_screen(location.x(), location.y()); Cool, will do. https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:47: : point_in_screen.x() - container_->GetBoundsInScreen().width(); Yes, I understand. My point is that this is a bezel controller, which is supposed to recognize bezel gestures. Bezel gestures by definition are supposed to start from the edge of the screen, so I think it makes sense to report the distance from the edge of the screen in the callbacks as well. Now, the current implementation is still wrong: the first clause (point_in_screen.x()) is okay, but the second clause (point_in_screen.x() - container_->GetBoundsInScreen().width()) should be changed to (point_in_screen.x() - <screen width>). If we want to account for the possibility that the main container is not full screen, I think we should do it in SplitViewController: we should take container_->x() and container_->right() into account when calculating the windows positions. I can make that change.
https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.cc File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:47: : point_in_screen.x() - container_->GetBoundsInScreen().width(); On 2014/07/26 02:08:43, mfomitchev wrote: > Yes, I understand. My point is that this is a bezel controller, which is > supposed to recognize bezel gestures. Bezel gestures by definition are supposed > to start from the edge of the screen, so I think it makes sense to report the > distance from the edge of the screen in the callbacks as well. Now, the current > implementation is still wrong: the first clause (point_in_screen.x()) is okay, > but the second clause (point_in_screen.x() - > container_->GetBoundsInScreen().width()) should be changed to > (point_in_screen.x() - <screen width>). > > If we want to account for the possibility that the main container is not full > screen, I think we should do it in SplitViewController: we should take > container_->x() and container_->right() into account when calculating the > windows positions. I can make that change. +1 to use screen_width.
https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... File athena/wm/window_manager_impl.cc (left): https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:71: container_->StackChildAtTop(window); This should have called wm::ActivateWindow instead. https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:133: aura::Window::Windows ordered_windows_; On 2014/07/25 23:27:38, mfomitchev wrote: > The order within the container changes when you interact with the UI. When a > window becomes the target of a gesture, it is stacked on top of all the other > windows. StackChildAtTop does change the order within the container. My question was why you need to keep separate list. I think we eventually need MRU like ash has, but it should live in resource management rather than here.
https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:133: aura::Window::Windows ordered_windows_; I can't rely on the container's window order for the purposes of window-cycling (alt-tab like behavior). The container's window order changes as the user interacts with the UI - e.g. when a window becomes the target of a gesture, it is stacked on top of all the other windows. So what happens is that the window which is shown always ends up on top of all the other windows. But in order to implement the cycling, we need the window order to be static - otherwise it's hard to figure out what the next/prev window is.
https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:133: aura::Window::Windows ordered_windows_; On 2014/07/28 18:15:00, mfomitchev wrote: > I can't rely on the container's window order for the purposes of window-cycling > (alt-tab like behavior). The container's window order changes as the user > interacts with the UI - e.g. when a window becomes the target of a gesture, it > is stacked on top of all the other windows. So what happens is that the window > which is shown always ends up on top of all the other windows. But in order to > implement the cycling, we need the window order to be static - otherwise it's > hard to figure out what the next/prev window is. I know that container's window order isn't enough to implement alt-tab, and ash has ash/wm/mru_window_tracker for it. What I'm saying is that\ if you're going to implement alt-tab (are you going to add it in this CL?), then it should be handled by separate entity like ash's mru tracker and use activation observer, not here.
https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:133: aura::Window::Windows ordered_windows_; SplitViewController implements window switching - you can swipe left/right to switch to the next/prev window. You can continue swiping to cycle through all windows. It's similar to alt-tab in that it needs to know the window order. I can factor the functionality out of window manager and into a separate class. athena::MruWindowTracker? Note that we can't update MRU order every time we switch to the previous window by swiping: if we do that, you'll keep cycling between 2 windows: the most recent and second most recent. Each time you swipe, the second most recent becomes the most recent, and the most recent becomes trhe second most recent. So currently the order is only updated when you switch to the overview mode. That is probably not enough - we'll likely also want to update the order once you've spent some time in the new window after swiping to it.. or something along those lines.
On 2014/07/28 18:57:42, mfomitchev wrote: > https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... > File athena/wm/window_manager_impl.cc (right): > > https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... > athena/wm/window_manager_impl.cc:133: aura::Window::Windows ordered_windows_; > SplitViewController implements window switching - you can swipe left/right to > switch to the next/prev window. You can continue swiping to cycle through all > windows. It's similar to alt-tab in that it needs to know the window order. > > I can factor the functionality out of window manager and into a separate class. > athena::MruWindowTracker? > > Note that we can't update MRU order every time we switch to the previous window > by swiping: if we do that, you'll keep cycling between 2 windows: the most > recent and second most recent. Each time you swipe, the second most recent > becomes the most recent, and the most recent becomes trhe second most recent. So > currently the order is only updated when you switch to the overview mode. That > is probably not enough - we'll likely also want to update the order once you've > spent some time in the new window after swiping to it.. or something along those > lines. Yes, it'll need logic that involves other features/scenarios. The swip has direction, so it's not possible to rely on activation for it. Another tricky thing is that alt-tab still should go back to previous one even after swipe. We probably should treat swipe/alt-tab as an events and let this class do the right thing for each event types. In any case, it's not simple and it's better to keep it separate class and has its own unit tests. Looking into ash's window tracker is a good idea. Note that you don't have to use it if you think that's not the right approach though.
https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.cc File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:43: gfx::Point point_in_screen(location.x(), location.y()); On 2014/07/25 23:44:23, Jun Mukai wrote: > On 2014/07/25 23:27:38, mfomitchev wrote: > > Doesn't look like I can pass PointF to a Point constructor... > > oops, missed your point. Well, in that case, consider using a function in > ui/gfx/geometry/pointer_conversions.h Done. https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:47: : point_in_screen.x() - container_->GetBoundsInScreen().width(); On 2014/07/26 20:30:37, Jun Mukai wrote: > On 2014/07/26 02:08:43, mfomitchev wrote: > > Yes, I understand. My point is that this is a bezel controller, which is > > supposed to recognize bezel gestures. Bezel gestures by definition are > supposed > > to start from the edge of the screen, so I think it makes sense to report the > > distance from the edge of the screen in the callbacks as well. Now, the > current > > implementation is still wrong: the first clause (point_in_screen.x()) is okay, > > but the second clause (point_in_screen.x() - > > container_->GetBoundsInScreen().width()) should be changed to > > (point_in_screen.x() - <screen width>). > > > > If we want to account for the possibility that the main container is not full > > screen, I think we should do it in SplitViewController: we should take > > container_->x() and container_->right() into account when calculating the > > windows positions. I can make that change. > > +1 to use screen_width. Done. https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:121: SetState(BEZEL_GESTURE_STARTED, kScrollDeltaNone); I've just created an overloaded SetState method that takes one argument and added a DCHECK there - this seems simpler and requires less changes to the header. Let me know if you are ok with this. https://codereview.chromium.org/420603011/diff/1/athena/wm/coordinate_convers... File athena/wm/coordinate_conversion.h (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/coordinate_convers... athena/wm/coordinate_conversion.h:20: void ConvertPointToScreen(const aura::Window* window, On 2014/07/25 21:41:10, oshima wrote: > On 2014/07/25 20:21:29, Jun Mukai wrote: > > This file would be great to be in ui/wm/core (or even a method of > aura::Window?) > > yep, and consolidate methods (that make sense) ash/wm/coordinate_conversion.h Done. Created a separate review for ash changes: https://codereview.chromium.org/425363002 https://codereview.chromium.org/420603011/diff/1/athena/wm/split_view_control... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/split_view_control... athena/wm/split_view_controller.cc:34: base::MessageLoop::current()->DeleteSoon(FROM_HERE, this); Yup, looks like I can. I think this used to cause a problem with 0-duration animations, but it doesn't anymore. Done. https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... File athena/wm/window_manager_impl.cc (left): https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:71: container_->StackChildAtTop(window); On 2014/07/28 17:58:43, oshima wrote: > This should have called wm::ActivateWindow instead. Done. https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:133: aura::Window::Windows ordered_windows_; I have implemented a very basic, stripped down MruWindowTracker. Currently it's not even tracking the window activations - only creations and deletions. It's quite possible that it will need to track activations as well in the future, but it's not clear until we have a defined UX. https://codereview.chromium.org/420603011/diff/1/athena/wm/window_overview_mo... File athena/wm/window_overview_mode.h (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/window_overview_mo... athena/wm/window_overview_mode.h:26: aura::Window::Windows windows, This is now obtained from MruWindowTracker which creates the vector of Windows on the stack, so I removed const& again.
https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.cc File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/420603011/diff/1/athena/wm/bezel_controller.c... athena/wm/bezel_controller.cc:121: SetState(BEZEL_GESTURE_STARTED, kScrollDeltaNone); On 2014/08/05 19:56:57, mfomitchev wrote: > I've just created an overloaded SetState method that takes one argument and > added a DCHECK there - this seems simpler and requires less changes to the > header. Let me know if you are ok with this. SGTM https://codereview.chromium.org/420603011/diff/160001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/420603011/diff/160001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:46: float BezelController::GetDistance(const gfx::PointF& location, Now GetDistance doesn't refer to any fields of this object. Let's make this as a static method. https://codereview.chromium.org/420603011/diff/160001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:51: gfx::Point point_in_screen(gfx::ToRoundedPoint(location)); RoundedPoint is slightly different from your previous behavior (originally it seems ToFlooredPoint). Is it intended? https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_list_p... File athena/wm/window_list_provider.h (right): https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_list_p... athena/wm/window_list_provider.h:8: // Interface for an ordered list of aura::Window objects. This would be good to share the interface among ash eventually. Can you move this to ui/wm/core? https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:196: shadow->SetContentBounds(gfx::Rect(container_->bounds().size())); Why this needs to be changed to container_? Is there some case where window != container_? also, if they should be same, it's a good manner to add DCHECK_EQ(window, container_); https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:297: aura::Window::Windows windows_; Can we set this as const? I think the order of |windows_| should not change in the overview mode itself (all it can do is invoking MoveToFront).
https://codereview.chromium.org/420603011/diff/160001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/420603011/diff/160001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:46: float BezelController::GetDistance(const gfx::PointF& location, It's private, so I moved it to the anonymous namespace. Makes sense? https://codereview.chromium.org/420603011/diff/160001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:51: gfx::Point point_in_screen(gfx::ToRoundedPoint(location)); Yes, I think ToRoundedPoint is better - we want the most accurate representation of the location. ToFlooredPoint will be less accurate, and it would also effectively round the distance down for the left bezel while rounding it up for the right bezel, which doesn't make sense for our purposes. https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_list_p... File athena/wm/window_list_provider.h (right): https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_list_p... athena/wm/window_list_provider.h:8: // Interface for an ordered list of aura::Window objects. On 2014/08/05 21:13:38, Jun Mukai wrote: > This would be good to share the interface among ash eventually. > Can you move this to ui/wm/core? Done. https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:196: shadow->SetContentBounds(gfx::Rect(container_->bounds().size())); conainer_ is window's parent. The reason this was changed is that the window can be transformed by the split view mode, so if we use window's bounds here, the shadow will be set on half the window's real bounds. https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:297: aura::Window::Windows windows_; On 2014/08/05 21:13:38, Jun Mukai wrote: > Can we set this as const? I think the order of |windows_| should not change in > the overview mode itself (all it can do is invoking MoveToFront). Done.
https://codereview.chromium.org/420603011/diff/160001/athena/wm/bezel_control... File athena/wm/bezel_controller.cc (right): https://codereview.chromium.org/420603011/diff/160001/athena/wm/bezel_control... athena/wm/bezel_controller.cc:46: float BezelController::GetDistance(const gfx::PointF& location, On 2014/08/06 18:11:32, mfomitchev wrote: > It's private, so I moved it to the anonymous namespace. Makes sense? +1 https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:196: shadow->SetContentBounds(gfx::Rect(container_->bounds().size())); On 2014/08/06 18:11:32, mfomitchev wrote: > conainer_ is window's parent. The reason this was changed is that the window can > be transformed by the split view mode, so if we use window's bounds here, the > shadow will be set on half the window's real bounds. I got confused. Here's my understanding: - container_ is the default container, whose size is same as the whole desktop - the window in overview mode shrunk a bit, to provide the stacking card UI - this is the shadow for the stacking cards So making content bounds for container_ doesn't make sense. It just doesn't appear. Am I wrong?
https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:196: shadow->SetContentBounds(gfx::Rect(container_->bounds().size())); It's the split view transform, not the overview mode transform that is the cause for this change. I believe this method is called before any overview mode transforms are applied (and when they are applied - they are applied to both the window and the shadow), so we don't worry about overview transforms. This method is implicitly assuming that all windows are full screen before we enter overview mode, which is incorrect now that windows can be transformed by split view mode. The fix is to use the container (which is always full screen) for bounds calculations instead.
lgtm but please wait for the review from oshima https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/420603011/diff/160001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:196: shadow->SetContentBounds(gfx::Rect(container_->bounds().size())); On 2014/08/06 20:09:59, mfomitchev wrote: > It's the split view transform, not the overview mode transform that is the cause > for this change. I believe this method is called before any overview mode > transforms are applied (and when they are applied - they are applied to both the > window and the shadow), so we don't worry about overview transforms. This method > is implicitly assuming that all windows are full screen before we enter overview > mode, which is incorrect now that windows can be transformed by split view mode. > The fix is to use the container (which is always full screen) for bounds > calculations instead. Okay https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.h:13: #include "ui/compositor/layer_animation_observer.h" Is this include necessary here?
https://codereview.chromium.org/420603011/diff/180001/athena/wm/bezel_control... File athena/wm/bezel_controller.h (right): https://codereview.chromium.org/420603011/diff/180001/athena/wm/bezel_control... athena/wm/bezel_controller.h:37: // Beginning of a bezel scroll gesture started from the |bezel|. can you document |delta| as well? https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:24: CallbackAnimationObserver(const base::Closure& closure) explicit https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:73: int container_width = container_->GetBoundsInScreen().width(); This should be work area. (there is a small area at the bottom that are occupied by bottom home card) https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:76: right_transform.Scale(.5, 1); doesn't this make window stretched vertically? https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:187: // Loop through all windows and hide them add me on TODO and crbug.com/388362 https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:192: // END HACK remove HACK/END HACK (as it's obvious) https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:206: if (abs(cont_width / 2 - separator_position_) <= kMaxDistanceFromMiddle) { std::abs include <cmath> https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.h:32: virtual ~SplitViewController(); move dtor after ctor https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.h:41: enum State {INACTIVE, SCROLLING, ACTIVE}; document these states. https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.h:48: void AnimationCompleted(); OnAnimationCompleted ( to be consistent with patterns used by other code) https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.h:52: aura::Window* GetCurrentWindow(); document what is *current* window. If it's active window, then you should use wm::GetActiveWindow() https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.h:60: // WindowManagerObserver overrides // WindowManagerObserver: (no "overrides" required now) https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.h:75: // Position of the separator between left_window_ and right_window_ in new line before comment https://codereview.chromium.org/420603011/diff/180001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/420603011/diff/180001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:95: aura::Window::Windows windows, WindowOverviewModeI should use the WindowListProvider too, shouldn't it? https://codereview.chromium.org/420603011/diff/180001/ui/wm/core/window_list_... File ui/wm/core/window_list_provider.h (right): https://codereview.chromium.org/420603011/diff/180001/ui/wm/core/window_list_... ui/wm/core/window_list_provider.h:9: class WindowListProvider { move this to athena/wm/public. The plan is that we'll have a class that manges manages the list of activities (which may not have an associated aura window). stefan will rename and move this later, but a/wm/public is fine for now. Please add GetInstance and keep it separated from WindowManagerImpl.
If I understand correctly, we can do without the MRU tracker for now? (since edge swipe will only switch between the last two active windows, instead of going farther back?) https://codereview.chromium.org/420603011/diff/180001/ui/wm/core/window_list_... File ui/wm/core/window_list_provider.h (right): https://codereview.chromium.org/420603011/diff/180001/ui/wm/core/window_list_... ui/wm/core/window_list_provider.h:9: class WindowListProvider { On 2014/08/06 21:54:10, oshima wrote: > move this to athena/wm/public. > > The plan is that we'll have a class that manges manages the list of activities > (which may not have an associated aura window). stefan will rename and move this > later, but a/wm/public is fine for now. > > Please add GetInstance and keep it separated from WindowManagerImpl. > ActivityManager would be a good place for this. Its responsibility is to keep track of the activities, and should be able to give us the list in the right order.
On 2014/08/07 15:26:16, sadrul wrote: > If I understand correctly, we can do without the MRU tracker for now? (since > edge swipe will only switch between the last two active windows, instead of > going farther back?) I'm fine with simpler implementation. It'll be temporary anyway as this should be controlled by the resource management that stefan is working on. > > https://codereview.chromium.org/420603011/diff/180001/ui/wm/core/window_list_... > File ui/wm/core/window_list_provider.h (right): > > https://codereview.chromium.org/420603011/diff/180001/ui/wm/core/window_list_... > ui/wm/core/window_list_provider.h:9: class WindowListProvider { > On 2014/08/06 21:54:10, oshima wrote: > > move this to athena/wm/public. > > > > The plan is that we'll have a class that manges manages the list of activities > > (which may not have an associated aura window). stefan will rename and move > this > > later, but a/wm/public is fine for now. > > > > Please add GetInstance and keep it separated from WindowManagerImpl. > > > > ActivityManager would be a good place for this. Its responsibility is to keep > track of the activities, and should be able to give us the list in the right > order. Something like that although we probably will have a separate class/object that manages them (stefan likes ActivityNavigator). Another thing is that we don't want to introduce the circular dependency, so may be the structure would be activity/ depends on wm/ WindowListProvider (maybe renamed) will be in wm/public ActivityNavigator (in activity) implements WindowListProvider and inject to wm/
I think it's worth keeping MRU tracker around since it's possible the UX will change or that we'll have some other entities influencing MRU in the future. It seems like a good idea to have window order manipulation go through WindowListProvider/MruWindowTracker so that it's all consolidated in one place.
On 2014/08/07 17:11:10, mfomitchev wrote: > I think it's worth keeping MRU tracker around since it's possible the UX will > change or that we'll have some other entities influencing MRU in the future. It > seems like a good idea to have window order manipulation go through > WindowListProvider/MruWindowTracker so that it's all consolidated in one place. What I meant was that I'm fine either to have MruWindowTracker : public WLP, or SimpleMLP : public WLP. I agree that it's better to have this in one place.
https://codereview.chromium.org/420603011/diff/180001/athena/wm/bezel_control... File athena/wm/bezel_controller.h (right): https://codereview.chromium.org/420603011/diff/180001/athena/wm/bezel_control... athena/wm/bezel_controller.h:37: // Beginning of a bezel scroll gesture started from the |bezel|. On 2014/08/06 21:54:08, oshima wrote: > can you document |delta| as well? Done. https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:24: CallbackAnimationObserver(const base::Closure& closure) On 2014/08/06 21:54:09, oshima wrote: > explicit Done. https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:73: int container_width = container_->GetBoundsInScreen().width(); Wouldn't the activities' container already set to the right bounds based on this? When I look at HomeCardLayoutManager::Layout, it is simply using container bounds.. Also, here we only care about the width, which shouldn't be impacted by home card. https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:76: right_transform.Scale(.5, 1); Yes, but that will be fixed in OnAnimationCOmpleted. I've added some comments and CHECKs to clarify. This code will likely change quite a bit when I implement the separator between the two windows and allow it to be dragged around starting from the ACTIVE state when the windows are half screen. So most of this is temporary. https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:187: // Loop through all windows and hide them On 2014/08/06 21:54:09, oshima wrote: > add me on TODO and crbug.com/388362 Done. https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:192: // END HACK On 2014/08/06 21:54:09, oshima wrote: > remove HACK/END HACK (as it's obvious) Done. https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:206: if (abs(cont_width / 2 - separator_position_) <= kMaxDistanceFromMiddle) { On 2014/08/06 21:54:09, oshima wrote: > std::abs > > include <cmath> Done. https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.h:13: #include "ui/compositor/layer_animation_observer.h" Leftover from old code, thanks for catching. Fixed. https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.h:32: virtual ~SplitViewController(); On 2014/08/06 21:54:09, oshima wrote: > move dtor after ctor Done. https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.h:41: enum State {INACTIVE, SCROLLING, ACTIVE}; On 2014/08/06 21:54:09, oshima wrote: > document these states. Done. https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.h:48: void AnimationCompleted(); On 2014/08/06 21:54:10, oshima wrote: > OnAnimationCompleted ( to be consistent with patterns used by other code) Done. https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.h:52: aura::Window* GetCurrentWindow(); I added comments. Unfortunately GetActiveWindow() may return a window which is not an activity window. E.g. when you swipe up, it would return the home card window. (Also, wm::GetActiveWIndow() is in ash, but we can use aura::client::GetActivationClient(container_->GetRootWindow()) instead). https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.h:60: // WindowManagerObserver overrides On 2014/08/06 21:54:09, oshima wrote: > // WindowManagerObserver: > > (no "overrides" required now) Done. https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.h:75: // Position of the separator between left_window_ and right_window_ in On 2014/08/06 21:54:09, oshima wrote: > new line before comment Done. https://codereview.chromium.org/420603011/diff/180001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (right): https://codereview.chromium.org/420603011/diff/180001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:95: aura::Window::Windows windows, On 2014/08/06 21:54:10, oshima wrote: > WindowOverviewModeI should use the WindowListProvider too, shouldn't it? Done. https://codereview.chromium.org/420603011/diff/180001/ui/wm/core/window_list_... File ui/wm/core/window_list_provider.h (right): https://codereview.chromium.org/420603011/diff/180001/ui/wm/core/window_list_... ui/wm/core/window_list_provider.h:9: class WindowListProvider { I moved the file. Did not add GetInstance() and didn't separate from WindowManagerImpl.. Currently MruWindowTracker's constructor needs Window Manager's container so that it can listen to new windows being added, so it makes sense to create it from Window Manager and for the window manager to own it. Once we have this wired to activities, I guess the mechanism will change. Can we wait with doing the rest of the changes until then?
https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:73: int container_width = container_->GetBoundsInScreen().width(); On 2014/08/08 16:03:22, mfomitchev wrote: > Wouldn't the activities' container already set to the right bounds based on > this? When I look at HomeCardLayoutManager::Layout, it is simply using container > bounds.. Also, here we only care about the width, which shouldn't be impacted by > home card. Container is the same size of the root window. You're right that it only uses width, so I'm ok for now. https://codereview.chromium.org/420603011/diff/180001/ui/wm/core/window_list_... File ui/wm/core/window_list_provider.h (right): https://codereview.chromium.org/420603011/diff/180001/ui/wm/core/window_list_... ui/wm/core/window_list_provider.h:9: class WindowListProvider { On 2014/08/08 16:03:23, mfomitchev wrote: > I moved the file. Did not add GetInstance() and didn't separate from > WindowManagerImpl.. Currently MruWindowTracker's constructor needs Window > Manager's container so that it can listen to new windows being added, so it > makes sense to create it from Window Manager and for the window manager to own > it. Once we have this wired to activities, I guess the mechanism will change. > Can we wait with doing the rest of the changes until then? They simply shares the same container, but no true ownership relation between them. But I'm ok as this will change anyway. https://codereview.chromium.org/420603011/diff/200001/athena/wm/bezel_control... File athena/wm/bezel_controller.h (right): https://codereview.chromium.org/420603011/diff/200001/athena/wm/bezel_control... athena/wm/bezel_controller.h:47: // position and the bezel. It will be zero or negative for the right bezel. nit: you may refer to the comment in ScrollBegin for |delta| instead of repeating the comment. https://codereview.chromium.org/420603011/diff/200001/athena/wm/public/window... File athena/wm/public/window_list_provider.h (right): https://codereview.chromium.org/420603011/diff/200001/athena/wm/public/window... athena/wm/public/window_list_provider.h:13: static WindowListProvider* GetInstance(); remove them? https://codereview.chromium.org/420603011/diff/200001/athena/wm/public/window... athena/wm/public/window_list_provider.h:19: // Moves the window to the front of the list. new line before the comment https://codereview.chromium.org/420603011/diff/200001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/420603011/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:87: right_transform.Scale(.5, 1); I assume this is temporary. Can you add TODO to fix this? One tip in case you didn't know: the name in TODO is NOT the person to fix it. (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#TODO_Comments) https://codereview.chromium.org/420603011/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:203: // TODO (oshima, mfomitchev), crbug.com/388362: TODO(oshima|mfmitchev): is the right format https://codereview.chromium.org/420603011/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:247: // TODO (mfomitchev): return false in vertical orientation, in full screen. nuke space between TODO and ( https://codereview.chromium.org/420603011/diff/200001/ui/wm/wm.gyp File ui/wm/wm.gyp (right): https://codereview.chromium.org/420603011/diff/200001/ui/wm/wm.gyp#newcode86 ui/wm/wm.gyp:86: 'core/window_list_provider.h', you need to add this in athena.gyp instead.
https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:73: int container_width = container_->GetBoundsInScreen().width(); Ok. How can I get the size of the "work area"? I'd expect WindowManager's layout to account for this, but we are just setting window bounds to container bounds in WindowManagerImpl::Layout()... https://codereview.chromium.org/420603011/diff/180001/ui/wm/core/window_list_... File ui/wm/core/window_list_provider.h (right): https://codereview.chromium.org/420603011/diff/180001/ui/wm/core/window_list_... ui/wm/core/window_list_provider.h:9: class WindowListProvider { Ah, good point. https://codereview.chromium.org/420603011/diff/200001/athena/wm/bezel_control... File athena/wm/bezel_controller.h (right): https://codereview.chromium.org/420603011/diff/200001/athena/wm/bezel_control... athena/wm/bezel_controller.h:47: // position and the bezel. It will be zero or negative for the right bezel. On 2014/08/08 16:45:34, oshima wrote: > nit: you may refer to the comment in ScrollBegin for |delta| instead of > repeating the comment. Done. https://codereview.chromium.org/420603011/diff/200001/athena/wm/public/window... File athena/wm/public/window_list_provider.h (right): https://codereview.chromium.org/420603011/diff/200001/athena/wm/public/window... athena/wm/public/window_list_provider.h:13: static WindowListProvider* GetInstance(); Oops https://codereview.chromium.org/420603011/diff/200001/athena/wm/public/window... athena/wm/public/window_list_provider.h:19: // Moves the window to the front of the list. On 2014/08/08 16:45:34, oshima wrote: > new line before the comment Done. https://codereview.chromium.org/420603011/diff/200001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/420603011/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:87: right_transform.Scale(.5, 1); Yeah, I didn't know, thanks. This is temporary because we will need to support more complicated layouts when you are able to start scrolling/dragging the separator from the ACTIVE state. However, what is it that needs to be fixed in the current state? https://codereview.chromium.org/420603011/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:203: // TODO (oshima, mfomitchev), crbug.com/388362: On 2014/08/08 16:45:34, oshima wrote: > TODO(oshima|mfmitchev): > > is the right format Done. https://codereview.chromium.org/420603011/diff/200001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:247: // TODO (mfomitchev): return false in vertical orientation, in full screen. On 2014/08/08 16:45:34, oshima wrote: > nuke space between TODO and ( Done. https://codereview.chromium.org/420603011/diff/200001/ui/wm/wm.gyp File ui/wm/wm.gyp (right): https://codereview.chromium.org/420603011/diff/200001/ui/wm/wm.gyp#newcode86 ui/wm/wm.gyp:86: 'core/window_list_provider.h', On 2014/08/08 16:45:34, oshima wrote: > you need to add this in athena.gyp instead. Done.
lgtm with nits https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/420603011/diff/180001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:73: int container_width = container_->GetBoundsInScreen().width(); On 2014/08/08 17:40:50, mfomitchev wrote: > Ok. How can I get the size of the "work area"? I'd expect WindowManager's layout > to account for this, but we are just setting window bounds to container bounds > in WindowManagerImpl::Layout()... Yes, that's a bug that we need to fix. I'll address them all in separate CL, so you can go ahead with this for now. https://codereview.chromium.org/420603011/diff/220001/athena/wm/public/window... File athena/wm/public/window_list_provider.h (right): https://codereview.chromium.org/420603011/diff/220001/athena/wm/public/window... athena/wm/public/window_list_provider.h:21: } // namespace wm nit: namespace athena https://codereview.chromium.org/420603011/diff/220001/athena/wm/split_view_co... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/420603011/diff/220001/athena/wm/split_view_co... athena/wm/split_view_controller.h:34: bool IsSplitViewModeActive(); nit: const https://codereview.chromium.org/420603011/diff/220001/athena/wm/window_manage... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/420603011/diff/220001/athena/wm/window_manage... athena/wm/window_manager_impl.cc:38: virtual bool IsOverviewModeActive() OVERRIDE { return overview_; } nit: virtual body should be in .cc https://codereview.chromium.org/420603011/diff/220001/athena/wm/window_manage... athena/wm/window_manager_impl.cc:153: } nit: nuke {}
https://codereview.chromium.org/420603011/diff/220001/athena/wm/public/window... File athena/wm/public/window_list_provider.h (right): https://codereview.chromium.org/420603011/diff/220001/athena/wm/public/window... athena/wm/public/window_list_provider.h:21: } // namespace wm On 2014/08/08 18:00:40, oshima wrote: > nit: namespace athena Done. https://codereview.chromium.org/420603011/diff/220001/athena/wm/split_view_co... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/420603011/diff/220001/athena/wm/split_view_co... athena/wm/split_view_controller.h:34: bool IsSplitViewModeActive(); On 2014/08/08 18:00:40, oshima wrote: > nit: const Done. https://codereview.chromium.org/420603011/diff/220001/athena/wm/window_manage... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/420603011/diff/220001/athena/wm/window_manage... athena/wm/window_manager_impl.cc:38: virtual bool IsOverviewModeActive() OVERRIDE { return overview_; } On 2014/08/08 18:00:40, oshima wrote: > nit: virtual body should be in .cc Done. https://codereview.chromium.org/420603011/diff/220001/athena/wm/window_manage... athena/wm/window_manager_impl.cc:153: } On 2014/08/08 18:00:40, oshima wrote: > nit: nuke {} Done.
The CQ bit was checked by mfomitchev@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/420603011/260001
https://codereview.chromium.org/420603011/diff/260001/athena/wm/public/window... File athena/wm/public/window_list_provider.h (right): https://codereview.chromium.org/420603011/diff/260001/athena/wm/public/window... athena/wm/public/window_list_provider.h:1: #ifndef ATHENA_WM_PUBLIC_WINDOW_LIST_PROVIDER_H_ you need license comment here. Sorry, I missed in the review.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Since the CL is out of the CQ anyway, some nits and comments: https://codereview.chromium.org/420603011/diff/260001/athena/wm/public/window... File athena/wm/public/window_list_provider.h (right): https://codereview.chromium.org/420603011/diff/260001/athena/wm/public/window... athena/wm/public/window_list_provider.h:14: // Returns an ordered list of windows. Please document the ordering here. https://codereview.chromium.org/420603011/diff/260001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/420603011/diff/260001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:59: window_manager->AddObserver(this); Should you RemoveObserver() in dtor? https://codereview.chromium.org/420603011/diff/260001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:70: if (left_window_) { early return if !left_window_ https://codereview.chromium.org/420603011/diff/260001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:131: right_window_->SetTransform(right_transform); You should set the identity-transform on the window here, and set x = container_width / 2 in the bounds. https://codereview.chromium.org/420603011/diff/260001/athena/wm/split_view_co... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/420603011/diff/260001/athena/wm/split_view_co... athena/wm/split_view_controller.h:75: base::WeakPtrFactory<SplitViewController> weak_factory_; This should be the last member variable. (see https://groups.google.com/a/chromium.org/d/msg/chromium-dev/A9_N8p40l7A/eThX2...) https://codereview.chromium.org/420603011/diff/260001/athena/wm/window_manage... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/420603011/diff/260001/athena/wm/window_manage... athena/wm/window_manager_impl.cc:154: OnOverviewModeEnter()); Can you clarify the doc for this that this is called before the windows are in overview mode? https://codereview.chromium.org/420603011/diff/260001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (left): https://codereview.chromium.org/420603011/diff/260001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:87: Keep the blank line between the functions here.
https://codereview.chromium.org/420603011/diff/260001/athena/wm/public/window... File athena/wm/public/window_list_provider.h (right): https://codereview.chromium.org/420603011/diff/260001/athena/wm/public/window... athena/wm/public/window_list_provider.h:1: #ifndef ATHENA_WM_PUBLIC_WINDOW_LIST_PROVIDER_H_ On 2014/08/09 05:07:03, oshima wrote: > you need license comment here. Sorry, I missed in the review. Done. https://codereview.chromium.org/420603011/diff/260001/athena/wm/public/window... athena/wm/public/window_list_provider.h:14: // Returns an ordered list of windows. My thinking was that this is just an interface and the ordering is up to the subclass. MryWindowTracker does document the ordering. https://codereview.chromium.org/420603011/diff/260001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/420603011/diff/260001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:59: window_manager->AddObserver(this); On 2014/08/09 13:32:40, sadrul wrote: > Should you RemoveObserver() in dtor? Done. https://codereview.chromium.org/420603011/diff/260001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:70: if (left_window_) { On 2014/08/09 13:32:40, sadrul wrote: > early return if !left_window_ Done. https://codereview.chromium.org/420603011/diff/260001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:131: right_window_->SetTransform(right_transform); Good point. Done. https://codereview.chromium.org/420603011/diff/260001/athena/wm/split_view_co... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/420603011/diff/260001/athena/wm/split_view_co... athena/wm/split_view_controller.h:75: base::WeakPtrFactory<SplitViewController> weak_factory_; Done, good to know, thanks for pointing this out. https://codereview.chromium.org/420603011/diff/260001/athena/wm/window_manage... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/420603011/diff/260001/athena/wm/window_manage... athena/wm/window_manager_impl.cc:154: OnOverviewModeEnter()); On 2014/08/09 13:32:40, sadrul wrote: > Can you clarify the doc for this that this is called before the windows are in > overview mode? Done. https://codereview.chromium.org/420603011/diff/260001/athena/wm/window_overvi... File athena/wm/window_overview_mode.cc (left): https://codereview.chromium.org/420603011/diff/260001/athena/wm/window_overvi... athena/wm/window_overview_mode.cc:87: On 2014/08/09 13:32:40, sadrul wrote: > Keep the blank line between the functions here. Done.
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/420603011/280001
Message was sent while issue was closed.
Change committed as 288593
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/451363002/ by rouslan@chromium.org. The reason for reverting is: Appears to have broken Linux Chromium OS ASan LSan Tests (3) (stats): http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20... athena_unittests failed 13 Basic Accelerators AppSelection BasicTransition VirtualKeyboardTransition Basic CreateContainer GrabAndMouseCapture GrabInputContainer GrabShouldNotBlockVirtualKeyboard NonActivatableContainer Zorder Empty [ RUN ] ActivityManagerTest.Basic ================================================================= ==8808==ERROR: AddressSanitizer: heap-use-after-free on address 0x60200000ad70 at pc 0x00000054eebe bp 0x7fff975aaa70 sp 0x7fff975aaa68 READ of size 8 at 0x60200000ad70 thread T0 #0 0x54eebd in __find<__gnu_cxx::__normal_iterator<athena::WindowManagerObserver **, std::vector<athena::WindowManagerObserver *, std::allocator<athena::WindowManagerObserver *> > >, athena::WindowManagerObserver *> /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_algo.h:190:8 #1 0x54eebd in find<__gnu_cxx::__normal_iterator<athena::WindowManagerObserver **, std::vector<athena::WindowManagerObserver *, std::allocator<athena::WindowManagerObserver *> > >, athena::WindowManagerObserver *> /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_algo.h:4402 #2 0x54eebd in ObserverListBase<athena::WindowManagerObserver>::RemoveObserver(athena::WindowManagerObserver*) base/observer_list.h:168 #3 0x560f79 in ~SplitViewController athena/wm/split_view_controller.cc:63:3 #4 0x560f79 in athena::SplitViewController::~SplitViewController() athena/wm/split_view_controller.cc:62 #5 0x54d340 in operator() base/memory/scoped_ptr.h:137:5 #6 0x54d340 in ~scoped_ptr_impl base/memory/scoped_ptr.h:220 #7 0x54d340 in ~scoped_ptr base/memory/scoped_ptr.h:432 #8 0x54d340 in athena::(anonymous namespace)::WindowManagerImpl::~WindowManagerImpl() athena/wm/window_manager_impl.cc:124 #9 0x54d57d in athena::(anonymous namespace)::WindowManagerImpl::~WindowManagerImpl() athena/wm/window_manager_impl.cc:116:41 #10 0x567127 in athena::ShutdownAthena() athena/main/athena_launcher.cc:86:3 #11 0x5657af in athena::test::AthenaTestHelper::TearDown() athena/test/athena_test_helper.cc:98:3 #12 0x5645b6 in athena::test::AthenaTestBase::TearDown() athena/test/athena_test_base.cc:51:3 #13 0x4fc938 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2610:5 #14 0x4fd676 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2728:5 #15 0x5138f5 in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4591:11 #16 0x512ee6 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2418:12 #17 0x512ee6 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4209 #18 0x1602b74 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2304:10 #19 0x1602b74 in base::TestSuite::Run() base/test/test_suite.cc:227 #20 0x15fa231 in Run base/callback.h:401:12 #21 0x15fa231 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback<int ()> const&, int, bool, base::Callback<void ()> const&) base/test/launcher/unit_test_launcher.cc:498 #22 0x15faf7e in base::LaunchUnitTestsSerially(int, char**, base::Callback<int ()> const&) base/test/launcher/unit_test_launcher.cc:564:10 #23 0x4d74ca in main athena/test/athena_unittests.cc:51:10 #24 0x7f9020d2d76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 #25 0x4d726c in _start (/mnt/data/b/build/slave/Linux_Chromium_OS_ASan_LSan_Tests__3_/build/src/out/Release/athena_unittests+0x4d726c) Please follow instructions on how to setup ASan on your workstation here: http://www.chromium.org/developers/testing/addresssanitizer LSan may also be of use: http://www.chromium.org/developers/testing/leaksanitizer You may also want run your patch through the linux_lsan try bot.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
