arc: Defer notification surface creation

ui/arc/notification/arc_custom_notification_item.cc

Index: ui/arc/notification/arc_custom_notification_item.cc
diff --git a/ui/arc/notification/arc_custom_notification_item.cc b/ui/arc/notification/arc_custom_notification_item.cc
index 0ef1db6884b24dae155ef8ffc5a8456465fda1a4..79e4a9a858914504661bbfadeb8144cdeedbc406 100644
--- a/ui/arc/notification/arc_custom_notification_item.cc
+++ b/ui/arc/notification/arc_custom_notification_item.cc
@@ -7,7 +7,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
-#include "components/exo/notification_surface.h"
+#include "components/arc/bitmap/bitmap_type_converters.h"
 #include "ui/arc/notification/arc_custom_notification_view.h"
 #include "ui/message_center/notification.h"
 #include "ui/message_center/notification_types.h"
@@ -24,14 +24,7 @@ class ArcNotificationDelegate : public message_center::NotificationDelegate {
       : item_(item) {}
 
   std::unique_ptr<views::View> CreateCustomContent() override {
-    if (!surface_)
-      return nullptr;
-
-    return base::MakeUnique<ArcCustomNotificationView>(item_, surface_);
-  }
-
-  void set_notification_surface(exo::NotificationSurface* surface) {
-    surface_ = surface;
+    return base::MakeUnique<ArcCustomNotificationView>(item_);
   }
 
  private:
@@ -39,7 +32,6 @@ class ArcNotificationDelegate : public message_center::NotificationDelegate {
   ~ArcNotificationDelegate() override {}
 
   ArcCustomNotificationItem* const item_;
-  exo::NotificationSurface* surface_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(ArcNotificationDelegate);
 };
@@ -55,13 +47,9 @@ ArcCustomNotificationItem::ArcCustomNotificationItem(
                           message_center,
                           notification_key,
                           profile_id) {
-  ArcNotificationSurfaceManager::Get()->AddObserver(this);
 }
 
 ArcCustomNotificationItem::~ArcCustomNotificationItem() {
-  if (ArcNotificationSurfaceManager::Get())
-    ArcNotificationSurfaceManager::Get()->RemoveObserver(this);
-
   FOR_EACH_OBSERVER(Observer, observers_, OnItemDestroying());
 }
 
@@ -92,13 +80,18 @@ void ArcCustomNotificationItem::UpdateWithArcNotificationData(
       GURL(),                    // empty origin url, for system component
       notifier_id, rich_data, new ArcNotificationDelegate(this)));
 
-  exo::NotificationSurface* surface =
-      ArcNotificationSurfaceManager::Get()->GetSurface(notification_key());
-  if (surface)
-    OnNotificationSurfaceAdded(surface);
-
   pinned_ = rich_data.pinned;
-  FOR_EACH_OBSERVER(Observer, observers_, OnItemPinnedChanged());
+
+  if (data.snapshot_image.is_null()) {
+    snapshot_ = gfx::ImageSkia();
+  } else {
+    snapshot_ = gfx::ImageSkia(gfx::ImageSkiaRep(
+        data.snapshot_image.To<SkBitmap>(), data.snapshot_image_scale));

[dcheng, 2016/08/31 05:57:54]

Do we have a bug filed to migrate ARC++ over to use typemapping?

[xiyuan, 2016/09/01 04:29:15]

Filed http://crbug.com/643050

+  }
+
+  FOR_EACH_OBSERVER(Observer, observers_, OnItemUpdated());
+
+  AddToMessageCenter();
 }
 
 void ArcCustomNotificationItem::CloseFromCloseButton() {
@@ -116,24 +109,17 @@ void ArcCustomNotificationItem::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-void ArcCustomNotificationItem::OnNotificationSurfaceAdded(
-    exo::NotificationSurface* surface) {
-  if (!pending_notification() ||
-      surface->notification_id() != notification_key()) {
-    return;
-  }
-
-  static_cast<ArcNotificationDelegate*>(pending_notification()->delegate())
-      ->set_notification_surface(surface);
-  AddToMessageCenter();
+void ArcCustomNotificationItem::IncrementWindowRefCount() {
+  ++window_ref_count_;
+  if (window_ref_count_ == 1)
+    manager()->CreateNotificationWindow(notification_key());
 }
 
-void ArcCustomNotificationItem::OnNotificationSurfaceRemoved(
-    exo::NotificationSurface* surface) {
-  if (surface->notification_id() != notification_key())
-    return;
-
-  FOR_EACH_OBSERVER(Observer, observers_, OnItemNotificationSurfaceRemoved());
+void ArcCustomNotificationItem::DecrementWindowRefCount() {
+  DCHECK_GT(window_ref_count_, 0);
+  --window_ref_count_;
+  if (window_ref_count_ == 0)
+    manager()->CloseNotificationWindow(notification_key());

[dcheng, 2016/08/31 05:57:54]

So I don't completely understand the design here, since I don't know how the
classes relate.

I assume there's only one ArcNotificationManager? But what about
ArcCustomNotificationItem/ArcCustomNotificationView? Are those always a 1:1
relationship? Why do we need this window "ref count"?

1) As far as I can tell, we construct the view with an item, and always
increment the refcount then, and then always decrement it when the view is
destroyed.

2) Why does ArcCustomNotificationItem have an ObserverList? It seems like
there's ever only one observer? SHould it just be a delegate instead, and then
we can key this on the delegate being set / unset?

[xiyuan, 2016/08/31 14:01:55]

ArcCustomNotificationItem contains the data and it could have two
ArcCustomNotificationView when we go from floating notification bubble to the
notification list. At that transition, there are two views, one is going away
and there is one being created. If we don't do ref count, the window could be
wrongly closed.

Same reason to have an ObserverList vs delegate here.

 }
 
 }  // namespace arc