Adds OnChildWindowVisibilityChanged to ui::WindowObserver

Adds OnChildWindowVisibilityChanged to ui::WindowObserver as well as making all visible functions take the 'visible' to match usage of aura.

OnChildWindowVisibilityChanged() is unfortunate given
OnWindowVisibilityChanged() is also sent, but it's the only way I can
match existing behavior. I came across this ordering dependency as
part of getting workspace tests running. It may be we end up adding a
LayoutManager to mus and pulling OnChildWindowVisibilityChanged into
it. But for now I'm going with just the OnChildWindowVisibilityChanged.

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

Committed: https://crrev.com/59515676ad3dca16bd37bfdfdc00519817b7eb2c
Cr-Commit-Position: refs/heads/master@{#422615}

[sky, 2016-10-03 17:07:01]

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

[commit-bot: I haz the power, 2016-10-03 17:07:29]

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-10-03 17:22:41]

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

[commit-bot: I haz the power, 2016-10-03 17:22:42]

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_...)

[sky, 2016-10-03 17:53:46]

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

[commit-bot: I haz the power, 2016-10-03 17:54:40]

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-10-03 18:48:02]

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

[commit-bot: I haz the power, 2016-10-03 18:48:03]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-10-03 20:33:08]

LGTM. It looks like you've got two CL descriptions mushed together -- maybe take
some of it out.

nit: "visible" in CL description.

https://codereview.chromium.org/2387013003/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/tests/window_unittest.cc (right):

https://codereview.chromium.org/2387013003/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/tests/window_unittest.cc:713: // Change visibility from
true to false and make sure we get notifications.
Thanks for fixing the wisibility.

https://codereview.chromium.org/2387013003/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/tests/window_unittest.cc:723: // Set visible to existing
walue and werify no notifications.
optional: walue and werify

https://codereview.chromium.org/2387013003/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/window_observer.h (right):

https://codereview.chromium.org/2387013003/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/window_observer.h:79: //   whose visibility changed, as
well as observers added to the ancestor and
All ancestors or immediate ancestor only?

https://codereview.chromium.org/2387013003/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/window_observer.h:80: //   descendants of the window.
Nice docs, btw.

https://codereview.chromium.org/2387013003/diff/20001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2387013003/diff/20001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:7: #define NOTIMPLEMENTED_POLICY 5
Gentle pushback: We're already pretty spammy at startup, even with this policy.
Are you sure you want to add these back?

[sky, 2016-10-03 20:52:13]

https://codereview.chromium.org/2387013003/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/tests/window_unittest.cc (right):

https://codereview.chromium.org/2387013003/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/tests/window_unittest.cc:723: // Set visible to existing
walue and werify no notifications.
On 2016/10/03 20:33:08, James Cook wrote:
> optional: walue and werify

Done.

https://codereview.chromium.org/2387013003/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/window_observer.h (right):

https://codereview.chromium.org/2387013003/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/window_observer.h:79: //   whose visibility changed, as
well as observers added to the ancestor and
On 2016/10/03 20:33:08, James Cook wrote:
> All ancestors or immediate ancestor only?

Added 'all' for both ancestors and descendants.

https://codereview.chromium.org/2387013003/diff/20001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2387013003/diff/20001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:7: #define NOTIMPLEMENTED_POLICY 5
On 2016/10/03 20:33:08, James Cook wrote:
> Gentle pushback: We're already pretty spammy at startup, even with this
policy.
> Are you sure you want to add these back?

I realize we're spammy, but I think that's the point. Otherwise we're going to
miss some things. I'll also add its nice to try and bring up a test, see
something logged about NOTIMPLEMENTED and realize it is likely related.

[sky, 2016-10-03 20:52:56]

The CQ bit was checked by sky@chromium.org

[sky, 2016-10-03 20:52: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/2387013003/#ps60001
(title: "feedback")

[commit-bot: I haz the power, 2016-10-03 20:53:32]

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

[sky, 2016-10-03 21:18:04]

Description was changed from

==========
Changes to ui::WindowObserver

Specifically:
. Makes all the functions take the 'visibile' to match usage of aura.
. Adds OnChildWindowVisibilityChanged() that is sent before
  OnWindowVisibilityChanged(). This is a bit unfortunate, but we have
  order dependencies that can only be satisfied by calling
  OnChildWindowVisibilityChanged() before OnWindowVisibilityChanged().

OnChildWindowVisibilityChanged() is unfortunate given
OnWindowVisibilityChanged() is also sent, but it's the only way I can
match existing behavior. I came across this ordering dependency as
part of getting workspace tests running. It may be we end up adding a
LayoutManager to mus and pulling OnChildWindowVisibilityChanged into
it. But for now I'm going with just the OnChildWindowVisibilityChanged.

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

to

==========
Adds OnChildWindowVisibilityChanged to ui::WindowObserver as well as making all
visible functions take the 'visible' to match usage of aura.

OnChildWindowVisibilityChanged() is unfortunate given
OnWindowVisibilityChanged() is also sent, but it's the only way I can
match existing behavior. I came across this ordering dependency as
part of getting workspace tests running. It may be we end up adding a
LayoutManager to mus and pulling OnChildWindowVisibilityChanged into
it. But for now I'm going with just the OnChildWindowVisibilityChanged.

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

[commit-bot: I haz the power, 2016-10-03 22:49:49]

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

[commit-bot: I haz the power, 2016-10-03 22:49:51]

Failed to apply patch for ui/views/mus/native_widget_mus.cc:
While running git apply --index -3 -p1;
  error: patch failed: ui/views/mus/native_widget_mus.cc:842
  Falling back to three-way merge...
  Applied patch to 'ui/views/mus/native_widget_mus.cc' with conflicts.
  U ui/views/mus/native_widget_mus.cc

Patch:       ui/views/mus/native_widget_mus.cc
Index: ui/views/mus/native_widget_mus.cc
diff --git a/ui/views/mus/native_widget_mus.cc
b/ui/views/mus/native_widget_mus.cc
index
976608f7168f90d8d4ed05386ad3effe25338e8b..96dd4ae0b98e0b68b1137eb0606257419e6d38c4
100644
--- a/ui/views/mus/native_widget_mus.cc
+++ b/ui/views/mus/native_widget_mus.cc
@@ -2,6 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// This has to be before any other includes, else default is picked up.
+// See base/logging for details on this.
+#define NOTIMPLEMENTED_POLICY 5
+
 #include "ui/views/mus/native_widget_mus.h"
 
 #include <utility>
@@ -408,11 +412,11 @@ class NativeWidgetMus::MusWindowObserver : public
ui::WindowObserver {
   }
 
   // ui::WindowObserver:
-  void OnWindowVisibilityChanging(ui::Window* window) override {
-    native_widget_mus_->OnMusWindowVisibilityChanging(window);
+  void OnWindowVisibilityChanging(ui::Window* window, bool visible) override {
+    native_widget_mus_->OnMusWindowVisibilityChanging(window, visible);
   }
-  void OnWindowVisibilityChanged(ui::Window* window) override {
-    native_widget_mus_->OnMusWindowVisibilityChanged(window);
+  void OnWindowVisibilityChanged(ui::Window* window, bool visible) override {
+    native_widget_mus_->OnMusWindowVisibilityChanged(window, visible);
   }
   void OnWindowPredefinedCursorChanged(ui::Window* window,
                                        ui::mojom::Cursor cursor) override {
@@ -775,17 +779,17 @@ void NativeWidgetMus::OnWidgetInitDone() {
 }
 
 bool NativeWidgetMus::ShouldUseNativeFrame() const {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
   return false;
 }
 
 bool NativeWidgetMus::ShouldWindowContentsBeTransparent() const {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
   return true;
 }
 
 void NativeWidgetMus::FrameTypeChanged() {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
 }
 
 Widget* NativeWidgetMus::GetWidget() {
@@ -817,11 +821,11 @@ const ui::Layer* NativeWidgetMus::GetLayer() const {
 }
 
 void NativeWidgetMus::ReorderNativeViews() {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
 }
 
 void NativeWidgetMus::ViewRemoved(View* view) {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
 }
 
 // These methods are wrong in mojo. They're not usually used to associate
@@ -842,7 +846,7 @@ void* NativeWidgetMus::GetNativeWindowProperty(const char*
name) const {
 }
 
 TooltipManager* NativeWidgetMus::GetTooltipManager() const {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
   return nullptr;
 }
 
@@ -881,7 +885,7 @@ void NativeWidgetMus::CenterWindow(const gfx::Size& size) {
 void NativeWidgetMus::GetWindowPlacement(
       gfx::Rect* bounds,
       ui::WindowShowState* maximized) const {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
 }
 
 bool NativeWidgetMus::SetWindowTitle(const base::string16& title) {
@@ -999,19 +1003,19 @@ void NativeWidgetMus::SetSize(const gfx::Size& size) {
 }
 
 void NativeWidgetMus::StackAbove(gfx::NativeView native_view) {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
 }
 
 void NativeWidgetMus::StackAtTop() {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
 }
 
 void NativeWidgetMus::StackBelow(gfx::NativeView native_view) {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
 }
 
 void NativeWidgetMus::SetShape(std::unique_ptr<SkRegion> shape) {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
 }
 
 void NativeWidgetMus::Close() {
@@ -1188,7 +1192,7 @@ void NativeWidgetMus::SetOpacity(float opacity) {
 }
 
 void NativeWidgetMus::FlashFrame(bool flash_frame) {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
 }
 
 void NativeWidgetMus::RunShellDrag(View* view,
@@ -1221,7 +1225,7 @@ void NativeWidgetMus::SetCursor(gfx::NativeCursor cursor)
{
 }
 
 bool NativeWidgetMus::IsMouseEventsEnabled() const {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
   return true;
 }
 
@@ -1239,7 +1243,7 @@ void NativeWidgetMus::ClearNativeFocus() {
 }
 
 gfx::Rect NativeWidgetMus::GetWorkAreaBoundsInScreen() const {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
   return gfx::Rect();
 }
 
@@ -1275,17 +1279,17 @@ void NativeWidgetMus::EndMoveLoop() {
 }
 
 void NativeWidgetMus::SetVisibilityChangedAnimationsEnabled(bool value) {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
 }
 
 void NativeWidgetMus::SetVisibilityAnimationDuration(
     const base::TimeDelta& duration) {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
 }
 
 void NativeWidgetMus::SetVisibilityAnimationTransition(
     Widget::VisibilityTransition transition) {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
 }
 
 ui::NativeTheme* NativeWidgetMus::GetNativeTheme() const {
@@ -1293,11 +1297,11 @@ ui::NativeTheme* NativeWidgetMus::GetNativeTheme() const
{
 }
 
 void NativeWidgetMus::OnRootViewLayout() {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
 }
 
 bool NativeWidgetMus::IsTranslucentWindowOpacitySupported() const {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
   return true;
 }
 
@@ -1311,7 +1315,7 @@ void NativeWidgetMus::OnSizeConstraintsChanged() {
 }
 
 void NativeWidgetMus::RepostNativeEvent(gfx::NativeEvent native_event) {
-  // NOTIMPLEMENTED();
+  NOTIMPLEMENTED();
 }
 
 std::string NativeWidgetMus::GetName() const {
@@ -1498,25 +1502,25 @@ void NativeWidgetMus::OnWindowInputEvent(
   // |ack_handler| acks the event on destruction if necessary.
 }
 
-void NativeWidgetMus::OnMusWindowVisibilityChanging(ui::Window* window) {
-  if (window == window_) {
-    native_widget_delegate_->OnNativeWidgetVisibilityChanging(
-        !window->visible());
-  }
+void NativeWidgetMus::OnMusWindowVisibilityChanging(ui::Window* window,
+                                                    bool visible) {
+  if (window == window_)
+    native_widget_delegate_->OnNativeWidgetVisibilityChanging(visible);
 }
 
-void NativeWidgetMus::OnMusWindowVisibilityChanged(ui::Window* window) {
+void NativeWidgetMus::OnMusWindowVisibilityChanged(ui::Window* window,
+                                                   bool visible) {
   if (window != window_)
     return;
 
-  if (window->visible()) {
+  if (visible) {
     window_tree_host_->Show();
     GetNativeWindow()->Show();
   } else {
     window_tree_host_->Hide();
     GetNativeWindow()->Hide();
   }
-  native_widget_delegate_->OnNativeWidgetVisibilityChanged(window->visible());
+  native_widget_delegate_->OnNativeWidgetVisibilityChanged(visible);
 }
 
 void NativeWidgetMus::UpdateHitTestMask() {

[sky, 2016-10-03 22:55:48]

The CQ bit was checked by sky@chromium.org

[sky, 2016-10-03 22:55:49]

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/2387013003/#ps80001
(title: "merge")

[commit-bot: I haz the power, 2016-10-03 22:56: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-10-04 00:11:53]

Description was changed from

==========
Adds OnChildWindowVisibilityChanged to ui::WindowObserver as well as making all
visible functions take the 'visible' to match usage of aura.

OnChildWindowVisibilityChanged() is unfortunate given
OnWindowVisibilityChanged() is also sent, but it's the only way I can
match existing behavior. I came across this ordering dependency as
part of getting workspace tests running. It may be we end up adding a
LayoutManager to mus and pulling OnChildWindowVisibilityChanged into
it. But for now I'm going with just the OnChildWindowVisibilityChanged.

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

to

==========
Adds OnChildWindowVisibilityChanged to ui::WindowObserver as well as making all
visible functions take the 'visible' to match usage of aura.

OnChildWindowVisibilityChanged() is unfortunate given
OnWindowVisibilityChanged() is also sent, but it's the only way I can
match existing behavior. I came across this ordering dependency as
part of getting workspace tests running. It may be we end up adding a
LayoutManager to mus and pulling OnChildWindowVisibilityChanged into
it. But for now I'm going with just the OnChildWindowVisibilityChanged.

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

[commit-bot: I haz the power, 2016-10-04 00:11:54]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-04 00:15:21]

Description was changed from

==========
Adds OnChildWindowVisibilityChanged to ui::WindowObserver as well as making all
visible functions take the 'visible' to match usage of aura.

OnChildWindowVisibilityChanged() is unfortunate given
OnWindowVisibilityChanged() is also sent, but it's the only way I can
match existing behavior. I came across this ordering dependency as
part of getting workspace tests running. It may be we end up adding a
LayoutManager to mus and pulling OnChildWindowVisibilityChanged into
it. But for now I'm going with just the OnChildWindowVisibilityChanged.

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

to

==========
Adds OnChildWindowVisibilityChanged to ui::WindowObserver as well as making all
visible functions take the 'visible' to match usage of aura.

OnChildWindowVisibilityChanged() is unfortunate given
OnWindowVisibilityChanged() is also sent, but it's the only way I can
match existing behavior. I came across this ordering dependency as
part of getting workspace tests running. It may be we end up adding a
LayoutManager to mus and pulling OnChildWindowVisibilityChanged into
it. But for now I'm going with just the OnChildWindowVisibilityChanged.

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

Committed: https://crrev.com/59515676ad3dca16bd37bfdfdc00519817b7eb2c
Cr-Commit-Position: refs/heads/master@{#422615}
==========

[commit-bot: I haz the power, 2016-10-04 00:15:22]

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