exo: Add notification surface to manager after commit

components/exo/notification_surface.cc

 // Copyright 2016 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/notification_surface.h"
 
 #include "components/exo/notification_surface_manager.h"
 #include "components/exo/surface.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_delegate.h"
@@ skipping 67 matching lines @@
       notification_id_(notification_id),
       window_(new aura::Window(new CustomWindowDelegate(surface))) {
   surface_->SetSurfaceDelegate(this);
   surface_->AddSurfaceObserver(this);
 
   window_->SetType(ui::wm::WINDOW_TYPE_CONTROL);
   window_->SetName("ExoNotificationSurface");
   window_->Init(ui::LAYER_NOT_DRAWN);
   window_->set_owned_by_parent(false);
 
   // TODO(xiyuan): Fix after Surface no longer has an auar::Window.

[reveman, 2016/07/20 18:58:02]

nit: aura::Window

[xiyuan, 2016/07/20 23:19:03]

Done.

   window_->AddChild(surface_->window());
   surface_->window()->Show();
-
-  manager_->AddSurface(this);
 }
 
 NotificationSurface::~NotificationSurface() {
   if (surface_) {
     surface_->SetSurfaceDelegate(nullptr);
     surface_->RemoveSurfaceObserver(this);
   }
   manager_->RemoveSurface(this);
 }
 
 gfx::Size NotificationSurface::GetSize() const {
   return surface_->content_size();
 }
 
 void NotificationSurface::OnSurfaceCommit() {
   surface_->CheckIfSurfaceHierarchyNeedsCommitToNewSurfaces();
   surface_->CommitSurfaceHierarchy();
 
   gfx::Rect bounds = window_->bounds();
   if (bounds.size() != surface_->content_size()) {
     bounds.set_size(surface_->content_size());
     window_->SetBounds(bounds);

[reveman, 2016/07/20 18:58:02]

as the bounds can change here, doesn't the manager code already have to handle
changes to size by observing the window size? and empty size -> non-empty size
is just a special case of that..

[xiyuan, 2016/07/20 19:52:41]

The hosting ArcCustomNotificationView is based on NativeViewHost, which does not
change its size (preferred size or bounds) in views system based on the hosted
NativeView. Even if we add bounds change handling to ArcCustomNotificationView,
it is still bad UX that a notification shows then immediately resizes.

We should not be showing the notification until the underlying surface is
committed at least once. Otherwise, I am not sure what user would see on screen.
If deferring AddSurface here sounds unreasonable, maybe we should add an
explicit method to notify manager about the commit?

[reveman, 2016/07/20 20:57:30]

I'm not suggesting we should show it unless bounds are non-empty. This code is
explicitly handling size changes so I assume that this can happen and is handled
correctly? If bounds changes are NOT handled correctly by the manager today then
maybe something like this would be appropriate:

gfx::Size old_size = window_->bounds().size();
gfx::Size new_size = surface_->content_size();
if (old_size != new_size()) {
  if (!old_size.IsEmpty())
    manager_->RemoveSurface(this);
  if (!new_size.IsEmpty())
    manager_->AddSurface(this);
}

and dtor would be updated accordingly.

[xiyuan, 2016/07/20 22:01:20]

I prefer not doing remove-then-add on bounds change (even though it works today
because the bounds is only changed once on the first commit). My concern is that
when the manager sees a surface is removed, it would not know the surface will
be added back later and have to update the UI to deal with the removed surface.
This could cause flashing later if we really needs to handle bounds change.

Think the main problem here is that hosting UI is created before surface is
committed. If the commit happens very late (or never happens), we are showing an
empty UI (or undefined contents).

   }
+
+  if (!added_to_manager_) {
+    added_to_manager_ = true;
+    manager_->AddSurface(this);
+  }
 }
 
 bool NotificationSurface::IsSurfaceSynchronized() const {
   return false;
 }
 
 void NotificationSurface::OnSurfaceDestroying(Surface* surface) {
   window_.reset();
   surface->RemoveSurfaceObserver(this);
   surface_ = nullptr;
 }
 
 }  // namespace exo