Show transient child when the transient parent is shown

ui/wm/core/transient_window_manager.cc

Index: ui/wm/core/transient_window_manager.cc
diff --git a/ui/wm/core/transient_window_manager.cc b/ui/wm/core/transient_window_manager.cc
index 120af84786c5b98a717491d7b4ff263d8facef53..6fb33d87bb41f04bb59876321034fc40693c326c 100644
--- a/ui/wm/core/transient_window_manager.cc
+++ b/ui/wm/core/transient_window_manager.cc
@@ -18,9 +18,12 @@
 using aura::Window;
 
 namespace wm {
+namespace {
 
 DEFINE_OWNED_WINDOW_PROPERTY_KEY(TransientWindowManager, kPropertyKey, NULL);
 
+}  // namespace
+
 TransientWindowManager::~TransientWindowManager() {
 }
 
@@ -97,7 +100,10 @@ bool TransientWindowManager::IsStackingTransient(
 TransientWindowManager::TransientWindowManager(Window* window)
     : window_(window),
       transient_parent_(NULL),
-      stacking_target_(NULL) {
+      stacking_target_(NULL),
+      parent_control_visibility_(false),
+      show_on_parent_visible_(false),
+      ignore_visibility_changed_event_(false) {
   window_->AddObserver(this);
 }
 
@@ -134,18 +140,45 @@ void TransientWindowManager::OnWindowParentChanged(aura::Window* window,
   }
 }
 
+void TransientWindowManager::UpdateTransientChildVisibility(
+    bool parent_visible) {
+  base::AutoReset<bool> reset(&ignore_visibility_changed_event_, true);
+  if (!parent_visible) {
+    show_on_parent_visible_ = window_->TargetVisibility();
+    window_->Hide();
+  } else {
+    if (show_on_parent_visible_ && parent_control_visibility_)
+      window_->Show();
+    show_on_parent_visible_ = false;
+  }
+}
+
 void TransientWindowManager::OnWindowVisibilityChanging(Window* window,
                                                         bool visible) {
-  // TODO(sky): move handling of becoming visible here.
-  if (!visible) {
-    std::for_each(transient_children_.begin(), transient_children_.end(),
-                  std::mem_fun(&Window::Hide));
+  if (window_ != window)

[sky, 2014/10/07 15:19:14]

Why the if, at best this should be a DCHECK

[oshima, 2014/10/07 17:41:14]

Done. I was confused by the difference between Changing (called on the target
window only) and Changing (called in hierarchy chain)

[oshima, 2014/10/07 17:41:56]

oops, I meant "Changed (called in hierarchy chain)"

+    return;
+
+  for (auto* child : transient_children_)

[sky, 2014/10/07 15:19:14]

From the style guide: "auto is permitted, for local variables only. Do not use
auto for file-scope or namespace-scope variables, or for class members."

[oshima, 2014/10/07 17:41:14]

Where is it? I was looking at

http://chromium-cpp.appspot.com/

and couldn't find that description.

[oshima, 2014/10/07 17:46:25]

I guess you meant the description in
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#auto 

I believe what this is prohibiting is something like

auto global = 1;

namespace {
auto namespace_scope = "xyz";
}

The auto in the loop creates local variable, so this should be fine.

[sky, 2014/10/07 19:34:21]

Yes, you are right. My mistake.

+    Get(child)->UpdateTransientChildVisibility(visible);
+}
+
+void TransientWindowManager::OnWindowVisibilityChanged(Window* window,

[sky, 2014/10/07 15:19:14]

Why do you need to do something in both changed and changing? Isn't one enough?

[oshima, 2014/10/07 17:41:14]

I tried and had problem because OnWindowVisibiltyChanged is called twice on the
same target (during Down and Up traversal). Is this by design?

[sky, 2014/10/07 19:34:21]

That seems wrong. It should only be called once.

+                                                       bool visible) {
+  if (window_ != window || ignore_visibility_changed_event_ ||

[sky, 2014/10/07 15:19:14]

You shouldn't need the window_ != window. Add a DCHECK if you want it.

[oshima, 2014/10/07 17:41:14]

I needed this because OnWindowVisibilityChanged is called on hierarchy chain

[sky, 2014/10/07 19:34:21]

Ah, ok.

+      !transient_parent_ || !parent_control_visibility_) {
+    return;
+  }
+  if (!transient_parent_->TargetVisibility() && visible) {
+    base::AutoReset<bool> reset(&ignore_visibility_changed_event_, true);
+    show_on_parent_visible_ = true;
+    window_->Hide();
+  } else if (!visible) {
+    DCHECK(!show_on_parent_visible_);
   }
 }
 
 void TransientWindowManager::OnWindowStackingChanged(Window* window) {
   DCHECK_EQ(window_, window);
-
   // Do nothing if we initiated the stacking change.
   const TransientWindowManager* transient_manager =
       Get(static_cast<const Window*>(window));