Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1978163003:
Restore enabled state if a "modal" window is destroyed by an owner (Closed)
DescriptionIf the window handle is destroyed because of an owner being destroyed, ensure any disabled windows are enabled.
BUG=612117
Committed: https://crrev.com/3972b0ed7c8531f67f7e796fa6f939a70f2694f9
Cr-Commit-Position: refs/heads/master@{#394861}
Patch Set 1 #Patch Set 2 : Added unit test to ensure windows are re-enabled after an owner dialog/popup is destroyed #
Total comments: 10
Patch Set 3 : Updates based on feedback. #
Total comments: 2
Patch Set 4 : nit removal in preparation to land change #
Messages
Total messages: 27 (7 generated)
Description was changed from ========== If the window handle is destroyed because of an owner being destroyed, ensure any modal disablements are undone here. BUG= ========== to ========== If the window handle is destroyed because of an owner being destroyed, ensure any disabled windows are enabled. BUG=612117 ==========
kylixrd@chromium.org changed reviewers: + sadrul@chromium.org
This patch specifically fixes the issue from the referenced bug. The problem is that Windows will implicitly destroy an owned window. In this case, the Close() method is never called. RestoreEnabledIfNecessary() can be called multiple times without issue, so always calling it during a WM_NCDESTROY should be OK even if it had already been called from Close().
On 2016/05/16 22:48:01, kylix_rd wrote: > This patch specifically fixes the issue from the referenced bug. The problem is > that Windows will implicitly destroy an owned window. In this case, the Close() > method is never called. RestoreEnabledIfNecessary() can be called multiple times > without issue, so always calling it during a WM_NCDESTROY should be OK even if > it had already been called from Close(). While this patch will ensure the main Chrome window is properly enabled, it does not fix the problem that selecting "Ask Google for suggestions" in this context doesn't show the bubble window. That is a separate issue.
sadrul@chromium.org changed reviewers: + sky@chromium.org
--> sky@ for Win32 change
How about test coverage?
On 2016/05/17 16:50:29, sky wrote: > How about test coverage? Are there any existing window/modal/popup/ownership tests from which I may draw inspiration and direction?
On 2016/05/17 17:53:52, kylix_rd wrote: > On 2016/05/17 16:50:29, sky wrote: > > How about test coverage? > > Are there any existing window/modal/popup/ownership tests from which I may draw > inspiration and direction? I've located ui/views/widget/widget_interactive_uitest.cc, which appears to contain some top-level window modality tests. I think this should be a Windows-only test since it is an artifact of how HWND ownership works with popup windows.
Tests in interactive_ui_tests are those that can't be run in parallel. I don't think the code you're executing has this restriction, so I wouldn't bother placing in interactive_ui_tests, browser_tests should be fine. I'm not sure of existing tests that are close to what you want to exercise, but it shouldn't be that hard to create a couple of widgets with the appropriate modality to trigger the sequence you're fixing. On Tue, May 17, 2016 at 12:02 PM, <kylixrd@chromium.org> wrote: > On 2016/05/17 17:53:52, kylix_rd wrote: >> On 2016/05/17 16:50:29, sky wrote: >> > How about test coverage? >> >> Are there any existing window/modal/popup/ownership tests from which I may > draw >> inspiration and direction? > > I've located ui/views/widget/widget_interactive_uitest.cc, which appears to > contain some top-level window modality tests. I think this should be a > Windows-only test since it is an artifact of how HWND ownership works with > popup > windows. > > https://codereview.chromium.org/1978163003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OK, I've added a unit test. views_unittests seemed more appropriate since this is a generic "views/widgets" issue rather than a specific browser issue. views_unittests runs in parallel using multiple processes.
On 2016/05/19 16:06:21, kylix_rd wrote: > OK, I've added a unit test. views_unittests seemed more appropriate since this > is a generic "views/widgets" issue rather than a specific browser issue. > views_unittests runs in parallel using multiple processes. FYI, I locally reverted the hwnd_message_handler.cc patch and this test does catch the failure.
It's unfortunate the test has to be windows only. https://codereview.chromium.org/1978163003/diff/20001/ui/views/widget/widget_... File ui/views/widget/widget_unittest.cc (right): https://codereview.chromium.org/1978163003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:3636: ui::ModalType type_; nit: const https://codereview.chromium.org/1978163003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:3643: // remaining top-level windows should be re-enabled nit: end with '.'. https://codereview.chromium.org/1978163003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:3659: // This instance will be destroyed when the dialog is destroyed. 'this' is confusing here. As the comment is right above owner_dialog_widget, it seems like it goes with owner_dialog_widget, but that's on the stack. I believe you want this above dialog_delegate? https://codereview.chromium.org/1978163003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:3678: // Now Create an owned modal dialog nit: end with '.' https://codereview.chromium.org/1978163003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:3679: // Like above, this instance will be destroyed when the dialog is destroyed. Similar comment. Also, it would more clear if you use a new variable rather than reusing dialog_delegate.
On 2016/05/19 16:20:16, sky wrote: > It's unfortunate the test has to be windows only. Do other platforms exhibit this same modal window behavior? I kept it Windows only unless and until it can be demonstrated that other platforms also have this problem.
https://codereview.chromium.org/1978163003/diff/20001/ui/views/widget/widget_... File ui/views/widget/widget_unittest.cc (right): https://codereview.chromium.org/1978163003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:3636: ui::ModalType type_; On 2016/05/19 16:20:16, sky wrote: > nit: const Done. https://codereview.chromium.org/1978163003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:3643: // remaining top-level windows should be re-enabled On 2016/05/19 16:20:16, sky wrote: > nit: end with '.'. Done. https://codereview.chromium.org/1978163003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:3659: // This instance will be destroyed when the dialog is destroyed. On 2016/05/19 16:20:16, sky wrote: > 'this' is confusing here. As the comment is right above owner_dialog_widget, it > seems like it goes with owner_dialog_widget, but that's on the stack. I believe > you want this above dialog_delegate? Done. https://codereview.chromium.org/1978163003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:3678: // Now Create an owned modal dialog On 2016/05/19 16:20:16, sky wrote: > nit: end with '.' Done. https://codereview.chromium.org/1978163003/diff/20001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:3679: // Like above, this instance will be destroyed when the dialog is destroyed. On 2016/05/19 16:20:16, sky wrote: > Similar comment. Also, it would more clear if you use a new variable rather than > reusing dialog_delegate. Done.
Not sure, but it's always nice to have more coverage. LGTM
On 2016/05/19 16:48:32, sky wrote: > Not sure, but it's always nice to have more coverage. LGTM No doubt.
On 2016/05/19 16:49:52, kylix_rd wrote: > On 2016/05/19 16:48:32, sky wrote: > > Not sure, but it's always nice to have more coverage. LGTM > > No doubt. I'm getting pressure to land this fix for M51. sadrul@ are you OK with this change?
sky@'s approval is sufficient; you don't need my review (I meant to remove myself from the reviewer list before, but rietveld helpfully added me back). Having said that, lgtm with one nit: https://codereview.chromium.org/1978163003/diff/40001/ui/views/widget/widget_... File ui/views/widget/widget_unittest.cc (right): https://codereview.chromium.org/1978163003/diff/40001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:3716: #endif #endif // defined(OS_WIN)
https://codereview.chromium.org/1978163003/diff/40001/ui/views/widget/widget_... File ui/views/widget/widget_unittest.cc (right): https://codereview.chromium.org/1978163003/diff/40001/ui/views/widget/widget_... ui/views/widget/widget_unittest.cc:3716: #endif On 2016/05/19 19:43:57, sadrul wrote: > #endif // defined(OS_WIN) Done.
The CQ bit was checked by kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1978163003/#ps60001 (title: "nit removal in preparation to land change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1978163003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1978163003/60001
Message was sent while issue was closed.
Description was changed from ========== If the window handle is destroyed because of an owner being destroyed, ensure any disabled windows are enabled. BUG=612117 ========== to ========== If the window handle is destroyed because of an owner being destroyed, ensure any disabled windows are enabled. BUG=612117 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== If the window handle is destroyed because of an owner being destroyed, ensure any disabled windows are enabled. BUG=612117 ========== to ========== If the window handle is destroyed because of an owner being destroyed, ensure any disabled windows are enabled. BUG=612117 Committed: https://crrev.com/3972b0ed7c8531f67f7e796fa6f939a70f2694f9 Cr-Commit-Position: refs/heads/master@{#394861} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3972b0ed7c8531f67f7e796fa6f939a70f2694f9 Cr-Commit-Position: refs/heads/master@{#394861} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
