|
|
DescriptionDo not disable minimize animation for maximized/fullscreened exo windows
BUG=717761
TEST=Fullscreen/Maximize Play Store app, then minimize/unminmize.
Review-Url: https://codereview.chromium.org/2861523002
Cr-Commit-Position: refs/heads/master@{#469171}
Committed: https://chromium.googlesource.com/chromium/src/+/30b31ad5f8d2cbfebe42a9288dd372edcaf0eca0
Patch Set 1 #
Total comments: 4
Patch Set 2 : addressed comments #Messages
Total messages: 25 (20 generated)
Description was changed from ========== Do not disalbe minimize animation for maximized/fullscren Bug: 36828408 Test: Fullscreen/Maximize Play Store app, then minimize/unminmize. ========== to ========== Do not disalbe minimize animation for maximized/fullscren Bug: 36828408 Test: Fullscreen/Maximize Play Store app, then minimize/unminmize. ==========
oshima@chromium.org changed reviewers: + reveman@chromium.org
Description was changed from ========== Do not disalbe minimize animation for maximized/fullscren Bug: 36828408 Test: Fullscreen/Maximize Play Store app, then minimize/unminmize. ========== to ========== Do not disalbe minimize animation for maximized/fullscreened exo windows Bug: 36828408 Test: Fullscreen/Maximize Play Store app, then minimize/unminmize. ==========
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Do not disalbe minimize animation for maximized/fullscreened exo windows Bug: 36828408 Test: Fullscreen/Maximize Play Store app, then minimize/unminmize. ========== to ========== Do not disalbe minimize animation for maximized/fullscreened exo windows Bug: crbug.com/36828408 Test: Fullscreen/Maximize Play Store app, then minimize/unminmize. ==========
Description was changed from ========== Do not disalbe minimize animation for maximized/fullscreened exo windows Bug: crbug.com/36828408 Test: Fullscreen/Maximize Play Store app, then minimize/unminmize. ========== to ========== Do not disalbe minimize animation for maximized/fullscreened exo windows Bug: crbug.com/717761 Test: Fullscreen/Maximize Play Store app, then minimize/unminmize. ==========
Description was changed from ========== Do not disalbe minimize animation for maximized/fullscreened exo windows Bug: crbug.com/717761 Test: Fullscreen/Maximize Play Store app, then minimize/unminmize. ========== to ========== Do not disalbe minimize animation for maximized/fullscreened exo windows BUG=crbug.com/717761 TEST=Fullscreen/Maximize Play Store app, then minimize/unminmize. ==========
Description was changed from ========== Do not disalbe minimize animation for maximized/fullscreened exo windows BUG=crbug.com/717761 TEST=Fullscreen/Maximize Play Store app, then minimize/unminmize. ========== to ========== Do not disalbe minimize animation for maximized/fullscreened exo windows BUG=717761 TEST=Fullscreen/Maximize Play Store app, then minimize/unminmize. ==========
lgtm with nit https://codereview.chromium.org/2861523002/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2861523002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:936: bool fromOrToMinimize = old_type == ash::wm::WINDOW_STATE_TYPE_MINIMIZED || nit: "bool from_or_to_minimize" but maybe a bit cleaner to early out instead. ie. // Comment that explains why. if (old_type == ash::wm::WINDOW_STATE_TYPE_MINIMIZED || new_type == ash::wm::WINDOW_STATE_TYPE_MINIMIZED) { return; } https://codereview.chromium.org/2861523002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:947: nit: while here, this blank line should not be here as comment refer to the code below
https://codereview.chromium.org/2861523002/diff/1/components/exo/shell_surfac... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2861523002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:936: bool fromOrToMinimize = old_type == ash::wm::WINDOW_STATE_TYPE_MINIMIZED || On 2017/05/03 20:47:42, reveman wrote: > nit: "bool from_or_to_minimize" but maybe a bit cleaner to early out instead. > ie. > > // Comment that explains why. > if (old_type == ash::wm::WINDOW_STATE_TYPE_MINIMIZED || > new_type == ash::wm::WINDOW_STATE_TYPE_MINIMIZED) { > return; > } Done. https://codereview.chromium.org/2861523002/diff/1/components/exo/shell_surfac... components/exo/shell_surface.cc:947: On 2017/05/03 20:47:42, reveman wrote: > nit: while here, this blank line should not be here as comment refer to the code > below Done.
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/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Do not disalbe minimize animation for maximized/fullscreened exo windows BUG=717761 TEST=Fullscreen/Maximize Play Store app, then minimize/unminmize. ========== to ========== Do not disable minimize animation for maximized/fullscreened exo windows BUG=717761 TEST=Fullscreen/Maximize Play Store app, then minimize/unminmize. ==========
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@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2861523002/#ps20001 (title: "addressed comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493850201557050, "parent_rev": "ae86b9eaf1e405587e30bb5b49731df653cc57dc", "commit_rev": "30b31ad5f8d2cbfebe42a9288dd372edcaf0eca0"}
Message was sent while issue was closed.
Description was changed from ========== Do not disable minimize animation for maximized/fullscreened exo windows BUG=717761 TEST=Fullscreen/Maximize Play Store app, then minimize/unminmize. ========== to ========== Do not disable minimize animation for maximized/fullscreened exo windows BUG=717761 TEST=Fullscreen/Maximize Play Store app, then minimize/unminmize. Review-Url: https://codereview.chromium.org/2861523002 Cr-Commit-Position: refs/heads/master@{#469171} Committed: https://chromium.googlesource.com/chromium/src/+/30b31ad5f8d2cbfebe42a9288dd3... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/30b31ad5f8d2cbfebe42a9288dd3... |