Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
68033003:
Undocks window first before side-snapping bounds (Closed)
|
Created:
7 years, 1 month ago by varkha Modified:
6 years, 10 months ago CC:
chromium-reviews, kalyank, sadrul, ben+ash_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUndocks window first before side-snapping bounds. Side-snapped state is now maintained in order to properly deal with undocking into a side-snapped state.
BUG=320119
TEST=ash_unittests --gtest_filter=WorkspaceWindowResizerTest.Edge
TEST=ash_unittests --gtest_filter=AcceleratorControllerTest.WindowSnap
TEST=ash_unittests --gtest_filter=*DockedWindowResizerTest.SideSnapDocked/*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237809
Patch Set 1 #
Total comments: 10
Patch Set 2 : Undocks window first before side-snapping bounds (comments) #
Total comments: 1
Patch Set 3 : Undocks window first before side-snapping bounds (rebase) #
Total comments: 5
Patch Set 4 : Undocks window first before side-snapping bounds (comments) #Patch Set 5 : Undocks window first before side-snapping bounds (corrected test) #Patch Set 6 : Undocks window first before side-snapping bounds (rebase) #
Total comments: 6
Patch Set 7 : Undocks window first before side-snapping bounds (test) #Patch Set 8 : Undocks window first before side-snapping bounds (comments) #Patch Set 9 : Undocks window first before side-snapping bounds (comments) #
Total comments: 14
Patch Set 10 : Undocks window first before side-snapping bounds (comments) #
Total comments: 24
Patch Set 11 : Undocks window first before side-snapping bounds (nit) #
Total comments: 12
Patch Set 12 : Undocks window first before side-snapping bounds (more comments) #
Total comments: 16
Patch Set 13 : Undocks window first before side-snapping bounds (nits) #
Total comments: 6
Messages
Total messages: 35 (0 generated)
pkotwicz@, could you please take a look. I pre-notify the layout manager about imminent ash window state change and it seems that the handling that we have for fullscreen / maximized notifications can also extended to the side-maximized case. I can foresee comments about how the ash window state type is now modifiable externally but this seems like a necessary evil here. Thanks.
https://codereview.chromium.org/68033003/diff/1/ash/wm/dock/docked_window_lay... File ash/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/1/ash/wm/dock/docked_window_lay... ash/wm/dock/docked_window_layout_manager.cc:568: } else if (window_state->IsMaximizedOrFullscreen() || How about: } else if (window_state->IsNormalShowState()) { if (old_type == wm::SHOW_TYPE_MINIMIZED) RestoreDockedWindow(window_state); } else { UndockWindow(window); RecordUmaAction(DOCKED_ACTION_MAXIMIZE, DOCKED_ACTION_SOURCE_UNKNOWN); } https://codereview.chromium.org/68033003/diff/1/ash/wm/dock/docked_window_lay... ash/wm/dock/docked_window_layout_manager.cc:571: // Reparenting changes the source bounds for the animation if a window is This comment is no longer relevant? https://codereview.chromium.org/68033003/diff/1/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/1/ash/wm/window_state.cc#newcode62 ash/wm/window_state.cc:62: window_show_type_ = window_show_type; SnapSizer actually calls WindowState::SnapWindow() so I do not think that we need to add this method. https://codereview.chromium.org/68033003/diff/1/ash/wm/window_state.cc#newcod... ash/wm/window_state.cc:293: left_or_right == SHOW_TYPE_RIGHT_SNAPPED); Oshima-san, is there a reason why we do not notify observers about the state change here?
https://codereview.chromium.org/68033003/diff/1/ash/wm/dock/docked_window_lay... File ash/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/1/ash/wm/dock/docked_window_lay... ash/wm/dock/docked_window_layout_manager.cc:568: } else if (window_state->IsMaximizedOrFullscreen() || On 2013/11/18 16:25:20, pkotwicz wrote: > How about: > } else if (window_state->IsNormalShowState()) { > if (old_type == wm::SHOW_TYPE_MINIMIZED) > RestoreDockedWindow(window_state); > } else { > UndockWindow(window); > RecordUmaAction(DOCKED_ACTION_MAXIMIZE, DOCKED_ACTION_SOURCE_UNKNOWN); > } sgtm https://codereview.chromium.org/68033003/diff/1/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/1/ash/wm/window_state.cc#newcod... ash/wm/window_state.cc:293: left_or_right == SHOW_TYPE_RIGHT_SNAPPED); On 2013/11/18 16:25:20, pkotwicz wrote: > Oshima-san, is there a reason why we do not notify observers about the state > change here? No specific reason other than LEFT/RIGHT SNAPPED state hasn't been fully implemented yet. Let's try and fix side effects if any.
PTAL. I have now implemented preservation for the snapped state when insets change or screen size change requires snapped window to be moved. https://codereview.chromium.org/68033003/diff/1/ash/wm/dock/docked_window_lay... File ash/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/1/ash/wm/dock/docked_window_lay... ash/wm/dock/docked_window_layout_manager.cc:568: } else if (window_state->IsMaximizedOrFullscreen() || On 2013/11/18 16:25:20, pkotwicz wrote: > How about: > } else if (window_state->IsNormalShowState()) { > if (old_type == wm::SHOW_TYPE_MINIMIZED) > RestoreDockedWindow(window_state); > } else { > UndockWindow(window); > RecordUmaAction(DOCKED_ACTION_MAXIMIZE, DOCKED_ACTION_SOURCE_UNKNOWN); > } SHOW_TYPE_*_SNAPPED types are both present with IsNormalShowState so with suggested code I would do nothing instead of undocking. I could do this: } else if (window_state->IsNormalShowState() && old_type == wm::SHOW_TYPE_MINIMIZED) { RestoreDockedWindow(window_state); } else { UndockWindow(window); RecordUmaAction(DOCKED_ACTION_MAXIMIZE, DOCKED_ACTION_SOURCE_UNKNOWN); } ... but I think this would be too general for undocking in all present and future cases other than (Normal+from minimized). I would prefer having an explicit code path for undocking when a window gets maximized. https://codereview.chromium.org/68033003/diff/1/ash/wm/dock/docked_window_lay... ash/wm/dock/docked_window_layout_manager.cc:571: // Reparenting changes the source bounds for the animation if a window is On 2013/11/18 16:25:20, pkotwicz wrote: > This comment is no longer relevant? Done. https://codereview.chromium.org/68033003/diff/1/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/1/ash/wm/window_state.cc#newcode62 ash/wm/window_state.cc:62: window_show_type_ = window_show_type; On 2013/11/18 16:25:20, pkotwicz wrote: > SnapSizer actually calls WindowState::SnapWindow() so I do not think that we > need to add this method. Done. Removed (in favor of sending notifications whenever window_show_type_ changes). https://codereview.chromium.org/68033003/diff/1/ash/wm/window_state.cc#newcod... ash/wm/window_state.cc:293: left_or_right == SHOW_TYPE_RIGHT_SNAPPED); On 2013/11/18 18:44:44, oshima wrote: > On 2013/11/18 16:25:20, pkotwicz wrote: > > Oshima-san, is there a reason why we do not notify observers about the state > > change here? > > No specific reason other than LEFT/RIGHT SNAPPED state hasn't been fully > implemented yet. Let's try and fix side effects if any. Done. I am notifying first to allow observers to take any action necessary before setting bounds. This allows docked layout to undock the window when snapped state is set. https://codereview.chromium.org/68033003/diff/80001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/80001/ash/wm/window_state.cc#ne... ash/wm/window_state.cc:139: SnapWindow(SHOW_TYPE_RIGHT_SNAPPED, bounds); We did not really use this show type so everything still worked - sort of. Of course we did not preserve the snapped state before so this was not important.
https://codereview.chromium.org/68033003/diff/210001/ash/accelerators/acceler... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/68033003/diff/210001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.cc:873: // Centering a window does not maintain state and state change This change should have its own CL Some comments: Can't you achieve the same effect as calling SetWindowShowTypeUnsnapped() by setting the restore bounds and calling WindowState::Restore(). You will have to avoid doing this for maximized and fullscreen windows which are already theoretically centered. Can you also please add a UMA metric. (That way we know if the accelerator is being used and if we can remove the accelerator in a future milestone). https://codereview.chromium.org/68033003/diff/210001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/210001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:258: SetWindowShowTypeUnsnapped(); oshima@ can you comment as to whether this method is optimal. This method is an improvement because previously if a user dragged a window which was snapped left or right the window's state would not change. https://codereview.chromium.org/68033003/diff/210001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:285: window_show_type_ = left_or_right; Can you set the restore bounds to the desired bounds before notifying the observers? Then the observer can use the restore bounds as the "desired bounds". Ideally we can avoid calling WindowState::Restore() at all.
PTAL. https://codereview.chromium.org/68033003/diff/210001/ash/accelerators/acceler... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/68033003/diff/210001/ash/accelerators/acceler... ash/accelerators/accelerator_controller.cc:873: // Centering a window does not maintain state and state change On 2013/11/21 17:26:14, pkotwicz wrote: > This change should have its own CL > > Some comments: > Can't you achieve the same effect as calling SetWindowShowTypeUnsnapped() by > setting the restore bounds and calling WindowState::Restore(). > You will have to avoid doing this for maximized and fullscreen windows which are > already theoretically centered. Can you also please add a UMA metric. (That way > we know if the accelerator is being used and if we can remove the accelerator in > a future milestone). > Done. Please see https://codereview.chromium.org/79023009. https://codereview.chromium.org/68033003/diff/210001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/210001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:285: window_show_type_ = left_or_right; On 2013/11/21 17:26:14, pkotwicz wrote: > Can you set the restore bounds to the desired bounds before notifying the > observers? Then the observer can use the restore bounds as the "desired bounds". > Ideally we can avoid calling WindowState::Restore() at all. This was actually a bug. Maximized window would not remember that it was snapped since Restore would cause OnWindowPropertyChanged and override the window_show_type_ that was set to one of the snapped states. I have corrected that as well as saving the restore_bounds properly in the other case (when a window is not maximized). Otherwise we get strange restored to snapped bounds behavior when using accelerators.
On 2013/11/21 22:47:27, varkha wrote: > PTAL. Let me know if you want me to review now, or want to wait for Peter's review first. > https://codereview.chromium.org/68033003/diff/210001/ash/accelerators/acceler... > File ash/accelerators/accelerator_controller.cc (right): > > https://codereview.chromium.org/68033003/diff/210001/ash/accelerators/acceler... > ash/accelerators/accelerator_controller.cc:873: // Centering a window does not > maintain state and state change > On 2013/11/21 17:26:14, pkotwicz wrote: > > This change should have its own CL > > > > Some comments: > > Can't you achieve the same effect as calling SetWindowShowTypeUnsnapped() by > > setting the restore bounds and calling WindowState::Restore(). > > You will have to avoid doing this for maximized and fullscreen windows which > are > > already theoretically centered. Can you also please add a UMA metric. (That > way > > we know if the accelerator is being used and if we can remove the accelerator > in > > a future milestone). > > > > Done. Please see https://codereview.chromium.org/79023009. > > https://codereview.chromium.org/68033003/diff/210001/ash/wm/window_state.cc > File ash/wm/window_state.cc (right): > > https://codereview.chromium.org/68033003/diff/210001/ash/wm/window_state.cc#n... > ash/wm/window_state.cc:285: window_show_type_ = left_or_right; > On 2013/11/21 17:26:14, pkotwicz wrote: > > Can you set the restore bounds to the desired bounds before notifying the > > observers? Then the observer can use the restore bounds as the "desired > bounds". > > Ideally we can avoid calling WindowState::Restore() at all. > > This was actually a bug. Maximized window would not remember that it was snapped > since Restore would cause OnWindowPropertyChanged and override the > window_show_type_ that was set to one of the snapped states. I have corrected > that as well as saving the restore_bounds properly in the other case (when a > window is not maximized). Otherwise we get strange restored to snapped bounds > behavior when using accelerators.
> Let me know if you want me to review now, or want to wait for Peter's review > first. Peter suggested to separate concerns so that afte WindowState::SnapWindow calls Restore there would not be a need to adjust the bounds inside SnapWindow. The code in WorkspaceLayoutManager would then perform the bounds change (set them to restore bounds) in OnWindowShowTypeChanged when detecting snapping type change. I tried this and something is missing so I will need to understand this a bit better. This should allow removing SetBounds in line 306 in windw_state.cc in the patch set 5 .
pkotwicz@, can you please look into it again? I have tried the approach with having the workspace layout manager worry about the bounds. While I can make most of the existing behavior unchanged, somewhere in the process the changes became too remote from the original goal. Specifically I would have to refactor docked and workspace layout managers to share code that gets executed from OnWindowShowTypeChanged and hope to not break multiple existing code paths unrelated to snapping. Consider that while dragging the windows may be not observed by the workspace layout. SetBounds() call in SnapWindow (which was there to begin with) seems to be a workaround that is much less disruptive since it is specific to snapping scenario. Thanks!
one high level q before going to more details. https://codereview.chromium.org/68033003/diff/480001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/480001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:252: window_show_type_ = SHOW_TYPE_NORMAL; if (window_show_type_ == || ) { window_show_type_ = SHOW_TYPE_NORMAL; } would be slightly better https://codereview.chromium.org/68033003/diff/480001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/480001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:140: window_state->window_show_type() == wm::SHOW_TYPE_RIGHT_SNAPPED) { Maybe we should have WindowState::IsLeftOrRightSnapped() like IsMaximizedOrFullscreen ? https://codereview.chromium.org/68033003/diff/480001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:312: switch (window_state->GetShowState()) { is it possible to switch to use WindowShowType instead now?
I am looking at the CL. I still think that it is possible to set the side maximized bounds in WorkspaceLayoutManager::UpdateBoundsFromShowState() without too much trouble so I am trying to prove the opposite to myself.
On 2013/11/25 23:43:48, pkotwicz wrote: > I am looking at the CL. I still think that it is possible to set the side > maximized bounds in WorkspaceLayoutManager::UpdateBoundsFromShowState() without > too much trouble so I am trying to prove the opposite to myself. The trouble I had was when using caption drag to snap rather than maximize button drag. In that case this handler is not even called (docked layout is getting called). I think getting the two layout managers to cooperate would be not trivial.
On 2013/11/25 23:50:21, varkha wrote: > On 2013/11/25 23:43:48, pkotwicz wrote: > > I am looking at the CL. I still think that it is possible to set the side > > maximized bounds in WorkspaceLayoutManager::UpdateBoundsFromShowState() > without > > too much trouble so I am trying to prove the opposite to myself. > > The trouble I had was when using caption drag to snap rather than maximize > button drag. In that case this handler is not even called (docked layout is > getting called). I think getting the two layout managers to cooperate would be > not trivial. Ok, if we have enough unit tests to cover, I can look into this after this CL.
I have also added a test to check that snapping out of dock works. I used non-interactive snapping since old maximized button is now disabled and will probably be gone, but the test would be useful once we implement docking keyboard shortcuts. PTAL. https://codereview.chromium.org/68033003/diff/480001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/480001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:252: window_show_type_ = SHOW_TYPE_NORMAL; On 2013/11/25 23:20:40, oshima wrote: > if (window_show_type_ == || > ) { > window_show_type_ = SHOW_TYPE_NORMAL; > } > > would be slightly better Done. https://codereview.chromium.org/68033003/diff/480001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/480001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:140: window_state->window_show_type() == wm::SHOW_TYPE_RIGHT_SNAPPED) { On 2013/11/25 23:20:40, oshima wrote: > Maybe we should have WindowState::IsLeftOrRightSnapped() like > IsMaximizedOrFullscreen ? Done. https://codereview.chromium.org/68033003/diff/480001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:312: switch (window_state->GetShowState()) { On 2013/11/25 23:20:40, oshima wrote: > is it possible to switch to use WindowShowType instead now? As we discussed this offline - let's do that in another CL once the behavior here lands. We will need to do more to make snapped show type a "proper" state, e.g. restore it when unmaximizing.
lgtm
varkha@ and I have discussed a number of things which I think could be improved wrt to this CL offline. For instance, I think that it may be possible to call wm::WindowState::SetWindowShowTypeUnsnapped() as part of wm::WindowState::Restore() instead of exposing the method as part of the public API. This CL could use some additional tests too. For instance, this CL changes what occurs when a window is (1) Side snapped L/R (2) Maximized (3) Restored. We should have a test for that. This CL does not look good to me as is.
Thanks, please take a look. I think the modified test in https://codereview.chromium.org/68033003/diff/640001/ash/accelerators/acceler... is doing the exact sequence that you suggest (snap-maximize-restore) and the changed outcome is captured by the test.
I have reverted the changes that were related to getting set_bounds_changed_by_user also clear snapped state. Snapped state is no cleared by calling WindowState::Restore(). PTAL.
I will continue to review tomorrow. Here is what I have so far https://codereview.chromium.org/68033003/diff/660001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/660001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:167: SetWindowShowTypeUnsnapped(); Nit: SetWindowShowTypeUnsapped() now has a single caller. Can we fold it into this function? https://codereview.chromium.org/68033003/diff/660001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:276: // Before we can set the bounds we need to restore the window. This comment needs to be updated. https://codereview.chromium.org/68033003/diff/660001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:285: // Restore will cause OnWindowPropertyChanged so it needs to be done Nit: OnWindowPropertyChanged() https://codereview.chromium.org/68033003/diff/660001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:286: // before notifying about show type change to snapped. Nit: about show type change to snapped -> that the WindowShowType has changed to |left_or_right|. https://codereview.chromium.org/68033003/diff/660001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:298: if (!was_maximized) Is there any harm in always setting the bounds? (If there is a reason we should avoid setting the bounds when the window used to be maximized, we should add a comment as to why) https://codereview.chromium.org/68033003/diff/660001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/660001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:40: const int kSlideDurationMs = 120; How about kBoundsChangeSlideDurationMs https://codereview.chromium.org/68033003/diff/660001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:139: if (window_state->IsSnapped()) Can you move AdjustSnappedBounds() within the IsSnapped() if block?
https://codereview.chromium.org/68033003/diff/660001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/660001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:167: SetWindowShowTypeUnsnapped(); On 2013/11/27 06:21:57, pkotwicz wrote: > Nit: SetWindowShowTypeUnsapped() now has a single caller. Can we fold it into > this function? Done. https://codereview.chromium.org/68033003/diff/660001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:276: // Before we can set the bounds we need to restore the window. On 2013/11/27 06:21:57, pkotwicz wrote: > This comment needs to be updated. Done. https://codereview.chromium.org/68033003/diff/660001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:285: // Restore will cause OnWindowPropertyChanged so it needs to be done On 2013/11/27 06:21:57, pkotwicz wrote: > Nit: OnWindowPropertyChanged() Done. https://codereview.chromium.org/68033003/diff/660001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:286: // before notifying about show type change to snapped. On 2013/11/27 06:21:57, pkotwicz wrote: > Nit: about show type change to snapped -> that the WindowShowType has changed to > |left_or_right|. Done. https://codereview.chromium.org/68033003/diff/660001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:298: if (!was_maximized) On 2013/11/27 06:21:57, pkotwicz wrote: > Is there any harm in always setting the bounds? (If there is a reason we should > avoid setting the bounds when the window used to be maximized, we should add a > comment as to why) In opposite case the bounds are already set correctly when Restore was called above. I have added a comment to clarify. https://codereview.chromium.org/68033003/diff/660001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/660001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:40: const int kSlideDurationMs = 120; On 2013/11/27 06:21:57, pkotwicz wrote: > How about kBoundsChangeSlideDurationMs Done. https://codereview.chromium.org/68033003/diff/660001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:139: if (window_state->IsSnapped()) On 2013/11/27 06:21:57, pkotwicz wrote: > Can you move AdjustSnappedBounds() within the IsSnapped() if block? Done.
lgtm https://codereview.chromium.org/68033003/diff/720001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/720001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:168: window_show_type_ = SHOW_TYPE_NORMAL; can you add comment why this is necessary?
https://codereview.chromium.org/68033003/diff/720001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/720001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:168: window_show_type_ = SHOW_TYPE_NORMAL; On 2013/11/27 18:28:14, oshima wrote: > can you add comment why this is necessary? Done.
Looks good! https://codereview.chromium.org/68033003/diff/720001/ash/wm/dock/docked_windo... File ash/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/720001/ash/wm/dock/docked_windo... ash/wm/dock/docked_window_layout_manager.cc:104: wm::WindowState* window_state = wm::GetWindowState(window); Can we nuke the early return? https://codereview.chromium.org/68033003/diff/720001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/720001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:275: // We set the restore bounds to the bounds we want, restore the window if This comment is not useful. It describes what the code does not why it does what it does. Can you remove it? https://codereview.chromium.org/68033003/diff/720001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:301: if (!was_maximized) Can we change the condition to: if (window_->parent() && window_->parent()->id() == internal::kShellWindowId_DockedContainer) { window_->SetBounds(bounds); } This allows us to: Stop calling WorkspaceLayoutManager::SetChildBoundsAnimatd() from WorkspaceLayoutManager::SetChildBounds() Remove the weird early exit from UndockWindow() in docked_window_layout_manager.cc This change makes a window not animate to its snapped position when a window is snapped via dragging but we do not animate when a window becomes docked via dragging either https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:39: // Duration of slide animation used when a snapped window is adjusted. Nit: Can we move the constant to the body of WorkspaceLayoutManager::SetChildBoundsAnimated(). The reason for this suggestion is that I cannot think of a better way of describing the constant other than "the duration of the animation started by SetChildBoundsAnimated()" https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:138: if (window_state->IsSnapped()) { As I mentioned in the comment in wm::WindowState, can we always call SetChildBoundsDirect(). https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:171: SetMaximizedOrFullscreenBounds(window_state); Shouldn't we call AdjustSnappedBounds() here for completeness? https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:264: SetChildBoundsAnimated(window_state->window(), bounds); Is the comment about mirroring relevant for non crossfade animations? https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:339: window_state->IsSnapped()) { Can you put the check for "last_show_state == ui::SHOW_STATE_MINIMIZED" above this one. Can you also move AdjustSnappedBounds() into the if statement. https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:410: !window_state->IsNormalShowState() || Can you remove the check for the normal show state? https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.h (right): https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.h:93: // Animates window bounds to a new location when work area bounds change or How about "Animates the window bounds to |bounds|." I do not think that mentioning when the method is invoked adds much. The comment does not list all of the invokers either. (e.g. when a side snapped window becomes restored) https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_window_resizer.cc (right): https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer.cc:439: bool is_snapped = false; Can you renamed to |snapped| ? I THINK that it is slightly clearer. https://codereview.chromium.org/68033003/diff/740001/ash/wm/dock/docked_windo... File ash/wm/dock/docked_window_resizer_unittest.cc (right): https://codereview.chromium.org/68033003/diff/740001/ash/wm/dock/docked_windo... ash/wm/dock/docked_window_resizer_unittest.cc:1314: // A window should be attached and snapped to the right edge. For the sake of sanity a window is either docked or snapped. How about "The window should be docked to the right edge" https://codereview.chromium.org/68033003/diff/740001/ash/wm/dock/docked_windo... ash/wm/dock/docked_window_resizer_unittest.cc:1340: // A window should be attached and snapped to the right edge. Can you change this comment too? https://codereview.chromium.org/68033003/diff/740001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/740001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:78: ui::WindowShowState state = window_->GetProperty(aura::client::kShowStateKey); In a separate CL we must absolutely kill this method. It is too confusing now that snapped left / right are of state normal. https://codereview.chromium.org/68033003/diff/740001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:167: // When a window |IsSnapped| it is auto-positioned by layout manager to stay You can actually kill the if statement completely. WindowState::OnWindowPropertyChanged() currently always sets |window_show_type_| to SHOW_TYPE_NORMAL when the show state is ui::SHOW_STATE_NORMAL. I suspect that WindowState::OnWindowPropertyChanged() should not have this behavior but that's for a different CL. https://codereview.chromium.org/68033003/diff/740001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): https://codereview.chromium.org/68033003/diff/740001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer_unittest.cc:638: // Test if the restore bounds is correct in multiple displays. Do we still need to clear the restored bounds if we are restoring the window immediately afterwards? https://codereview.chromium.org/68033003/diff/740001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer_unittest.cc:659: resizer->Drag(CalculateDragPoint(*resizer, 499, 00), 0); Nit: 00 -> 0
Thanks again, PTAL. https://codereview.chromium.org/68033003/diff/720001/ash/wm/dock/docked_windo... File ash/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/720001/ash/wm/dock/docked_windo... ash/wm/dock/docked_window_layout_manager.cc:104: wm::WindowState* window_state = wm::GetWindowState(window); On 2013/11/27 20:24:43, pkotwicz wrote: > Can we nuke the early return? Done. Possible now. https://codereview.chromium.org/68033003/diff/720001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/720001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:275: // We set the restore bounds to the bounds we want, restore the window if On 2013/11/27 20:24:43, pkotwicz wrote: > This comment is not useful. It describes what the code does not why it does what > it does. Can you remove it? I have changed the comment to make sure the resulting behavior is captured. https://codereview.chromium.org/68033003/diff/720001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:301: if (!was_maximized) On 2013/11/27 20:24:43, pkotwicz wrote: > Can we change the condition to: > if (window_->parent() && > window_->parent()->id() == internal::kShellWindowId_DockedContainer) { > window_->SetBounds(bounds); > } > > This allows us to: > Stop calling WorkspaceLayoutManager::SetChildBoundsAnimatd() from > WorkspaceLayoutManager::SetChildBounds() > Remove the weird early exit from UndockWindow() in > docked_window_layout_manager.cc > > This change makes a window not animate to its snapped position when a window is > snapped via dragging but we do not animate when a window becomes docked via > dragging either Done. https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:39: // Duration of slide animation used when a snapped window is adjusted. On 2013/11/27 20:24:43, pkotwicz wrote: > Nit: Can we move the constant to the body of > WorkspaceLayoutManager::SetChildBoundsAnimated(). The reason for this suggestion > is that I cannot think of a better way of describing the constant other than > "the duration of the animation started by SetChildBoundsAnimated()" Done. https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:138: if (window_state->IsSnapped()) { On 2013/11/27 20:24:43, pkotwicz wrote: > As I mentioned in the comment in wm::WindowState, can we always call > SetChildBoundsDirect(). Done. https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:171: SetMaximizedOrFullscreenBounds(window_state); On 2013/11/27 20:24:43, pkotwicz wrote: > Shouldn't we call AdjustSnappedBounds() here for completeness? Done. https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:264: SetChildBoundsAnimated(window_state->window(), bounds); On 2013/11/27 20:24:43, pkotwicz wrote: > Is the comment about mirroring relevant for non crossfade animations? I don't think so since we are not changing the layers. https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:339: window_state->IsSnapped()) { On 2013/11/27 20:24:43, pkotwicz wrote: > Can you put the check for "last_show_state == ui::SHOW_STATE_MINIMIZED" above > this one. > > Can you also move AdjustSnappedBounds() into the if statement. Done. https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:410: !window_state->IsNormalShowState() || On 2013/11/27 20:24:43, pkotwicz wrote: > Can you remove the check for the normal show state? Done. https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.h (right): https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.h:93: // Animates window bounds to a new location when work area bounds change or On 2013/11/27 20:24:43, pkotwicz wrote: > How about "Animates the window bounds to |bounds|." > > I do not think that mentioning when the method is invoked adds much. The comment > does not list all of the invokers either. (e.g. when a side snapped window > becomes restored) Done. https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_window_resizer.cc (right): https://codereview.chromium.org/68033003/diff/720001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer.cc:439: bool is_snapped = false; On 2013/11/27 20:24:43, pkotwicz wrote: > Can you renamed to |snapped| ? I THINK that it is slightly clearer. Done. https://codereview.chromium.org/68033003/diff/740001/ash/wm/dock/docked_windo... File ash/wm/dock/docked_window_resizer_unittest.cc (right): https://codereview.chromium.org/68033003/diff/740001/ash/wm/dock/docked_windo... ash/wm/dock/docked_window_resizer_unittest.cc:1314: // A window should be attached and snapped to the right edge. On 2013/11/27 20:24:43, pkotwicz wrote: > For the sake of sanity a window is either docked or snapped. How about > "The window should be docked to the right edge" Done. https://codereview.chromium.org/68033003/diff/740001/ash/wm/dock/docked_windo... ash/wm/dock/docked_window_resizer_unittest.cc:1340: // A window should be attached and snapped to the right edge. On 2013/11/27 20:24:43, pkotwicz wrote: > Can you change this comment too? Done. Also changed throughout this file. Seems like I used those terms interchangeably - wrong. https://codereview.chromium.org/68033003/diff/740001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/740001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:78: ui::WindowShowState state = window_->GetProperty(aura::client::kShowStateKey); On 2013/11/27 20:24:43, pkotwicz wrote: > In a separate CL we must absolutely kill this method. It is too confusing now > that snapped left / right are of state normal. > Yes. https://codereview.chromium.org/68033003/diff/740001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:167: // When a window |IsSnapped| it is auto-positioned by layout manager to stay On 2013/11/27 20:24:43, pkotwicz wrote: > You can actually kill the if statement completely. > WindowState::OnWindowPropertyChanged() currently always sets |window_show_type_| > to SHOW_TYPE_NORMAL when the show state is ui::SHOW_STATE_NORMAL. > > I suspect that WindowState::OnWindowPropertyChanged() should not have this > behavior but that's for a different CL. I don't think we can rely on WindowState::OnWindowPropertyChanged() getting invoked before other observers. Once we merge show type and show state this problem will not exist, but this is for another CL. https://codereview.chromium.org/68033003/diff/740001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): https://codereview.chromium.org/68033003/diff/740001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer_unittest.cc:638: // Test if the restore bounds is correct in multiple displays. On 2013/11/27 20:24:43, pkotwicz wrote: > Do we still need to clear the restored bounds if we are restoring the window > immediately afterwards? Done. https://codereview.chromium.org/68033003/diff/740001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer_unittest.cc:659: resizer->Drag(CalculateDragPoint(*resizer, 499, 00), 0); On 2013/11/27 20:24:43, pkotwicz wrote: > Nit: 00 -> 0 Done.
LGTM with nits https://codereview.chromium.org/68033003/diff/760001/ash/wm/dock/docked_windo... File ash/wm/dock/docked_window_resizer_unittest.cc (right): https://codereview.chromium.org/68033003/diff/760001/ash/wm/dock/docked_windo... ash/wm/dock/docked_window_resizer_unittest.cc:1330: EXPECT_EQ(ScreenAsh::GetDisplayWorkAreaBoundsInParent(w1.get()).height(), Nit: You could probably cache the work area bounds. https://codereview.chromium.org/68033003/diff/760001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/760001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:171: window_show_type_ = SHOW_TYPE_NORMAL; Nit: Let's set |window_show_type_| to SHOW_TYPE_NORMAL always. How about: "Set |window_show_type_| to SHOW_TYPE_NORMAL now so that an observer observing kShowStateKey gets the correct value when querying IsSnapped()." https://codereview.chromium.org/68033003/diff/760001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:281: // got snapped. How about: "Compute the bounds that the window will restore to. If the window does not already have restore bounds, the window will be restored to its current bounds when unsnapped." https://codereview.chromium.org/68033003/diff/760001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:284: SetRestoreBoundsInParent(bounds); How about: "Set the window's restore bounds so that WorkspaceLayoutManager knows which width to use for the snapped window." https://codereview.chromium.org/68033003/diff/760001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:301: // observing the WindowShowType change. How about: "If the window is a child of kShellWindowId_DockedContainer, the window's bounds are not updated as a result of OnWindowShowTypeChanged(). Set them here. Skip setting the bounds otherwise to avoid stopping the slide animation which was started as a result of OnWindowShowTypeChanged()." https://codereview.chromium.org/68033003/diff/760001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/760001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:409: // TODO(varkha): DockedWindowResizer sets |tracked_by_workspace_| to false Nit: |WindowState::tracked_by_workspace_| https://codereview.chromium.org/68033003/diff/760001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:410: // during the drag. The same should be true for WorkspaceWindowResizer and Nit: "The same should be true for WorkspaceWindowResizer. This will allow us to remove the check for window_resizer() to determine" ... https://codereview.chromium.org/68033003/diff/760001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.h (right): https://codereview.chromium.org/68033003/diff/760001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.h:90: // Restores side-snapped state when window or work area bounds change. How about: "Adjusts the window's bounds so that they are flush with the edge of the workspace if the window is side snapped."
https://codereview.chromium.org/68033003/diff/760001/ash/wm/dock/docked_windo... File ash/wm/dock/docked_window_resizer_unittest.cc (right): https://codereview.chromium.org/68033003/diff/760001/ash/wm/dock/docked_windo... ash/wm/dock/docked_window_resizer_unittest.cc:1330: EXPECT_EQ(ScreenAsh::GetDisplayWorkAreaBoundsInParent(w1.get()).height(), On 2013/11/28 06:15:10, pkotwicz wrote: > Nit: You could probably cache the work area bounds. Done. https://codereview.chromium.org/68033003/diff/760001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/68033003/diff/760001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:171: window_show_type_ = SHOW_TYPE_NORMAL; On 2013/11/28 06:15:10, pkotwicz wrote: > Nit: Let's set |window_show_type_| to SHOW_TYPE_NORMAL always. > How about: "Set |window_show_type_| to SHOW_TYPE_NORMAL now so that an observer > observing kShowStateKey gets the correct value when querying IsSnapped()." Done. https://codereview.chromium.org/68033003/diff/760001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:281: // got snapped. On 2013/11/28 06:15:10, pkotwicz wrote: > How about: "Compute the bounds that the window will restore to. If the window > does not already have restore bounds, the window will be restored to its current > bounds when unsnapped." Done (slightly modified). https://codereview.chromium.org/68033003/diff/760001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:284: SetRestoreBoundsInParent(bounds); On 2013/11/28 06:15:10, pkotwicz wrote: > How about: "Set the window's restore bounds so that WorkspaceLayoutManager knows > which width to use for the snapped window." Done. https://codereview.chromium.org/68033003/diff/760001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:301: // observing the WindowShowType change. On 2013/11/28 06:15:10, pkotwicz wrote: > How about: "If the window is a child of kShellWindowId_DockedContainer, the > window's bounds are not updated as a result of OnWindowShowTypeChanged(). Set > them here. Skip setting the bounds otherwise to avoid stopping the slide > animation which was started as a result of OnWindowShowTypeChanged()." Done. https://codereview.chromium.org/68033003/diff/760001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/760001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:409: // TODO(varkha): DockedWindowResizer sets |tracked_by_workspace_| to false On 2013/11/28 06:15:10, pkotwicz wrote: > Nit: |WindowState::tracked_by_workspace_| Done. https://codereview.chromium.org/68033003/diff/760001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:410: // during the drag. The same should be true for WorkspaceWindowResizer and On 2013/11/28 06:15:10, pkotwicz wrote: > Nit: "The same should be true for WorkspaceWindowResizer. This will allow us to > remove the check for window_resizer() to determine" ... Done. https://codereview.chromium.org/68033003/diff/760001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.h (right): https://codereview.chromium.org/68033003/diff/760001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.h:90: // Restores side-snapped state when window or work area bounds change. On 2013/11/28 06:15:10, pkotwicz wrote: > How about: "Adjusts the window's bounds so that they are flush with the edge of > the workspace if the window is side snapped." Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/68033003/910001
Message was sent while issue was closed.
Change committed as 237809
Message was sent while issue was closed.
https://codereview.chromium.org/68033003/diff/910001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/910001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:261: AdjustSnappedBounds(window_state, &bounds); varkha@, sorry to ask you about old CL. All tests passed without this. and I'm wondering what kind of operation requires this call. Do you remember?
Message was sent while issue was closed.
https://codereview.chromium.org/68033003/diff/910001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/910001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:261: AdjustSnappedBounds(window_state, &bounds); On 2014/02/12 01:43:41, oshima wrote: > varkha@, sorry to ask you about old CL. All tests passed without this. and I'm > wondering what kind of operation requires this call. Do you remember? I think this was to keep a window snapped when docked area width changes. It could be that this scenario is now handled differently. I will be back on Tue - will check then.
Message was sent while issue was closed.
https://codereview.chromium.org/68033003/diff/910001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/910001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:261: AdjustSnappedBounds(window_state, &bounds); I just checked. AdjustSnappedBounds() is similar to SetMaximizedOrFullscreenBounds(). Here is a scenario where we need this call: Dock window#1 right Snap window#2 right Undock window#1 Snapped window should move so that it is flush to the right edge of the workspace
Message was sent while issue was closed.
https://codereview.chromium.org/68033003/diff/910001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/910001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:135: AdjustSnappedBounds(window_state, &child_bounds); This is the one I had a question about. https://codereview.chromium.org/68033003/diff/910001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:261: AdjustSnappedBounds(window_state, &bounds); On 2014/02/12 03:14:52, pkotwicz wrote: > I just checked. > AdjustSnappedBounds() is similar to SetMaximizedOrFullscreenBounds(). > > Here is a scenario where we need this call: > Dock window#1 right > Snap window#2 right > Undock window#1 > Snapped window should move so that it is flush to the right edge of the > workspace Doh, sorry I put the comment in wrong place. See above.
Message was sent while issue was closed.
https://codereview.chromium.org/68033003/diff/910001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_layout_manager.cc (right): https://codereview.chromium.org/68033003/diff/910001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_layout_manager.cc:135: AdjustSnappedBounds(window_state, &child_bounds); AdjustSnappedBounds() is needed to prevent v2 apps from moving themselves via Javascript to an arbitrary position on screen when the app is snapped
Message was sent while issue was closed.
Message was sent while issue was closed.
On 2014/02/12 21:15:21, pkotwicz wrote: > https://codereview.chromium.org/68033003/diff/910001/ash/wm/workspace/workspa... > File ash/wm/workspace/workspace_layout_manager.cc (right): > > https://codereview.chromium.org/68033003/diff/910001/ash/wm/workspace/workspa... > ash/wm/workspace/workspace_layout_manager.cc:135: > AdjustSnappedBounds(window_state, &child_bounds); > AdjustSnappedBounds() is needed to prevent v2 apps from moving themselves via > Javascript to an arbitrary position on screen when the app is snapped Thanks. I'll add test (there is similar issue with docked state. just fyi). |
