Changes views_aura_mus_unittests to create DesktopNativeWidgetAura

Changes views_aura_mus_unittests to create DesktopNativeWidgetAura

And disables a bunch of tests that are either broke or don't make
sense.

Tests that don't call CreatePlatformNativeWidgetImpl() already
create a DesktopNativeWidgetAura, but those calling
CreatePlatformNativeWidgetImpl() ended up with a
NativeWidgetAura. This patch makes it so when aura-mus-client is used
CreatePlatformNativeWidgetImpl() creates a DesktopNativeWidgetAura
(assuming that is appropriate).

Additionally I'm making ViewsTestBase::GetContext() return null for
aura-mus-client. That way we sure see failures early on.

BUG=663903
663809 663561 663868 664261 664280 615033
TEST=covered by tests
R=msw@chromium.org

Committed: https://crrev.com/a178499ab5e3a157c30198dc94deed31104360d9
Cr-Commit-Position: refs/heads/master@{#431505}

[sky, 2016-11-10 23:00:20]

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

[commit-bot: I haz the power, 2016-11-10 23:01:10]

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, 2016-11-10 23:08:13]

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

[commit-bot: I haz the power, 2016-11-10 23:08:14]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sky, 2016-11-10 23:31:17]

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

[commit-bot: I haz the power, 2016-11-10 23:32:12]

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, 2016-11-10 23:46:36]

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

[commit-bot: I haz the power, 2016-11-10 23:46:37]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sky, 2016-11-11 00:21:47]

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

[commit-bot: I haz the power, 2016-11-11 00:22:20]

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

[msw, 2016-11-11 00:29:25]

The pervasive test early returns scare me a bit, but I don't have a great idea
other than somehow ensuring a concerted effort to actually resolve them later...
this doesn't happen automatically (eg. I just pinged erg for 614037).

https://codereview.chromium.org/2488393003/diff/60001/ui/views/focus/focus_ma...
File ui/views/focus/focus_manager_unittest.cc (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager_unittest.cc:126: // the Widget and expecting that
to change acivation. This should work for
nit: activation

https://codereview.chromium.org/2488393003/diff/60001/ui/views/mus/mus_client.cc
File ui/views/mus/mus_client.cc (left):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/mus/mus_client...
ui/views/mus/mus_client.cc:86: // through, so that the default
NativeWidgetPrivate::CreateNativeWidget() is
nit: keeps some comment about returning null to use the default
NativeWidgetPrivate::CreateNativeWidget()

https://codereview.chromium.org/2488393003/diff/60001/ui/views/mus/mus_client.h
File ui/views/mus/mus_client.h (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/mus/mus_client...
ui/views/mus/mus_client.h:66: // specified params.
nit: "Otherwise, the appropriate type to use is NativeWidgetAura." or similar?

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/native_wi...
File ui/views/test/native_widget_factory_desktop.cc (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/native_wi...
ui/views/test/native_widget_factory_desktop.cc:45: #elif defined(USE_AURA)
I guess this isn't compiled anywhere that lacks a return statement, still, maybe
add a NOTREACHED(); return nullptr; in an #else block or outside the
preprocessor macro blocks?

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/platform_...
File ui/views/test/platform_test_helper.cc (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/platform_...
ui/views/test/platform_test_helper.cc:26: class DefaultPlatformTestHelper :
public PlatformTestHelper {
Can we move the default SimulateNativeDestroy impl onto PlatformTestHelper and
use that, removing this default subclass altogether?

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/test_plat...
File ui/views/test/test_platform_native_widget.h (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/test_plat...
ui/views/test/test_platform_native_widget.h:20: // . a boolean is set when the
NativeWiget is destroyed.
nit: NativeWidget

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/views_tes...
File ui/views/test/views_test_helper_aura.cc (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/views_tes...
ui/views/test/views_test_helper_aura.cc:77: // GetContext() returns a root of a
WindowTreeHost that doesn't make sense
nit: "WindowTreeHost, which doesn't", maybe explain why?

https://codereview.chromium.org/2488393003/diff/60001/ui/views/touchui/touch_...
File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/touchui/touch_...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:84: if
(!IsAuraMusClient()) {
nit: only use the conditional for test_cursor_client_.reset(); it seems like if
a test case managed to call CreateTextfield or CreateWidget, we should still
close those widgets regardless.

https://codereview.chromium.org/2488393003/diff/60001/ui/views/touchui/touch_...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:600: // TODO: test
uses GetContext(), which is not applicable to aura-mus.
optional nit: Move the meaty comment to SetUp and make these just: 
  // See comment in SetUp().

https://codereview.chromium.org/2488393003/diff/60001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.h (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.h:100:
internal::NativeWidgetDelegate* delegate() { return native_widget_delegate_; }
Can the caller(s) for this just use DesktopNativeWidgetAura::GetWidget()?

https://codereview.chromium.org/2488393003/diff/60001/ui/views/widget/widget_...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:313: if (!IsAuraMusClient())
Definitely cite a bug here; maybe just early return at the top?

https://codereview.chromium.org/2488393003/diff/60001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:1768: new ui::test::EventGenerator(IsMus() ||
IsAuraMusClient()
nit: MakeUnique here and elsewhere.

https://codereview.chromium.org/2488393003/diff/80001/ui/views/test/platform_...
File ui/views/test/platform_test_helper.mm (right):

https://codereview.chromium.org/2488393003/diff/80001/ui/views/test/platform_...
ui/views/test/platform_test_helper.mm:28: // last
nit: wrapping

[commit-bot: I haz the power, 2016-11-11 00:29:30]

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

[commit-bot: I haz the power, 2016-11-11 00:29:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sky, 2016-11-11 01:08:18]

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

[commit-bot: I haz the power, 2016-11-11 01:08:38]

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

[sky, 2016-11-11 01:09:14]

Thanks for the great feedback!

As to the number of tests being disabled I'm not super concerned. One thing you
have to remember is I'm basically making it so a bunch of tests end up using
DesktopNativeWidgetAura (for aura-mus-client) that previously used
NativeWidgetAura. I wouldn't be surprised to see many of the same failures on
windows/linux if I did the change their.

https://codereview.chromium.org/2488393003/diff/60001/ui/views/focus/focus_ma...
File ui/views/focus/focus_manager_unittest.cc (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager_unittest.cc:126: // the Widget and expecting that
to change acivation. This should work for
On 2016/11/11 00:29:24, msw wrote:
> nit: activation

Done.

https://codereview.chromium.org/2488393003/diff/60001/ui/views/mus/mus_client.cc
File ui/views/mus/mus_client.cc (left):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/mus/mus_client...
ui/views/mus/mus_client.cc:86: // through, so that the default
NativeWidgetPrivate::CreateNativeWidget() is
On 2016/11/11 00:29:24, msw wrote:
> nit: keeps some comment about returning null to use the default
> NativeWidgetPrivate::CreateNativeWidget()

Done.

https://codereview.chromium.org/2488393003/diff/60001/ui/views/mus/mus_client.h
File ui/views/mus/mus_client.h (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/mus/mus_client...
ui/views/mus/mus_client.h:66: // specified params.
On 2016/11/11 00:29:24, msw wrote:
> nit: "Otherwise, the appropriate type to use is NativeWidgetAura." or similar?

That's right. Updated.

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/native_wi...
File ui/views/test/native_widget_factory_desktop.cc (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/native_wi...
ui/views/test/native_widget_factory_desktop.cc:45: #elif defined(USE_AURA)
On 2016/11/11 00:29:24, msw wrote:
> I guess this isn't compiled anywhere that lacks a return statement, still,
maybe
> add a NOTREACHED(); return nullptr; in an #else block or outside the
> preprocessor macro blocks?

Done.

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/platform_...
File ui/views/test/platform_test_helper.cc (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/platform_...
ui/views/test/platform_test_helper.cc:26: class DefaultPlatformTestHelper :
public PlatformTestHelper {
On 2016/11/11 00:29:24, msw wrote:
> Can we move the default SimulateNativeDestroy impl onto PlatformTestHelper and
> use that, removing this default subclass altogether?

Done.

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/test_plat...
File ui/views/test/test_platform_native_widget.h (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/test_plat...
ui/views/test/test_platform_native_widget.h:20: // . a boolean is set when the
NativeWiget is destroyed.
On 2016/11/11 00:29:24, msw wrote:
> nit: NativeWidget

Done.

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/views_tes...
File ui/views/test/views_test_helper_aura.cc (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/test/views_tes...
ui/views/test/views_test_helper_aura.cc:77: // GetContext() returns a root of a
WindowTreeHost that doesn't make sense
On 2016/11/11 00:29:24, msw wrote:
> nit: "WindowTreeHost, which doesn't", maybe explain why?

Done.

https://codereview.chromium.org/2488393003/diff/60001/ui/views/touchui/touch_...
File ui/views/touchui/touch_selection_controller_impl_unittest.cc (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/touchui/touch_...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:84: if
(!IsAuraMusClient()) {
On 2016/11/11 00:29:24, msw wrote:
> nit: only use the conditional for test_cursor_client_.reset(); it seems like
if
> a test case managed to call CreateTextfield or CreateWidget, we should still
> close those widgets regardless.

Done.

https://codereview.chromium.org/2488393003/diff/60001/ui/views/touchui/touch_...
ui/views/touchui/touch_selection_controller_impl_unittest.cc:600: // TODO: test
uses GetContext(), which is not applicable to aura-mus.
On 2016/11/11 00:29:24, msw wrote:
> optional nit: Move the meaty comment to SetUp and make these just: 
>   // See comment in SetUp().

Done.

https://codereview.chromium.org/2488393003/diff/60001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.h (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.h:100:
internal::NativeWidgetDelegate* delegate() { return native_widget_delegate_; }
On 2016/11/11 00:29:24, msw wrote:
> Can the caller(s) for this just use DesktopNativeWidgetAura::GetWidget()?

done.

https://codereview.chromium.org/2488393003/diff/60001/ui/views/widget/widget_...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:313: if (!IsAuraMusClient())
On 2016/11/11 00:29:25, msw wrote:
> Definitely cite a bug here; maybe just early return at the top?

The problem is for aura-mus-client top level z-order calculation as this
function does isn't possible. The reason for that is IsWindowStackedAbove
calculates z-order using layers, which isn't applicable for
DesktopNativeWidgetAura. To really calculate top-level z-order requires api on
mus. AFAICT we don't actually need to calculate z-order of top levels, which is
why I didn't reference any bug here.

It's possible we could add a test api to calculate z-order, but I would rather
hold off on that.

As to an early out, I didn't as the other tests are useful.

https://codereview.chromium.org/2488393003/diff/60001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:1768: new ui::test::EventGenerator(IsMus() ||
IsAuraMusClient()
On 2016/11/11 00:29:24, msw wrote:
> nit: MakeUnique here and elsewhere.

I converted these to be values, e.g. not use EventGenerator.

https://codereview.chromium.org/2488393003/diff/80001/ui/views/test/platform_...
File ui/views/test/platform_test_helper.mm (right):

https://codereview.chromium.org/2488393003/diff/80001/ui/views/test/platform_...
ui/views/test/platform_test_helper.mm:28: // last
On 2016/11/11 00:29:25, msw wrote:
> nit: wrapping

Done.

[commit-bot: I haz the power, 2016-11-11 01:16:41]

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

[commit-bot: I haz the power, 2016-11-11 01:16:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msw, 2016-11-11 01:25:40]

lgtm with nits.

https://codereview.chromium.org/2488393003/diff/60001/ui/views/widget/widget_...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/2488393003/diff/60001/ui/views/widget/widget_...
ui/views/widget/widget_unittest.cc:313: if (!IsAuraMusClient())
On 2016/11/11 01:09:14, sky wrote:
> On 2016/11/11 00:29:25, msw wrote:
> > Definitely cite a bug here; maybe just early return at the top?
> 
> The problem is for aura-mus-client top level z-order calculation as this
> function does isn't possible. The reason for that is IsWindowStackedAbove
> calculates z-order using layers, which isn't applicable for
> DesktopNativeWidgetAura. To really calculate top-level z-order requires api on
> mus. AFAICT we don't actually need to calculate z-order of top levels, which
is
> why I didn't reference any bug here.
> 
> It's possible we could add a test api to calculate z-order, but I would rather
> hold off on that.
> 
> As to an early out, I didn't as the other tests are useful.

Thanks for explaining and adding a comment. I'd say it still a *little*
confusing as-is with some IsWindowStackedAbove checks skipped, and others not
(but readers might understand the distinction between top-level and parent-child
ordering checks?). It might be nice if the top-level z-order checks were in
their own test fixture, where a single early return there would make sense, and
have another text fixture with the checks that work in both environments. But
I'll leave it up to you.

https://codereview.chromium.org/2488393003/diff/100001/ui/views/test/test_pla...
File ui/views/test/test_platform_native_widget.h (right):

https://codereview.chromium.org/2488393003/diff/100001/ui/views/test/test_pla...
ui/views/test/test_platform_native_widget.h:51:
this->GetWidget()->OnMouseCaptureLost();
nit: remove |this->|?

[sky, 2016-11-11 04:28:46]

I didn't feel strongly about the z-order test, so I left as is.

https://codereview.chromium.org/2488393003/diff/100001/ui/views/test/test_pla...
File ui/views/test/test_platform_native_widget.h (right):

https://codereview.chromium.org/2488393003/diff/100001/ui/views/test/test_pla...
ui/views/test/test_platform_native_widget.h:51:
this->GetWidget()->OnMouseCaptureLost();
On 2016/11/11 01:25:40, msw wrote:
> nit: remove |this->|?

It's necessary. I changed it to PlatformNativeWidget::GetWidget() to be more
consistent though.

[sky, 2016-11-11 04:29:23]

The CQ bit was checked by sky@chromium.org

[sky, 2016-11-11 04:29:24]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2488393003/#ps120001
(title: "remove this and fix mac")

[commit-bot: I haz the power, 2016-11-11 04:29:33]

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, 2016-11-11 05:35:49]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-11 05:44:45]

Description was changed from

==========
Changes views_aura_mus_unittests to create DesktopNativeWidgetAura

And disables a bunch of tests that are either broke or don't make
sense.

Tests that don't call CreatePlatformNativeWidgetImpl() already
create a DesktopNativeWidgetAura, but those calling
CreatePlatformNativeWidgetImpl() ended up with a
NativeWidgetAura. This patch makes it so when aura-mus-client is used
CreatePlatformNativeWidgetImpl() creates a DesktopNativeWidgetAura
(assuming that is appropriate).

Additionally I'm making ViewsTestBase::GetContext() return null for
aura-mus-client. That way we sure see failures early on.

BUG=663903 663809 663561 663868 664261 664280 615033
TEST=covered by tests
R=msw@chromium.org
==========

to

==========
Changes views_aura_mus_unittests to create DesktopNativeWidgetAura

And disables a bunch of tests that are either broke or don't make
sense.

Tests that don't call CreatePlatformNativeWidgetImpl() already
create a DesktopNativeWidgetAura, but those calling
CreatePlatformNativeWidgetImpl() ended up with a
NativeWidgetAura. This patch makes it so when aura-mus-client is used
CreatePlatformNativeWidgetImpl() creates a DesktopNativeWidgetAura
(assuming that is appropriate).

Additionally I'm making ViewsTestBase::GetContext() return null for
aura-mus-client. That way we sure see failures early on.

BUG=663903 663809 663561 663868 664261 664280 615033
TEST=covered by tests
R=msw@chromium.org

Committed: https://crrev.com/a178499ab5e3a157c30198dc94deed31104360d9
Cr-Commit-Position: refs/heads/master@{#431505}
==========

[commit-bot: I haz the power, 2016-11-11 05:44:46]

Patchset 7 (id:??) landed as
https://crrev.com/a178499ab5e3a157c30198dc94deed31104360d9
Cr-Commit-Position: refs/heads/master@{#431505}