Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
557693002:
[Ash] Only snap windows that can maximize. (Closed)
Description[Ash] Only snap windows that can maximize.
Previously, windows that have only one of maxWidth or maxHeight defined
could not be maximized by the user, but they would still snap when
dragged to the edge of the screen.
This changes WindowState::CanSnap to use CanMaximize which is false
when there is a maxWidth or maxHeight, or when the window is not
resizable.
BUG=412610, 408737
Committed: https://crrev.com/578fb750af5d9a810f671eba83a3cc4c14a7792a
Cr-Commit-Position: refs/heads/master@{#295657}
Patch Set 1 #Patch Set 2 : Update tests. #
Total comments: 6
Patch Set 3 : Address comments #
Total comments: 4
Patch Set 4 : Address comments. #Patch Set 5 : Sync and rebase #Patch Set 6 : Fix up other tests. #
Total comments: 2
Patch Set 7 : Change ash_test_base.cc #
Total comments: 5
Patch Set 8 : Sync and rebase #Patch Set 9 : Return true if there is no delegate. #
Messages
Total messages: 26 (3 generated)
jackhou@chromium.org changed reviewers: + oshima@chromium.org
oshima, could you please review this?
can you update the bug #?
On 2014/09/09 18:30:26, oshima wrote: > can you update the bug #? Done. I couldn't find an existing bug for this so filed a new one. http://crbug.com/412610
thanks. change lg. can you add test for this scenario? (I guess there is already a test that you need to update, but not 100% sure)
https://codereview.chromium.org/557693002/diff/20001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): https://codereview.chromium.org/557693002/diff/20001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer_unittest.cc:1419: window_->SetProperty(aura::client::kCanMaximizeKey, true); This test fails at line 1444. I'm not sure exactly why, it seems like it should still pass. Adding this line fixes it. Maybe there's something I'm missing?
https://codereview.chromium.org/557693002/diff/20001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/557693002/diff/20001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:144: return window_->GetProperty(aura::client::kCanMaximizeKey); can you add GetMaximizedSize check here? I think that's the correct behavior. https://codereview.chromium.org/557693002/diff/20001/ash/wm/window_state_unit... File ash/wm/window_state_unittest.cc (left): https://codereview.chromium.org/557693002/diff/20001/ash/wm/window_state_unit... ash/wm/window_state_unittest.cc:128: delegate.set_minimum_size(gfx::Size()); and keep this scenario. https://codereview.chromium.org/557693002/diff/20001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): https://codereview.chromium.org/557693002/diff/20001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer_unittest.cc:559: window_->SetProperty(aura::client::kCanMaximizeKey, true); this is because this CL changed the default value.
https://codereview.chromium.org/557693002/diff/20001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/557693002/diff/20001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:144: return window_->GetProperty(aura::client::kCanMaximizeKey); On 2014/09/10 15:59:31, oshima wrote: > can you add GetMaximizedSize check here? I think that's the correct behavior. Done. https://codereview.chromium.org/557693002/diff/20001/ash/wm/window_state_unit... File ash/wm/window_state_unittest.cc (left): https://codereview.chromium.org/557693002/diff/20001/ash/wm/window_state_unit... ash/wm/window_state_unittest.cc:128: delegate.set_minimum_size(gfx::Size()); On 2014/09/10 15:59:32, oshima wrote: > and keep this scenario. Done.
https://codereview.chromium.org/557693002/diff/40001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/557693002/diff/40001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:150: return window_->GetProperty(aura::client::kCanMaximizeKey); This should be window->GetProperty(aura::client::kCanMaximizeKey) && (delegate && delegate->GetMaximumSize().IsEmpty()); https://codereview.chromium.org/557693002/diff/40001/ash/wm/window_state_unit... File ash/wm/window_state_unittest.cc (right): https://codereview.chromium.org/557693002/diff/40001/ash/wm/window_state_unit... ash/wm/window_state_unittest.cc:132: EXPECT_FALSE(window_state->CanSnap()); I probably wasn't clear, sorry: You should also test: delegate.set_maximum_size(gfx::Size()); window->SetProperty(aura::client::kCanMaximizeKey, false) EXPECT_FALSE(window_state->CanSnap());
https://codereview.chromium.org/557693002/diff/40001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/557693002/diff/40001/ash/wm/window_state.cc#n... ash/wm/window_state.cc:150: return window_->GetProperty(aura::client::kCanMaximizeKey); On 2014/09/11 01:00:05, oshima wrote: > This should be > > window->GetProperty(aura::client::kCanMaximizeKey) && > (delegate && delegate->GetMaximumSize().IsEmpty()); Ah I see. BTW, GetMaximumSize will return 0x200 if only the height is set. IsEmpty is true in this case, even though the size is constrained. https://codereview.chromium.org/557693002/diff/40001/ash/wm/window_state_unit... File ash/wm/window_state_unittest.cc (right): https://codereview.chromium.org/557693002/diff/40001/ash/wm/window_state_unit... ash/wm/window_state_unittest.cc:132: EXPECT_FALSE(window_state->CanSnap()); On 2014/09/11 01:00:05, oshima wrote: > I probably wasn't clear, sorry: > > You should also test: > > delegate.set_maximum_size(gfx::Size()); > window->SetProperty(aura::client::kCanMaximizeKey, false) > EXPECT_FALSE(window_state->CanSnap()); Done.
lgtm
While updating the affected tests, I realised it might be better to change ash_test_base.cc instead of each test (compare Patch Set 6 vs 7). WDYT?
oshima@chromium.org changed reviewers: + skuhne@chromium.org
+skuhne for maximized_mode_window_manager_unittest.cc change. path set 6 doesn't seem to have a ash_test_base change. https://codereview.chromium.org/557693002/diff/100001/ash/display/display_con... File ash/display/display_controller_unittest.cc (right): https://codereview.chromium.org/557693002/diff/100001/ash/display/display_con... ash/display/display_controller_unittest.cc:561: gfx::Rect())); what kind of error you had? https://codereview.chromium.org/557693002/diff/120001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc (left): https://codereview.chromium.org/557693002/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc:123: delegate = aura::test::TestWindowDelegate::CreateSelfDestroyingDelegate(); I wonder why this is done only when the window can't be maximzied. stefan?
> path set 6 doesn't seem to have a ash_test_base change. Sorry I meant Patch Set 7 has the change to ash_test_base. Patch Set 6 had changes to the individual tests. I think Patch Set 7 is better but I wanted to check that you're okay with it. https://codereview.chromium.org/557693002/diff/100001/ash/display/display_con... File ash/display/display_controller_unittest.cc (right): https://codereview.chromium.org/557693002/diff/100001/ash/display/display_con... ash/display/display_controller_unittest.cc:561: gfx::Rect())); On 2014/09/11 17:25:49, oshima wrote: > what kind of error you had? Basically they'd fail because CanMaximize is false is the delegate is null.
Please see comments. https://codereview.chromium.org/557693002/diff/120001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc (left): https://codereview.chromium.org/557693002/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc:123: delegate = aura::test::TestWindowDelegate::CreateSelfDestroyingDelegate(); There are two reasons for this delegate: 1. Allow to specify a maximum size for a window which is smaller then the screen and / or something which prevents the window from maximizing. 2. There is a drag test which makes sure that the non maximizable window in the "maximize mode state" cannot be dragged. Thus it'll return HTCAPTION as hit test since that would allow to do so. Each of these attributes could be a problem for other tests and as such this is an optional delegate. However - it appears that none of the unit tests fails (yet) by always doing it, so it might be (for now) okay to do so, but somehow I like it cleaner and only do this when needed. So I would keep it as is - unless there is a particular reason for removing? https://codereview.chromium.org/557693002/diff/120001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/557693002/diff/120001/ash/wm/window_state.cc#... ash/wm/window_state.cc:149: A window which can be maximized and has a maximum size beyond the x and y extent of the screen is potentially be maximizable. Is it not?
I'll take a look at the new patch a bit later today. https://codereview.chromium.org/557693002/diff/120001/ash/wm/window_state.cc File ash/wm/window_state.cc (right): https://codereview.chromium.org/557693002/diff/120001/ash/wm/window_state.cc#... ash/wm/window_state.cc:149: On 2014/09/11 20:24:17, Mr4D wrote: > A window which can be maximized and has a maximum size beyond the x and y extent > of the screen is potentially be maximizable. Is it not? potentially yes, but that conflicts when the display size changes. I think it's safe to assume that if an application has a limit on size, it does not want to be maximized.
Ping.
i'm ok with ash_test_base change. Please address skuhe@'s comment.
PTAL I realised that the original CanSnap returns true if there is no delegate, so CanMaximize should do the same. This conveniently reduces the changes that need to be made to the tests. Sorry to keep changing this =S https://codereview.chromium.org/557693002/diff/120001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc (left): https://codereview.chromium.org/557693002/diff/120001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc:123: delegate = aura::test::TestWindowDelegate::CreateSelfDestroyingDelegate(); On 2014/09/11 20:24:17, Mr4D wrote: > There are two reasons for this delegate: > 1. Allow to specify a maximum size for a window which is smaller then the screen > and / or something which prevents the window from maximizing. > 2. There is a drag test which makes sure that the non maximizable window in the > "maximize mode state" cannot be dragged. Thus it'll return HTCAPTION as hit test > since that would allow to do so. > > Each of these attributes could be a problem for other tests and as such this is > an optional delegate. However - it appears that none of the unit tests fails > (yet) by always doing it, so it might be (for now) okay to do so, but somehow I > like it cleaner and only do this when needed. So I would keep it as is - unless > there is a particular reason for removing? Done.
lgtm
lgtm
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/557693002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as 774c88289397d100b9fc3557becef7f96896c2b9
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/578fb750af5d9a810f671eba83a3cc4c14a7792a Cr-Commit-Position: refs/heads/master@{#295657} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
