Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
25961002:
Retain tray bubble's rounded corners when the bubble animates out (Closed)
|
Created:
7 years, 2 months ago by michaelpg Modified:
7 years ago CC:
chromium-reviews, msw+watch_chromium.org, Ian Vollick, tfarina, sievers+watch_chromium.org, jbauman+watch_chromium.org, piman+watch_chromium.org, danakj+watch_chromium.org, alicet1, cc-bugs_chromium.org, stevenjb Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRetain tray bubble's rounded corners when the bubble animates out
by transferring ownership of a layer mask to the associated layer.
BUG=235563
TEST=Manual: when closing an Ash tray menu or tray notification, watch corners
to make sure they remain rounded.
R=piman@chromium.org, msw@chromium.org, mukai@chromium.org, skuhne@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239930
Patch Set 1 #
Total comments: 19
Patch Set 2 : Addressed feedback #
Total comments: 8
Patch Set 3 : Changed layer ownership #
Total comments: 4
Patch Set 4 : remove redundant check #
Messages
Total messages: 21 (0 generated)
piman: layer changes msw: tray_bubble_view changes skuhne, mukai: FYI Please make sure my use of scoped_ptr is reasonable. From what I can tell this works and won't leak memory. (btw, is there really no operator bool() overload equivalent to "ptr.get() != NULL"?) https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... ui/views/bubble/tray_bubble_view.cc:190: ui::Layer* layer_; 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?
https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.cc File ui/compositor/layer.cc (right): https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.cc#newcode66 ui/compositor/layer.cc:66: layer_mask_.reset(NULL); nit: just reset(), NULL is not necessary https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.cc#newcode90 ui/compositor/layer.cc:90: layer_mask_.reset(NULL); ditto https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.cc#newcod... ui/compositor/layer.cc:107: if (layer_mask_back_link_) this is weird, if the layer_mask_ is owned by the layer, layer_mask_ cannot be released by someone else. It means that back_link_ isn't necessary at all. https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... ui/views/bubble/tray_bubble_view.cc:190: ui::Layer* layer_; 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.
https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... ui/views/bubble/tray_bubble_view.cc:190: ui::Layer* layer_; On 2013/10/04 01:39:36, Jun Mukai (OOO Sep 30-Oct 15) wrote: > 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. Can the bounds of the mask layer not change during the lifetime of this class?
https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... ui/views/bubble/tray_bubble_view.cc:190: ui::Layer* layer_; On 2013/10/04 01:46:03, Michael Giuffrida wrote: > On 2013/10/04 01:39:36, Jun Mukai (OOO Sep 30-Oct 15) wrote: > > 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. > > Can the bounds of the mask layer not change during the lifetime of this class? unfortunately the tray can change its size. For example, the message center bubble can shrink after a notification is removed.
On 2013/10/04 01:56:19, Jun Mukai (OOO Sep 30-Oct 15) wrote: > https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... > File ui/views/bubble/tray_bubble_view.cc (right): > > https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... > ui/views/bubble/tray_bubble_view.cc:190: ui::Layer* layer_; > On 2013/10/04 01:46:03, Michael Giuffrida wrote: > > On 2013/10/04 01:39:36, Jun Mukai (OOO Sep 30-Oct 15) wrote: > > > 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. > > > > Can the bounds of the mask layer not change during the lifetime of this class? > > unfortunately the tray can change its size. For example, the message center > bubble can shrink after a notification is removed. Okay. How can I update the behavior of OnPaintLayer (214) without keeping a pointer to the layer in TrayBubbleContentMask, or am I misunderstanding?
On 2013/10/04 03:14:57, Michael Giuffrida wrote: > On 2013/10/04 01:56:19, Jun Mukai (OOO Sep 30-Oct 15) wrote: > > > https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... > > File ui/views/bubble/tray_bubble_view.cc (right): > > > > > https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... > > ui/views/bubble/tray_bubble_view.cc:190: ui::Layer* layer_; > > On 2013/10/04 01:46:03, Michael Giuffrida wrote: > > > On 2013/10/04 01:39:36, Jun Mukai (OOO Sep 30-Oct 15) wrote: > > > > 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. > > > > > > Can the bounds of the mask layer not change during the lifetime of this > class? > > > > unfortunately the tray can change its size. For example, the message center > > bubble can shrink after a notification is removed. > > Okay. How can I update the behavior of OnPaintLayer (214) without keeping a > pointer to the layer in TrayBubbleContentMask, or am I misunderstanding? What I imagined is: - TrayBubbleContentMask only holds a gfx::Size - update the size at UpdateBubble
https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.cc File ui/compositor/layer.cc (right): https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.cc#newcode66 ui/compositor/layer.cc:66: layer_mask_.reset(NULL); On 2013/10/04 01:39:36, Jun Mukai (OOO Sep 30-Oct 15) wrote: > nit: just reset(), NULL is not necessary Just remove. The default scoped_ptr constructor does the right thing. https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.cc#newcod... ui/compositor/layer.cc:107: if (layer_mask_back_link_) On 2013/10/04 01:39:36, Jun Mukai (OOO Sep 30-Oct 15) wrote: > this is weird, if the layer_mask_ is owned by the layer, layer_mask_ cannot be > released by someone else. It means that back_link_ isn't necessary at all. Agreed. Please remove. https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.h File ui/compositor/layer.h (right): https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.h#newcode203 ui/compositor/layer.h:203: void SetMaskLayer(Layer* layer_mask); Make this take a scoped_ptr<Layer>, to indicate it's taking ownership. https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... ui/views/bubble/tray_bubble_view.cc:210: } 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.
Thanks for the feedback. I hope this patch makes more sense. I've studied scoped_ptr some more and have a better handle on the internals but still have some questions -- I'll take another look in the morning and see if I can better understand what I'm doing! https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.cc File ui/compositor/layer.cc (right): https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.cc#newcode66 ui/compositor/layer.cc:66: layer_mask_.reset(NULL); On 2013/10/04 03:44:39, piman wrote: > On 2013/10/04 01:39:36, Jun Mukai (OOO Sep 30-Oct 15) wrote: > > nit: just reset(), NULL is not necessary > > Just remove. The default scoped_ptr constructor does the right thing. Done. https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.cc#newcode90 ui/compositor/layer.cc:90: layer_mask_.reset(NULL); On 2013/10/04 01:39:36, Jun Mukai (OOO Sep 30-Oct 15) wrote: > ditto Removed https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.cc#newcod... ui/compositor/layer.cc:107: if (layer_mask_back_link_) On 2013/10/04 03:44:39, piman wrote: > On 2013/10/04 01:39:36, Jun Mukai (OOO Sep 30-Oct 15) wrote: > > this is weird, if the layer_mask_ is owned by the layer, layer_mask_ cannot be > > released by someone else. It means that back_link_ isn't necessary at all. > > Agreed. Please remove. Done. https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.cc#newcod... ui/compositor/layer.cc:288: !layer_mask->layer_mask_back_link_)); Removing this debug check (because we remove the member) means we no longer ensure that the layer mask is not already a layer mask for some other layer. But since SetMaskLayer takes ownership I suppose this shouldn't be a concern anymore. https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.h File ui/compositor/layer.h (right): https://codereview.chromium.org/25961002/diff/1/ui/compositor/layer.h#newcode203 ui/compositor/layer.h:203: void SetMaskLayer(Layer* layer_mask); On 2013/10/04 03:44:39, piman wrote: > Make this take a scoped_ptr<Layer>, to indicate it's taking ownership. Done. https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... ui/views/bubble/tray_bubble_view.cc:190: ui::Layer* layer_; On 2013/10/04 01:39:36, Jun Mukai (OOO Sep 30-Oct 15) wrote: > 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. Actually I think the pointer is still needed in the destructor, because the layer should outlive its delegate. https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... ui/views/bubble/tray_bubble_view.cc:210: } On 2013/10/04 03:44:39, piman wrote: > 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. Ok, this makes more sense. I've made these changes. https://codereview.chromium.org/25961002/diff/19001/ui/compositor/layer.cc File ui/compositor/layer.cc (right): https://codereview.chromium.org/25961002/diff/19001/ui/compositor/layer.cc#ne... ui/compositor/layer.cc:104: cc_layer_->SetMaskLayer(NULL); in lieu of calling SetMaskLayer with an empty scoped_ptr<Layer> https://codereview.chromium.org/25961002/diff/19001/ui/compositor/layer.cc#ne... ui/compositor/layer.cc:289: layer_mask_.reset(layer_mask.release()); layer_mask is a scoped_ptr that now owns the layer, so we have to transfer that ownership to the layer_mask_ member variable... is there a cleaner way to do this? If not, should I use layer_mask.Pass() instead of release()? Digging through scoped_ptr and move.h seems to indicate that this will ultimately call swap() thus transferring ownership to layer_mask_.
https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/25961002/diff/1/ui/views/bubble/tray_bubble_v... ui/views/bubble/tray_bubble_view.cc:190: ui::Layer* layer_; On 2013/10/04 05:56:41, Michael Giuffrida wrote: > On 2013/10/04 01:39:36, Jun Mukai (OOO Sep 30-Oct 15) wrote: > > 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. > > Actually I think the pointer is still needed in the destructor, because the > layer should outlive its delegate. 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().
On Thu, Oct 3, 2013 at 9:12 PM, <michaelpg@chromium.org> wrote: > Reviewers: msw, Jun Mukai (OOO Sep 30-Oct 15), piman, Mr4D, > > Message: > piman: layer changes > msw: tray_bubble_view changes > skuhne, mukai: FYI > > Please make sure my use of scoped_ptr is reasonable. From what I can tell > this > works and won't leak memory. (btw, is there really no operator bool() > overload > equivalent to "ptr.get() != NULL"?) There is a (safe) operator bool for scoped_ptr, and it's worth using it. There is not for scoped_refptr. https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
+bartfab@ +dewittj@chromium.org - Can one of you look at the tray_bubble_view.cc changes? I'm not really familiar with ui::Layer.
couple of nits. https://codereview.chromium.org/25961002/diff/19001/ui/compositor/layer.cc File ui/compositor/layer.cc (right): https://codereview.chromium.org/25961002/diff/19001/ui/compositor/layer.cc#ne... ui/compositor/layer.cc:289: layer_mask_.reset(layer_mask.release()); On 2013/10/04 05:56:41, Michael Giuffrida wrote: > layer_mask is a scoped_ptr that now owns the layer, so we have to transfer that > ownership to the layer_mask_ member variable... is there a cleaner way to do > this? > > If not, should I use layer_mask.Pass() instead of release()? Digging through > scoped_ptr and move.h seems to indicate that this will ultimately call swap() > thus transferring ownership to layer_mask_. Yes, layer_mask_ = layer_mask.Pass(); is more idiomatic. https://codereview.chromium.org/25961002/diff/19001/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/25961002/diff/19001/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:357: mask_layer->set_delegate(bubble_content_mask_.get()); nit: I would do this in TrayBubbleContentMask::set_layer, which is more symmetrical since the destructor does reset the delegate. TrayBubbleContentMask::SetLayer(ui::Layer* layer) { DCHECK(layer); DCHECK(!layer_); // alternatively, if (layer_) layer_->set_delegate(NULL); layer_ = layer; layer_->set_delegate(this); }
https://codereview.chromium.org/25961002/diff/19001/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/25961002/diff/19001/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:190: ui::Layer* layer_; one more notes: the original layer of the TrayBubbleView can be recreated through RecreateLayer(), and the caller maintains the ownership of the old layer. That means that the lifetime of |layer_| can be unrelated to this class and potentially cause a use-after-free. Right now, this RecreateLayer is used for the disappearing animation of TrayBubbleView so TrayBubbleContentMask is removed earlier than the layer, but there is no guarantee. For example: ui::Layer* foo = tray_bubble->RecreateLayer(); // some process delete foo; tray_bubble->SizeToContents(); this code is legal and causes a use-after-free with your code (because |layer_| is owned by foo). This type of recreating layer is used to show the window on both displays during the drag-move across the displays. -- Of course a tray-bubble is tied to the shelf of a display and no 'drag-across-the-displays' situation happens right now. However, I think it's still better to take that into account. One idea is to remove |layer_| pointer to guarantee use-after-free can't happen. Another idea is to add a new method such like 'OnLayerRecreated()' to views::View and update |layer_| properly at that point.
This issue was further along than I thought. As far as I can tell these are the right changes to make. I've closed the other one, sorry for the inconvenience! https://codereview.chromium.org/25961002/diff/19001/ui/compositor/layer.cc File ui/compositor/layer.cc (right): https://codereview.chromium.org/25961002/diff/19001/ui/compositor/layer.cc#ne... ui/compositor/layer.cc:289: layer_mask_.reset(layer_mask.release()); On 2013/10/04 18:05:33, piman wrote: > On 2013/10/04 05:56:41, Michael Giuffrida wrote: > > layer_mask is a scoped_ptr that now owns the layer, so we have to transfer > that > > ownership to the layer_mask_ member variable... is there a cleaner way to do > > this? > > > > If not, should I use layer_mask.Pass() instead of release()? Digging through > > scoped_ptr and move.h seems to indicate that this will ultimately call swap() > > thus transferring ownership to layer_mask_. > > Yes, layer_mask_ = layer_mask.Pass(); is more idiomatic. Ok, thanks. https://codereview.chromium.org/25961002/diff/19001/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/25961002/diff/19001/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:190: ui::Layer* layer_; On 2013/10/05 01:23:50, Jun Mukai wrote: > one more notes: > the original layer of the TrayBubbleView can be recreated through > RecreateLayer(), and the caller maintains the ownership of the old layer. > That means that the lifetime of |layer_| can be unrelated to this class and > potentially cause a use-after-free. > > Right now, this RecreateLayer is used for the disappearing animation of > TrayBubbleView so TrayBubbleContentMask is removed earlier than the layer, but > there is no guarantee. > > For example: > ui::Layer* foo = tray_bubble->RecreateLayer(); > // some process > delete foo; > tray_bubble->SizeToContents(); > > this code is legal and causes a use-after-free with your code (because |layer_| > is owned by foo). > > This type of recreating layer is used to show the window on both displays during > the drag-move across the displays. > > -- > > Of course a tray-bubble is tied to the shelf of a display and no > 'drag-across-the-displays' situation happens right now. However, I think it's > still better to take that into account. One idea is to remove |layer_| pointer > to guarantee use-after-free can't happen. Another idea is to add a new method > such like 'OnLayerRecreated()' to views::View and update |layer_| properly at > that point. Yes. I've removed the layer_ pointer. I believe it doesn't matter any more which object(s) are deleted first. https://codereview.chromium.org/25961002/diff/19001/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:357: mask_layer->set_delegate(bubble_content_mask_.get()); On 2013/10/04 18:05:33, piman wrote: > nit: I would do this in TrayBubbleContentMask::set_layer, which is more > symmetrical since the destructor does reset the delegate. > > TrayBubbleContentMask::SetLayer(ui::Layer* layer) { > DCHECK(layer); > DCHECK(!layer_); // alternatively, if (layer_) layer_->set_delegate(NULL); > layer_ = layer; > layer_->set_delegate(this); > } I'm keeping the actual layer outside of TrayBubbleContentMask and just using it as the delegate to paint the mask. I've also made TrayBubbleView reset the delegate now.
lgtm
lgtm lgtm https://codereview.chromium.org/25961002/diff/49001/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/25961002/diff/49001/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:204: SkPath path; Could you please add a comment towards why you are using this bounds rather then the layer's bounds?
https://codereview.chromium.org/25961002/diff/49001/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/25961002/diff/49001/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:204: SkPath path; On 2013/12/10 15:28:58, Mr4D wrote: > Could you please add a comment towards why you are using this bounds rather then > the layer's bounds? As discussed, we are using the layer's bounds. :)
owner lgtm (with one suggestion) https://codereview.chromium.org/25961002/diff/49001/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/25961002/diff/49001/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:341: if (get_use_acceleration_when_possible() && Do we need the first check? i.e. if it's false, then layer()->parent()->layer_mask_layer() is always going to be NULL anyway, yes? Removing the extra test means that if the logic changes, it only needs to happen in one place.
https://codereview.chromium.org/25961002/diff/49001/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/25961002/diff/49001/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:341: if (get_use_acceleration_when_possible() && On 2013/12/10 22:16:02, stevenjb wrote: > Do we need the first check? i.e. if it's false, then > layer()->parent()->layer_mask_layer() is always going to be NULL anyway, yes? > Removing the extra test means that if the logic changes, it only needs to happen > in one place. Yes, you're right. I added the second check because in most cases the layer is recreated before this destructor is called, and that makes the first check redundant.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelpg@chromium.org/25961002/69001
Message was sent while issue was closed.
Change committed as 239930 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
