MacViews: Fix child window z-order and SetBounds

MacViews: Fix child window z-order and SetBounds

The search box in the app list is a child views::Widget (so that it can
animate over WebContents). Its position and z-order were broken on
MacViews.

The z-order issue boils down to a quirk in Cocoa where it is unable to
correctly process a call to -[NSWindow orderWindow:relativeTo:] that is
made in response to a visibility change in the same iteration over the
event loop.

To fix, make use of -[NSWindow addChildWindow:]. However, we must work
around the issues it has. Specifically, it falls apart when we want to
have hidden windows: Adding a child window always shows it, and hiding a
child window detaches it.

To fix that, child windows are removed from -[NSWindow childWindows]
when ordered out and added back when the NSWindow becomes visible again.

Then, to ensure visibility changes not initiated by
BridgedNativeWidget::SetVisibilityState() correctly manage these
children, calls to NotifyVisibilityChangeDown() are moved to a single
call in OnVisibilityChangedTo(), called from the NSWindowDelegate after
the visibility change is fed through to Cocoa.

The x/y position bug was simpler. Mac was just missing the logic in
aura::Window::SetBoundsInternal() that offsets the bounds when there is
a parent.

Cross-platform regression tests are added for these.

The app_list_unittest
AppListViewTestDesktopInstance/AppListViewTestDesktop.SearchResultsTest/2
also passes after this on MacViews.

BUG=378134, 365977
Committed: https://crrev.com/1d11217e72c14e3515a01c016a280dac7e1deb21
Cr-Commit-Position: refs/heads/master@{#310647}

[tapted, 2015-01-06 06:06:35]

Patchset #6 (id:100001) has been deleted

[tapted, 2015-01-06 06:06:42]

Patchset #2 (id:20001) has been deleted

[tapted, 2015-01-06 06:06:52]

Patchset #4 (id:80001) has been deleted

[tapted, 2015-01-06 06:07:02]

Patchset #2 (id:40001) has been deleted

[tapted, 2015-01-06 06:46:38]

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

[tapted, 2015-01-06 10:03:20]

Hi Robert - please take a look. Lots of failed attempts to get here.. Not much
code in the end, but some subtle stuff.. so I went to town on DCHECKS/comments
(and tests - woo!)

[Robert Sesek, 2015-01-07 00:11:40]

LGTM.

This child window business is getting quite subtle.

https://codereview.chromium.org/831643004/diff/160001/ui/views/test/widget_te...
File ui/views/test/widget_test_aura.cc (right):

https://codereview.chromium.org/831643004/diff/160001/ui/views/test/widget_te...
ui/views/test/widget_test_aura.cc:21: bool FindLayersInOrder(const
std::vector<ui::Layer*>& children,
This function is not easy to reason about, but I don't really have suggestions.

Would this work instead? Create two arrays of the first's and second's parent
hierarchy up to the root, and then just linearly compare the two. If that's
messier/won't work, this is fine, I just had to stare at it for a while.

[tapted, 2015-01-07 01:44:12]

+sky for widget_unittest and widget_test{.h,aura,etc.} OWNERS and in case you
know a better approach for the `IsWindowStackedAbove` stuff in the test,
particularly on Aura.

Thanks!

https://codereview.chromium.org/831643004/diff/160001/ui/views/test/widget_te...
File ui/views/test/widget_test_aura.cc (right):

https://codereview.chromium.org/831643004/diff/160001/ui/views/test/widget_te...
ui/views/test/widget_test_aura.cc:21: bool FindLayersInOrder(const
std::vector<ui::Layer*>& children,
On 2015/01/07 00:11:40, Robert Sesek wrote:
> This function is not easy to reason about, but I don't really have
suggestions.
> 
> Would this work instead? Create two arrays of the first's and second's parent
> hierarchy up to the root, and then just linearly compare the two. If that's
> messier/won't work, this is fine, I just had to stare at it for a while.

Yeah - I felt bad putting recursion in a test function :(, but I think it's
warranted. The tricky part with walking up to the root is that each child would
need to know its index in its parent's array of children, since that also
influences stacking. But I think you're right - I could just create two arrays
of these indexes (going up), reverse the arrays, and then do a
std::lexicographical_compare() (handling corner cases like not-sharing a root).
But it's quite a bit more state to maintain over a pre-order traversal.

sky might know a way to get this info from something that exists already, but I
couldn't find anything.

Or maybe ui::Layer (or aura::Window or aura::WindowStackingClient, or even
Widget ..) could benefit from something like [NSApp orderedWindows] that does
the traversal, then have the same algorithm as mac. As it is now, the
`IsStackedAbove` logic is the same idea as mac, just with a recursive `visitor`,
so I think keeping that bit consistent has merit.

[tapted, 2015-01-08 03:13:18]

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

[tapted, 2015-01-08 03:13:19]

(+sky) Whoops it would help if I actually added sky to reviewers... ಠ_ಠ

(+sky for widget_unittest and widget_test{.h,aura,etc.} OWNERS and in case you
know a better approach for the `IsWindowStackedAbove` stuff in the test,
particularly on Aura.)

[sky, 2015-01-08 04:17:44]

LGTM

https://codereview.chromium.org/831643004/diff/180001/ui/views/test/widget_te...
File ui/views/test/widget_test_aura.cc (right):

https://codereview.chromium.org/831643004/diff/180001/ui/views/test/widget_te...
ui/views/test/widget_test_aura.cc:57: CHECK(above->IsVisible());
CHECKs aren't good for tests. If it hits it means the harness crashes and no
more tests run.

[tapted, 2015-01-08 09:07:56]

https://codereview.chromium.org/831643004/diff/180001/ui/views/test/widget_te...
File ui/views/test/widget_test_aura.cc (right):

https://codereview.chromium.org/831643004/diff/180001/ui/views/test/widget_te...
ui/views/test/widget_test_aura.cc:57: CHECK(above->IsVisible());
On 2015/01/08 04:17:44, sky wrote:
> CHECKs aren't good for tests. If it hits it means the harness crashes and no
> more tests run.

changed to EXPECT_TRUE. The thinking went that these were more of a precondition
for the function, but yeah - no need to CHECK - there's no imminent explosion.

[tapted, 2015-01-08 09:08:16]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-01-08 09:08:46]

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

[commit-bot: I haz the power, 2015-01-08 11:10:30]

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

[commit-bot: I haz the power, 2015-01-08 11:10:31]

Exceeded time limit waiting for builds to trigger.

[tapted, 2015-01-08 23:42:43]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-01-08 23:44:39]

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

[commit-bot: I haz the power, 2015-01-09 01:04:02]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2015-01-09 01:06:29]

Patchset 8 (id:??) landed as
https://crrev.com/1d11217e72c14e3515a01c016a280dac7e1deb21
Cr-Commit-Position: refs/heads/master@{#310647}