Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
424463002:
Makes a window that has been resized to maximized bounds, then maximized and then restored shrink (Closed)
|
Created:
6 years, 5 months ago by varkha Modified:
6 years, 4 months ago CC:
chromium-reviews, kalyank, sadrul, ben+ash_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionMakes a window that has been resized to maximized bounds, then maximized and then restored shrink. This allows it to be resized easier after this sequence.
BUG=392599
TEST=ash_unittests --gtest_filter="WindowStateTest.RestoredWindowBoundsShrink"
TEST=unit_tests --gtest_filter="WindowSizerAshTest.DefaultStateBecomesMaximized"
TEST=unit_tests --gtest_filter="WindowSizerAshTest*"
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287675
Patch Set 1 #Patch Set 2 : Maximizes a window that has been resized to maximized bounds (added a test) #
Total comments: 6
Patch Set 3 : Maximizes a window that has been resized to maximized bounds (added a test) #
Total comments: 6
Patch Set 4 : Maximizes a window that has been resized to maximized bounds (comments) #Patch Set 5 : Maximizes a window that has been resized to maximized bounds (only update size when unmaximizing) #
Total comments: 6
Patch Set 6 : Maximizes a window that has been resized to maximized bounds (comments and restrict to ash) #
Total comments: 8
Patch Set 7 : Maximizes a window that has been resized to maximized bounds (comments and nits) #Patch Set 8 : Maximizes a window that has been resized to maximized bounds (fixed a test) #
Total comments: 17
Patch Set 9 : Maximizes a window that has been resized to maximized bounds (nits, restricts to tabbed browsers) #Patch Set 10 : Maximizes a window that has been resized to maximized bounds (nits, restricts to tabbed browsers) #
Messages
Total messages: 38 (0 generated)
oshima@, could you please take a look?
change lg. can you add a test?
Added a test. PTAL.
Once maximized by drag, a user cannot drag it back to the other size. Can you confirm with Ksucher this is what we want, or we want to allow resizing maximzied window? https://codereview.chromium.org/424463002/diff/80001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): https://codereview.chromium.org/424463002/diff/80001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer_unittest.cc:428: { can you add comment what this is doing? (resize the window to fit the top/left) https://codereview.chromium.org/424463002/diff/80001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer_unittest.cc:438: scoped_ptr<WindowResizer> resizer(CreateResizerForTest( and here. https://codereview.chromium.org/424463002/diff/80001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer_unittest.cc:453: } can you also add the case where you create the window that is the same size as maximized with some offset and move it?
On 2014/07/29 17:00:36, oshima wrote: > Once maximized by drag, a user cannot drag it back to the other size. Can you > confirm with Ksucher this is what we want, or we want to allow resizing > maximzied window? > > https://codereview.chromium.org/424463002/diff/80001/ash/wm/workspace/workspa... > File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): > > https://codereview.chromium.org/424463002/diff/80001/ash/wm/workspace/workspa... > ash/wm/workspace/workspace_window_resizer_unittest.cc:428: { > can you add comment what this is doing? (resize the window to fit the top/left) > > https://codereview.chromium.org/424463002/diff/80001/ash/wm/workspace/workspa... > ash/wm/workspace/workspace_window_resizer_unittest.cc:438: > scoped_ptr<WindowResizer> resizer(CreateResizerForTest( > and here. > > https://codereview.chromium.org/424463002/diff/80001/ash/wm/workspace/workspa... > ash/wm/workspace/workspace_window_resizer_unittest.cc:453: } > can you also add the case where you create the window that is the same size as > maximized with some offset and move it? More more edge case. When a new window has the bounds that matches the maximized size (either by restore, or by using previous window state), shouldn't it be also maximized?
> On 2014/07/29 17:00:36, oshima wrote: > More more edge case. When a new window has the bounds that matches the maximized > size (either by restore, or by using previous window state), shouldn't it be > also maximized? How would you get into this state with a normal flow? I think you can only get into this with a stored state from a previous version, correct? I would prefer to only "fix" the state on explicit drag resize rather than adjust state transitions although creating a window seems exceptional enough so I actually agree with you if you only mean adjusting at creation time. I think the correct place to do that will be where size and state get restored from storage, right?
On 2014/07/29 17:16:43, varkha wrote: > > On 2014/07/29 17:00:36, oshima wrote: > > More more edge case. When a new window has the bounds that matches the > maximized > > size (either by restore, or by using previous window state), shouldn't it be > > also maximized? > > How would you get into this state with a normal flow? I think you can only get > into this with a stored state from a previous version, correct? I would prefer > to only "fix" the state on explicit drag resize rather than adjust state > transitions although creating a window seems exceptional enough so I actually > agree with you if you only mean adjusting at creation time. > I think the correct place to do that will be where size and state get restored > from storage, right? Chrome limit the restored or new window's bounds to the workspace, so if a user somehow had larger bounds (either from multiple display environment, or after the user changed the resolution), it may set these windows to the maximzied size.
> On 2014/07/29 17:00:36, oshima wrote: > One more edge case. When a new window has the bounds that matches the maximized > size (either by restore, or by using previous window state), shouldn't it be > also maximized? Done, I think. PTAL. I am also resetting the restore bounds to a "sensible default" so that the window doesn't stay visibly maximized when it gets unmaximized. https://codereview.chromium.org/424463002/diff/80001/ash/wm/workspace/workspa... File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): https://codereview.chromium.org/424463002/diff/80001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer_unittest.cc:428: { On 2014/07/29 17:00:36, oshima wrote: > can you add comment what this is doing? (resize the window to fit the top/left) Done. https://codereview.chromium.org/424463002/diff/80001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer_unittest.cc:438: scoped_ptr<WindowResizer> resizer(CreateResizerForTest( On 2014/07/29 17:00:36, oshima wrote: > and here. Done. https://codereview.chromium.org/424463002/diff/80001/ash/wm/workspace/workspa... ash/wm/workspace/workspace_window_resizer_unittest.cc:453: } On 2014/07/29 17:00:36, oshima wrote: > can you also add the case where you create the window that is the same size as > maximized with some offset and move it? Done.
lgtm
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/424463002/100001
+msw@ for OWNERS (chrome/browser/ui/window_sizer/window_sizer.cc).
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/424463002/diff/100001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_window_resizer.cc (right): https://codereview.chromium.org/424463002/diff/100001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_window_resizer.cc:467: if (!snapped && nit: a brief comment for this block would be nice. https://codereview.chromium.org/424463002/diff/100001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer.cc (right): https://codereview.chromium.org/424463002/diff/100001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer.cc:227: if (*show_state == ui::SHOW_STATE_DEFAULT) { Can you add unit tests for this new behavior, similar to Ash? https://codereview.chromium.org/424463002/diff/100001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer.cc:231: sizer.GetDefaultWindowBounds(display, window_bounds); Why do you need to set |window_bounds| to the default value here, if it's equal to display.work_area() above? Will the window get restored to it's bounds prior to dragging when it's brought out of the maximized state? A comment or two would be helpful here, since the behavior isn't entirely obvious.
I have uploaded two patches following the offline discussion about the bug. One (PS4) merely follows up on the comments and adds a unit test for the new behaviour in WindowSizer. Another (PS5) reverts the change in WorkspaceWindowResizer (and the test for it) and instead shrinks the window slightly if it gets unmaximized while having restore bounds same as the work area. This makes the behaviour described in the bug at least 2x less annoying. PS5 is less intrusive and has no problem with not being reversible (if we adjust the state to maximized as a result of drag resizing it is not possible to resize it back by another drag). Please let me know what you think. https://codereview.chromium.org/424463002/diff/100001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_window_resizer.cc (right): https://codereview.chromium.org/424463002/diff/100001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_window_resizer.cc:467: if (!snapped && On 2014/07/30 05:46:37, msw wrote: > nit: a brief comment for this block would be nice. Done. https://codereview.chromium.org/424463002/diff/100001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer.cc (right): https://codereview.chromium.org/424463002/diff/100001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer.cc:227: if (*show_state == ui::SHOW_STATE_DEFAULT) { On 2014/07/30 05:46:37, msw wrote: > Can you add unit tests for this new behavior, similar to Ash? Done. https://codereview.chromium.org/424463002/diff/100001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer.cc:231: sizer.GetDefaultWindowBounds(display, window_bounds); On 2014/07/30 05:46:37, msw wrote: > Why do you need to set |window_bounds| to the default value here, if it's equal > to display.work_area() above? Will the window get restored to it's bounds prior > to dragging when it's brought out of the maximized state? A comment or two would > be helpful here, since the behavior isn't entirely obvious. Yes, the bounds returned here become the restore bounds once the window gets maximized after this returns. I've added a comment that explains this.
https://codereview.chromium.org/424463002/diff/180001/ash/wm/default_state.cc File ash/wm/default_state.cc (right): https://codereview.chromium.org/424463002/diff/180001/ash/wm/default_state.cc... ash/wm/default_state.cc:520: bounds_in_parent.Inset(10, 10, 10, 10); |bounds_in_parent| might be bigger than |work_area_in_parent|. (e.g. the display bounds shrunk after the window got maximized) Confusingly enough, AdjustBoundsToEnsureMinimumWindowVisibility() enforces a maximum size in addition to a minimum size.
https://codereview.chromium.org/424463002/diff/180001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right): https://codereview.chromium.org/424463002/diff/180001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:899: // Make a window bigger than the display work area. Users can still make windows excessively large and span multiple displays, right? Perhaps a test ensuring that's still possible would be good.
https://codereview.chromium.org/424463002/diff/180001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right): https://codereview.chromium.org/424463002/diff/180001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:899: // Make a window bigger than the display work area. On 2014/07/31 04:03:24, msw wrote: > Users can still make windows excessively large and span multiple displays, > right? Perhaps a test ensuring that's still possible would be good. No, with ash they cannot. The biggest a window is allowed to be is its root window size. There's no support for windows spanning multiple root windows.
https://codereview.chromium.org/424463002/diff/180001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right): https://codereview.chromium.org/424463002/diff/180001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:899: // Make a window bigger than the display work area. On 2014/08/01 14:18:34, varkha wrote: > On 2014/07/31 04:03:24, msw wrote: > > Users can still make windows excessively large and span multiple displays, > > right? Perhaps a test ensuring that's still possible would be good. > > No, with ash they cannot. The biggest a window is allowed to be is its root > window size. There's no support for windows spanning multiple root windows. But windows could span multiple displays on Windows desktop, last I knew. That's not something this CL breaks, is it? Does the window_sizer.cc change belong in window_sizer_ash.cc, or should there be a new window_sizer_unittest.cc or window_sizer_common_unittest.cc test?
On 2014/08/01 16:24:48, msw wrote: > https://codereview.chromium.org/424463002/diff/180001/chrome/browser/ui/windo... > File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right): > > https://codereview.chromium.org/424463002/diff/180001/chrome/browser/ui/windo... > chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:899: // Make a > window bigger than the display work area. > On 2014/08/01 14:18:34, varkha wrote: > > On 2014/07/31 04:03:24, msw wrote: > > > Users can still make windows excessively large and span multiple displays, > > > right? Perhaps a test ensuring that's still possible would be good. > > > > No, with ash they cannot. The biggest a window is allowed to be is its root > > window size. There's no support for windows spanning multiple root windows. > > But windows could span multiple displays on Windows desktop, last I knew. That's > not something this CL breaks, is it? Does the window_sizer.cc change belong in > window_sizer_ash.cc, or should there be a new window_sizer_unittest.cc or > window_sizer_common_unittest.cc test? Good point, I maybe looked at the comment too narrowly because it was in window_sizer_ash_unittest. I'll check if the behavior changes on Windows (I don't intend to change it).
I've reworked this so as to not change behavior other then on ash. PTAL. https://codereview.chromium.org/424463002/diff/180001/ash/wm/default_state.cc File ash/wm/default_state.cc (right): https://codereview.chromium.org/424463002/diff/180001/ash/wm/default_state.cc... ash/wm/default_state.cc:520: bounds_in_parent.Inset(10, 10, 10, 10); On 2014/07/31 03:39:57, pkotwicz wrote: > |bounds_in_parent| might be bigger than |work_area_in_parent|. (e.g. the display > bounds shrunk after the window got maximized) > Confusingly enough, AdjustBoundsToEnsureMinimumWindowVisibility() enforces a > maximum size in addition to a minimum size. I think checking for a larger size should solve it. I prefer not to change the origin for a window that is in at least once dimension is smaller than the work area. https://codereview.chromium.org/424463002/diff/180001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right): https://codereview.chromium.org/424463002/diff/180001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:899: // Make a window bigger than the display work area. On 2014/08/01 16:24:47, msw wrote: > On 2014/08/01 14:18:34, varkha wrote: > > On 2014/07/31 04:03:24, msw wrote: > > > Users can still make windows excessively large and span multiple displays, > > > right? Perhaps a test ensuring that's still possible would be good. > > > > No, with ash they cannot. The biggest a window is allowed to be is its root > > window size. There's no support for windows spanning multiple root windows. > > But windows could span multiple displays on Windows desktop, last I knew. That's > not something this CL breaks, is it? Does the window_sizer.cc change belong in > window_sizer_ash.cc, or should there be a new window_sizer_unittest.cc or > window_sizer_common_unittest.cc test? I've moved the behavior in window_sizer_ash.cc so Windows behavior should be unaffected.
https://codereview.chromium.org/424463002/diff/200001/ash/wm/default_state.cc File ash/wm/default_state.cc (right): https://codereview.chromium.org/424463002/diff/200001/ash/wm/default_state.cc... ash/wm/default_state.cc:517: // This may happen if a window was resized to maimized bounds or if the nit: s/maimized/maximized/ https://codereview.chromium.org/424463002/diff/200001/ash/wm/default_state.cc... ash/wm/default_state.cc:523: // work area bounds and it is easier to resize the window. nit: "the work area bounds" https://codereview.chromium.org/424463002/diff/200001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right): https://codereview.chromium.org/424463002/diff/200001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash.cc:54: bool WindowSizer::GetBrowserBoundsAshAdjusted( Merge this into GetBrowserBoundsAsh. Its one call site now calls this anyway. https://codereview.chromium.org/424463002/diff/200001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash.cc:69: // |window_bounds| returned here become the restore bounds once the window nit: s/|window_bounds|/|bounds|/ to match the actual parameter name.
https://codereview.chromium.org/424463002/diff/200001/ash/wm/default_state.cc File ash/wm/default_state.cc (right): https://codereview.chromium.org/424463002/diff/200001/ash/wm/default_state.cc... ash/wm/default_state.cc:517: // This may happen if a window was resized to maimized bounds or if the On 2014/08/05 01:42:58, msw wrote: > nit: s/maimized/maximized/ Done. https://codereview.chromium.org/424463002/diff/200001/ash/wm/default_state.cc... ash/wm/default_state.cc:523: // work area bounds and it is easier to resize the window. On 2014/08/05 01:42:58, msw wrote: > nit: "the work area bounds" Done. https://codereview.chromium.org/424463002/diff/200001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right): https://codereview.chromium.org/424463002/diff/200001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash.cc:54: bool WindowSizer::GetBrowserBoundsAshAdjusted( On 2014/08/05 01:42:58, msw wrote: > Merge this into GetBrowserBoundsAsh. Its one call site now calls this anyway. Done. Was considering doing that but wanted to see an incremental logic change. Second opinion helps. https://codereview.chromium.org/424463002/diff/200001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash.cc:69: // |window_bounds| returned here become the restore bounds once the window On 2014/08/05 01:42:58, msw wrote: > nit: s/|window_bounds|/|bounds|/ to match the actual parameter name. Done.
Taking a look at win test bots failing...
Fixed the failing tests, PTAL.
+skuhne for GetWindowShowState changes. +jschuh for security thoughts on auto-maximizing popups. https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer.h (right): https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer.h:166: // If the window is too big to fit in the display work area then the |bounds| nit: start this on the line above. https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right): https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash.cc:47: if (browser_->is_type_popup() && bounds->origin().IsOrigin()) { nit: combine this into an "else if" at line 43. https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash.cc:54: if (*show_state == ui::SHOW_STATE_DEFAULT) { If this new behavior also applies to popup browser windows, popups can now open themselves in a maximized state (by specifying any bounds >= the work area bounds). Consider excluding popup windows from this new behavior, or I'd highly encourage your to ping the security team to ensure that isn't problematic. https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right): https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:757: // A window that is less than the whole work area is set to default state. nit: s/that is less/smaller/ https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:918: // Create a browser which we can use to pass into the GetWindowBounds nit: remove "which we can use " https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_common_unittest.cc (right): https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_common_unittest.cc:245: sp->SetPersistentState(bounds, work_area, show_state_persisted, true); nit: nix |work_area|, inline |display_config| here. https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_common_unittest.h (right): https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:129: // |bounds| specifies the |browser| last or persistent bounds depending on nit: start this on the line above. https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:132: ui::WindowShowState GetWindowShowState( You should have skuhne@ review the changes to GetWindowShowState and its call sites; the parameters and use of this function aren't entirely obvious to me.
lgtm https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_common_unittest.h (right): https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:127: // Sets up the various show states and get the resulting show state from This comment appears to be somewhat outdated through time. Maybe: Sets up the various system states which have an influence on the WindowSizer and then determines the resulting show state from it. [..]
https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right): https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash.cc:47: if (browser_->is_type_popup() && bounds->origin().IsOrigin()) { On 2014/08/05 16:44:38, msw wrote: > nit: combine this into an "else if" at line 43. Done. https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash.cc:54: if (*show_state == ui::SHOW_STATE_DEFAULT) { On 2014/08/05 16:44:38, msw wrote: > If this new behavior also applies to popup browser windows, popups can now open > themselves in a maximized state (by specifying any bounds >= the work area > bounds). Consider excluding popup windows from this new behavior, or I'd highly > encourage your to ping the security team to ensure that isn't problematic. Makes sense to restrict this only to tabbed browsers. Done and thanks for the suggestion! https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right): https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:757: // A window that is less than the whole work area is set to default state. On 2014/08/05 16:44:38, msw wrote: > nit: s/that is less/smaller/ Done. https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:918: // Create a browser which we can use to pass into the GetWindowBounds On 2014/08/05 16:44:38, msw wrote: > nit: remove "which we can use " Done. Here and elsewhere in this file. https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_common_unittest.cc (right): https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_common_unittest.cc:245: sp->SetPersistentState(bounds, work_area, show_state_persisted, true); On 2014/08/05 16:44:38, msw wrote: > nit: nix |work_area|, inline |display_config| here. Done. https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... File chrome/browser/ui/window_sizer/window_sizer_common_unittest.h (right): https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:127: // Sets up the various show states and get the resulting show state from On 2014/08/05 17:29:11, Mr4D wrote: > This comment appears to be somewhat outdated through time. Maybe: > > Sets up the various system states which have an influence on the WindowSizer and > then determines the resulting show state from it. > [..] Done. https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:129: // |bounds| specifies the |browser| last or persistent bounds depending on On 2014/08/05 16:44:39, msw wrote: > nit: start this on the line above. Done. https://codereview.chromium.org/424463002/diff/240001/chrome/browser/ui/windo... chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:132: ui::WindowShowState GetWindowShowState( On 2014/08/05 16:44:38, msw wrote: > You should have skuhne@ review the changes to GetWindowShowState and its call > sites; the parameters and use of this function aren't entirely obvious to me. Acknowledged.
lgtm, thanks for addressing all my concerns.
Thanks, very useful review. -jschuh (popups are unaffected in the latest patch).
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/424463002/280001
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/424463002/300001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Message was sent while issue was closed.
Change committed as 287675 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
