Ensure that mask layer is of the same size as the owning layer.

Ensure that mask layer is of the same size as the owning layer.

We do not support applying a mask layer of different size to the owning
layer, this CL adds DCHECKs to prevent that situation.

BUG=567296
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2908073003
Cr-Commit-Position: refs/heads/master@{#479502}
Committed: https://chromium.googlesource.com/chromium/src/+/0fe1c3b9d8ebf6654405c5aeb116eabe9728aa1e

[sunxd, 2017-05-29 20:07:02]

Description was changed from

==========
Ensure that mask layer is of the same size as the owning layer.

We do not support applying a mask layer of different size to the owning
layer, this CL adds DCHECKs to prevent that situation.

BUG=567296
==========

to

==========
Ensure that mask layer is of the same size as the owning layer.

We do not support applying a mask layer of different size to the owning
layer, this CL adds DCHECKs to prevent that situation.

BUG=567296
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sunxd, 2017-05-29 20:07:29]

The CQ bit was checked by sunxd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 20:07:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunxd, 2017-05-29 20:07:57]

Description was changed from

==========
Ensure that mask layer is of the same size as the owning layer.

We do not support applying a mask layer of different size to the owning
layer, this CL adds DCHECKs to prevent that situation.

BUG=567296
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Ensure that mask layer is of the same size as the owning layer.

We do not support applying a mask layer of different size to the owning
layer, this CL adds DCHECKs to prevent that situation.

BUG=567296
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sunxd, 2017-05-29 20:07:58]

sunxd@chromium.org changed reviewers:
+ enne@chromium.org, ericrk@chromium.org, trchen@chromium.org

[sunxd, 2017-05-29 20:09:36]

PTAL.

Per discussion in
https://codereview.chromium.org/2859483006/diff/100001/cc/layers/layer.cc#new...,
I made a new patch adding DCHECK to SetMaskLayer.

I think we still need the cc one, as tests may set the wrong size.

[commit-bot: I haz the power, 2017-05-29 21:03:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-29 21:03:23]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[enne (OOO), 2017-05-29 22:30:50]

lgtm

[trchen, 2017-05-30 23:43:29]

The DCHECK will trigger because mask layers are attached immediately when
created, before its geometry gets updated.
I think it is okay for the main tree to be in the "bad" state intermittently, as
long as we get into a valid state at the time of commit.

[sunxd, 2017-05-31 14:35:21]

The CQ bit was checked by sunxd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 14:35:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 15:40:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 15:40:39]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sunxd, 2017-05-31 21:56:20]

PTAL.

I moved the DCHECK to cc::Layer::PushPropertiesTo.

[sunxd, 2017-05-31 21:56:20]

PTAL.

I moved the DCHECK to cc::Layer::PushPropertiesTo.

[sunxd, 2017-06-06 17:48:46]

On 2017/05/31 21:56:20, sunxd wrote:
> PTAL.
> 
> I moved the DCHECK to cc::Layer::PushPropertiesTo.

Hi trchen@, can you review the updated patch, please?

[trchen, 2017-06-06 20:45:06]

The CQ bit was checked by trchen@chromium.org

[trchen, 2017-06-06 20:45:07]

lgtm

lgtm. Sorry for the delay!

[trchen, 2017-06-06 20:45:07]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2908073003/#ps40001
(title: "Check mask size at commit.")

[commit-bot: I haz the power, 2017-06-06 20:45:24]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 21:51:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 21:51:54]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunxd, 2017-06-07 14:11:54]

The CQ bit was checked by sunxd@chromium.org

[commit-bot: I haz the power, 2017-06-07 14:12:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 15:14:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 15:14:04]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunxd, 2017-06-08 15:58:54]

sunxd@chromium.org changed reviewers:
+ jamescook@chromium.org

[sunxd, 2017-06-08 16:04:15]

Hi jamescook@,

I have met a problem with ash_unittests-mus.SystemTrayTest: in cc we should not
support a situation where a mask layer have different size with its owner
because this would cause tiling and rasterization problems. So we want to land
this change to ensure the mask layer size is set correctly across chrome.

But the DCHECK is triggered in the above tests by SystemTray::ShowDefaultView()
on the trybot. I see that you have recently worked on this file, can you provide
some insights of what's wrong here, please? I have printed the stack trace of
the call that incorrectly sets the mask size on the trybot.

Thanks!

[James Cook, 2017-06-08 17:45:43]

jamescook@chromium.org changed reviewers:
+ sky@chromium.org, tdanderson@chromium.org

[James Cook, 2017-06-08 17:45:44]

sky, do you have any insight here why this is mash-only?

tdanderson, could you take a look at the ink drop mask code and see if this
might be related?

The failure is only in mash_unittests, not ash_unittests, see patch set 3 -
 https://codereview.chromium.org/2908073003/#ps40001

There seem to be a couple failures:
[19710:19710:0607/072816.044136:945283911:FATAL:layer.cc(1208)] Check failed:
bounds().ToString() == mask_layer()->bounds().ToString() (352x259 vs. 352x34)

and

[22855:22855:0607/072822.495277:951735050:FATAL:layer.cc(1208)] Check failed:
bounds().ToString() == mask_layer()->bounds().ToString() (352x259 vs. 352x211)

The second one is off by 48 pixels, which is the size of the shelf (kShelfSize)
the size of an ink drop ripple (kInkDropSmallSize) and also the min height for a
tray popup (kTrayPopupItemMinHeight).

sunxd, I suggest looking at TrayPopupUtils::CreateInkDropMask() and seeing what
it is doing.
https://cs.chromium.org/chromium/src/ash/system/tray/tray_popup_utils.cc?type...

[tdanderson, 2017-06-08 18:16:47]

tdanderson@chromium.org changed reviewers:
+ bruthig@chromium.org

[tdanderson, 2017-06-08 18:16:48]

+bruthig@, as he is a better person to ask about the ink drop. Ben, please take
a look at https://codereview.chromium.org/2908073003/#msg31

[sky, 2017-06-08 21:20:55]

sky@chromium.org changed reviewers:
+ fsamuel@chromium.org

[sky, 2017-06-08 21:21:50]

On 2017/06/08 17:45:44, James Cook wrote:
> sky, do you have any insight here why this is mash-only?
> 
> tdanderson, could you take a look at the ink drop mask code and see if this
> might be related?
> 
> The failure is only in mash_unittests, not ash_unittests, see patch set 3 -
>  https://codereview.chromium.org/2908073003/#ps40001
> 
> There seem to be a couple failures:
> [19710:19710:0607/072816.044136:945283911:FATAL:layer.cc(1208)] Check failed:
> bounds().ToString() == mask_layer()->bounds().ToString() (352x259 vs. 352x34)
> 
> and
> 
> [22855:22855:0607/072822.495277:951735050:FATAL:layer.cc(1208)] Check failed:
> bounds().ToString() == mask_layer()->bounds().ToString() (352x259 vs. 352x211)
> 
> The second one is off by 48 pixels, which is the size of the shelf
(kShelfSize)
> the size of an ink drop ripple (kInkDropSmallSize) and also the min height for
a
> tray popup (kTrayPopupItemMinHeight).
> 
> sunxd, I suggest looking at TrayPopupUtils::CreateInkDropMask() and seeing
what
> it is doing.
>
https://cs.chromium.org/chromium/src/ash/system/tray/tray_popup_utils.cc?type...

+fsamuel
I can't think of why that would be the case either. Can you narrow down exactly
what view (or aura::Window) this is happening on?

[bruthig, 2017-06-09 15:49:09]

On 2017/06/08 17:45:44, James Cook wrote:
> sky, do you have any insight here why this is mash-only?
> 
> tdanderson, could you take a look at the ink drop mask code and see if this
> might be related?
> 
> The failure is only in mash_unittests, not ash_unittests, see patch set 3 -
>  https://codereview.chromium.org/2908073003/#ps40001
> 
> There seem to be a couple failures:
> [19710:19710:0607/072816.044136:945283911:FATAL:layer.cc(1208)] Check failed:
> bounds().ToString() == mask_layer()->bounds().ToString() (352x259 vs. 352x34)
> 
> and
> 
> [22855:22855:0607/072822.495277:951735050:FATAL:layer.cc(1208)] Check failed:
> bounds().ToString() == mask_layer()->bounds().ToString() (352x259 vs. 352x211)
> 
> The second one is off by 48 pixels, which is the size of the shelf
(kShelfSize)
> the size of an ink drop ripple (kInkDropSmallSize) and also the min height for
a
> tray popup (kTrayPopupItemMinHeight).
> 
> sunxd, I suggest looking at TrayPopupUtils::CreateInkDropMask() and seeing
what
> it is doing.
>
https://cs.chromium.org/chromium/src/ash/system/tray/tray_popup_utils.cc?type...

Those sizes are very large for an InkDrop and it would surprise me somewhat to
know that a Layer is being created that big.

I believe all the InkDrop layers have a name set on them.  If InkDrops are
causing this I would expect the Layer owning the mask to be named
"InkDropImpl::RootLayer". [These are set on the ui::Layers, I'm not sure if they
are passed along to the cc::Layers.)

After a quick search on callers of ui::Layer::SetMaskLayer() I would suspect
TrayBubbleView's use of TrayBubbleContentMask, especially given the sizes. See
https://cs.chromium.org/chromium/src/ui/views/bubble/tray_bubble_view.cc?rcl=...

My guess is TrayBubbleView isn't updating the bounds of it's
|bubble_content_mask_|'s layer appropriately.

PS ash::kTrayMenuMinimumWidth = 352, which is the width of the system menu
bubble.

[sky, 2017-06-09 17:23:20]

On 2017/06/09 15:49:09, bruthig wrote:
> On 2017/06/08 17:45:44, James Cook wrote:
> > sky, do you have any insight here why this is mash-only?
> > 
> > tdanderson, could you take a look at the ink drop mask code and see if this
> > might be related?
> > 
> > The failure is only in mash_unittests, not ash_unittests, see patch set 3 -
> >  https://codereview.chromium.org/2908073003/#ps40001
> > 
> > There seem to be a couple failures:
> > [19710:19710:0607/072816.044136:945283911:FATAL:layer.cc(1208)] Check
failed:
> > bounds().ToString() == mask_layer()->bounds().ToString() (352x259 vs.
352x34)
> > 
> > and
> > 
> > [22855:22855:0607/072822.495277:951735050:FATAL:layer.cc(1208)] Check
failed:
> > bounds().ToString() == mask_layer()->bounds().ToString() (352x259 vs.
352x211)
> > 
> > The second one is off by 48 pixels, which is the size of the shelf
> (kShelfSize)
> > the size of an ink drop ripple (kInkDropSmallSize) and also the min height
for
> a
> > tray popup (kTrayPopupItemMinHeight).
> > 
> > sunxd, I suggest looking at TrayPopupUtils::CreateInkDropMask() and seeing
> what
> > it is doing.
> >
>
https://cs.chromium.org/chromium/src/ash/system/tray/tray_popup_utils.cc?type...
> 
> Those sizes are very large for an InkDrop and it would surprise me somewhat to
> know that a Layer is being created that big.
> 
> I believe all the InkDrop layers have a name set on them.  If InkDrops are
> causing this I would expect the Layer owning the mask to be named
> "InkDropImpl::RootLayer". [These are set on the ui::Layers, I'm not sure if
they
> are passed along to the cc::Layers.)
> 
> After a quick search on callers of ui::Layer::SetMaskLayer() I would suspect
> TrayBubbleView's use of TrayBubbleContentMask, especially given the sizes. See
>
https://cs.chromium.org/chromium/src/ui/views/bubble/tray_bubble_view.cc?rcl=...
> 
> My guess is TrayBubbleView isn't updating the bounds of it's
> |bubble_content_mask_|'s layer appropriately.
> 
> PS ash::kTrayMenuMinimumWidth = 352, which is the width of the system menu
> bubble.

Thanks for trying to narrow this down Ben.

If this is TrayBubbleView then I think the bug is there. On a quick look of the
code it seems like TrayBubbleView may not be calling
bubble_content_mask_->layer()->SetBounds(layer()->bounds()); in enough places. I
would expect it to override OnBoundsChanged() and update bubble_context_mask_'s
bounds.

[sunxd, 2017-06-13 14:33:13]

On 2017/06/09 17:23:20, sky OOO wrote:
> On 2017/06/09 15:49:09, bruthig wrote:
> > On 2017/06/08 17:45:44, James Cook wrote:
> > > sky, do you have any insight here why this is mash-only?
> > > 
> > > tdanderson, could you take a look at the ink drop mask code and see if
this
> > > might be related?
> > > 
> > > The failure is only in mash_unittests, not ash_unittests, see patch set 3
-
> > >  https://codereview.chromium.org/2908073003/#ps40001
> > > 
> > > There seem to be a couple failures:
> > > [19710:19710:0607/072816.044136:945283911:FATAL:layer.cc(1208)] Check
> failed:
> > > bounds().ToString() == mask_layer()->bounds().ToString() (352x259 vs.
> 352x34)
> > > 
> > > and
> > > 
> > > [22855:22855:0607/072822.495277:951735050:FATAL:layer.cc(1208)] Check
> failed:
> > > bounds().ToString() == mask_layer()->bounds().ToString() (352x259 vs.
> 352x211)
> > > 
> > > The second one is off by 48 pixels, which is the size of the shelf
> > (kShelfSize)
> > > the size of an ink drop ripple (kInkDropSmallSize) and also the min height
> for
> > a
> > > tray popup (kTrayPopupItemMinHeight).
> > > 
> > > sunxd, I suggest looking at TrayPopupUtils::CreateInkDropMask() and seeing
> > what
> > > it is doing.
> > >
> >
>
https://cs.chromium.org/chromium/src/ash/system/tray/tray_popup_utils.cc?type...
> > 
> > Those sizes are very large for an InkDrop and it would surprise me somewhat
to
> > know that a Layer is being created that big.
> > 
> > I believe all the InkDrop layers have a name set on them.  If InkDrops are
> > causing this I would expect the Layer owning the mask to be named
> > "InkDropImpl::RootLayer". [These are set on the ui::Layers, I'm not sure if
> they
> > are passed along to the cc::Layers.)
> > 
> > After a quick search on callers of ui::Layer::SetMaskLayer() I would suspect
> > TrayBubbleView's use of TrayBubbleContentMask, especially given the sizes.
See
> >
>
https://cs.chromium.org/chromium/src/ui/views/bubble/tray_bubble_view.cc?rcl=...
> > 
> > My guess is TrayBubbleView isn't updating the bounds of it's
> > |bubble_content_mask_|'s layer appropriately.
> > 
> > PS ash::kTrayMenuMinimumWidth = 352, which is the width of the system menu
> > bubble.
> 
> Thanks for trying to narrow this down Ben.
> 
> If this is TrayBubbleView then I think the bug is there. On a quick look of
the
> code it seems like TrayBubbleView may not be calling
> bubble_content_mask_->layer()->SetBounds(layer()->bounds()); in enough places.
I
> would expect it to override OnBoundsChanged() and update
bubble_context_mask_'s
> bounds.

It seems originally the TrayBubbleView is of size 352*211, and in
TrayBubbleView::SetMaxHeight(), BubbleDialogDelegateView::SizeToContents() which
sets the widget bounds to 352*259 gets called.

Does it make sense to force resetting mask layer size in
TrayBubbleView::SizeToContents()?

[James Cook, 2017-06-13 15:14:56]

On 2017/06/13 14:33:13, sunxd wrote:
> On 2017/06/09 17:23:20, sky OOO wrote:
> > On 2017/06/09 15:49:09, bruthig wrote:
> > > On 2017/06/08 17:45:44, James Cook wrote:
> > > > sky, do you have any insight here why this is mash-only?
> > > > 
> > > > tdanderson, could you take a look at the ink drop mask code and see if
> this
> > > > might be related?
> > > > 
> > > > The failure is only in mash_unittests, not ash_unittests, see patch set
3
> -
> > > >  https://codereview.chromium.org/2908073003/#ps40001
> > > > 
> > > > There seem to be a couple failures:
> > > > [19710:19710:0607/072816.044136:945283911:FATAL:layer.cc(1208)] Check
> > failed:
> > > > bounds().ToString() == mask_layer()->bounds().ToString() (352x259 vs.
> > 352x34)
> > > > 
> > > > and
> > > > 
> > > > [22855:22855:0607/072822.495277:951735050:FATAL:layer.cc(1208)] Check
> > failed:
> > > > bounds().ToString() == mask_layer()->bounds().ToString() (352x259 vs.
> > 352x211)
> > > > 
> > > > The second one is off by 48 pixels, which is the size of the shelf
> > > (kShelfSize)
> > > > the size of an ink drop ripple (kInkDropSmallSize) and also the min
height
> > for
> > > a
> > > > tray popup (kTrayPopupItemMinHeight).
> > > > 
> > > > sunxd, I suggest looking at TrayPopupUtils::CreateInkDropMask() and
seeing
> > > what
> > > > it is doing.
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/ash/system/tray/tray_popup_utils.cc?type...
> > > 
> > > Those sizes are very large for an InkDrop and it would surprise me
somewhat
> to
> > > know that a Layer is being created that big.
> > > 
> > > I believe all the InkDrop layers have a name set on them.  If InkDrops are
> > > causing this I would expect the Layer owning the mask to be named
> > > "InkDropImpl::RootLayer". [These are set on the ui::Layers, I'm not sure
if
> > they
> > > are passed along to the cc::Layers.)
> > > 
> > > After a quick search on callers of ui::Layer::SetMaskLayer() I would
suspect
> > > TrayBubbleView's use of TrayBubbleContentMask, especially given the sizes.
> See
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/views/bubble/tray_bubble_view.cc?rcl=...
> > > 
> > > My guess is TrayBubbleView isn't updating the bounds of it's
> > > |bubble_content_mask_|'s layer appropriately.
> > > 
> > > PS ash::kTrayMenuMinimumWidth = 352, which is the width of the system menu
> > > bubble.
> > 
> > Thanks for trying to narrow this down Ben.
> > 
> > If this is TrayBubbleView then I think the bug is there. On a quick look of
> the
> > code it seems like TrayBubbleView may not be calling
> > bubble_content_mask_->layer()->SetBounds(layer()->bounds()); in enough
places.
> I
> > would expect it to override OnBoundsChanged() and update
> bubble_context_mask_'s
> > bounds.
> 
> It seems originally the TrayBubbleView is of size 352*211, and in
> TrayBubbleView::SetMaxHeight(), BubbleDialogDelegateView::SizeToContents()
which
> sets the widget bounds to 352*259 gets called.
> 
> Does it make sense to force resetting mask layer size in
> TrayBubbleView::SizeToContents()?

That sounds reasonable to me.

[sunxd, 2017-06-13 17:46:39]

The CQ bit was checked by sunxd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 17:47:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 18:04:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-13 18:04:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunxd, 2017-06-13 18:22:03]

The CQ bit was checked by sunxd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 18:22:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 19:46:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-13 19:46:52]

Dry run: This issue passed the CQ dry run.

[sunxd, 2017-06-13 20:29:29]

PTAL. I attempt to fix this issue, but not sure about whether it's a good fix.

[James Cook, 2017-06-13 20:43:46]

ui/views/bubble lgtm with nits, but you'll still need an owner for that
directory

https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
File ui/views/bubble/tray_bubble_view.cc (right):

https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.cc:386: void TrayBubbleView::SizeToContents() {
nit: move this up to match new header order

https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.cc:387:
GetWidget()->SetBounds(GetBubbleBounds());
This should call the base class, BubbleDialogDelegateView::SizeToContents()

https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
File ui/views/bubble/tray_bubble_view.h (right):

https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.h:154: void SizeToContents() override;
nit: move this up to line 148 to put it with the other BubbleDialogDelegateView
method

[sunxd, 2017-06-13 22:09:24]

sunxd@chromium.org changed reviewers:
+ stevenjb@chromium.org

[sunxd, 2017-06-13 22:13:45]

sunxd@chromium.org changed reviewers:
+ thakis@chromium.org

[sunxd, 2017-06-13 22:13:47]

Thanks James.

Hi stevenjb@,

Can you take a look ui/views/bubble/tray_bubble_view.* changes please?

Hi thankis@,

Can you take a look at ui/views/bubble and
chrome/browser/ui/views/perimission_bubble changes please?

Thanks.

https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
File ui/views/bubble/tray_bubble_view.cc (right):

https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.cc:386: void TrayBubbleView::SizeToContents() {
On 2017/06/13 20:43:45, James Cook wrote:
> nit: move this up to match new header order

Done.

https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.cc:387:
GetWidget()->SetBounds(GetBubbleBounds());
On 2017/06/13 20:43:46, James Cook wrote:
> This should call the base class, BubbleDialogDelegateView::SizeToContents()

Done.

https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
File ui/views/bubble/tray_bubble_view.h (right):

https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.h:154: void SizeToContents() override;
On 2017/06/13 20:43:46, James Cook wrote:
> nit: move this up to line 148 to put it with the other
BubbleDialogDelegateView
> method

Done.

[sunxd, 2017-06-13 22:15:21]

The CQ bit was checked by sunxd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 22:15:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 22:32:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-13 22:32:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunxd, 2017-06-14 15:08:33]

The CQ bit was checked by sunxd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 15:09:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 17:28:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 17:28:30]

Dry run: This issue passed the CQ dry run.

[sunxd, 2017-06-14 17:38:28]

On 2017/06/13 22:13:47, sunxd wrote:
> Thanks James.
> 
> Hi stevenjb@,
> 
> Can you take a look ui/views/bubble/tray_bubble_view.* changes please?
> 
> Hi thankis@,
> 
> Can you take a look at ui/views/bubble and
> chrome/browser/ui/views/perimission_bubble changes please?
> 
> Thanks.
> 
>
https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
> File ui/views/bubble/tray_bubble_view.cc (right):
> 
>
https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
> ui/views/bubble/tray_bubble_view.cc:386: void TrayBubbleView::SizeToContents()
{
> On 2017/06/13 20:43:45, James Cook wrote:
> > nit: move this up to match new header order
> 
> Done.
> 
>
https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
> ui/views/bubble/tray_bubble_view.cc:387:
> GetWidget()->SetBounds(GetBubbleBounds());
> On 2017/06/13 20:43:46, James Cook wrote:
> > This should call the base class, BubbleDialogDelegateView::SizeToContents()
> 
> Done.
> 
>
https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
> File ui/views/bubble/tray_bubble_view.h (right):
> 
>
https://codereview.chromium.org/2908073003/diff/100001/ui/views/bubble/tray_b...
> ui/views/bubble/tray_bubble_view.h:154: void SizeToContents() override;
> On 2017/06/13 20:43:46, James Cook wrote:
> > nit: move this up to line 148 to put it with the other
> BubbleDialogDelegateView
> > method
> 
> Done.

Sorry I spelt the wrong ldap, thakis@, can you please review the two directories
mentioned in the above message? Thanks!

[Nico, 2017-06-14 17:50:38]

lgtm

[stevenjb, 2017-06-14 17:58:59]

https://codereview.chromium.org/2908073003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc
(right):

https://codereview.chromium.org/2908073003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:58: void
SizeToContents() override;
Assuming this overrides a BubbleDialogDelegateView method, it should be listed
below with other BubbleDialogDelegateView overrides.

[sunxd, 2017-06-14 18:19:57]

PTAL.

https://codereview.chromium.org/2908073003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc
(right):

https://codereview.chromium.org/2908073003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:58: void
SizeToContents() override;
On 2017/06/14 17:58:58, stevenjb wrote:
> Assuming this overrides a BubbleDialogDelegateView method, it should be listed
> below with other BubbleDialogDelegateView overrides.

Done.

[stevenjb, 2017-06-14 18:33:48]

lgtm

[sunxd, 2017-06-14 18:40:31]

The CQ bit was checked by sunxd@chromium.org

[sunxd, 2017-06-14 18:40:32]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
trchen@chromium.org, jamescook@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2908073003/#ps160001
(title: "style change")

[commit-bot: I haz the power, 2017-06-14 18:41:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 21:02:41]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1497465631684340,
"parent_rev": "e062336432adb699cda43e3cd781c8d1b9cae368", "commit_rev":
"0fe1c3b9d8ebf6654405c5aeb116eabe9728aa1e"}

[commit-bot: I haz the power, 2017-06-14 21:02:50]

Description was changed from

==========
Ensure that mask layer is of the same size as the owning layer.

We do not support applying a mask layer of different size to the owning
layer, this CL adds DCHECKs to prevent that situation.

BUG=567296
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Ensure that mask layer is of the same size as the owning layer.

We do not support applying a mask layer of different size to the owning
layer, this CL adds DCHECKs to prevent that situation.

BUG=567296
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2908073003
Cr-Commit-Position: refs/heads/master@{#479502}
Committed:
https://chromium.googlesource.com/chromium/src/+/0fe1c3b9d8ebf6654405c5aeb116...
==========

[commit-bot: I haz the power, 2017-06-14 21:02:53]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/0fe1c3b9d8ebf6654405c5aeb116...