Test coverage for bug in WmWindowAura::OnWindowVisibilityChanged()

Test coverage for bug in WmWindowAura::OnWindowVisibilityChanged()

Also gets mus side working too.

BUG=650755
TEST=none
R=jamescook@chromium.org

Committed: https://crrev.com/3fdae2f73beeb79c2b1f2c964e3fd290c4bb3e6c
Cr-Commit-Position: refs/heads/master@{#421633}

[James Cook, 2016-09-28 01:19:58]

https://codereview.chromium.org/2377853002/diff/20001/ash/common/test/ash_test.h
File ash/common/test/ash_test.h (right):

https://codereview.chromium.org/2377853002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:69: // Creates a visible window parented to |parent|
with the specified bounds and
nit: document that it creates and shows. (I've been burned many times in my own
code by forgetting to show windows. :-)

https://codereview.chromium.org/2377853002/diff/20001/ash/common/wm_window_un...
File ash/common/wm_window_unittest.cc (right):

https://codereview.chromium.org/2377853002/diff/20001/ash/common/wm_window_un...
ash/common/wm_window_unittest.cc:67: EXPECT_TRUE(observer.got_expected_value());
optional: This might be easier to read if the VisibilityObserver just stored a
last_changed_window_ and last_changed_visible_ and the test did
EXPECT_EQ(child_window, observer.last_changed_window()) and
EXPECT_FALSE(observer.last_changed_visible())

Then I wouldn't have to think about the two WmWindow* parameters or what "false"
means on line 65.

(If this doesn't work because VisibilityObserver gets called multiple times and
you want to check if it hits your expectations at least once you should document
that. I just thought of that now.)

https://codereview.chromium.org/2377853002/diff/20001/ash/mus/BUILD.gn
File ash/mus/BUILD.gn (right):

https://codereview.chromium.org/2377853002/diff/20001/ash/mus/BUILD.gn#newcod...
ash/mus/BUILD.gn:215: "../common/wm_window_unittest.cc",
Woo hoo, a common test!

https://codereview.chromium.org/2377853002/diff/20001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2377853002/diff/20001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:933: OnWindowVisibilityChanged(Get(window),
window->visible()));
Why doesn't it use |this|?

[sky, 2016-09-28 18:16:37]

https://codereview.chromium.org/2377853002/diff/20001/ash/common/test/ash_test.h
File ash/common/test/ash_test.h (right):

https://codereview.chromium.org/2377853002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:69: // Creates a visible window parented to |parent|
with the specified bounds and
On 2016/09/28 01:19:58, James Cook wrote:
> nit: document that it creates and shows. (I've been burned many times in my
own
> code by forgetting to show windows. :-)

Doesn't the visible imply shows?

https://codereview.chromium.org/2377853002/diff/20001/ash/common/wm_window_un...
File ash/common/wm_window_unittest.cc (right):

https://codereview.chromium.org/2377853002/diff/20001/ash/common/wm_window_un...
ash/common/wm_window_unittest.cc:67: EXPECT_TRUE(observer.got_expected_value());
On 2016/09/28 01:19:58, James Cook wrote:
> optional: This might be easier to read if the VisibilityObserver just stored a
> last_changed_window_ and last_changed_visible_ and the test did
> EXPECT_EQ(child_window, observer.last_changed_window()) and
> EXPECT_FALSE(observer.last_changed_visible())
> 
> Then I wouldn't have to think about the two WmWindow* parameters or what
"false"
> means on line 65.
> 
> (If this doesn't work because VisibilityObserver gets called multiple times
and
> you want to check if it hits your expectations at least once you should
document
> that. I just thought of that now.)

Great idea! Done.

https://codereview.chromium.org/2377853002/diff/20001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2377853002/diff/20001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:933: OnWindowVisibilityChanged(Get(window),
window->visible()));
On 2016/09/28 01:19:58, James Cook wrote:
> Why doesn't it use |this|?

You would think so, eh? aura::WindowObserver:OnWindowVisibiltyChanged (and
ui::WindowObserver::onWindowVisibilityChanged()) is called on observers attached
to all descendants and ancestors. The |window| parameter to
OnWindowVisibilityChanged() is the window Show/Hide was called on, not
necessarily the one the observer was added to. Confusing, eh?

[James Cook, 2016-09-28 19:02:49]

LGTM. Thanks for adding the extra test.

https://codereview.chromium.org/2377853002/diff/20001/ash/common/test/ash_test.h
File ash/common/test/ash_test.h (right):

https://codereview.chromium.org/2377853002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:69: // Creates a visible window parented to |parent|
with the specified bounds and
On 2016/09/28 18:16:36, sky wrote:
> On 2016/09/28 01:19:58, James Cook wrote:
> > nit: document that it creates and shows. (I've been burned many times in my
> own
> > code by forgetting to show windows. :-)
> 
> Doesn't the visible imply shows?

I guess, but I missed that. Up to you.

https://codereview.chromium.org/2377853002/diff/20001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2377853002/diff/20001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:933: OnWindowVisibilityChanged(Get(window),
window->visible()));
On 2016/09/28 18:16:36, sky wrote:
> On 2016/09/28 01:19:58, James Cook wrote:
> > Why doesn't it use |this|?
> 
> You would think so, eh? aura::WindowObserver:OnWindowVisibiltyChanged (and
> ui::WindowObserver::onWindowVisibilityChanged()) is called on observers
attached
> to all descendants and ancestors. The |window| parameter to
> OnWindowVisibilityChanged() is the window Show/Hide was called on, not
> necessarily the one the observer was added to. Confusing, eh?

Wow, it's super confusing that Changing() and Changed() have different
semantics. I guess we're stuck with that behavior and those names from aura.
Yuck.

https://codereview.chromium.org/2377853002/diff/40001/ash/common/wm_window_un...
File ash/common/wm_window_unittest.cc (right):

https://codereview.chromium.org/2377853002/diff/40001/ash/common/wm_window_un...
ash/common/wm_window_unittest.cc:65: EXPECT_EQ(false,
observer.last_observed_visible_value());
nit: I think we usually EXPECT_FALSE

(and below)

https://codereview.chromium.org/2377853002/diff/40001/ash/common/wm_window_un...
ash/common/wm_window_unittest.cc:72: WmWindow* window = window_owner->window();
optional: parent_window ?

[sky, 2016-09-28 19:29:57]

The CQ bit was checked by sky@chromium.org

[sky, 2016-09-28 19:29:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2377853002/#ps70001
(title: "parent and merge")

[sky, 2016-09-28 19:30:16]

https://codereview.chromium.org/2377853002/diff/40001/ash/common/wm_window_un...
File ash/common/wm_window_unittest.cc (right):

https://codereview.chromium.org/2377853002/diff/40001/ash/common/wm_window_un...
ash/common/wm_window_unittest.cc:65: EXPECT_EQ(false,
observer.last_observed_visible_value());
On 2016/09/28 19:02:49, James Cook wrote:
> nit: I think we usually EXPECT_FALSE
> 
> (and below)

Done.

https://codereview.chromium.org/2377853002/diff/40001/ash/common/wm_window_un...
ash/common/wm_window_unittest.cc:72: WmWindow* window = window_owner->window();
On 2016/09/28 19:02:49, James Cook wrote:
> optional: parent_window ?

Done.

[commit-bot: I haz the power, 2016-09-28 19:30:34]

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-09-28 20:44:48]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-09-28 20:47:07]

Description was changed from

==========
Test coverage for bug in WmWindowAura::OnWindowVisibilityChanged()

Also gets mus side working too.

BUG=650755
TEST=none
R=jamescook@chromium.org
==========

to

==========
Test coverage for bug in WmWindowAura::OnWindowVisibilityChanged()

Also gets mus side working too.

BUG=650755
TEST=none
R=jamescook@chromium.org

Committed: https://crrev.com/3fdae2f73beeb79c2b1f2c964e3fd290c4bb3e6c
Cr-Commit-Position: refs/heads/master@{#421633}
==========

[commit-bot: I haz the power, 2016-09-28 20:47:08]

Patchset 5 (id:??) landed as
https://crrev.com/3fdae2f73beeb79c2b1f2c964e3fd290c4bb3e6c
Cr-Commit-Position: refs/heads/master@{#421633}