Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1905333004:
Corrects NativeWidgetAura state model to not minimize on every restore (Closed)
DescriptionCorrects NativeWidgetAura state model to not minimize on every restore
CL https://codereview.chromium.org/1130033003 made it so that a call to Show() can make window (show state NORMAL) only to minimize it again. This makes the state change sequence unnecessarily complex and prone to having visible artifacts such as the docking restore that is described in the bug.
This CL keeps most of the changes from https://codereview.chromium.org/1130033003 but corrects Widget::Show() to only perform the extra call to Minimize() once after Widget::Init().
BUG=605998
TEST=Restore a previously minimized docked window.
There should be only a fade-in animation.
Committed: https://crrev.com/696f45a78fb690a7f19bdbafbb8ba84be06ee056
Cr-Commit-Position: refs/heads/master@{#389506}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Corrects NativeWidgetAura state model to not minimize on every restore #Patch Set 3 : Corrects NativeWidgetAura state model to not minimize on every restore (test) #
Total comments: 6
Patch Set 4 : Corrects NativeWidgetAura state model to not minimize on every restore (nits) #
Messages
Total messages: 24 (11 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/1905333004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905333004/1
varkha@chromium.org changed reviewers: + sky@chromium.org
sky@, can you please take a look? Thanks! https://codereview.chromium.org/1905333004/diff/1/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1905333004/diff/1/ui/views/widget/widget.cc#n... ui/views/widget/widget.cc:612: IsFullscreen() ? ui::SHOW_STATE_FULLSCREEN : saved_show_state_); This reverts part of the change from the breaking CL: https://codereview.chromium.org/1130033003/diff/380001/ui/views/widget/widget.cc. The flow which the other CL was trying to fix is still working but only once after a widget is created.
Description was changed from ========== Corrects NativeWidgetAura state model to not minimize on every restore BUG=605998 ========== to ========== Corrects NativeWidgetAura state model to not minimize on every restore CL https://codereview.chromium.org/1130033003 made it so that a call to Show() can make window (show state NORMAL) only to minimize it again. This makes the state change sequence unnecessarily complex and prone to having visible artifacts such as the docking restore that is described in the bug. This CL keeps most of the changes from https://codereview.chromium.org/1130033003 but corrects Widget::Show() to only perform the extra call to Minimize() once after Widget::Init(). BUG=605998 TEST=Restore a previously minimized docked window. There should be only a fade-in animation. ==========
This is subtle. How about test coverage?
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/1905333004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905333004/40001
On 2016/04/22 20:23:24, sky wrote: > This is subtle. How about test coverage? Done. The new test would have failed reporting count of 2 in line 212 without the change in Widget::Show(). In reality the number of transitions is 3 because we restore in layout managers (in OnChildWindowVisibilityChanged), then minimize in NativeWidgetAura::ShowWithWindowState and then restore again in layout managers (in OnWindowActivated). So looks like it all worked as expected by some loose chain of side effects. It is interesting that more wasn't visibly broken.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/1905333004/diff/40001/ui/views/widget/native_... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/1905333004/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_aura_unittest.cc:157: TestWindowObserver(gfx::NativeWindow window) : window_(window) { explicit https://codereview.chromium.org/1905333004/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_aura_unittest.cc:191: params.parent = NULL; nullptr https://codereview.chromium.org/1905333004/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_aura_unittest.cc:195: std::unique_ptr<Widget> widget(new Widget()); nit: no need for a unique_ptr here, declare widget on the stack.
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/1905333004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905333004/60001
https://codereview.chromium.org/1905333004/diff/40001/ui/views/widget/native_... File ui/views/widget/native_widget_aura_unittest.cc (right): https://codereview.chromium.org/1905333004/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_aura_unittest.cc:157: TestWindowObserver(gfx::NativeWindow window) : window_(window) { On 2016/04/25 15:01:18, sky wrote: > explicit Done. https://codereview.chromium.org/1905333004/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_aura_unittest.cc:191: params.parent = NULL; On 2016/04/25 15:01:18, sky wrote: > nullptr Done. Here and elsewhere in this file. https://codereview.chromium.org/1905333004/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_aura_unittest.cc:195: std::unique_ptr<Widget> widget(new Widget()); On 2016/04/25 15:01:18, sky wrote: > nit: no need for a unique_ptr here, declare widget on the stack. Done.
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 varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1905333004/#ps60001 (title: "Corrects NativeWidgetAura state model to not minimize on every restore (nits)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905333004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905333004/60001
Message was sent while issue was closed.
Description was changed from ========== Corrects NativeWidgetAura state model to not minimize on every restore CL https://codereview.chromium.org/1130033003 made it so that a call to Show() can make window (show state NORMAL) only to minimize it again. This makes the state change sequence unnecessarily complex and prone to having visible artifacts such as the docking restore that is described in the bug. This CL keeps most of the changes from https://codereview.chromium.org/1130033003 but corrects Widget::Show() to only perform the extra call to Minimize() once after Widget::Init(). BUG=605998 TEST=Restore a previously minimized docked window. There should be only a fade-in animation. ========== to ========== Corrects NativeWidgetAura state model to not minimize on every restore CL https://codereview.chromium.org/1130033003 made it so that a call to Show() can make window (show state NORMAL) only to minimize it again. This makes the state change sequence unnecessarily complex and prone to having visible artifacts such as the docking restore that is described in the bug. This CL keeps most of the changes from https://codereview.chromium.org/1130033003 but corrects Widget::Show() to only perform the extra call to Minimize() once after Widget::Init(). BUG=605998 TEST=Restore a previously minimized docked window. There should be only a fade-in animation. ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Corrects NativeWidgetAura state model to not minimize on every restore CL https://codereview.chromium.org/1130033003 made it so that a call to Show() can make window (show state NORMAL) only to minimize it again. This makes the state change sequence unnecessarily complex and prone to having visible artifacts such as the docking restore that is described in the bug. This CL keeps most of the changes from https://codereview.chromium.org/1130033003 but corrects Widget::Show() to only perform the extra call to Minimize() once after Widget::Init(). BUG=605998 TEST=Restore a previously minimized docked window. There should be only a fade-in animation. ========== to ========== Corrects NativeWidgetAura state model to not minimize on every restore CL https://codereview.chromium.org/1130033003 made it so that a call to Show() can make window (show state NORMAL) only to minimize it again. This makes the state change sequence unnecessarily complex and prone to having visible artifacts such as the docking restore that is described in the bug. This CL keeps most of the changes from https://codereview.chromium.org/1130033003 but corrects Widget::Show() to only perform the extra call to Minimize() once after Widget::Init(). BUG=605998 TEST=Restore a previously minimized docked window. There should be only a fade-in animation. Committed: https://crrev.com/696f45a78fb690a7f19bdbafbb8ba84be06ee056 Cr-Commit-Position: refs/heads/master@{#389506} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/696f45a78fb690a7f19bdbafbb8ba84be06ee056 Cr-Commit-Position: refs/heads/master@{#389506} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
