Change Widget::GetAllOwnedWidgets to return child widgets as well

Change Widget::GetAllOwnedWidgets to return child widgets as well

Whether or not Windows Aero is enabled on Windows platforms changes
whether a dialog is owned as a transient child or a child widget, and
this distinction is not useful. Changed Widget::GetAllOwnedWidgets to
return child widgets as well so widgets like dialogs can be detected in
tests and at runtime, without special checks for whether Aero is
currently enabled.

This also fixes the test AppListControllerAppInfoDialogBrowserTest.

BUG=378251
Committed: https://crrev.com/20f85cd78d717d58547ba3e8a42ada14108b950e
Cr-Commit-Position: refs/heads/master@{#301055}

[sashab, 2014-09-22 05:59:58]

Patchset #1 (id:1) has been deleted

[sashab, 2014-10-15 06:29:15]

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

[sashab, 2014-10-15 06:29:16]

tapted wdyt?

[tapted, 2014-10-15 23:59:24]

It looks like the only other user is
NativeViewAccessibilityWin::PopulateChildWidgetVector()

But that will need to be updated.

https://codereview.chromium.org/588113002/diff/40001/ui/views/widget/native_w...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/588113002/diff/40001/ui/views/widget/native_w...
ui/views/widget/native_widget_aura.cc:1106: void
NativeWidgetPrivate::GetAllOwnedWidgets(gfx::NativeView native_view,
you'll need to update the comment in widget.h

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/wi...

https://codereview.chromium.org/588113002/diff/40001/ui/views/widget/native_w...
ui/views/widget/native_widget_aura.cc:1120: // Add all owned windows.
owned windows -> child windows

https://codereview.chromium.org/588113002/diff/40001/ui/views/widget/native_w...
ui/views/widget/native_widget_aura.cc:1122: for
(aura::Window::Windows::const_iterator i = child_windows.begin();
might as well start using C++11. Pretty sure this becomes just

for (const auto& child : native_view->children())
  GetAllChildWidgets(child, owned);

the other loop in this function should be updated too

[sashab, 2014-10-20 03:03:01]

https://codereview.chromium.org/588113002/diff/40001/ui/views/widget/native_w...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/588113002/diff/40001/ui/views/widget/native_w...
ui/views/widget/native_widget_aura.cc:1106: void
NativeWidgetPrivate::GetAllOwnedWidgets(gfx::NativeView native_view,
On 2014/10/15 23:59:24, tapted wrote:
> you'll need to update the comment in widget.h
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/wi...

Done to just remove 'non-child'. Didn't think I needed to be explicit about
including children.

https://codereview.chromium.org/588113002/diff/40001/ui/views/widget/native_w...
ui/views/widget/native_widget_aura.cc:1120: // Add all owned windows.
On 2014/10/15 23:59:24, tapted wrote:
> owned windows -> child windows

Done.

https://codereview.chromium.org/588113002/diff/40001/ui/views/widget/native_w...
ui/views/widget/native_widget_aura.cc:1122: for
(aura::Window::Windows::const_iterator i = child_windows.begin();
On 2014/10/15 23:59:24, tapted wrote:
> might as well start using C++11. Pretty sure this becomes just
> 
> for (const auto& child : native_view->children())
>   GetAllChildWidgets(child, owned);
> 
> the other loop in this function should be updated too

Done. Git cl format wants to change "x->y()" to "x -> y()" but I prevented it...
Looking for a place to file bugs on clang-format.

https://codereview.chromium.org/588113002/diff/80001/ui/views/accessibility/n...
File ui/views/accessibility/native_view_accessibility_win.cc (left):

https://codereview.chromium.org/588113002/diff/80001/ui/views/accessibility/n...
ui/views/accessibility/native_view_accessibility_win.cc:1494:
Widget::GetAllChildWidgets(widget->GetNativeView(), &child_widgets);
Originally, this function was taking the concatenation of all the child and
owned widgets. Now it should still get both the child and owned widgets - the
only widget missing is the one for widget->GetNativeView() itself. However, see
below...

https://codereview.chromium.org/588113002/diff/80001/ui/views/accessibility/n...
ui/views/accessibility/native_view_accessibility_win.cc:1499: if (child_widget
== widget)
... their own widget is ignored anyway. Leaving this if-statement in for clarity
but it shouldn't change the behaviour now that this widget is missing from the
list.

[tapted, 2014-10-20 04:40:05]

lgtm after updating the CL description. It needs to give rationale for why this
is better than just having the test do the same as what
NativeViewAccessibilityWin was doing. i.e. something like ~Whether or not
Windows' Aero is enabled changes whether a dialog is owned as a transient child
or owned as a child widget, and the distinction is not useful

https://codereview.chromium.org/588113002/diff/80001/ui/views/accessibility/n...
File ui/views/accessibility/native_view_accessibility_win.cc (left):

https://codereview.chromium.org/588113002/diff/80001/ui/views/accessibility/n...
ui/views/accessibility/native_view_accessibility_win.cc:1499: if (child_widget
== widget)
On 2014/10/20 03:03:01, sasha_b wrote:
> ... their own widget is ignored anyway. Leaving this if-statement in for
clarity
> but it shouldn't change the behaviour now that this widget is missing from the
> list.

If it's just for clarity, something like

DCHECK_NE(widget, child_widget);

would be more appropriate

https://codereview.chromium.org/588113002/diff/80001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/588113002/diff/80001/ui/views/widget/widget.h...
ui/views/widget/widget.h:321: // Returns all Widgets owned by |native_view|.
The distinction that GetAllOwnedWidgets isn't returning |native_view|'s widget
(versus GetAllChildWidgets above) is probably subtle enough that it deserves a
mention here.

[sashab, 2014-10-20 05:15:34]

https://codereview.chromium.org/588113002/diff/80001/ui/views/accessibility/n...
File ui/views/accessibility/native_view_accessibility_win.cc (left):

https://codereview.chromium.org/588113002/diff/80001/ui/views/accessibility/n...
ui/views/accessibility/native_view_accessibility_win.cc:1499: if (child_widget
== widget)
On 2014/10/20 04:40:05, tapted (OOO until Nov 1st) wrote:
> On 2014/10/20 03:03:01, sasha_b wrote:
> > ... their own widget is ignored anyway. Leaving this if-statement in for
> clarity
> > but it shouldn't change the behaviour now that this widget is missing from
the
> > list.
> 
> If it's just for clarity, something like
> 
> DCHECK_NE(widget, child_widget);
> 
> would be more appropriate

Good idea; done.

https://codereview.chromium.org/588113002/diff/80001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/588113002/diff/80001/ui/views/widget/widget.h...
ui/views/widget/widget.h:321: // Returns all Widgets owned by |native_view|.
On 2014/10/20 04:40:05, tapted (OOO until Nov 1st) wrote:
> The distinction that GetAllOwnedWidgets isn't returning |native_view|'s widget
> (versus GetAllChildWidgets above) is probably subtle enough that it deserves a
> mention here.

Done.

[sashab, 2014-10-21 21:41:57]

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

[sky, 2014-10-22 00:11:35]

https://codereview.chromium.org/588113002/diff/120001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/588113002/diff/120001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:1111: for (const auto& transient_child :
wm::GetTransientChildren(native_view)) {
nit: IMO if it were me I would change auto to aura::Window*: clearer and not
much more code. Same comment on 1120.

https://codereview.chromium.org/588113002/diff/120001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/588113002/diff/120001/ui/views/widget/widget....
ui/views/widget/widget.h:318: static void GetAllChildWidgets(gfx::NativeView
native_view,
Is it possible to merge GetAllChildWidget and GetAllOwnedWidgets?

[sashab, 2014-10-23 00:33:30]

https://codereview.chromium.org/588113002/diff/120001/ui/views/widget/native_...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/588113002/diff/120001/ui/views/widget/native_...
ui/views/widget/native_widget_aura.cc:1111: for (const auto& transient_child :
wm::GetTransientChildren(native_view)) {
On 2014/10/22 00:11:34, sky wrote:
> nit: IMO if it were me I would change auto to aura::Window*: clearer and not
> much more code. Same comment on 1120.

Done.

[sky, 2014-10-23 02:29:18]

https://codereview.chromium.org/588113002/diff/140001/ui/views/controls/nativ...
File ui/views/controls/native/native_view_host.cc (right):

https://codereview.chromium.org/588113002/diff/140001/ui/views/controls/nativ...
ui/views/controls/native/native_view_host.cc:215:
Widget::GetAllChildAndOwnedWidgets(native_view(), &widgets);
GAH! Sorry, didn't realize these other windows don't need the owned ones. You'll
have to go back to two functions. sorry.

[sky, 2014-10-23 16:58:31]

LGTM

https://codereview.chromium.org/588113002/diff/160001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/588113002/diff/160001/ui/views/widget/widget....
ui/views/widget/widget.h:321: // Returns all Widgets owned by |native_view|, not
including itself.
Explicitly say this includes all child widgets too.

[sky, 2014-10-23 16:58:38]

And sorry for the run around.

[sashab, 2014-10-24 01:03:56]

Not at all :) Just reverted one commit in my history haha. And good to know all
the callsites of these functions anyway - maybe down the track they really
should be separated, or we should clarify the meaning of owned vs child widgtes.

https://codereview.chromium.org/588113002/diff/160001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/588113002/diff/160001/ui/views/widget/widget....
ui/views/widget/widget.h:321: // Returns all Widgets owned by |native_view|, not
including itself.
On 2014/10/23 16:58:31, sky wrote:
> Explicitly say this includes all child widgets too.

Was thinking about this :) Done.

[sashab, 2014-10-24 01:04:10]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-10-24 01:05:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/588113002/180001

[commit-bot: I haz the power, 2014-10-24 05:12:56]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2014-10-24 05:13:47]

Patchset 9 (id:??) landed as
https://crrev.com/20f85cd78d717d58547ba3e8a42ada14108b950e
Cr-Commit-Position: refs/heads/master@{#301055}