Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23471004:
Only support left/right maximizing at 50% width when the --ash-enable-alternate-caption-button (Closed)
|
Created:
7 years, 3 months ago by pkotwicz Modified:
7 years, 3 months ago CC:
chromium-reviews, sadrul, dcheng, ben+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionOnly support left/right maximizing at 50% width when the --ash-enable-alternate-caption-button command line parameter is passed
BUG=180123
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222603
Patch Set 1 : #
Total comments: 8
Patch Set 2 : Patch is now friendlier to pending docking changes #
Total comments: 20
Patch Set 3 : Cleaned up unittests + varkha@'s suggestions #
Total comments: 4
Patch Set 4 : #
Total comments: 1
Patch Set 5 : #
Total comments: 6
Patch Set 6 : A lot less code :) #
Total comments: 2
Patch Set 7 : #
Total comments: 6
Patch Set 8 : #
Messages
Total messages: 17 (0 generated)
skuhne@, can you please take a look? I killed SnapSizer because I did not want to have another static class. I updated CanSnapWindow() because we can otherwise get some confusing behavior on the desktop with small screen resolutions.
I am not sure if the 50% also apply to window minimal / maximal sizes since that could be really bad looking at window defaults and so on. I will check with kuscher tomorrow. Beside that I have found some nits. Since the SnapSizer is getting that primitive it is surely a good idea to get rid of it. (besides I remember that I was debugging that thing for a lot of edge cases over time). https://codereview.chromium.org/23471004/diff/3001/ash/accelerators/accelerat... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/23471004/diff/3001/ash/accelerators/accelerat... ash/accelerators/accelerator_controller_unittest.cc:506: EXPECT_NE(window->bounds().ToString(), window_bounds.ToString()); Could you do some more tests towards the result (e.g. left side = 0, right side ~half width, ...)? https://codereview.chromium.org/23471004/diff/3001/ash/accelerators/accelerat... ash/accelerators/accelerator_controller_unittest.cc:509: EXPECT_NE(window->bounds().ToString(), window_bounds.ToString()); Same here. https://codereview.chromium.org/23471004/diff/3001/ash/wm/custom_frame_view_a... File ash/wm/custom_frame_view_ash_unittest.cc (right): https://codereview.chromium.org/23471004/diff/3001/ash/wm/custom_frame_view_a... ash/wm/custom_frame_view_ash_unittest.cc:252: EXPECT_EQ(target_bounds.ToString(), window->bounds().ToString()); again - better tests of results? https://codereview.chromium.org/23471004/diff/3001/ash/wm/window_util.cc File ash/wm/window_util.cc (right): https://codereview.chromium.org/23471004/diff/3001/ash/wm/window_util.cc#newc... ash/wm/window_util.cc:97: if (minimum_size.width() > snapped_size.width()) Hmm. Does this mean that if you have a window which cannot be half screen size it cannot L/R maximize? That is not good. Especially because we said one that Browser windows should have a generous minimum size (thinking about all our defaults of that windows come up maximized if the screen res is smaller then 1440 width and what not). As such: If we have this limitation build in there will be only a few things left which can be "maximized that way". Did Kuscher agree on that? There is no harm of having maximized windows be overlap if they need to be bigger... https://codereview.chromium.org/23471004/diff/3001/ash/wm/window_util_unittes... File ash/wm/window_util_unittest.cc (right): https://codereview.chromium.org/23471004/diff/3001/ash/wm/window_util_unittes... ash/wm/window_util_unittest.cc:37: EXPECT_EQ(gfx::Rect(0,0,250,kSnappedHeight).ToString(), 0, 0, 250, kSna.. (Spaces are missing) https://codereview.chromium.org/23471004/diff/3001/ash/wm/window_util_unittes... ash/wm/window_util_unittest.cc:40: EXPECT_EQ(gfx::Rect(250,0,250,kSnappedHeight).ToString(), Same here and more below. https://codereview.chromium.org/23471004/diff/3001/ash/wm/window_util_unittes... ash/wm/window_util_unittest.cc:77: EXPECT_EQ(gfx::Rect(750,kCenteredY,100,100).ToString(), And more spaces are missing.
varkha@, can you please take a look at how this integrates with the work wrt to docking that you are doing? skuhne@, I am not ready for another round of review yet. I need to clean up and add unittests
Thanks pkotwicz@ for unclashing the [still pending] docking CL. https://codereview.chromium.org/23471004/diff/10001/ash/accelerators/accelera... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/23471004/diff/10001/ash/accelerators/accelera... ash/accelerators/accelerator_controller.cc:839: using internal::SnapSizer; Is there a reason that ScopedLayerAnimationSettings::SetPreemptionStrategy(ui::LayerAnimator::REPLACE_QUEUED_ANIMATIONS); is not used here? Would it belong inside the SnapWindow()? https://codereview.chromium.org/23471004/diff/10001/ash/accelerators/accelera... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/23471004/diff/10001/ash/accelerators/accelera... ash/accelerators/accelerator_controller_unittest.cc:505: internal::SnapSizer snap_sizer_left(window.get(), "using internal::SnapSizer" could shorten this similar to the other place you did just that. https://codereview.chromium.org/23471004/diff/10001/ash/wm/custom_frame_view_... File ash/wm/custom_frame_view_ash_unittest.cc (left): https://codereview.chromium.org/23471004/diff/10001/ash/wm/custom_frame_view_... ash/wm/custom_frame_view_ash_unittest.cc:304: MAYBE_TouchDragResizeCloseToCornerDiffersFromMouse) { I understand that you don't need to test that touch is different from mouse anymore but do we have this touch coverage elsewhere? https://codereview.chromium.org/23471004/diff/10001/ash/wm/toplevel_window_ev... File ash/wm/toplevel_window_event_handler.cc (right): https://codereview.chromium.org/23471004/diff/10001/ash/wm/toplevel_window_ev... ash/wm/toplevel_window_event_handler.cc:304: } else { Did you leave out the x-velocity check intentionally? https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/frame_ma... File ash/wm/workspace/frame_maximize_button.cc (right): https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/frame_ma... ash/wm/workspace/frame_maximize_button.cc:463: return snap_sizer.target_bounds(); Does it not need to be in screen coordinates? I thought SnapSizer::target_bounds() uses parent coordinates. https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/frame_ma... ash/wm/workspace/frame_maximize_button.cc:495: ash::UMA_WINDOW_MAXIMIZE_BUTTON_MAXIMIZE_LEFT); It would be nice to add UMA metrics to workspace resizer. I think it would be useful to see if the split-screen layout is being used and how much - as well as which exact way of triggering it users use. I will do it in my CL if you think it belongs with the docking. https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/frame_ma... ash/wm/workspace/frame_maximize_button.cc:499: break; Would it be simpler and less code duplication if you drop the switch and use ?: for LEFT/RIGHT parameters? https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/snap_siz... File ash/wm/workspace/snap_sizer.h (right): https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/snap_siz... ash/wm/workspace/snap_sizer.h:23: // maximized or docked to the left or right side of the screen. Just "side-maximized" (with this CL). Thanks for breaking this up so that docking work does not clash as much anymore! https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/snap_siz... ash/wm/workspace/snap_sizer.h:48: // maximizing the window or docking it. just "side-maximizing" for now with this CL. You could leave TODO(varkha) and I will see how this plays with the docking. I may need a separate Can...() method after all to distinguish the docking case. https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/workspac... File ash/wm/workspace/workspace_window_resizer.cc (right): https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/workspac... ash/wm/workspace/workspace_window_resizer.cc:875: if (snap_type_ != last_type || Will the animations be smoother if we don't re-create the snap-sizer and / or snap_phantom_window_controller_ when alternating between left and right edges? https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/workspac... File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/workspac... ash/wm/workspace/workspace_window_resizer_unittest.cc:554: window_->bounds().ToString()); Does not this break if you restrict side-maximized behavior to 50%?
skuhne@, can you please take a look? Unfortunately, I cannot kill SnapSizer because we need it so that when a user drags a window to the edge of the screen and there is no dock visible, the phantom window cycles between side maximized and docked bounds. Ideally, the callers of SnapSizer::SnapWindow() will not know about whether the method will side maximize or dock the window. I did not add the methods related to side maximizing to window_util.cc because I want to keep the concept of "side maximized" private to SnapSizer. I expect that when the SnapSizer is constructed with SnapSizer::STEP_NO, the window will be side maximized if it is maximizable and docked otherwise. https://codereview.chromium.org/23471004/diff/3001/ash/wm/custom_frame_view_a... File ash/wm/custom_frame_view_ash_unittest.cc (right): https://codereview.chromium.org/23471004/diff/3001/ash/wm/custom_frame_view_a... ash/wm/custom_frame_view_ash_unittest.cc:252: EXPECT_EQ(target_bounds.ToString(), window->bounds().ToString()); I think this is good enough for these tests which are mostly for the sake of sanity. I added SnapSizerTest which tests the actual return value more thorougly. (In a similar way, we trust that wm::IsWindowMaximized() is sufficient for determining if the window is maximized here and do a more thorough test in ScreenAshTest.Bounds). https://codereview.chromium.org/23471004/diff/10001/ash/accelerators/accelera... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/23471004/diff/10001/ash/accelerators/accelera... ash/accelerators/accelerator_controller.cc:839: using internal::SnapSizer; Good catch! You are right that we probably should be animating snapping left and right. We do not animate when snapping left and right as a result of FrameMaximizeButton and we probably should. I need to figure out whether we want to animate as a result of side maximizing via WorkspaceWindowResizer In terms of AcceleratorController, we do not animate for WINDOW_POSITION_CENTER (via Alt+Shift+"=") So for better or for worse we are consistent there. (For the sake of fun, there is a completely unrelated partial maximize mode that you can enter by double clicking the borders of a window which is also not animated) https://codereview.chromium.org/23471004/diff/10001/ash/accelerators/accelera... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/23471004/diff/10001/ash/accelerators/accelera... ash/accelerators/accelerator_controller_unittest.cc:505: internal::SnapSizer snap_sizer_left(window.get(), I do not think that there is much gained via "using internal::SnapSizer" here. (It could be argued that there is not much gained in the other places I used "using internal::SnapSizer" either, but I would beg to differ) https://codereview.chromium.org/23471004/diff/10001/ash/wm/custom_frame_view_... File ash/wm/custom_frame_view_ash_unittest.cc (left): https://codereview.chromium.org/23471004/diff/10001/ash/wm/custom_frame_view_... ash/wm/custom_frame_view_ash_unittest.cc:304: MAYBE_TouchDragResizeCloseToCornerDiffersFromMouse) { Yes, we test snapping left and right via touch in CustomFrameViewAshTest.ResizeButtonDrag https://codereview.chromium.org/23471004/diff/10001/ash/wm/toplevel_window_ev... File ash/wm/toplevel_window_event_handler.cc (right): https://codereview.chromium.org/23471004/diff/10001/ash/wm/toplevel_window_ev... ash/wm/toplevel_window_event_handler.cc:304: } else { Good catch! https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/frame_ma... File ash/wm/workspace/frame_maximize_button.cc (right): https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/frame_ma... ash/wm/workspace/frame_maximize_button.cc:463: return snap_sizer.target_bounds(); Nice catch! https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/frame_ma... ash/wm/workspace/frame_maximize_button.cc:495: ash::UMA_WINDOW_MAXIMIZE_BUTTON_MAXIMIZE_LEFT); I think it belongs in a separate CL from this CL and a separate CL from the docking CL. I can write a CL for the sake of measuring UMA metrics. I also want to get UMA metrics for WorkspaceEventHandler::HandleVerticalResizeDoubleClick() https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/frame_ma... ash/wm/workspace/frame_maximize_button.cc:499: break; I think this way is clearer. Note that a different UMA metric is recorded for SNAP_LEFT and SNAP_RIGHT as well. https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/workspac... File ash/wm/workspace/workspace_window_resizer.cc (right): https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/workspac... ash/wm/workspace/workspace_window_resizer.cc:875: if (snap_type_ != last_type || I believe that you always transition through SNAP_NONE when alternating between the left and right edges. https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/workspac... File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): https://codereview.chromium.org/23471004/diff/10001/ash/wm/workspace/workspac... ash/wm/workspace/workspace_window_resizer_unittest.cc:554: window_->bounds().ToString()); Modified unittest https://codereview.chromium.org/23471004/diff/19001/ash/wm/workspace/frame_ma... File ash/wm/workspace/frame_maximize_button.cc (left): https://codereview.chromium.org/23471004/diff/19001/ash/wm/workspace/frame_ma... ash/wm/workspace/frame_maximize_button.cc:546: bool is_managed = ash::wm::IsWindowPositionManaged(window); This hack is no longer needed. I believe that this is because we do not have workspaces anymore (ie we do not reparent the window in the process of restoring the window from maximized) https://codereview.chromium.org/23471004/diff/19001/ash/wm/workspace/snap_siz... File ash/wm/workspace/snap_sizer.cc (right): https://codereview.chromium.org/23471004/diff/19001/ash/wm/workspace/snap_siz... ash/wm/workspace/snap_sizer.cc:48: int width = work_area.width() * kSideMaximizedScreenWidthPercent / 100; - I now make the side maximized window take the window's minimum width if that's bigger than 50%. https://codereview.chromium.org/23471004/diff/19001/ash/wm/workspace/snap_siz... ash/wm/workspace/snap_sizer.cc:74: SetRestoreBoundsInScreen(window, window->GetBoundsInScreen()); With the new caption button style we are removing the ability to restore the window to the restore bounds when side maximized. I am hoping that this will allow us to not muck around with the restore bounds as much as a result. https://codereview.chromium.org/23471004/diff/19001/ash/wm/workspace/snap_siz... File ash/wm/workspace/snap_sizer.h (right): https://codereview.chromium.org/23471004/diff/19001/ash/wm/workspace/snap_siz... ash/wm/workspace/snap_sizer.h:57: static void SnapWindow(aura::Window* window, I believe that passing in |step_behavior| is useful. It makes the expected behavior a lot more explicit. Previously, SnapWindow() would always go to the next size even though this was not always the desired effect. Going forward, we want the maximize button to only be able to side maximize the window and not dock the window. In particular, if the window is already side maximized, the side maximize button should not dock the window. https://codereview.chromium.org/23471004/diff/21001/ash/wm/workspace/frame_ma... File ash/wm/workspace/frame_maximize_button.cc (right): https://codereview.chromium.org/23471004/diff/21001/ash/wm/workspace/frame_ma... ash/wm/workspace/frame_maximize_button.cc:410: void FrameMaximizeButton::UpdateSnap(const gfx::Point& location) { I removed the now unused extra parameters from UpdateSnap().
skuhne@, ping?
Still waiting for kuscher to show up. Will ping him again...
Sorry for the long delay. Somehow I had in the back of my mind that you mentioned I shouldn't review just yet + I wanted to ask kuscher if this was what we wanted. I looked now at the code and have seen that you in fact did not change the meaning from before (so very sorry for that confusion). Nevertheless I have a few nits. https://codereview.chromium.org/23471004/diff/31001/ash/accelerators/accelera... File ash/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/23471004/diff/31001/ash/accelerators/accelera... ash/accelerators/accelerator_controller.cc:720: break; Can you remove this break? https://codereview.chromium.org/23471004/diff/31001/ash/accelerators/accelera... ash/accelerators/accelerator_controller.cc:724: break; ditto https://codereview.chromium.org/23471004/diff/31001/ash/accelerators/accelera... ash/accelerators/accelerator_controller.cc:728: break; And so on https://codereview.chromium.org/23471004/diff/31001/ash/wm/workspace/snap_siz... File ash/wm/workspace/snap_sizer.cc (right): https://codereview.chromium.org/23471004/diff/31001/ash/wm/workspace/snap_siz... ash/wm/workspace/snap_sizer.cc:50: width = std::max(width, window->delegate()->GetMinimumSize().width()); Just wondering: You are looking at the minimum size but not at the maximum size. Is that intentional? https://codereview.chromium.org/23471004/diff/31001/ash/wm/workspace/snap_siz... ash/wm/workspace/snap_sizer.cc:143: // capped to a size of 1. Is this a TODO? (since I cannot see step_behavior here used and I cannot see a TODO either). And somehow I thought there was somewhere mentioned that we only support half screens anyway? https://codereview.chromium.org/23471004/diff/31001/ash/wm/workspace/snap_siz... File ash/wm/workspace/snap_sizer.h (right): https://codereview.chromium.org/23471004/diff/31001/ash/wm/workspace/snap_siz... ash/wm/workspace/snap_sizer.h:35: enum StepBehavior { Could you please rename these to something better understandable and add some comments towards what these entries exactly mean?
skuhne@, can you please take another look?
lgtm
LGTM with a nit. https://codereview.chromium.org/23471004/diff/4020/ash/wm/workspace/snap_size... File ash/wm/workspace/snap_sizer.cc (right): https://codereview.chromium.org/23471004/diff/4020/ash/wm/workspace/snap_size... ash/wm/workspace/snap_sizer.cc:90: // restore, and close). nit: Instead of repeating the comment maybe just refer to see the comment above in GetDefaultWidth.
jamescook@, can you please take a look? I made a couple of changes you suggested in https://codereview.chromium.org/23645009/ https://codereview.chromium.org/23471004/diff/4020/ash/wm/workspace/snap_size... File ash/wm/workspace/snap_sizer.cc (right): https://codereview.chromium.org/23471004/diff/4020/ash/wm/workspace/snap_size... ash/wm/workspace/snap_sizer.cc:90: // restore, and close). I do not think that this comment is long enough (alternatively that "See comment for GetDefaultWidth() is short enough) to validate this change.
James, can you please take a look? (I forgot to add you as a reviewer)
LGTM with an optional idea and an unrelated question. https://codereview.chromium.org/23471004/diff/50001/ash/wm/workspace/snap_siz... File ash/wm/workspace/snap_sizer.cc (right): https://codereview.chromium.org/23471004/diff/50001/ash/wm/workspace/snap_siz... ash/wm/workspace/snap_sizer.cc:213: if (current < 0) Ah, thanks for adding this. https://codereview.chromium.org/23471004/diff/50001/ash/wm/workspace/snap_siz... File ash/wm/workspace/snap_sizer_unittest.cc (right): https://codereview.chromium.org/23471004/diff/50001/ash/wm/workspace/snap_siz... ash/wm/workspace/snap_sizer_unittest.cc:120: 921, optional idea: what do you think of using kWorkAreaBounds * 9 / 10 here? harrym had a hard time getting tests fixed when we made some changes that altered the work area size. Admittedly the width of the work area is unlikely to change, but it might save us from updating this test later. https://codereview.chromium.org/23471004/diff/50001/ash/wm/workspace/snap_siz... ash/wm/workspace/snap_sizer_unittest.cc:174: } Nice test coverage. https://codereview.chromium.org/23471004/diff/50001/ash/wm/workspace/snap_siz... ash/wm/workspace/snap_sizer_unittest.cc:201: // caption button style, a second call to SnapSizer::SnapWindow() should have Random UX question not related to this CL. Have we considered using different snap behavior for keyboard vs. mouse triggered snap? I could imagine a power-user feature that lets you cycle through different window widths by hitting the snap key multiple times. It might be kind of bizarre to start with 50% left then get wider if you snap left again again, though.
https://codereview.chromium.org/23471004/diff/50001/ash/wm/workspace/snap_siz... File ash/wm/workspace/snap_sizer_unittest.cc (right): https://codereview.chromium.org/23471004/diff/50001/ash/wm/workspace/snap_siz... ash/wm/workspace/snap_sizer_unittest.cc:120: 921, On 2013/09/10 20:49:08, James Cook wrote: > optional idea: what do you think of using kWorkAreaBounds * 9 / 10 here? harrym > had a hard time getting tests fixed when we made some changes that altered the > work area size. Admittedly the width of the work area is unlikely to change, > but it might save us from updating this test later. Done. https://codereview.chromium.org/23471004/diff/50001/ash/wm/workspace/snap_siz... ash/wm/workspace/snap_sizer_unittest.cc:201: // caption button style, a second call to SnapSizer::SnapWindow() should have I agree that this may be a good idea. I think that power users are well represented within ChromeOS users at Google so I am confident I will get a bug assigned to me if a power user wants the feature. (If you want the feature please file a bug and I will ping kuscher@ on his thoughts)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/23471004/60001
Message was sent while issue was closed.
Change committed as 222603 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
