Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1838003002:
Create drag windows on all displays (Closed)
DescriptionCreate drag windows on all displays
This refactors and simplify the logic as follows:
* DragWindowDetails is now responsible for drag window state management.
- DragWindowDetails is created for all displays except for the drag source.
- This creates/deletes the drag window based on the
requested bounds.
- This also updates the opacity based on the requested bounds.
This also fixes minor issue in the DragWindowController tests where the window was snapped.
BUG=598438
TEST=covered by unit tests.
Committed: https://crrev.com/32347aea90191f4558531d5dccb05bd9946cddce
Cr-Commit-Position: refs/heads/master@{#384625}
Patch Set 1 : #
Total comments: 13
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : fix silly typo #
Messages
Total messages: 37 (21 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Create drag windows an all displays BUG=598438 TEST= ========== to ========== Create drag windows on all displays BUG=598438 TEST=covered by unit tests. ==========
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by oshima@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/1838003002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838003002/80001
Description was changed from ========== Create drag windows on all displays BUG=598438 TEST=covered by unit tests. ========== to ========== Create drag windows on all displays This refactors and simplify the logic as follows: * DragWindowDetails is now responsible for drag window state management. - DragWindowDetails is created for all displays except for the drag source. - This creates/deletes the drag window based on the requested bounds. - This also updates the opacity based on the requested bounds. This also fixes minor issue in the DragWindowController tests where the window was snapped. BUG=598438 TEST=covered by unit tests. ==========
oshima@chromium.org changed reviewers: + stevenjb@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_cont... File ash/wm/drag_window_controller.cc (right): https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_cont... ash/wm/drag_window_controller.cc:41: delete drag_window_; nit: I had to remind myself that this was safe, maybe add a comment like "Deleting the window will remove itself from its parent, see aura::Window::~Window." https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_cont... ash/wm/drag_window_controller.cc:42: drag_window_ = nullptr; This should be handled in OnWindowDestroying, right? https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_cont... ash/wm/drag_window_controller.cc:52: drag_window_ = nullptr; Same here? This pattern implies ownership. Perhaps something like this would be more clear: delete drag_window_; // Unparented in destructor, nulled in OnWindowDestroying Or just "Nulled in OnWindowDestroying" since that implies that ownership is addressed in the destructor. https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_resi... File ash/wm/drag_window_resizer.cc (right): https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_resi... ash/wm/drag_window_resizer.cc:158: GetTarget()->layer()->SetOpacity(opacity); nit: float opacity; if (root_bounds_in_screen.Contains(drag_location_in_screen)) opacity = 1.0; } else { ... } ...SetOpacity(opacity) https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_resi... File ash/wm/drag_window_resizer_unittest.cc (right): https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_resi... ash/wm/drag_window_resizer_unittest.cc:353: // when cragging around the edge of the display. s/cragging/dragging/ combine lines https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_resi... ash/wm/drag_window_resizer_unittest.cc:427: // Layout so that all three displays touches each other. s/touches/touch/ https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_resi... ash/wm/drag_window_resizer_unittest.cc:450: // when cragging around the edge of the display. dragging
https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_cont... File ash/wm/drag_window_controller.cc (right): https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_cont... ash/wm/drag_window_controller.cc:42: drag_window_ = nullptr; On 2016/03/29 18:56:13, stevenjb wrote: > This should be handled in OnWindowDestroying, right? Yep, changed to DCHECK. https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_cont... ash/wm/drag_window_controller.cc:52: drag_window_ = nullptr; On 2016/03/29 18:56:13, stevenjb wrote: > Same here? This pattern implies ownership. Perhaps something like this would be > more clear: > > delete drag_window_; // Unparented in destructor, nulled in OnWindowDestroying > > Or just "Nulled in OnWindowDestroying" since that implies that ownership is > addressed in the destructor. > per offline chat, I changed to DCHECK and added comment https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_resi... File ash/wm/drag_window_resizer.cc (right): https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_resi... ash/wm/drag_window_resizer.cc:158: GetTarget()->layer()->SetOpacity(opacity); On 2016/03/29 18:56:13, stevenjb wrote: > nit: > float opacity; > if (root_bounds_in_screen.Contains(drag_location_in_screen)) > opacity = 1.0; > } else { > ... > } > ...SetOpacity(opacity) > Done, with slightly less code. https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_resi... File ash/wm/drag_window_resizer_unittest.cc (right): https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_resi... ash/wm/drag_window_resizer_unittest.cc:353: // when cragging around the edge of the display. On 2016/03/29 18:56:13, stevenjb wrote: > s/cragging/dragging/ > combine lines Done. looks like git cl format isn't smart enough to do this right :p https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_resi... ash/wm/drag_window_resizer_unittest.cc:427: // Layout so that all three displays touches each other. On 2016/03/29 18:56:14, stevenjb wrote: > s/touches/touch/ Done. https://codereview.chromium.org/1838003002/diff/80001/ash/wm/drag_window_resi... ash/wm/drag_window_resizer_unittest.cc:450: // when cragging around the edge of the display. On 2016/03/29 18:56:14, stevenjb wrote: > dragging Done.
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1838003002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838003002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838003002/100001
lgtm https://codereview.chromium.org/1838003002/diff/100001/ash/wm/drag_window_res... File ash/wm/drag_window_resizer_unittest.cc (right): https://codereview.chromium.org/1838003002/diff/100001/ash/wm/drag_window_res... ash/wm/drag_window_resizer_unittest.cc:352: // edge when cragging around the edge of the display. dragging
The CQ bit was unchecked by oshima@chromium.org
https://codereview.chromium.org/1838003002/diff/100001/ash/wm/drag_window_res... File ash/wm/drag_window_resizer_unittest.cc (right): https://codereview.chromium.org/1838003002/diff/100001/ash/wm/drag_window_res... ash/wm/drag_window_resizer_unittest.cc:352: // edge when cragging around the edge of the display. On 2016/03/30 21:22:44, stevenjb wrote: > dragging Done.
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1838003002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838003002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838003002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by oshima@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/1838003002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838003002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1838003002/#ps140001 (title: "fix silly typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838003002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838003002/140001
Message was sent while issue was closed.
Description was changed from ========== Create drag windows on all displays This refactors and simplify the logic as follows: * DragWindowDetails is now responsible for drag window state management. - DragWindowDetails is created for all displays except for the drag source. - This creates/deletes the drag window based on the requested bounds. - This also updates the opacity based on the requested bounds. This also fixes minor issue in the DragWindowController tests where the window was snapped. BUG=598438 TEST=covered by unit tests. ========== to ========== Create drag windows on all displays This refactors and simplify the logic as follows: * DragWindowDetails is now responsible for drag window state management. - DragWindowDetails is created for all displays except for the drag source. - This creates/deletes the drag window based on the requested bounds. - This also updates the opacity based on the requested bounds. This also fixes minor issue in the DragWindowController tests where the window was snapped. BUG=598438 TEST=covered by unit tests. ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Create drag windows on all displays This refactors and simplify the logic as follows: * DragWindowDetails is now responsible for drag window state management. - DragWindowDetails is created for all displays except for the drag source. - This creates/deletes the drag window based on the requested bounds. - This also updates the opacity based on the requested bounds. This also fixes minor issue in the DragWindowController tests where the window was snapped. BUG=598438 TEST=covered by unit tests. ========== to ========== Create drag windows on all displays This refactors and simplify the logic as follows: * DragWindowDetails is now responsible for drag window state management. - DragWindowDetails is created for all displays except for the drag source. - This creates/deletes the drag window based on the requested bounds. - This also updates the opacity based on the requested bounds. This also fixes minor issue in the DragWindowController tests where the window was snapped. BUG=598438 TEST=covered by unit tests. Committed: https://crrev.com/32347aea90191f4558531d5dccb05bd9946cddce Cr-Commit-Position: refs/heads/master@{#384625} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/32347aea90191f4558531d5dccb05bd9946cddce Cr-Commit-Position: refs/heads/master@{#384625} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
