exo: Set widget parent to target root window

components/exo/shell_surface.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/exo/shell_surface.h"
 
 #include "ash/aura/wm_window_aura.h"
 #include "ash/common/shell_window_ids.h"
 #include "ash/common/wm/window_resizer.h"
 #include "ash/common/wm/window_state.h"
@@ skipping 869 matching lines @@
   DCHECK(!widget_);
 
   views::Widget::InitParams params;
   params.type = views::Widget::InitParams::TYPE_WINDOW;
   params.ownership = views::Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET;
   params.delegate = this;
   params.shadow_type = views::Widget::InitParams::SHADOW_TYPE_NONE;
   params.opacity = views::Widget::InitParams::TRANSLUCENT_WINDOW;
   params.show_state = show_state;
   params.parent =
-      ash::Shell::GetContainer(ash::Shell::GetPrimaryRootWindow(), container_);
+      ash::Shell::GetContainer(ash::Shell::GetTargetRootWindow(), container_);

[reveman, 2016/07/26 14:46:01]

When |parent_| is set then this needs to be the same root window as the parent
otherwise popups might show up on a different root window.

[Dominik Laskowski, 2016/07/26 23:13:38]

Good point. See my reply below.

   params.bounds = initial_bounds_;

[oshima, 2016/07/26 01:07:11]

This isn't correct (at least for arc++).

I think the correct way is to simply pass the context, not parent, and let the
initial bounds find the correct root window.

I believe it's empty for arc++ windows?

[reveman, 2016/07/26 14:46:01]

Initial bounds are always "gfx::Rect(1, 1)" for arc++ windows to provide
absolute positioning of sub-surfaces in the root window coordinate space.

Initial bounds is never set for normal xdg shell surfaces. It will be set for
popups and is derived from the parent position in that case.

[oshima, 2016/07/26 20:40:56]

If the initial bounds is set in screen coordinate, it should pick the right root
window. That's being said, for popup, it probably should set the transient
parent so that it stays in the same root window.

[reveman, 2016/07/26 21:38:33]

Initial bounds is parent origin + offset provided by the client so part of this
is in the client's control and we should probably not allow the client to put
the popup in the wrong root window.

I think transient parent is already set correctly for popups.

[Dominik Laskowski, 2016/07/26 23:13:38]

Initial bounds are in screen coordinates regardless of |params.parent| (hence
the need for [1]), but I think part of the reason why popups show up on the
wrong root window (in addition to the client issue above) is that they are made
transient after Widget::Init has set their bounds. This should be fixed by
setting |params.parent| to |parent_|. I believe you're right about passing a
context instead for ARC windows.

[1] https://codereview.chromium.org/2175373003/

   bool activatable = activatable_;
   // ShellSurfaces in system modal container are only activatable if input
   // region is non-empty. See OnCommitSurface() for more details.
   if (container_ == ash::kShellWindowId_SystemModalContainer)
     activatable &= !surface_->GetHitTestBounds().IsEmpty();
   params.activatable = activatable ? views::Widget::InitParams::ACTIVATABLE_YES
                                    : views::Widget::InitParams::ACTIVATABLE_NO;
 
   // Note: NativeWidget owns this widget.
   widget_ = new ShellSurfaceWidget(this);
@@ skipping 322 matching lines @@
     shadow_origin -= origin.OffsetFromOrigin();
     gfx::Rect shadow_bounds(shadow_origin, shadow_content_bounds_.size());
 
     shadow_overlay_->SetBounds(shadow_bounds);
     shadow_underlay_->SetBounds(shadow_bounds);
     shadow->SetContentBounds(gfx::Rect(shadow_bounds.size()));
   }
 }
 
 }  // namespace exo