Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
668513003:
Selecting an app window from the overview mode maximizes the window only if it's maximizable (Closed)
DescriptionAthena AppActivities should open maximized unless they specify otherwise.
Selecting an app window from the overview mode maximizes the window only if it's maximizable.
A change proposal to fix the below 2 bugs: - crbug.com/424710 - crbug.com/424708
R=oshima@chromium.org
BUG=424710, 424708
TEST=Manual & athena_unittests @ OnSelectWindow
Committed: https://crrev.com/63a5975ca272b44c8209055f38605e6598b6142d
Cr-Commit-Position: refs/heads/master@{#301438}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Changed the previous fix according to suggested review #
Total comments: 18
Patch Set 3 : Acknowledging the code review and fixing other relevant bugs related to the fix and split mode. #
Total comments: 11
Patch Set 4 : Made the suggested changes and fixed the test #Patch Set 5 : Now using IsWindowInList. #
Total comments: 11
Patch Set 6 : Following the review comments #
Total comments: 2
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Messages
Total messages: 23 (3 generated)
The CQ bit was checked by afakhry@chromium.org
The CQ bit was unchecked by afakhry@chromium.org
please update BUG= in the description, and add unittest for 424710. https://codereview.chromium.org/668513003/diff/1/athena/extensions/athena_nat... File athena/extensions/athena_native_app_window_views.cc (right): https://codereview.chromium.org/668513003/diff/1/athena/extensions/athena_nat... athena/extensions/athena_native_app_window_views.cc:16: void AthenaNativeAppWindowViews::InitializeWindow( I think we need to handle this in different way. Let's discuss offline, and keep this separated from this CL https://codereview.chromium.org/668513003/diff/1/athena/wm/window_manager_imp... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/668513003/diff/1/athena/wm/window_manager_imp... athena/wm/window_manager_impl.cc:310: bool can_maximize = window->GetProperty(aura::client::kCanMaximizeKey); it should check both can maximize and if it has maximium size.
The second iteration of the patch
Updating description
https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:50: delegate ? delegate->GetMaximumSize().IsEmpty() : true; nit: how about bool no_max_size = !delegate || delegate->GetMaximiumSize().IsEmpty(); return no_max_size && window->GetProperty(aura::client::kCanMaximizeKey); https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:153: if(visible && CanWindowMaximize(child)) nit: you need {} in this case https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... File athena/wm/window_manager_unittest.cc (right): https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:124: // Get the current bounds nit: you don't need this comment as it's obvious https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:129: // Get the screen bounds ditto https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:133: // Enter the overview mode and select window 1 (has a max size). This is probably simpler. // Select w1 which has a max size in the overview mode and make sure it's not maximized. https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:138: // Test that the window was not maximized and remove this comment. https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:141: EXPECT_NE(work_area, new_bounds1); can you check using string (ToString)? That'll give nicer message when it fails. same for other conditions. https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:144: // maximized). ditto https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:154: // maximized). ditto
https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:50: delegate ? delegate->GetMaximumSize().IsEmpty() : true; On 2014/10/23 01:05:46, oshima wrote: > nit: how about > > bool no_max_size = !delegate || delegate->GetMaximiumSize().IsEmpty(); > > return no_max_size && window->GetProperty(aura::client::kCanMaximizeKey); Acknowledged. https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:153: if(visible && CanWindowMaximize(child)) On 2014/10/23 01:05:46, oshima wrote: > nit: you need {} in this case Acknowledged. https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... File athena/wm/window_manager_unittest.cc (right): https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:124: // Get the current bounds On 2014/10/23 01:05:46, oshima wrote: > nit: you don't need this comment as it's obvious Acknowledged. https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:129: // Get the screen bounds On 2014/10/23 01:05:46, oshima wrote: > ditto Acknowledged. https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:133: // Enter the overview mode and select window 1 (has a max size). On 2014/10/23 01:05:46, oshima wrote: > This is probably simpler. > > // Select w1 which has a max size in the overview mode and make sure it's not > maximized. Acknowledged. https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:138: // Test that the window was not maximized On 2014/10/23 01:05:46, oshima wrote: > and remove this comment. Acknowledged. https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:141: EXPECT_NE(work_area, new_bounds1); On 2014/10/23 01:05:46, oshima wrote: > can you check using string (ToString)? That'll give nicer message when it fails. > same for other conditions. Acknowledged. https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:144: // maximized). On 2014/10/23 01:05:46, oshima wrote: > ditto Acknowledged. https://codereview.chromium.org/668513003/diff/20001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:154: // maximized). On 2014/10/23 01:05:46, oshima wrote: > ditto Acknowledged.
you should use "Done" when you addressed the comment. Ack for reviewers. https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:52: window->GetProperty(aura::client::kCanResizeKey)); nit: we don't use outmost () unless it's complicated. https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:107: if (CanWindowMaximize(window)) else if https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:111: if (CanWindowMaximize(window)) ditto https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:167: } Can you add // TODO(oshima): Fix WindowListProvider::InWindowInList and use it instead. https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... File athena/wm/window_manager_unittest.cc (right): https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:411: EXPECT_FALSE(w1->IsVisible()); can you activate w1 and check if it's properly resized?
https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:167: } On 2014/10/24 18:39:56, oshima wrote: > Can you add > > // TODO(oshima): Fix WindowListProvider::InWindowInList and use it instead. > Actually, you should be able to just use InWnidowInList now. Let me know if it doesn't work.
https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:52: window->GetProperty(aura::client::kCanResizeKey)); On 2014/10/24 18:39:55, oshima wrote: > nit: we don't use outmost () unless it's complicated. Done. https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:107: if (CanWindowMaximize(window)) On 2014/10/24 18:39:56, oshima wrote: > else if Done. https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:111: if (CanWindowMaximize(window)) On 2014/10/24 18:39:56, oshima wrote: > ditto Done. https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... athena/wm/window_manager_impl.cc:167: } On 2014/10/24 18:39:56, oshima wrote: > Can you add > > // TODO(oshima): Fix WindowListProvider::InWindowInList and use it instead. > Done. https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... File athena/wm/window_manager_unittest.cc (right): https://codereview.chromium.org/668513003/diff/40001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:411: EXPECT_FALSE(w1->IsVisible()); On 2014/10/24 18:39:56, oshima wrote: > can you activate w1 and check if it's properly resized? Done.
https://codereview.chromium.org/668513003/diff/80001/athena/wm/window_manager... File athena/wm/window_manager_unittest.cc (right): https://codereview.chromium.org/668513003/diff/80001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:396: w2->SetProperty(aura::client::kCanMaximizeKey, true); you can set this in CreateAndActivateWindow https://codereview.chromium.org/668513003/diff/80001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:418: w1->Show(); I assume wm::ActivateWindow(w1.get()) didn't work, correct? https://codereview.chromium.org/668513003/diff/80001/ui/views/widget/widget_d... File ui/views/widget/widget_delegate.cc (right): https://codereview.chromium.org/668513003/diff/80001/ui/views/widget/widget_d... ui/views/widget/widget_delegate.cc:55: return true; you don't need these change right?
https://codereview.chromium.org/668513003/diff/80001/athena/wm/window_manager... File athena/wm/window_manager_unittest.cc (right): https://codereview.chromium.org/668513003/diff/80001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:396: w2->SetProperty(aura::client::kCanMaximizeKey, true); On 2014/10/25 01:35:43, oshima wrote: > you can set this in CreateAndActivateWindow I wanted to do this, but I was afraid that it might affect other tests. It doesn't, so consider it done. https://codereview.chromium.org/668513003/diff/80001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:396: w2->SetProperty(aura::client::kCanMaximizeKey, true); On 2014/10/25 01:35:43, oshima wrote: > you can set this in CreateAndActivateWindow Done. https://codereview.chromium.org/668513003/diff/80001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:418: w1->Show(); On 2014/10/25 01:35:43, oshima wrote: > I assume wm::ActivateWindow(w1.get()) didn't work, correct? It actually works but it doesn't make the window visible. Even in WindowManagerImpl::OnSelectWindow() window activation and showing are two separate operations. Would you like me to use wm::ActivateWindow() instead? https://codereview.chromium.org/668513003/diff/80001/ui/views/widget/widget_d... File ui/views/widget/widget_delegate.cc (right): https://codereview.chromium.org/668513003/diff/80001/ui/views/widget/widget_d... ui/views/widget/widget_delegate.cc:55: return true; On 2014/10/25 01:35:43, oshima wrote: > you don't need these change right? We need these changes, because returning false will always prevent web activities windows from maximizing after toggling split mode off.
https://codereview.chromium.org/668513003/diff/80001/athena/wm/window_manager... File athena/wm/window_manager_unittest.cc (right): https://codereview.chromium.org/668513003/diff/80001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:418: w1->Show(); On 2014/10/27 16:10:05, afakhry wrote: > On 2014/10/25 01:35:43, oshima wrote: > > I assume wm::ActivateWindow(w1.get()) didn't work, correct? > > It actually works but it doesn't make the window visible. Even in > WindowManagerImpl::OnSelectWindow() window activation and showing are two > separate operations. That's what I expected and meant by "doesn't work". > > Would you like me to use wm::ActivateWindow() instead? No, this is ok for now. Can you add the following? // TODO(oshima): To change to wm::ActivateWindow once the correct window state is implemented. https://codereview.chromium.org/668513003/diff/80001/ui/views/widget/widget_d... File ui/views/widget/widget_delegate.cc (right): https://codereview.chromium.org/668513003/diff/80001/ui/views/widget/widget_d... ui/views/widget/widget_delegate.cc:55: return true; On 2014/10/27 16:10:06, afakhry wrote: > On 2014/10/25 01:35:43, oshima wrote: > > you don't need these change right? > > We need these changes, because returning false will always prevent web > activities windows from maximizing after toggling split mode off. If that's the case, please change ActivityWidgetDelegate instead.
https://codereview.chromium.org/668513003/diff/80001/athena/wm/window_manager... File athena/wm/window_manager_unittest.cc (right): https://codereview.chromium.org/668513003/diff/80001/athena/wm/window_manager... athena/wm/window_manager_unittest.cc:418: w1->Show(); On 2014/10/27 18:20:53, oshima wrote: > On 2014/10/27 16:10:05, afakhry wrote: > > On 2014/10/25 01:35:43, oshima wrote: > > > I assume wm::ActivateWindow(w1.get()) didn't work, correct? > > > > It actually works but it doesn't make the window visible. Even in > > WindowManagerImpl::OnSelectWindow() window activation and showing are two > > separate operations. > > That's what I expected and meant by "doesn't work". > > > > > Would you like me to use wm::ActivateWindow() instead? > > No, this is ok for now. Can you add the following? > > // TODO(oshima): To change to wm::ActivateWindow once the correct window state > is implemented. > Done. https://codereview.chromium.org/668513003/diff/80001/ui/views/widget/widget_d... File ui/views/widget/widget_delegate.cc (right): https://codereview.chromium.org/668513003/diff/80001/ui/views/widget/widget_d... ui/views/widget/widget_delegate.cc:55: return true; On 2014/10/27 18:20:53, oshima wrote: > On 2014/10/27 16:10:06, afakhry wrote: > > On 2014/10/25 01:35:43, oshima wrote: > > > you don't need these change right? > > > > We need these changes, because returning false will always prevent web > > activities windows from maximizing after toggling split mode off. > > If that's the case, please change ActivityWidgetDelegate instead. Done.
https://codereview.chromium.org/668513003/diff/100001/athena/activity/activit... File athena/activity/activity_widget_delegate.h (right): https://codereview.chromium.org/668513003/diff/100001/athena/activity/activit... athena/activity/activity_widget_delegate.h:31: // Returns true if the window can be resized. you don't need comment for overridden methods. Just put them in the same order as views::WidgetDelegate (which means above |GetWindowTitle()| with no lines between. Please sort the method body in the same order.
https://codereview.chromium.org/668513003/diff/100001/athena/activity/activit... File athena/activity/activity_widget_delegate.h (right): https://codereview.chromium.org/668513003/diff/100001/athena/activity/activit... athena/activity/activity_widget_delegate.h:31: // Returns true if the window can be resized. On 2014/10/27 19:12:27, oshima wrote: > you don't need comment for overridden methods. Just put them in the same order > as views::WidgetDelegate (which means above |GetWindowTitle()| with no lines > between. Please sort the method body in the same order. Done.
lgtm https://codereview.chromium.org/668513003/diff/120001/athena/wm/window_manage... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/668513003/diff/120001/athena/wm/window_manage... athena/wm/window_manager_impl.cc:109: window->SetBounds(gfx::Rect(work_area)); nit: you need {} in this case.
https://codereview.chromium.org/668513003/diff/120001/athena/wm/window_manage... File athena/wm/window_manager_impl.cc (right): https://codereview.chromium.org/668513003/diff/120001/athena/wm/window_manage... athena/wm/window_manager_impl.cc:109: window->SetBounds(gfx::Rect(work_area)); On 2014/10/27 19:41:13, oshima wrote: > nit: you need {} in this case. Done.
The CQ bit was checked by afakhry@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/668513003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/63a5975ca272b44c8209055f38605e6598b6142d Cr-Commit-Position: refs/heads/master@{#301438} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
