Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1156893008:
Fixes tab dragging out of a window with maximzied bounds (Closed)
DescriptionFixes tab dragging out of a window with maximzied bounds
The breaking change was a fix for http://crbug.com/392599 (https://codereview.chromium.org/424463002/) that causes a browser window to be maximized at creation time if its size is as big as a work area. When a tab is dragged into a new browser window this new window may be maximized and so not draggable.
BUG=491502
TEST=DetachToBrowserTabDragControllerTest.DetachFromFullsizeWindow
Committed: https://crrev.com/3cf4fa484a5aa92211f937148c9ab80d6f0be163
Cr-Commit-Position: refs/heads/master@{#333220}
Patch Set 1 #Patch Set 2 : Fixes tab dragging out of a window with maximzied bounds (test) #
Total comments: 7
Patch Set 3 : Fixes tab dragging out of a window with maximzied bounds (nits) #
Total comments: 8
Messages
Total messages: 25 (9 generated)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156893008/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
varkha@chromium.org changed reviewers: + msw@chromium.org
msw@, can you please take a look?
lgtm with comment nits, but oshima or sky are better reviewers here. https://codereview.chromium.org/1156893008/diff/20001/ash/wm/default_state.cc File ash/wm/default_state.cc (right): https://codereview.chromium.org/1156893008/diff/20001/ash/wm/default_state.cc... ash/wm/default_state.cc:35: // unmaximized inset the bounds slightly so that they are not exactly same. This nit: comma here: "unmaximized," and "exactly the same" (maybe just move the other comment here, they seem redundant) https://codereview.chromium.org/1156893008/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1156893008/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:80: // Dragged window is forced to be a bit smaller than maximized bounds during a nit: "Dragged windows are" or "A dragged window" https://codereview.chromium.org/1156893008/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1650: // If the restore bounds is larger than work area make dragging window nit: comma here "area," and "make the dragging" https://codereview.chromium.org/1156893008/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1650: // If the restore bounds is larger than work area make dragging window ditto nit: comment redundancy
varkha@chromium.org changed reviewers: + oshima@chromium.org
Thanks. +oshima@ in case there are comments about this behaviour. https://codereview.chromium.org/1156893008/diff/20001/ash/wm/default_state.cc File ash/wm/default_state.cc (right): https://codereview.chromium.org/1156893008/diff/20001/ash/wm/default_state.cc... ash/wm/default_state.cc:35: // unmaximized inset the bounds slightly so that they are not exactly same. This On 2015/06/05 17:20:51, msw wrote: > nit: comma here: "unmaximized," and "exactly the same" > (maybe just move the other comment here, they seem redundant) Done. https://codereview.chromium.org/1156893008/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1156893008/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:80: // Dragged window is forced to be a bit smaller than maximized bounds during a On 2015/06/05 17:20:51, msw wrote: > nit: "Dragged windows are" or "A dragged window" Done. https://codereview.chromium.org/1156893008/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1650: // If the restore bounds is larger than work area make dragging window On 2015/06/05 17:20:51, msw wrote: > nit: comma here "area," and "make the dragging" Done (removed the original comment here in favour of the comment above. I have kept (but moved lower) the part of the comment about maximizing after the drag ends since this may be not obvious.
sky@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/1156893008/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1156893008/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1651: screen_->GetDisplayNearestPoint(last_point_in_screen_).work_area(); Shouldn't this be an else condition of maximized? Also, make sure you try this on windows.
Thanks sky@, see my comment below. https://codereview.chromium.org/1156893008/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1156893008/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1651: screen_->GetDisplayNearestPoint(last_point_in_screen_).work_area(); On 2015/06/05 19:24:27, sky wrote: > Shouldn't this be an else condition of maximized? Also, make sure you try this > on windows. I thinks this should be complimentary rather than an else on IsMaximized. Consider the case when a window is manually resized to work_area bounds and then maximized. Its restore bounds will be equal to work_area and unless we adjust the restore bounds the tab will be undraggable. I have tested this theory and indeed the bug remains in that corner case if I move the logic under else. Both this corner case and the one described in the bug are fixed if I leave the logic where it is now. I have added a unit test that tests this corner case on Windows (where the only new behavior is that the window will be maximized after drag). Would you trust that?
Fair enough, LGTM https://codereview.chromium.org/1156893008/diff/40001/ash/wm/default_state.cc File ash/wm/default_state.cc (right): https://codereview.chromium.org/1156893008/diff/40001/ash/wm/default_state.cc... ash/wm/default_state.cc:37: const int kMaximizedWindowInset = 10; // Pixels. Is this really pixels? https://codereview.chromium.org/1156893008/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1156893008/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:84: const int kMaximizedWindowInset = 10; // Pixels. Again, is this really pixels?
Thanks! Not sure what the bounds are in other than pixels? Are those device independent in some cases? https://codereview.chromium.org/1156893008/diff/40001/ash/wm/default_state.cc File ash/wm/default_state.cc (right): https://codereview.chromium.org/1156893008/diff/40001/ash/wm/default_state.cc... ash/wm/default_state.cc:37: const int kMaximizedWindowInset = 10; // Pixels. On 2015/06/06 17:59:30, sky wrote: > Is this really pixels? I thought so. Isn't that what window bounds are expressed in? https://codereview.chromium.org/1156893008/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1156893008/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:84: const int kMaximizedWindowInset = 10; // Pixels. On 2015/06/06 17:59:30, sky wrote: > Again, is this really pixels? I thought so. This is same units as mouse travel, window bounds, etc. Are there cases when this is something else?
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/1156893008/#ps40001 (title: "Fixes tab dragging out of a window with maximzied bounds (nits)")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156893008/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156893008/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3cf4fa484a5aa92211f937148c9ab80d6f0be163 Cr-Commit-Position: refs/heads/master@{#333220}
Message was sent while issue was closed.
https://codereview.chromium.org/1156893008/diff/40001/ash/wm/default_state.cc File ash/wm/default_state.cc (right): https://codereview.chromium.org/1156893008/diff/40001/ash/wm/default_state.cc... ash/wm/default_state.cc:37: const int kMaximizedWindowInset = 10; // Pixels. On 2015/06/06 20:13:17, varkha wrote: > On 2015/06/06 17:59:30, sky wrote: > > Is this really pixels? > > I thought so. Isn't that what window bounds are expressed in? It may depend upon the platform, but I have to look. Certainly on chromeos they are DIPs.
Message was sent while issue was closed.
https://codereview.chromium.org/1156893008/diff/40001/ash/wm/default_state.cc File ash/wm/default_state.cc (right): https://codereview.chromium.org/1156893008/diff/40001/ash/wm/default_state.cc... ash/wm/default_state.cc:37: const int kMaximizedWindowInset = 10; // Pixels. On 2015/06/07 00:05:20, sky wrote: > On 2015/06/06 20:13:17, varkha wrote: > > On 2015/06/06 17:59:30, sky wrote: > > > Is this really pixels? > > > > I thought so. Isn't that what window bounds are expressed in? > > It may depend upon the platform, but I have to look. Certainly on chromeos they > are DIPs. This is DIP. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
