DesktopAura: owned (i.e. parented) "top level" windows are not included in Widget::GetAllOwnedWidgets() |
|||||
Issue descriptionChrome Version : 57.0.2984.0 OS Version: OS X 10.12.2 The following currently fails on Windows 10 (and Windows 7 when run locally), but not on Linux, ChromeOS, or Mac: Widget* parent_widget = CreateWidget(); Widget* child_widget = CreateWidgetWithParent(parent_widget->GetNativeView()); std::set<Widget*> owned_widgets; Widget::GetAllOwnedWidgets(parent_widget->GetNativeView(), &owned_widgets); EXPECT_EQ(1u, owned_widgets.count(child_widget)); See also test errors at http://crbug.com/681049#c5 - these were Windows 10 only. The code fails because, on Windows 10, there is always a compositing window manager (on Windows 7, the trybots use a virtual framebuffer, so they get non-toplevel windows). This causes ChromeViewsDelegate::OnBeforeWidgetInit() to create a "toplevel" window so that it interacts nicely with transparency and things. But, with a top-level window, the child window no longer appears in Widget::GetAllOwnedWidgets(). However, the child window is still "owned": destroying/closing |parent_widget| will also destroy |child_widget|. So the child should appear in GetAllOwnedWidgets(). On Linux (which uses non-toplevel Widgets more often) and ChromeOS, GetAllOwnedWidgets() populates from wm::GetTransientChildren(parent_window) in native_widget_aura.cc. However, only NativeWidgetAura uses wm::AddTransientChild(). I think to fix, we can rope in "toplevel" children into the TransientWindowManager: the functionality TransientWindowManager provides seems to make sense at a high level for top_level windows as well as non-top-level windows (i.e. in the same WindowTreeHost). CL for this - https://codereview.chromium.org/2645253002/
,
Jan 23 2017
OS Mac->Windows. But Linux could feasibly make top-level windows too. E.g. for menus.
,
Jan 23 2017
Note this became an problem for Issue 681049 because the dialog-testing harness (TestBrowserDialog) wants to use Widget::GetAllOwnedWidgets() to detect when a dialog is added to the Chrome Browser window.
,
Jan 30 2017
Alternate fix: https://codereview.chromium.org/2660813002/
,
Jan 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b39bb8d4942bbf79cbd36f24ae8a861e3f78fa4 commit 3b39bb8d4942bbf79cbd36f24ae8a861e3f78fa4 Author: tapted <tapted@chromium.org> Date: Tue Jan 31 11:01:38 2017 Add WidgetTest::GetAllWidgets() to find dialogs created by TestBrowserDialog. TestBrowserDialog currently fails to detect some browser dialogs on Windows 10. This is because TestBrowserDialog uses the set difference from calls to Widget::GetAllChildWidgets() to detect when a dialog is added to a browser window. On Aura, this is populated from Aura child windows (in the same WindowTreeHost). However, a parented Widget using a DesktopNativeWidgetAura (e.g. for a top-level Widget on Windows Aero), rather than a NativeWidgetAura, will not be reported in its parent window's "GetAllChildWidgets()" (nor in Widget::GetAllOwnedWidgets()). Ownership of these top-level Widgets is managed in mus-, win-, or x11-specific code. To fix, add WidgetTest::GetAllWidgets() which just returns all Widget*s that can be found in the current process. In addition, this approach is able to detect Widgets created with no parent at all, such as the HungRenderer dialog. BUG= 683808 , 654151 Review-Url: https://codereview.chromium.org/2660813002 Cr-Commit-Position: refs/heads/master@{#447216} [modify] https://crrev.com/3b39bb8d4942bbf79cbd36f24ae8a861e3f78fa4/chrome/browser/ui/test/test_browser_dialog.cc [modify] https://crrev.com/3b39bb8d4942bbf79cbd36f24ae8a861e3f78fa4/chrome/browser/ui/test/test_browser_dialog.h [modify] https://crrev.com/3b39bb8d4942bbf79cbd36f24ae8a861e3f78fa4/ui/aura/test/aura_test_helper.cc [modify] https://crrev.com/3b39bb8d4942bbf79cbd36f24ae8a861e3f78fa4/ui/aura/test/aura_test_helper.h [modify] https://crrev.com/3b39bb8d4942bbf79cbd36f24ae8a861e3f78fa4/ui/views/BUILD.gn [modify] https://crrev.com/3b39bb8d4942bbf79cbd36f24ae8a861e3f78fa4/ui/views/test/widget_test.h [modify] https://crrev.com/3b39bb8d4942bbf79cbd36f24ae8a861e3f78fa4/ui/views/test/widget_test_aura.cc [modify] https://crrev.com/3b39bb8d4942bbf79cbd36f24ae8a861e3f78fa4/ui/views/test/widget_test_mac.mm [add] https://crrev.com/3b39bb8d4942bbf79cbd36f24ae8a861e3f78fa4/ui/views/test/widget_test_unittest.cc
,
Jan 31 2017
The summary is still true -- i.e. owned "top level" windows are not included in Widget::GetAllOwnedWidgets() -- but #c5 has unblocked tests that would have required this functionality with an alternative. So closing this.
,
Feb 1 2017
Even with this change, tests utilizing the TestBrowserDialog framework can fail on Linux. For example, the test added in https://codereview.chromium.org/2650583012/ fails with the following error message on my Linux workstation: ../../chrome/browser/ui/test/test_browser_dialog.cc:116: Failure Value of: added.size() Actual: 0 Expected: 1u Which is: 1
,
Feb 2 2017
figured out a fix for the desktop linux thing, but I want to add a test for it - https://codereview.chromium.org/2663163004/
,
Feb 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f0c12ecaa44ccc4511ea33c1705dec4caa972304 commit f0c12ecaa44ccc4511ea33c1705dec4caa972304 Author: tapted <tapted@chromium.org> Date: Tue Feb 07 00:04:50 2017 Run the "Relaunch Chrome" dialog test everywhere. For Linux, this requires an update to WidgetTest::GetAllWidgets() (added in r447216) in order to include Widgets created with a parent aura::Window that is a DesktopWindowTreeHost's root rather than the host's content window. BUG= 684167 , 683808 Review-Url: https://codereview.chromium.org/2663163004 Cr-Commit-Position: refs/heads/master@{#448452} [modify] https://crrev.com/f0c12ecaa44ccc4511ea33c1705dec4caa972304/chrome/browser/ui/update_chrome_dialog_browsertest.cc [modify] https://crrev.com/f0c12ecaa44ccc4511ea33c1705dec4caa972304/ui/views/test/widget_test_aura.cc [modify] https://crrev.com/f0c12ecaa44ccc4511ea33c1705dec4caa972304/ui/views/test/widget_test_unittest.cc
,
Feb 7 2017
This should be resolved I'll follow up in Issue 654151 with documentation for the dialog testing framework. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by tapted@chromium.org
, Jan 23 2017