Call Widget::Init() after OverlayWindowViews constructor

Calling Widget::Init() from OverlayWindowViews constructor seems too
early. Widget::Init() triggers calls to virtual functions such as
OnNativeWidgetMove() while OverlayWindowViews hasn't been constructed
fully. To cope with this, the |is_initialized_| flag was used as a
trigger for early returns. This turned out error-prone as some crucial
parts of the virtual functions were skipped, resulting in the PiP window
becoming blank and unusable on some systems. It seems safer in general
to call Widget::Init() on a constructed OverlayWindowViews object
anyway.

With this CL, we still call SetUpViews() in the constructor to set up
all the views::View pointers that are dereferenced as a result of
Widget::Init(). Since the root view is not available at this stage (it
gets created by Widget::Init()), we keep the child views in a temporary
container. We defer the initialization steps that require the root View
to a new helper function OverlayWindowViews::OnRootViewReady().

Bug: 1058852
Change-Id: I7434fc6067fae0b5fcc0135d6c3b7b8ff62d79d0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2107991
Commit-Queue: Wojciech Dzierżanowski <wdzierzanowski@opera.com>
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751824}

chrome/browser/ui/views/overlay/overlay_window_views.cc

@@ -44,12 +44,6 @@
 #include "ui/aura/window.h"
 #endif
 
-// static
-std::unique_ptr<content::OverlayWindow> content::OverlayWindow::Create(
-    content::PictureInPictureWindowController* controller) {
-  return base::WrapUnique(new OverlayWindowViews(controller));
-}
-
 namespace {
 constexpr gfx::Size kMinWindowSize(260, 146);
 
@@ -100,6 +94,13 @@ OverlayWindowViews::WindowQuadrant GetCurrentWindowQuadrant(
              : OverlayWindowViews::WindowQuadrant::kBottomRight;
 }
 
+template <typename T>
+T* AddChildView(std::vector<std::unique_ptr<views::View>>* views,
+                std::unique_ptr<T> child) {
+  views->push_back(std::move(child));
+  return static_cast<T*>(views->back().get());
+}
+
 }  // namespace
 
 // OverlayWindow implementation of NonClientFrameView.
@@ -193,18 +194,19 @@ class OverlayWindowWidgetDelegate : public views::WidgetDelegate {
   DISALLOW_COPY_AND_ASSIGN(OverlayWindowWidgetDelegate);
 };
 
-OverlayWindowViews::OverlayWindowViews(
-    content::PictureInPictureWindowController* controller)
-    : controller_(controller),
-      hide_controls_timer_(
-          FROM_HERE,
-          base::TimeDelta::FromMilliseconds(2500),
-          base::BindRepeating(&OverlayWindowViews::UpdateControlsVisibility,
-                              base::Unretained(this),
-                              false /* is_visible */)) {
+// static
+std::unique_ptr<content::OverlayWindow> OverlayWindowViews::Create(
+    content::PictureInPictureWindowController* controller) {
+  // Can't use make_unique(), which doesn't have access to the private
+  // constructor. It's important that the constructor be private, because it
+  // doesn't initialize the object fully.
+  auto overlay_window = base::WrapUnique(new OverlayWindowViews(controller));
+
   views::Widget::InitParams params(views::Widget::InitParams::TYPE_WINDOW);
   params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
-  params.bounds = CalculateAndUpdateWindowBounds();
+  // Just to have any non-empty bounds as required by Init(). The window is
+  // resized to fit the video that is embedded right afterwards, anyway.
+  params.bounds = gfx::Rect(overlay_window->GetMinimumSize());
   params.z_order = ui::ZOrderLevel::kFloatingWindow;
   params.visible_on_all_workspaces = true;
   params.remove_standard_frame = true;
@@ -219,18 +221,32 @@ OverlayWindowViews::OverlayWindowViews(
   // TODO(mukai): allow synchronizing activatability and remove this.
   params.activatable = views::Widget::InitParams::ACTIVATABLE_NO;
 #endif
-
   // Set WidgetDelegate for more control over |widget_|.
-  params.delegate = new OverlayWindowWidgetDelegate(this);
+  params.delegate = new OverlayWindowWidgetDelegate(overlay_window.get());
 
-  Init(std::move(params));
-  SetUpViews();
+  overlay_window->Init(std::move(params));
+  overlay_window->OnRootViewReady();
 
-#if defined(OS_CHROMEOS)
-  GetNativeWindow()->SetProperty(ash::kWindowPipTypeKey, true);
-#endif  // defined(OS_CHROMEOS)
+  return overlay_window;
+}
+
+// static
+std::unique_ptr<content::OverlayWindow> content::OverlayWindow::Create(
+    content::PictureInPictureWindowController* controller) {
+  return OverlayWindowViews::Create(controller);
+}
 
-  is_initialized_ = true;
+OverlayWindowViews::OverlayWindowViews(
+    content::PictureInPictureWindowController* controller)
+    : controller_(controller),
+      hide_controls_timer_(
+          FROM_HERE,
+          base::TimeDelta::FromMilliseconds(2500),
+          base::BindRepeating(&OverlayWindowViews::UpdateControlsVisibility,
+                              base::Unretained(this),
+                              false /* is_visible */)) {
+  CalculateAndUpdateWindowBounds();
+  SetUpViews();
 }
 
 OverlayWindowViews::~OverlayWindowViews() = default;
@@ -315,10 +331,6 @@ gfx::Rect OverlayWindowViews::CalculateAndUpdateWindowBounds() {
 }
 
 void OverlayWindowViews::SetUpViews() {
-  GetRootView()->SetPaintToLayer(ui::LAYER_TEXTURED);
-  GetRootView()->layer()->SetName("RootView");
-  GetRootView()->layer()->SetMasksToBounds(true);
-
   // views::View that is displayed when video is hidden. ----------------------
   // Adding an extra pixel to width/height makes sure controls background cover
   // entirely window when platform has fractional scale applied.
@@ -343,23 +355,17 @@ void OverlayWindowViews::SetUpViews() {
   auto resize_handle_view = std::make_unique<views::ResizeHandleButton>(this);
 #endif
 
-  gfx::Rect larger_window_bounds =
-      gfx::Rect(0, 0, GetBounds().width(), GetBounds().height());
-  larger_window_bounds.Inset(-1, -1);
-  window_background_view->SetBoundsRect(larger_window_bounds);
   window_background_view->SetPaintToLayer(ui::LAYER_SOLID_COLOR);
   window_background_view->layer()->SetName("WindowBackgroundView");
   window_background_view->layer()->SetColor(SK_ColorBLACK);
 
   // view::View that holds the video. -----------------------------------------
   video_view->SetPaintToLayer(ui::LAYER_TEXTURED);
-  video_view->SetSize(GetBounds().size());
   video_view->layer()->SetMasksToBounds(true);
   video_view->layer()->SetFillsBoundsOpaquely(false);
   video_view->layer()->SetName("VideoView");
 
   // views::View that holds the scrim, which appears with the controls. -------
-  controls_scrim_view->SetSize(GetBounds().size());
   controls_scrim_view->SetPaintToLayer(ui::LAYER_SOLID_COLOR);
   controls_scrim_view->layer()->SetName("ControlsScrimView");
   controls_scrim_view->layer()->SetColor(gfx::kGoogleGrey900);
@@ -407,29 +413,44 @@ void OverlayWindowViews::SetUpViews() {
 
   // Set up view::Views hierarchy. --------------------------------------------
   window_background_view_ =
-      GetContentsView()->AddChildView(std::move(window_background_view));
-  video_view_ = GetContentsView()->AddChildView(std::move(video_view));
+      AddChildView(&view_holder_, std::move(window_background_view));
+  video_view_ = AddChildView(&view_holder_, std::move(video_view));
   controls_scrim_view_ =
-      GetContentsView()->AddChildView(std::move(controls_scrim_view));
+      AddChildView(&view_holder_, std::move(controls_scrim_view));
   close_controls_view_ =
-      GetContentsView()->AddChildView(std::move(close_controls_view));
+      AddChildView(&view_holder_, std::move(close_controls_view));
   back_to_tab_controls_view_ =
-      GetContentsView()->AddChildView(std::move(back_to_tab_controls_view));
+      AddChildView(&view_holder_, std::move(back_to_tab_controls_view));
   previous_track_controls_view_ =
-      GetContentsView()->AddChildView(std::move(previous_track_controls_view));
+      AddChildView(&view_holder_, std::move(previous_track_controls_view));
   play_pause_controls_view_ =
-      GetContentsView()->AddChildView(std::move(play_pause_controls_view));
+      AddChildView(&view_holder_, std::move(play_pause_controls_view));
   next_track_controls_view_ =
-      GetContentsView()->AddChildView(std::move(next_track_controls_view));
+      AddChildView(&view_holder_, std::move(next_track_controls_view));
   skip_ad_controls_view_ =
-      GetContentsView()->AddChildView(std::move(skip_ad_controls_view));
+      AddChildView(&view_holder_, std::move(skip_ad_controls_view));
 #if defined(OS_CHROMEOS)
   resize_handle_view_ =
-      GetContentsView()->AddChildView(std::move(resize_handle_view));
+      AddChildView(&view_holder_, std::move(resize_handle_view));
 #endif
   UpdateControlsVisibility(false);
 }
 
+void OverlayWindowViews::OnRootViewReady() {
+#if defined(OS_CHROMEOS)
+  GetNativeWindow()->SetProperty(ash::kWindowPipTypeKey, true);
+#endif  // defined(OS_CHROMEOS)
+
+  GetRootView()->SetPaintToLayer(ui::LAYER_TEXTURED);
+  GetRootView()->layer()->SetName("RootView");
+  GetRootView()->layer()->SetMasksToBounds(true);
+
+  views::View* const contents_view = GetContentsView();
+  for (std::unique_ptr<views::View>& child : view_holder_)
+    contents_view->AddChildView(std::move(child));
+  view_holder_.clear();
+}
+
 void OverlayWindowViews::UpdateLayerBoundsWithLetterboxing(
     gfx::Size window_size) {
   // This is the case when the window is initially created or the video surface
@@ -667,11 +688,11 @@ void OverlayWindowViews::Hide() {
 }
 
 bool OverlayWindowViews::IsVisible() {
-  return is_initialized_ ? views::Widget::IsVisible() : false;
+  return views::Widget::IsVisible();
 }
 
 bool OverlayWindowViews::IsVisible() const {
-  return is_initialized_ ? views::Widget::IsVisible() : false;
+  return views::Widget::IsVisible();
 }
 
 bool OverlayWindowViews::IsAlwaysOnTop() {
@@ -745,8 +766,6 @@ void OverlayWindowViews::OnNativeBlur() {
   // Controls should be hidden when there is no more focus on the window. This
   // is used for tabbing and touch interactions. For mouse interactions, the
   // window cannot be blurred before the ui::ET_MOUSE_EXITED event is handled.
-  if (!is_initialized_)
-    return;
   UpdateControlsVisibility(false);
 
   views::Widget::OnNativeBlur();
@@ -767,8 +786,6 @@ gfx::Size OverlayWindowViews::GetMaximumSize() const {
 void OverlayWindowViews::OnNativeWidgetMove() {
   // Hide the controls when the window is moving. The controls will reappear
   // when the user interacts with the window again.
-  if (!is_initialized_)
-    return;
   UpdateControlsVisibility(false);
 
   // Update the existing |window_bounds_| when the window moves. This allows
@@ -791,8 +808,6 @@ void OverlayWindowViews::OnNativeWidgetMove() {
 void OverlayWindowViews::OnNativeWidgetSizeChanged(const gfx::Size& new_size) {
   // Hide the controls when the window is being resized. The controls will
   // reappear when the user interacts with the window again.
-  if (!is_initialized_)
-    return;
   UpdateControlsVisibility(false);
 
   // Update the view layers to scale to |new_size|.
@@ -1017,7 +1032,7 @@ ui::Layer* OverlayWindowViews::GetResizeHandleLayer() {
 
 gfx::Rect OverlayWindowViews::GetWorkAreaForWindow() const {
   return display::Screen::GetScreen()
-      ->GetDisplayNearestWindow(IsVisible()
+      ->GetDisplayNearestWindow(native_widget()
                                     ? GetNativeWindow()
                                     : controller_->GetInitiatorWebContents()
                                           ->GetTopLevelNativeWindow())
@@ -1028,7 +1043,7 @@ gfx::Size OverlayWindowViews::UpdateMaxSize(const gfx::Rect& work_area,
                                             const gfx::Size& window_size) {
   max_size_ = gfx::Size(work_area.width() / 2, work_area.height() / 2);
 
-  if (!IsVisible())
+  if (!native_widget())
     return window_size;
 
   if (window_size.width() <= max_size_.width() &&

chrome/browser/ui/views/overlay/overlay_window_views.h

@@ -31,8 +31,9 @@ class OverlayWindowViews : public content::OverlayWindow,
                            public views::ButtonListener,
                            public views::Widget {
  public:
-  explicit OverlayWindowViews(
+  static std::unique_ptr<content::OverlayWindow> Create(
       content::PictureInPictureWindowController* controller);
+
   ~OverlayWindowViews() override;
 
   enum class WindowQuadrant { kBottomLeft, kBottomRight, kTopLeft, kTopRight };
@@ -105,6 +106,9 @@ class OverlayWindowViews : public content::OverlayWindow,
                           const gfx::Size& window_size);
 
  private:
+  explicit OverlayWindowViews(
+      content::PictureInPictureWindowController* controller);
+
   // Return the work area for the nearest display the widget is on.
   gfx::Rect GetWorkAreaForWindow() const;
 
@@ -117,6 +121,9 @@ class OverlayWindowViews : public content::OverlayWindow,
   // Set up the views::Views that will be shown on the window.
   void SetUpViews();
 
+  // Finish initialization by performing the steps that require the root View.
+  void OnRootViewReady();
+
   // Update the bounds of the layers on the window. This may introduce
   // letterboxing.
   void UpdateLayerBoundsWithLetterboxing(gfx::Size window_size);
@@ -171,11 +178,6 @@ class OverlayWindowViews : public content::OverlayWindow,
   // Not owned; |controller_| owns |this|.
   content::PictureInPictureWindowController* controller_;
 
-  // Whether or not the components of the window has been set up. This is used
-  // as a check as some event handlers (e.g. focus) is propogated to the window
-  // before its contents is initialized. This is only set once.
-  bool is_initialized_ = false;
-
   // Whether or not the window has been shown before. This is used to determine
   // sizing and placement. This is different from checking whether the window
   // components has been initialized.
@@ -202,6 +204,11 @@ class OverlayWindowViews : public content::OverlayWindow,
   // ensuring factors such as aspect ratio is maintained.
   gfx::Size natural_size_;
 
+  // Temporary storage for child Views. Used during the time between
+  // construction and initialization, when the views::View pointer members must
+  // already be initialized, but there is no root view to add them to yet.
+  std::vector<std::unique_ptr<views::View>> view_holder_;
+
   // Views to be shown.
   views::View* window_background_view_ = nullptr;
   views::View* video_view_ = nullptr;