Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829226:
Fixing many problems around the maximize bubble (Closed)
|
Created:
8 years, 4 months ago by Mr4D (OOO till 08-26) Modified:
8 years, 4 months ago CC:
chromium-reviews, sadrul, ben+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFixing many problems around the maximize bubble
- Text for default action was not properly presented before
- Crash on minimize of file dialog
- Clicking in tip area of button did not do anything
- Added padding for text
- Launcher did show gray background upon maximize/minimize
- Added requested animations
BUG=140834, 140958, 140840
TEST=Visual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150788
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressed first review #Patch Set 3 : Addressed simplification request #Patch Set 4 : Fixed build problem #
Total comments: 2
Patch Set 5 : Addressed review #Patch Set 6 : Removed workaround visibility #
Total comments: 2
Patch Set 7 : Fixed last comment #Patch Set 8 : Fixed crash from photo editor #Patch Set 9 : Removed merge problem: Header file was removed #
Messages
Total messages: 20 (0 generated)
Sky, please have a look!
http://codereview.chromium.org/10829226/diff/1/ash/wm/maximize_bubble_control... File ash/wm/maximize_bubble_controller.cc (right): http://codereview.chromium.org/10829226/diff/1/ash/wm/maximize_bubble_control... ash/wm/maximize_bubble_controller.cc:75: // Get the mouse active area of the window. Move this above overriden methods. http://codereview.chromium.org/10829226/diff/1/ash/wm/maximize_bubble_control... ash/wm/maximize_bubble_controller.cc:224: // Since the window delegate does not support horizontal shifts, this This comment doesn't make any sense. Either way, move the comment to the implementation so that overriden stays above the method. http://codereview.chromium.org/10829226/diff/1/ash/wm/maximize_bubble_control... ash/wm/maximize_bubble_controller.cc:402: initial_position_ = bubble_widget_->GetNativeWindow()->bounds(); Do you really need this here since you only care about initial_position_ when shutting down? http://codereview.chromium.org/10829226/diff/1/ash/wm/maximize_bubble_control... ash/wm/maximize_bubble_controller.cc:438: int shift = ceil((1.0 - animation->GetCurrentValue()) * animation->CurrentValueBetween(...); http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/frame_maximiz... File ash/wm/workspace/frame_maximize_button.cc (right): http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/frame_maximiz... ash/wm/workspace/frame_maximize_button.cc:116: maximizer_.reset(); Add a comment as to why this needs to be first. http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/frame_maximiz... ash/wm/workspace/frame_maximize_button.cc:155: // SNAP_MINIMIZE might destroy |this| for windows like the file manager. This is too fragile. Can't the code be like ProcessEndEvent? In fact can't we make the two the same? http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/workspace_man... File ash/wm/workspace/workspace_manager.cc (right): http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/workspace_man... ash/wm/workspace/workspace_manager.cc:155: wm::IsWindowMinimized(*i)) minimized windows should be hidden. So, I'm not sure why this is needed.
Addressed. Please have another look! http://codereview.chromium.org/10829226/diff/1/ash/wm/maximize_bubble_control... File ash/wm/maximize_bubble_controller.cc (right): http://codereview.chromium.org/10829226/diff/1/ash/wm/maximize_bubble_control... ash/wm/maximize_bubble_controller.cc:75: // Get the mouse active area of the window. On 2012/08/07 23:11:18, sky wrote: > Move this above overriden methods. Done. http://codereview.chromium.org/10829226/diff/1/ash/wm/maximize_bubble_control... ash/wm/maximize_bubble_controller.cc:224: // Since the window delegate does not support horizontal shifts, this On 2012/08/07 23:11:18, sky wrote: > This comment doesn't make any sense. Either way, move the comment to the > implementation so that overriden stays above the method. Done. http://codereview.chromium.org/10829226/diff/1/ash/wm/maximize_bubble_control... ash/wm/maximize_bubble_controller.cc:402: initial_position_ = bubble_widget_->GetNativeWindow()->bounds(); On 2012/08/07 23:11:18, sky wrote: > Do you really need this here since you only care about initial_position_ when > shutting down? Done. http://codereview.chromium.org/10829226/diff/1/ash/wm/maximize_bubble_control... ash/wm/maximize_bubble_controller.cc:438: int shift = ceil((1.0 - animation->GetCurrentValue()) * On 2012/08/07 23:11:18, sky wrote: > animation->CurrentValueBetween(...); Done. http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/frame_maximiz... File ash/wm/workspace/frame_maximize_button.cc (right): http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/frame_maximiz... ash/wm/workspace/frame_maximize_button.cc:116: maximizer_.reset(); On 2012/08/07 23:11:18, sky wrote: > Add a comment as to why this needs to be first. Done. http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/frame_maximiz... ash/wm/workspace/frame_maximize_button.cc:155: // SNAP_MINIMIZE might destroy |this| for windows like the file manager. Okay. Changed it. However - I did not move the cases together because the ProcessEndEvent is clearly only meant for touch/drag operations and adding a flag to avoid most actions there is clearly not desirable. http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/workspace_man... File ash/wm/workspace/workspace_manager.cc (right): http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/workspace_man... ash/wm/workspace/workspace_manager.cc:155: wm::IsWindowMinimized(*i)) If the window is maximized and is transformed directly into the minimized state this seems not to apply. It might be because of animations going on?
http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/workspace_man... File ash/wm/workspace/workspace_manager.cc (right): http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/workspace_man... ash/wm/workspace/workspace_manager.cc:155: wm::IsWindowMinimized(*i)) On 2012/08/08 01:02:28, Mr4D wrote: > If the window is maximized and is transformed directly into the minimized state > this seems not to apply. It might be because of animations going on? The target visibility should be 0. Do you have a test case I can try? http://codereview.chromium.org/10829226/diff/7002/ash/wm/workspace/frame_maxi... File ash/wm/workspace/frame_maximize_button.cc (right): http://codereview.chromium.org/10829226/diff/7002/ash/wm/workspace/frame_maxi... ash/wm/workspace/frame_maximize_button.cc:335: snap_sizer_.reset(); Persisting the snap_sizer_ makes me nervous. In particular I'm worried we might not reset snap_sizer_ at the appropriate time resulting it in containing the wrong value. Do we need to reset snap_sizer_ in DestroyMaximizeMenu now?
Please have a look and / or please come over so that we can quickly go over your comments! http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/workspace_man... File ash/wm/workspace/workspace_manager.cc (right): http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/workspace_man... ash/wm/workspace/workspace_manager.cc:155: wm::IsWindowMinimized(*i)) I debugged it and here is what I see: GetLayerTargetVisibility(): true GetTargetOpacity(): 1.0 IsWindowMinimized: true Looking at the code I can see that WorkspaceManager::SetVisibilityOfWorkspaceWindows gets called with no active windows in the workspace. I can also see that the window gets explicitly set to visible. I looked through the callstack but after a while I realized that there are many dependencies I don't know yet and changing there something seems dangerous. But here is a clue: When first normalizing the window and then minimizing it works. I assume that the changed order of calls with the also disappearing bubble must confuse the state machine. See the call stack at the time: ash::internal::WorkspaceManager::GetWindowState() at workspace_manager.cc:159 0x7ffff16e363a ash::internal::WorkspaceController::GetWindowState() at workspace_controller.cc:63 0x7ffff16d92ba ash::internal::ShelfLayoutManager::UpdateVisibilityState() at shelf_layout_manager.cc:234 0x7ffff16bd92d ash::internal::WorkspaceManager::UpdateShelfVisibility() at workspace_manager.cc:139 0x7ffff16e341f ash::internal::WorkspaceManager::SetActiveWorkspace() at workspace_manager.cc:292 0x7ffff16e403e ash::internal::WorkspaceManager::RemoveWorkspace() at workspace_manager.cc:221 0x7ffff16e3b62 ash::internal::Workspace::~Workspace() at workspace.cc:27 0x7ffff16e12d1 ash::internal::MaximizedWorkspace::~MaximizedWorkspace() at maximized_workspace.cc:26 0x7ffff16dc7be ash::internal::WorkspaceManager::CleanupWorkspace() at workspace_manager.cc:358 0x7ffff16e4507 ash::internal::WorkspaceManager::RemoveWindow() at workspace_manager.cc:128 0x7ffff16e33b4 ash::internal::WorkspaceLayoutManager::ShowStateChanged() at workspace_layout_manager.cc:103 0x7ffff16e2b86 ash::internal::BaseLayoutManager::OnWindowPropertyChanged() at base_layout_manager.cc:148 0x7ffff169fa0e ash::internal::WorkspaceLayoutManager::OnWindowPropertyChanged() at workspace_layout_manager.cc:89 0x7ffff16e2aea aura::Window::SetPropertyInternal() at window.cc:645 0x7ffff1d2bfaa aura::Window::SetProperty<ui::WindowShowState>() at window_property.h:87 0x7ffff1cfc3ae views::NativeWidgetAura::Minimize() at native_widget_aura.cc:574 0x7fffeb4ce946 views::Widget::Minimize() at widget.cc:577 0x7fffeb4d577f ash::FrameMaximizeButton::Snap() at frame_maximize_button.cc:474 0x7ffff16db972 You can come over - or I can tell you offline how to repro. http://codereview.chromium.org/10829226/diff/7002/ash/wm/workspace/frame_maxi... File ash/wm/workspace/frame_maximize_button.cc (right): http://codereview.chromium.org/10829226/diff/7002/ash/wm/workspace/frame_maxi... ash/wm/workspace/frame_maximize_button.cc:335: snap_sizer_.reset(); When doing the change I thought the same thing - but I thought this is okay because: a. It is a scoped_ptr -> So it destroys itself guaranteed. b. ProcessEndEvent doesn't destroy the snap_sizer_ either.
On Wed, Aug 8, 2012 at 7:26 AM, <skuhne@chromium.org> wrote: > Please have a look and / or please come over so that we can quickly go over > your > comments! I won't be in until later today, so I'm sending comments. > http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/workspace_man... > File ash/wm/workspace/workspace_manager.cc (right): > > http://codereview.chromium.org/10829226/diff/1/ash/wm/workspace/workspace_man... > ash/wm/workspace/workspace_manager.cc:155: wm::IsWindowMinimized(*i)) > I debugged it and here is what I see: > > GetLayerTargetVisibility(): true > GetTargetOpacity(): 1.0 > IsWindowMinimized: true > > Looking at the code I can see that > WorkspaceManager::SetVisibilityOfWorkspaceWindows > gets called with no active windows in the workspace. I can also see that > the window gets explicitly set to visible. I looked through the > callstack but after a while I realized that there are many dependencies > I don't know yet and changing there something seems dangerous. > > But here is a clue: When first normalizing the window and then > minimizing it works. I assume that the changed order of calls with the > also disappearing bubble must confuse the state machine. > > See the call stack at the time: > > ash::internal::WorkspaceManager::GetWindowState() at > workspace_manager.cc:159 0x7ffff16e363a > ash::internal::WorkspaceController::GetWindowState() at > workspace_controller.cc:63 0x7ffff16d92ba > ash::internal::ShelfLayoutManager::UpdateVisibilityState() at > shelf_layout_manager.cc:234 0x7ffff16bd92d > ash::internal::WorkspaceManager::UpdateShelfVisibility() at > workspace_manager.cc:139 0x7ffff16e341f > ash::internal::WorkspaceManager::SetActiveWorkspace() at > workspace_manager.cc:292 0x7ffff16e403e > ash::internal::WorkspaceManager::RemoveWorkspace() at > workspace_manager.cc:221 0x7ffff16e3b62 > ash::internal::Workspace::~Workspace() at workspace.cc:27 > 0x7ffff16e12d1 > ash::internal::MaximizedWorkspace::~MaximizedWorkspace() at > maximized_workspace.cc:26 0x7ffff16dc7be > ash::internal::WorkspaceManager::CleanupWorkspace() at > workspace_manager.cc:358 0x7ffff16e4507 > ash::internal::WorkspaceManager::RemoveWindow() at > workspace_manager.cc:128 0x7ffff16e33b4 > ash::internal::WorkspaceLayoutManager::ShowStateChanged() at > workspace_layout_manager.cc:103 0x7ffff16e2b86 > ash::internal::BaseLayoutManager::OnWindowPropertyChanged() at > base_layout_manager.cc:148 0x7ffff169fa0e > ash::internal::WorkspaceLayoutManager::OnWindowPropertyChanged() at > workspace_layout_manager.cc:89 0x7ffff16e2aea > aura::Window::SetPropertyInternal() at window.cc:645 0x7ffff1d2bfaa > aura::Window::SetProperty<ui::WindowShowState>() at window_property.h:87 > 0x7ffff1cfc3ae > views::NativeWidgetAura::Minimize() at native_widget_aura.cc:574 > 0x7fffeb4ce946 > views::Widget::Minimize() at widget.cc:577 0x7fffeb4d577f > ash::FrameMaximizeButton::Snap() at frame_maximize_button.cc:474 > 0x7ffff16db972 > > You can come over - or I can tell you offline how to repro. Actually, can you add a test case for this. That way I can poke at whats happening. > > http://codereview.chromium.org/10829226/diff/7002/ash/wm/workspace/frame_maxi... > File ash/wm/workspace/frame_maximize_button.cc (right): > > http://codereview.chromium.org/10829226/diff/7002/ash/wm/workspace/frame_maxi... > ash/wm/workspace/frame_maximize_button.cc:335: snap_sizer_.reset(); > When doing the change I thought the same thing - but I thought this is > okay because: > a. It is a scoped_ptr -> So it destroys itself guaranteed. This isn't my worry. My worry is that snap_sizer_ won't be reset when starting a new snap (by way of bubble) which might cause problems because it ends up using stale state. > b. ProcessEndEvent doesn't destroy the snap_sizer_ either. This is true, but that was when we had only one path to using snap_sizer_. With snap_sizer_ being used by the bubble as well there are now two paths to possibly using the snap_sizer and I'm not sure the bubble case resets state appropriately. -Scott
Removed the workaround. Please have another look!
LGTM with the following change http://codereview.chromium.org/10829226/diff/10002/ash/wm/workspace/frame_max... File ash/wm/workspace/frame_maximize_button.h (right): http://codereview.chromium.org/10829226/diff/10002/ash/wm/workspace/frame_max... ash/wm/workspace/frame_maximize_button.h:100: internal::SnapSizer* snap_sizer) const; Make this and Snap take a const SnapSizer& (and remove your DCHECK in ScreenBoundsForType).
Sky, addressed. http://codereview.chromium.org/10829226/diff/10002/ash/wm/workspace/frame_max... File ash/wm/workspace/frame_maximize_button.h (right): http://codereview.chromium.org/10829226/diff/10002/ash/wm/workspace/frame_max... ash/wm/workspace/frame_maximize_button.h:100: internal::SnapSizer* snap_sizer) const; On 2012/08/08 17:26:01, sky wrote: > Make this and Snap take a const SnapSizer& (and remove your DCHECK in > ScreenBoundsForType). Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/10829226/11008
Failed to apply patch for ash/wm/maximize_bubble_controller.cc: While running patch -p1 --forward --force; patching file ash/wm/maximize_bubble_controller.cc Hunk #1 FAILED at 11. Hunk #2 succeeded at 41 (offset -1 lines). Hunk #3 succeeded at 60 (offset -1 lines). Hunk #4 succeeded at 70 (offset -1 lines). Hunk #5 succeeded at 86 (offset -1 lines). Hunk #6 succeeded at 120 (offset -1 lines). Hunk #7 succeeded at 223 (offset -1 lines). Hunk #8 succeeded at 267 (offset -1 lines). Hunk #9 succeeded at 359 (offset -1 lines). Hunk #10 succeeded at 392 (offset -1 lines). Hunk #11 succeeded at 426 (offset -1 lines). Hunk #12 succeeded at 470 (offset -1 lines). Hunk #13 succeeded at 514 (offset -1 lines). Hunk #14 succeeded at 601 (offset -1 lines). 1 out of 14 hunks FAILED -- saving rejects to file ash/wm/maximize_bubble_controller.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/10829226/11009
Try job failure for 10829226-11009 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/10829226/11009
Try job failure for 10829226-11009 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/10829226/11009
Try job failure for 10829226-11009 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/10829226/11009
Change committed as 150788 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
