Retain tray bubble's rounded corners when the bubble animates out

ui/views/bubble/tray_bubble_view.cc

Index: ui/views/bubble/tray_bubble_view.cc
diff --git a/ui/views/bubble/tray_bubble_view.cc b/ui/views/bubble/tray_bubble_view.cc
index 8c402fea06b3dd35b808204e5db4dc84ea0a88d2..6a2f9236650c5558317acb2d6b22638e9743e794 100644
--- a/ui/views/bubble/tray_bubble_view.cc
+++ b/ui/views/bubble/tray_bubble_view.cc
@@ -179,7 +179,7 @@ class TrayBubbleContentMask : public ui::LayerDelegate {
   explicit TrayBubbleContentMask(int corner_radius);
   virtual ~TrayBubbleContentMask();
 
-  ui::Layer* layer() { return &layer_; }
+  ui::Layer* GetLayer();
 
   // Overridden from LayerDelegate.
   virtual void OnPaintLayer(gfx::Canvas* canvas) OVERRIDE;
@@ -187,25 +187,33 @@ class TrayBubbleContentMask : public ui::LayerDelegate {
   virtual base::Closure PrepareForLayerBoundsChange() OVERRIDE;
 
  private:
-  ui::Layer layer_;
+  ui::Layer* layer_;

[michaelpg, 2013/10/04 01:12:18]

Theoretically this pointer can be invalidated by the destruction of the layer
that will own it. That's not a problem in this code but is it worth
future-proofing, and if so what's the recommended method?

[Jun Mukai, 2013/10/04 01:39:36]

I think you don't need to keep the pointer of the mask layer here?
The only thing this class should know is the bounds or the size of the layer
(used in OnPaintLayer()).  Other references below, you can use
layer()->parent()->layer_mask_layer() instead.

[michaelpg, 2013/10/04 01:46:03]

Can the bounds of the mask layer not change during the lifetime of this class?

[Jun Mukai, 2013/10/04 01:56:19]

unfortunately the tray can change its size. For example, the message center
bubble can shrink after a notification is removed.

[michaelpg, 2013/10/04 05:56:41]

Actually I think the pointer is still needed in the destructor, because the
layer should outlive its delegate.

[Jun Mukai, 2013/10/04 06:55:45]

I think it can be moved to TrayBubbleView's destructor. TrayBubbleView owns this
class and knows the mask layer.

Oh, then, I think you should call set_delegate(NULL) when the main layer is
recreated through RecreateLayer().

   SkScalar corner_radius_;
 
   DISALLOW_COPY_AND_ASSIGN(TrayBubbleContentMask);
 };
 
 TrayBubbleContentMask::TrayBubbleContentMask(int corner_radius)
-    : layer_(ui::LAYER_TEXTURED),
+    : layer_(NULL),
       corner_radius_(corner_radius) {
-  layer_.set_delegate(this);
+  // Delay construction of mask layer so it can be owned by its parent layer.
 }
 
 TrayBubbleContentMask::~TrayBubbleContentMask() {
-  layer_.set_delegate(NULL);
+  layer_->set_delegate(NULL);
+}
+
+ui::Layer* TrayBubbleContentMask::GetLayer() {
+  if (!layer_) {
+    layer_ = new ui::Layer(ui::LAYER_TEXTURED);
+    layer_->set_delegate(this);
+  }

[piman, 2013/10/04 03:44:39]

This is weird and upside down. If this doesn't keep ownership of the layer, it
shouldn't be the one creating it.
Instead you should have TrayBubbleView::InitializeAndShowBubble create the mask
layer and install it on the TrayBubbleContentMask (e.g. add a set_layer()), and
then set it as the parent layer's mask (?). That makes it clear it's the
responsibility of the TrayBubbleContentMask owner to make sure the ui::Layer
outlives the TrayBubbleContentMask. It would also avoid calling GetLayer
accidentally in a case where it will not be set as the parent layer's mask,
which would cause a leak.

[michaelpg, 2013/10/04 05:56:41]

Ok, this makes more sense. I've made these changes.

+  return layer_;
 }
 
 void TrayBubbleContentMask::OnPaintLayer(gfx::Canvas* canvas) {
   SkPath path;
-  path.addRoundRect(gfx::RectToSkRect(gfx::Rect(layer()->bounds().size())),
+  path.addRoundRect(gfx::RectToSkRect(gfx::Rect(layer_->bounds().size())),
                     corner_radius_, corner_radius_);
   SkPaint paint;
   paint.setAlpha(255);
@@ -350,7 +358,7 @@ void TrayBubbleView::InitializeAndShowBubble() {
   bubble_border_->UpdateArrowOffset();
 
   if (get_use_acceleration_when_possible())
-    layer()->parent()->SetMaskLayer(bubble_content_mask_->layer());
+    layer()->parent()->SetMaskLayer(bubble_content_mask_->GetLayer());
 
   GetWidget()->Show();
   UpdateBubble();
@@ -359,7 +367,7 @@ void TrayBubbleView::InitializeAndShowBubble() {
 void TrayBubbleView::UpdateBubble() {
   SizeToContents();
   if (get_use_acceleration_when_possible())
-    bubble_content_mask_->layer()->SetBounds(layer()->bounds());
+    bubble_content_mask_->GetLayer()->SetBounds(layer()->bounds());
   GetWidget()->GetRootView()->SchedulePaint();
 }