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

ui/views/bubble/tray_bubble_view.cc

 // Copyright (c) 2012 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 "ui/views/bubble/tray_bubble_view.h"
 
 #include <algorithm>
 
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkColor.h"
@@ skipping 161 matching lines @@
 
 // This mask layer clips the bubble's content so that it does not overwrite the
 // rounded bubble corners.
 // TODO(miket): This does not work on Windows. Implement layer masking or
 // alternate solutions if the TrayBubbleView is needed there in the future.
 class TrayBubbleContentMask : public ui::LayerDelegate {
  public:
   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;
   virtual void OnDeviceScaleFactorChanged(float device_scale_factor) OVERRIDE;
   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);
   paint.setStyle(SkPaint::kFill_Style);
   canvas->DrawPath(path, paint);
 }
 
 void TrayBubbleContentMask::OnDeviceScaleFactorChanged(
     float device_scale_factor) {
   // Redrawing will take care of scale factor change.
@@ skipping 124 matching lines @@
   if (delegate_)
     delegate_->BubbleViewDestroyed();
 }
 
 void TrayBubbleView::InitializeAndShowBubble() {
   // Must occur after call to BubbleDelegateView::CreateBubble().
   SetAlignment(params_.arrow_alignment);
   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();
 }
 
 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();
 }
 
 void TrayBubbleView::SetMaxHeight(int height) {
   params_.max_height = height;
   if (GetWidget())
     SizeToContents();
 }
 
 void TrayBubbleView::SetWidth(int width) {
@@ skipping 128 matching lines @@
     const ViewHierarchyChangedDetails& details) {
   if (get_use_acceleration_when_possible() && details.is_add &&
       details.child == this) {
     details.parent->SetPaintToLayer(true);
     details.parent->SetFillsBoundsOpaquely(true);
     details.parent->layer()->SetMasksToBounds(true);
   }
 }
 
 }  // namespace views