DesktopAura: Track windows "owned" via the DesktopWindowTreeHost

DesktopAura: Track windows "owned" via the DesktopWindowTreeHost

Widget::GetAllOwnedWidgets() is currently populated from Aura child
windows (in the same WindowTreeHost), and "transient" child windows
added using wm::AddTransientChild(). However, only NativeWidgetAura uses
wm::AddTransientChild(). This means that 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 "GetAllOwnedWidgets()".

Ownership of these top-level Widgets is managed in mus-, win-, or
x11-specific code.

To fix, add a method to DesktopWindowTreeHost that allows an
implementation  to provide a vector<DesktopWindowTreeHost*> of windows
it owns. Widgets are usually associated with the aura::Window that is a
grandchild of the aura::Window associated with the WindowTreeHost - this
is done by DesktopNativeWidgetAura, so a method for NativeWidgetPrivate
to obtain Widgets is also added.

BUG=683808, 654151

[tapted, 2017-01-23 03:00:29]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 03:00:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 03:13:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 03:13:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tapted, 2017-01-23 03:42:11]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 03:42:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 04:31:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 04:31:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-01-23 05:15:13]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 05:15:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-01-23 05:38:57]

Description was changed from

==========
DesktopAura: Track windows "owned" via the WindowTreeHost

Add TopLevelChildWindowDestoyedWithParent

BUG=
==========

to

==========
DesktopAura: Track windows "owned" via the DesktopWindowTreeHost

Add TopLevelChildWindowDestoyedWithParent

BUG=
==========

[tapted, 2017-01-23 06:08:19]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 06:08:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-01-23 06:21:24]

Description was changed from

==========
DesktopAura: Track windows "owned" via the DesktopWindowTreeHost

Add TopLevelChildWindowDestoyedWithParent

BUG=
==========

to

==========
DesktopAura: Track windows "owned" via the DesktopWindowTreeHost

Add TopLevelChildWindowDestoyedWithParent

BUG=683808
==========

[tapted, 2017-01-23 06:23:37]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 06:23:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 06:44:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 06:44:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-01-23 07:47:16]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 07:47:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 08:04:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 08:04:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-01-23 09:06:49]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 09:07:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-01-23 10:17:40]

Patchset #1 (id:1) has been deleted

[tapted, 2017-01-23 10:18:38]

Patchset #4 (id:80001) has been deleted

[tapted, 2017-01-23 10:18:49]

Patchset #4 (id:100001) has been deleted

[tapted, 2017-01-23 10:25:28]

Description was changed from

==========
DesktopAura: Track windows "owned" via the DesktopWindowTreeHost

Add TopLevelChildWindowDestoyedWithParent

BUG=683808
==========

to

==========
DesktopAura: Track windows "owned" via the DesktopWindowTreeHost

Widget::GetAllOwnedWidgets() is currently populated from aura child
windows (in the same WindowTreeHost), and "transient" child windows
added using wm::AddTransientChild(). However, only NativeWidgetAura uses
wm::AddTransientChild(). This means that 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 "GetAllOwnedWidgets()".

Ownership of these Widgets is managed in mus-, win-, or x11-specific
code.

To fix, call wm::AddTransientChild() when the parent/child relationship
is created in the platform-specific DesktopWindowTreeHost
implementations.

BUG=683808
==========

[tapted, 2017-01-23 10:37:24]

Description was changed from

==========
DesktopAura: Track windows "owned" via the DesktopWindowTreeHost

Widget::GetAllOwnedWidgets() is currently populated from aura child
windows (in the same WindowTreeHost), and "transient" child windows
added using wm::AddTransientChild(). However, only NativeWidgetAura uses
wm::AddTransientChild(). This means that 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 "GetAllOwnedWidgets()".

Ownership of these Widgets is managed in mus-, win-, or x11-specific
code.

To fix, call wm::AddTransientChild() when the parent/child relationship
is created in the platform-specific DesktopWindowTreeHost
implementations.

BUG=683808
==========

to

==========
DesktopAura: Track windows "owned" via the DesktopWindowTreeHost

Widget::GetAllOwnedWidgets() is currently populated from Aura child
windows (in the same WindowTreeHost), and "transient" child windows
added using wm::AddTransientChild(). However, only NativeWidgetAura uses
wm::AddTransientChild(). This means that 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 "GetAllOwnedWidgets()".

Ownership of these top-level Widgets is managed in mus-, win-, or
x11-specific code.

To fix, call wm::AddTransientChild() when the parent/child relationship
is created in the platform-specific DesktopWindowTreeHost
implementations.

BUG=683808
==========

[tapted, 2017-01-23 10:52:54]

tapted@chromium.org changed reviewers:
+ sky@chromium.org

[tapted, 2017-01-23 10:52:55]

Hi sky - this is not ready to land yet (there's a test failure - just one, and
only Windows, so it might be solvable).

But I don't know much about these Transient windows - it's possible this
approach is opting into a bunch of stuff we don't want. Do you think trying to
use wm::AddTransientChild() for this is crazy?

An alternative may be to expose something on aura::WindowTreeHost like
GetAllChildHosts() so that native_widget_aura's GetAllOwnedWidgets() can look
there.

Or something else (perhaps making GetAllOwnedWidgets non-static so that
DesktopNativeWidgetAura can query DesktopWindowTreeHost for children).

Or I can explore ways to maybe not depend on Widget::GetAllOwnedWidgets() at all
(but currently it seems to report incorrect results when the owned window is
top-level).

More info in the linked http://crbug.com/683808

Thanks!

https://codereview.chromium.org/2645253002/diff/120001/chrome/test/base/view_...
File chrome/test/base/view_event_test_platform_part_aura.cc (right):

https://codereview.chromium.org/2645253002/diff/120001/chrome/test/base/view_...
chrome/test/base/view_event_test_platform_part_aura.cc:42: wm_state_ =
base::MakeUnique<wm::WMState>();
Some menu-related interactive_ui_tests fail with `Check failed:
TransientWindowStackingClient::instance_` without this. (Usually AuraTestHelper
adds this, but ViewEventTestBase doesn't use that).

https://codereview.chromium.org/2645253002/diff/120001/ui/views/widget/widget...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/2645253002/diff/120001/ui/views/widget/widget...
ui/views/widget/widget_unittest.cc:3118: EXPECT_EQ(1u,
owned_widgets.count(child_widget));
This fails on desktop linux and windows (and mus) without the
desktop_window_host_foo changes.

https://codereview.chromium.org/2645253002/diff/120001/ui/views/widget/widget...
ui/views/widget/widget_unittest.cc:3126: EXPECT_TRUE(child_destroyed);
This already passes - it's just to assert that the child is actually owned.

[commit-bot: I haz the power, 2017-01-23 11:07:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 11:07:40]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sky, 2017-01-23 16:56:42]

I worry that adding transients at the aura level is going to cause all sorts of
side effects. I would rather see us special case for GetAllOwnedWidgets for
DesktopNativeWidgetAura.

[tapted, 2017-01-24 00:51:47]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 00:52:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 01:31:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 01:31:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-01-24 01:44:57]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 01:45:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-01-24 02:48:51]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 02:49:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-01-24 02:49:58]

Patchset #5 (id:140001) has been deleted

[tapted, 2017-01-24 02:50:27]

Patchset #5 (id:160001) has been deleted

[commit-bot: I haz the power, 2017-01-24 03:53:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 03:53:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-01-25 03:46:42]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 03:47:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 04:48:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 04:48:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-01-25 05:25:17]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 05:25:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-01-25 05:33:23]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 05:33:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 06:05:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 06:05:19]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tapted, 2017-01-25 08:00:19]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 08:00:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 08:55:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 08:55:56]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-01-25 11:04:45]

Patchset #7 (id:220001) has been deleted

[tapted, 2017-01-25 11:04:56]

Patchset #7 (id:240001) has been deleted

[tapted, 2017-01-25 11:21:59]

Description was changed from

==========
DesktopAura: Track windows "owned" via the DesktopWindowTreeHost

Widget::GetAllOwnedWidgets() is currently populated from Aura child
windows (in the same WindowTreeHost), and "transient" child windows
added using wm::AddTransientChild(). However, only NativeWidgetAura uses
wm::AddTransientChild(). This means that 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 "GetAllOwnedWidgets()".

Ownership of these top-level Widgets is managed in mus-, win-, or
x11-specific code.

To fix, call wm::AddTransientChild() when the parent/child relationship
is created in the platform-specific DesktopWindowTreeHost
implementations.

BUG=683808
==========

to

==========
DesktopAura: Track windows "owned" via the DesktopWindowTreeHost

Widget::GetAllOwnedWidgets() is currently populated from Aura child
windows (in the same WindowTreeHost), and "transient" child windows
added using wm::AddTransientChild(). However, only NativeWidgetAura uses
wm::AddTransientChild(). This means that 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 "GetAllOwnedWidgets()".

Ownership of these top-level Widgets is managed in mus-, win-, or
x11-specific code.

To fix, add a method to DesktopWindowTreeHost that allows an
implementation  to provide a vector<DesktopWindowTreeHost*> of windows
it owns. Widgets are usually associated with the aura::Window that is a
grandchild of the aura::Window associated with the WindowTreeHost - this
is done by DesktopNativeWidgetAura, so a method for NativeWidgetPrivate
to obtain Widgets is also added.

BUG=683808
==========

[tapted, 2017-01-25 11:23:03]

Hi sky, PTAL. Turned out to be rather tricky... maybe you see a simpler approach
:)

https://codereview.chromium.org/2645253002/diff/260001/ui/views/accessibility...
File ui/views/accessibility/native_view_accessibility.cc (right):

https://codereview.chromium.org/2645253002/diff/260001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility.cc:258:
Widget::GetAllOwnedWidgets(widget->GetNativeView(), &child_widgets, false);
I don't think the concept of "ownership" here extends to top-level Widgets. That
is, I think a11y clients would get confused if we started populating a11y info
from another toplevel window in the OS. So, I added the extra argument.

https://codereview.chromium.org/2645253002/diff/260001/ui/views/mus/desktop_w...
File ui/views/mus/desktop_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2645253002/diff/260001/ui/views/mus/desktop_w...
ui/views/mus/desktop_window_tree_host_mus.cc:188:
NativeWidgetAura::RegisterNativeWidgetForWindow(desktop_native_widget_aura,
I tried moving this up to DesktopNativeWidgetAura to be shared by all
DesktopWindowTreeHosts in Patchsets 5/6. The code was much simpler, but it
resulted in a very obscure ASAN error in a single test. It looks like the
aura::Window that WebContentsViewAura creates directly is triggering the
destruction of a Widget that it didn't create in a way that causes some weird
lifetime problems. So, there may be a trap lurking here for mus in the future..

https://codereview.chromium.org/2645253002/diff/260001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/2645253002/diff/260001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:199: };
(this struct was orphaned, but it was a handy link back to a revision that was
using `EnumThreadWindows` already :)

[sky, 2017-01-25 16:41:24]

Sadly I can't think of an easier approach.

https://codereview.chromium.org/2645253002/diff/260001/ui/views/accessibility...
File ui/views/accessibility/native_view_accessibility.cc (right):

https://codereview.chromium.org/2645253002/diff/260001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility.cc:258:
Widget::GetAllOwnedWidgets(widget->GetNativeView(), &child_widgets, false);
On 2017/01/25 11:23:03, tapted wrote:
> I don't think the concept of "ownership" here extends to top-level Widgets.
That
> is, I think a11y clients would get confused if we started populating a11y info
> from another toplevel window in the OS. So, I added the extra argument.

It's my understanding GetAllOwnedWidget is only called by accessibility. Did you
need to start using it outside of that?

https://codereview.chromium.org/2645253002/diff/260001/ui/views/mus/desktop_w...
File ui/views/mus/desktop_window_tree_host_mus.cc (right):

https://codereview.chromium.org/2645253002/diff/260001/ui/views/mus/desktop_w...
ui/views/mus/desktop_window_tree_host_mus.cc:188:
NativeWidgetAura::RegisterNativeWidgetForWindow(desktop_native_widget_aura,
On 2017/01/25 11:23:03, tapted wrote:
> I tried moving this up to DesktopNativeWidgetAura to be shared by all
> DesktopWindowTreeHosts in Patchsets 5/6. The code was much simpler, but it
> resulted in a very obscure ASAN error in a single test. It looks like the
> aura::Window that WebContentsViewAura creates directly is triggering the
> destruction of a Widget that it didn't create in a way that causes some weird
> lifetime problems. So, there may be a trap lurking here for mus in the
future..

Interesting...

https://codereview.chromium.org/2645253002/diff/260001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/2645253002/diff/260001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1187: return
std::vector<DesktopWindowTreeHost*>(window_children_.begin(),
Do you need to recurse in all these implementations?

[tapted, 2017-01-27 03:29:51]

Description was changed from

==========
DesktopAura: Track windows "owned" via the DesktopWindowTreeHost

Widget::GetAllOwnedWidgets() is currently populated from Aura child
windows (in the same WindowTreeHost), and "transient" child windows
added using wm::AddTransientChild(). However, only NativeWidgetAura uses
wm::AddTransientChild(). This means that 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 "GetAllOwnedWidgets()".

Ownership of these top-level Widgets is managed in mus-, win-, or
x11-specific code.

To fix, add a method to DesktopWindowTreeHost that allows an
implementation  to provide a vector<DesktopWindowTreeHost*> of windows
it owns. Widgets are usually associated with the aura::Window that is a
grandchild of the aura::Window associated with the WindowTreeHost - this
is done by DesktopNativeWidgetAura, so a method for NativeWidgetPrivate
to obtain Widgets is also added.

BUG=683808
==========

to

==========
DesktopAura: Track windows "owned" via the DesktopWindowTreeHost

Widget::GetAllOwnedWidgets() is currently populated from Aura child
windows (in the same WindowTreeHost), and "transient" child windows
added using wm::AddTransientChild(). However, only NativeWidgetAura uses
wm::AddTransientChild(). This means that 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 "GetAllOwnedWidgets()".

Ownership of these top-level Widgets is managed in mus-, win-, or
x11-specific code.

To fix, add a method to DesktopWindowTreeHost that allows an
implementation  to provide a vector<DesktopWindowTreeHost*> of windows
it owns. Widgets are usually associated with the aura::Window that is a
grandchild of the aura::Window associated with the WindowTreeHost - this
is done by DesktopNativeWidgetAura, so a method for NativeWidgetPrivate
to obtain Widgets is also added.

BUG=683808, 654151
==========

[tapted, 2017-01-27 04:16:40]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 04:17:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 05:31:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 05:31:14]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-01-27 05:34:56]

https://codereview.chromium.org/2645253002/diff/260001/ui/views/accessibility...
File ui/views/accessibility/native_view_accessibility.cc (right):

https://codereview.chromium.org/2645253002/diff/260001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility.cc:258:
Widget::GetAllOwnedWidgets(widget->GetNativeView(), &child_widgets, false);
On 2017/01/25 16:41:24, sky wrote:
> On 2017/01/25 11:23:03, tapted wrote:
> > I don't think the concept of "ownership" here extends to top-level Widgets.
> That
> > is, I think a11y clients would get confused if we started populating a11y
info
> > from another toplevel window in the OS. So, I added the extra argument.
> 
> It's my understanding GetAllOwnedWidget is only called by accessibility. Did
you
> need to start using it outside of that?

Yes - it forms part of http://crbug.com/654151 (added that to BUG=). In the
latest patchset, I've included the change to test_browser_dialog.cc that would
use GetAllOwnedWidget() to give specific context.


There's a bit more in that first BUG= too, i.e. http://crbug.com/683808#c3 says,

"""Note this became an problem for http://crbug.com/681049 because the
dialog-testing harness (TestBrowserDialog) wants to use
Widget::GetAllOwnedWidgets() to detect when a dialog is added to the Chrome
Browser window."""

That is, currently TestBrowserDialog needs a way to (1.) verify that a test case
added a dialog to Chrome, and (2.) close that dialog. GetAllOwnedWidgets() comes
really close to solving this, but didn't work on Windows 10 where
chrome_views_delegate.cc detects Aero and makes a top-level window.


Of course, that's still testing code :/. I didn't realise how much thread I'd be
pulling on when I started down this path.

It does seem that `GetAllOwnedWidgets()` isn't fulfilling its contract right
now, so maybe this is the right approach.

An alternative may be to add some Win- and X11- specific code to
test_browser_dialog.cc that scans through OS window handles and checks for new
Widgets that appear there. 

E.g. 
for (XID xid : DWTHX11PrivateApi::open_windows()) {
 
Widget::GetWidgetForNativeWindow(DestkopWindowTreeHostX11::GetContentWindowForXID(xid));
}

and... something more verbose for Windows.

Not sure how to do this on mus -- probably something involving
views::MusClient::Get()->window_tree_client()->GetRoots() -- which is something
I stole from GetLocalProcessWindowAtPointMus(). I note that appears to be
relying on the duplicate `RegisterNativeWidgetForWindow` entry, I think it could
instead use Widgets::GetAllOwnedWidgets() (I don't think it would need the
changes in this CL, however).

https://codereview.chromium.org/2645253002/diff/260001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/2645253002/diff/260001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1187: return
std::vector<DesktopWindowTreeHost*>(window_children_.begin(),
On 2017/01/25 16:41:24, sky wrote:
> Do you need to recurse in all these implementations?

No - this approach means recursion is only necessary in the implementation of
NativeWidgetPrivate::GetAllOwnedWidgets() which I think reduces complexity
somewhat. Added a comment for GetOwnedTopLevelHosts() so that this is clear.

[tapted, 2017-01-30 10:33:04]

https://codereview.chromium.org/2645253002/diff/260001/ui/views/accessibility...
File ui/views/accessibility/native_view_accessibility.cc (right):

https://codereview.chromium.org/2645253002/diff/260001/ui/views/accessibility...
ui/views/accessibility/native_view_accessibility.cc:258:
Widget::GetAllOwnedWidgets(widget->GetNativeView(), &child_widgets, false);
On 2017/01/27 05:34:55, tapted wrote:
> An alternative may be to add some Win- and X11- specific code to
> test_browser_dialog.cc that scans through OS window handles and checks for new
> Widgets that appear there. 

/* snip */

In case this pops in your queue first, I've explored this alternative at
https://codereview.chromium.org/2660813002/