Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
513313003:
Fix crash when using title drag on a window opened while split view is active (Closed)
DescriptionFix crash when using title drag on a window opened while split view is active.
This CL also centralizes setting the activity's bounds and transform while in
split view to SplitViewController::UpdateLayout(). In particular, the bounds of
an activity are now correct even if it is opened while some other activity is
animating into split view
BUG=405964
TEST=WindowManagerTest.NewWindowBounds
Committed: https://crrev.com/be1f70ab8f61124877c69378dfce860ed8786deb
Cr-Commit-Position: refs/heads/master@{#293019}
Patch Set 1 #
Total comments: 5
Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #
Total comments: 3
Patch Set 6 : #
Total comments: 14
Patch Set 7 : #
Messages
Total messages: 27 (3 generated)
pkotwicz@chromium.org changed reviewers: + sadrul@chromium.org
Sadrul, PTAL https://codereview.chromium.org/513313003/diff/1/athena/wm/split_view_control... File athena/wm/split_view_controller.cc (left): https://codereview.chromium.org/513313003/diff/1/athena/wm/split_view_control... athena/wm/split_view_controller.cc:207: left_window_->Hide(); I couldn't figure out why the window is hidden. Activating the window seems sufficient
When using bezel-gesture to switch between windows (instead of going into split view), we should continue hiding the old window. The rest of the changes looks good. You should also get a review from mfomitchev@ https://codereview.chromium.org/513313003/diff/1/athena/wm/split_view_control... File athena/wm/split_view_controller.cc (left): https://codereview.chromium.org/513313003/diff/1/athena/wm/split_view_control... athena/wm/split_view_controller.cc:207: left_window_->Hide(); On 2014/08/28 22:03:45, pkotwicz wrote: > I couldn't figure out why the window is hidden. Activating the window seems > sufficient I believe the idea here is to hide the window that's not visible anymore. We should continue doing this. (and add a check for this in BezelGestureToSwitchBetweenWindows)
Patchset #2 (id:20001) has been deleted
Sadrul, can you please take another look? https://codereview.chromium.org/513313003/diff/1/athena/wm/split_view_control... File athena/wm/split_view_controller.cc (left): https://codereview.chromium.org/513313003/diff/1/athena/wm/split_view_control... athena/wm/split_view_controller.cc:207: left_window_->Hide(); I have brought back the 'hide window' functionality. I have not expanded the unit test because non-visible windows are not hidden by default and overview mode does not hide windows. We should add / expand unit tests at that time
https://codereview.chromium.org/513313003/diff/1/athena/wm/split_view_control... File athena/wm/split_view_controller.cc (left): https://codereview.chromium.org/513313003/diff/1/athena/wm/split_view_control... athena/wm/split_view_controller.cc:207: left_window_->Hide(); On 2014/08/29 14:44:45, pkotwicz wrote: > I have brought back the 'hide window' functionality. I have not expanded the > unit test because non-visible windows are not hidden by default and overview > mode does not hide windows. We should add / expand unit tests at that time We do hide non-visible windows in some cases (e.g. when switching windows using title-drag), and we have test coverage for those. You should update the test for this case too. (I have a fix in progress to hide the windows after exiting from overview mode too, but that's independent of this behaviour). https://codereview.chromium.org/513313003/diff/60001/athena/wm/split_view_con... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/513313003/diff/60001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:213: UpdateLayout(false); Do we not want to reset left_window_ and right_window_ to NULL when state_ == INACTIVE anymore?
Sadrul, can you please take another look? https://codereview.chromium.org/513313003/diff/1/athena/wm/split_view_control... File athena/wm/split_view_controller.cc (left): https://codereview.chromium.org/513313003/diff/1/athena/wm/split_view_control... athena/wm/split_view_controller.cc:207: left_window_->Hide(); I have modified the unit test as requested https://codereview.chromium.org/513313003/diff/60001/athena/wm/split_view_con... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/513313003/diff/60001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:213: UpdateLayout(false); Yes we do. I need to be more careful...
https://codereview.chromium.org/513313003/diff/80001/athena/wm/split_view_con... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/513313003/diff/80001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:155: right_window_->Hide(); Can you move this code with the block in line 174 below? https://codereview.chromium.org/513313003/diff/80001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:157: left_window_->Show(); Do we ever call UpdateLayout() with state_ == INACTIVE, animate == true? https://codereview.chromium.org/513313003/diff/80001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:176: gfx::Transform left_transform; Do we expect state_ here to always be SCROLLING? If so, add a [D]CHECK_EQ?
Sadrul, can you please take another look? https://codereview.chromium.org/513313003/diff/80001/athena/wm/split_view_con... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/513313003/diff/80001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:155: right_window_->Hide(); On 2014/08/29 19:53:53, sadrul wrote: > Can you move this code with the block in line 174 below? Done. https://codereview.chromium.org/513313003/diff/80001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:157: left_window_->Show(); Yes, from ScrollEnd() https://codereview.chromium.org/513313003/diff/80001/athena/wm/split_view_con... athena/wm/split_view_controller.cc:176: gfx::Transform left_transform; We do not. We want to go through here when called from ScrollEnd()
lgtm
pkotwicz@chromium.org changed reviewers: + oshima@chromium.org
Oshima for OWNERs
nits and one q. https://codereview.chromium.org/513313003/diff/100001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/513313003/diff/100001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:148: return; can this happen? https://codereview.chromium.org/513313003/diff/100001/athena/wm/split_view_co... File athena/wm/split_view_controller.h (right): https://codereview.chromium.org/513313003/diff/100001/athena/wm/split_view_co... athena/wm/split_view_controller.h:41: void DeactivateSplitMode(); nit: can you update the comment? https://codereview.chromium.org/513313003/diff/100001/athena/wm/split_view_co... athena/wm/split_view_controller.h:76: void OnAnimationCompleted(); can you add comment that this is called once for each transition that involves animation? (even if we animate two windows, this gets called once, right?)
pkotwicz@chromium.org changed reviewers: + mfomitchev@chromium.org
Oshima, can you please take another look? Adding mfomitchev@ as a reviewer too per sadrul@'s suggestion
lgtm
mfomitchev@ Ping?
Sorry for the delay. https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:203: base::Bind(&SplitViewController::OnAnimationCompleted, Is it guaranteed that the right window animation always completes after the left window animation? We wouldn't want OnAnimationCompleted evert executed before the left window animation completes.. https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... File athena/wm/split_view_controller.h (left): https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... athena/wm/split_view_controller.h:38: // Resets the internal state to an inactive state. Calling this does not It looks like we call UpdateLayout(false), which does update the transform... https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... athena/wm/split_view_controller.h:43: void ReplaceWindow(aura::Window* window, Would be good to document that this does a layout as well updating bounds/transform of |replace_with| and doesn't affect the bounds of |window|. https://codereview.chromium.org/513313003/diff/120001/athena/wm/window_manage... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/513313003/diff/120001/athena/wm/window_manage... athena/wm/window_manager_impl.cc:105: instance->split_view_controller_->ReplaceWindow( Hmm.. So normally the two windows shown in split view are the two top-most windows from WindowListProvider. I think this code may violate this invariant: Consider the case when left is #1 and right is #2. We add a new window and get: #1 new_window #2 left #3 right However split view shows new_window and right, not new_window and left. It seems like we should either update the window order here, or replace window #3 rather than always replacing left_window. This is actually a UX questions which we should perhaps as Kuscher - should we always replace a fixed position window (e.g. left), or the window which wasn't the one most recently used. https://codereview.chromium.org/513313003/diff/120001/athena/wm/window_manage... athena/wm/window_manager_impl.cc:105: instance->split_view_controller_->ReplaceWindow( Nit: A short comment explaining that ReplaceWindow will update the bounds/transform might be useful here
https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:203: base::Bind(&SplitViewController::OnAnimationCompleted, It is not guaranteed. However, in practice it is ok for OnAnimationCompleted() to run shortly before the left window animation completes https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... File athena/wm/split_view_controller.h (left): https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... athena/wm/split_view_controller.h:38: // Resets the internal state to an inactive state. Calling this does not Yup Aside: It turns out that there are two callers of DeactivateSplitMode(). WindowManagerImpl::OnSelectWindow() expects DeactivateSplitMode() to animate (or not tamper with the current animation) and WindowManagerImpl::ToggleSplitview() expects DeactivateSplitMode() to reset the transforms. - The CL as is fixes a bug where the animation would do super weird things if Ctrl+F6 was hit while exiting overview into split mode - The CL as is prematurely hides the left and right windows when exiting overview mode. In a follow up CL (a fancier version of https://codereview.chromium.org/527313002/), I will add a parameter to DeactivateSplitMode() which indicates whether split mode should animate deactivation. https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... athena/wm/split_view_controller.h:43: void ReplaceWindow(aura::Window* window, Done. Perhaps this method should also update |window|'s bounds... That's something for a future CL which needs to be debated with sadrul@ https://codereview.chromium.org/513313003/diff/120001/athena/wm/window_manage... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/513313003/diff/120001/athena/wm/window_manage... athena/wm/window_manager_impl.cc:105: instance->split_view_controller_->ReplaceWindow( You are completely right. OnTitleDragCompleted() produces the same bug. I have filed a bug at http://crbug.com/409888 Will fix in follow up CL https://codereview.chromium.org/513313003/diff/120001/athena/wm/window_manage... athena/wm/window_manager_impl.cc:105: instance->split_view_controller_->ReplaceWindow( I think the comment in ReplaceWindow() is sufficient. Otherwise we would have to add a comment describing what ReplaceWindow() does at each call site which is not scalable.
https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:203: base::Bind(&SplitViewController::OnAnimationCompleted, What if we are switching to the next window rather than entering overview and we've started the drag from the right edge? left window will animate the translate transform from 0 to -<container width>, but OnAnimationCompleted is supposed to reset the translate back to 0. If the animation runs past OnAnimationCompleted, left window will left translated which will be visible when we enter overview - the animation won't look right. https://codereview.chromium.org/513313003/diff/120001/athena/wm/window_manage... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/513313003/diff/120001/athena/wm/window_manage... athena/wm/window_manager_impl.cc:105: instance->split_view_controller_->ReplaceWindow( On 2014/09/02 17:54:35, pkotwicz wrote: > I think the comment in ReplaceWindow() is sufficient. Otherwise we would have to > add a comment describing what ReplaceWindow() does at each call site which is > not scalable. Acknowledged.
https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:203: base::Bind(&SplitViewController::OnAnimationCompleted, Calling aura::Window::SetTransform() will terminate the transform animation on the window that it is called on. So the problem you describe should not occur.
LGTM https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... File athena/wm/split_view_controller.cc (right): https://codereview.chromium.org/513313003/diff/120001/athena/wm/split_view_co... athena/wm/split_view_controller.cc:203: base::Bind(&SplitViewController::OnAnimationCompleted, Ok then, great!
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/513313003/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as 7fd49ba141106bb857b20d4aaee154d1dbb26954
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/be1f70ab8f61124877c69378dfce860ed8786deb Cr-Commit-Position: refs/heads/master@{#293019} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
