Address some bubble clipping issues

Allow NonClientFrameView to define a clip for the client view. This fixes the website settings bubble.

Also draw the border on top in BubbleFrameView. This fixes the bookmark bubble sync promo.

This patch does not address client views with layers, such as ExtensionPopup.

BUG=581600
Committed: https://crrev.com/9c5853b50cb270fabac8533c2c6844fd83f00158
Cr-Commit-Position: refs/heads/master@{#382895}

[Evan Stade, 2016-03-09 04:21:40]

estade@chromium.org changed reviewers:
+ tapted@chromium.org

[Evan Stade, 2016-03-09 04:25:05]

hold on, for some reason this doesn't work on the bottom left of the bookmark
bubble with the signin promo

[tapted, 2016-03-09 04:32:35]

On 2016/03/09 04:25:05, Evan Stade wrote:
> hold on, for some reason this doesn't work on the bottom left of the bookmark
> bubble with the signin promo

Did you see the comments at
https://codereview.chromium.org/1633403002/diff/1/ui/views/view.cc ? Karan ran
into some weirdness when trying to apply a ClipPath. Maybe something to do with
the raster border assets pushing out the ContentBounds. But that might have
affected the chrome://chrome popup in the same way, so maybe not related.

[Evan Stade, 2016-03-09 05:07:32]

On 2016/03/09 04:32:35, tapted wrote:
> On 2016/03/09 04:25:05, Evan Stade wrote:
> > hold on, for some reason this doesn't work on the bottom left of the
bookmark
> > bubble with the signin promo
> 
> Did you see the comments at
> https://codereview.chromium.org/1633403002/diff/1/ui/views/view.cc ? Karan ran
> into some weirdness when trying to apply a ClipPath. Maybe something to do
with
> the raster border assets pushing out the ContentBounds. But that might have
> affected the chrome://chrome popup in the same way, so maybe not related.

I guess I should have read more before diving in. It's kind of late here. I'll
leave this CL alone and probably close it after Karan's CL lands.

[Evan Stade, 2016-03-22 03:24:10]

Description was changed from

==========
Clip bubble frame view's children so they can't square off edges of
bubble.

BUG=581600
==========

to

==========
Allow NonClientFrameView to define a clip for the client view. This fixes the
website settings bubble.

Also draw the border on top in BubbleFrameView. This fixes the bookmark bubble
sync promo.

This patch does not address client views with layers, such as ExtensionPopup.

BUG=581600
==========

[Evan Stade, 2016-03-22 03:24:20]

updated, wdyt?

[tapted, 2016-03-22 07:38:04]

a couple of questions, but it looks pretty neat! don't wait for me before
looping in OWNERS

https://codereview.chromium.org/1773393002/diff/40001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1773393002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:815:
clip_recorder.ClipPathWithAntiAliasing(clip_path_in_parent);
I don't have a sense for how computationally expensive it is to use this versus
a boring old rectangle. I guess somewhere between "a bit" and "a lot". But do
you think it's worth only using path-based clipping when the |clip_path_| is
non-empty?  (e.g., it's probably fine for bubbles and SearchResultPageView to
use path-based clipping, but doing it for the general case might be
unnecessarily wasteful -- cc::ClipDisplayItem does seem like it has a lot of
custom codepaths for rectangles. In fact, there's even custom code in
ClipDisplayItem for *rounded* rectangles, but ClipRecorder::ClipRect always
passes an empty vector for the `SkRRect` argument.  Using
cc::clip*Path*DisplayItem would miss out on all that

https://codereview.chromium.org/1773393002/diff/40001/ui/views/window/non_cli...
File ui/views/window/non_client_view.h (right):

https://codereview.chromium.org/1773393002/diff/40001/ui/views/window/non_cli...
ui/views/window/non_client_view.h:68: virtual bool GetClientMask(const
gfx::Size& size,
I think this is kindof a nice abstraction (it should have a comment :), but
could BubbleFrameView::Layout() just call
GetWidget()->client_view()->set_clip_path(...); ? Maybe that's simpler, but
maybe it's also nice to have this as a method on NonClientFrameView, so it's
clear any NonClientFrameView can clip its ClientView - up to you (or sky/msw :)

[Evan Stade, 2016-03-22 17:59:57]

estade@chromium.org changed reviewers:
+ msw@chromium.org, sky@chromium.org

[Evan Stade, 2016-03-22 17:59:58]

+sky, msw, thoughts?

https://codereview.chromium.org/1773393002/diff/40001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1773393002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:815:
clip_recorder.ClipPathWithAntiAliasing(clip_path_in_parent);
On 2016/03/22 07:38:04, tapted wrote:
> I don't have a sense for how computationally expensive it is to use this
versus
> a boring old rectangle. I guess somewhere between "a bit" and "a lot". But do
> you think it's worth only using path-based clipping when the |clip_path_| is
> non-empty?  (e.g., it's probably fine for bubbles and SearchResultPageView to
> use path-based clipping, but doing it for the general case might be
> unnecessarily wasteful -- cc::ClipDisplayItem does seem like it has a lot of
> custom codepaths for rectangles. In fact, there's even custom code in
> ClipDisplayItem for *rounded* rectangles, but ClipRecorder::ClipRect always
> passes an empty vector for the `SkRRect` argument.  Using
> cc::clip*Path*DisplayItem would miss out on all that

I was hoping it would be smart, i.e. if the path is a rectangle there's no AA to
be done. But I guess it's no more LOC to explicitly use ClipRect in the unset
case.

https://codereview.chromium.org/1773393002/diff/40001/ui/views/window/non_cli...
File ui/views/window/non_client_view.h (right):

https://codereview.chromium.org/1773393002/diff/40001/ui/views/window/non_cli...
ui/views/window/non_client_view.h:68: virtual bool GetClientMask(const
gfx::Size& size,
On 2016/03/22 07:38:04, tapted wrote:
> I think this is kindof a nice abstraction (it should have a comment :), but
> could BubbleFrameView::Layout() just call
> GetWidget()->client_view()->set_clip_path(...); ? Maybe that's simpler, but
> maybe it's also nice to have this as a method on NonClientFrameView, so it's
> clear any NonClientFrameView can clip its ClientView - up to you (or sky/msw
:)

That approach is fine with me, but it requires BubbleFrameView::Layout() to have
a side effect outside of its own child hierarchy, which seems like some sort of
violation. I'll let sky/msw comment on which approach they prefer. Agreed that
this function needs a comment.

[sky, 2016-03-22 18:09:40]

I'm fine with what you have here. LGTM

[msw, 2016-03-22 18:42:45]

I'm okay with this if Scott approves; just one minor nit.

https://codereview.chromium.org/1773393002/diff/60001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/1773393002/diff/60001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:142:
path->addRoundRect(gfx::RectToSkRect(gfx::Rect(gfx::Point(), size)), radius,
nit: use gfx::Rect(size))

[Evan Stade, 2016-03-22 20:00:45]

I discovered/remembered path AA wasn't necessary if I inset the path by 0.5f.
(You can see the difference in two of the four corners of the website settings
popup.)

https://codereview.chromium.org/1773393002/diff/60001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/1773393002/diff/60001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:142:
path->addRoundRect(gfx::RectToSkRect(gfx::Rect(gfx::Point(), size)), radius,
On 2016/03/22 18:42:45, msw wrote:
> nit: use gfx::Rect(size))

Done.

[Evan Stade, 2016-03-22 20:00:51]

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

[commit-bot: I haz the power, 2016-03-22 20:01:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1773393002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1773393002/100001

[tapted, 2016-03-22 20:20:05]

lgtm

[commit-bot: I haz the power, 2016-03-22 21:12:23]

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

[commit-bot: I haz the power, 2016-03-22 21:12:25]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-03-23 18:58:47]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-03-23 18:58:48]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1773393002/#ps100001
(title: "no aa")

[commit-bot: I haz the power, 2016-03-23 18:59:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1773393002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1773393002/100001

[commit-bot: I haz the power, 2016-03-23 19:07:21]

Description was changed from

==========
Allow NonClientFrameView to define a clip for the client view. This fixes the
website settings bubble.

Also draw the border on top in BubbleFrameView. This fixes the bookmark bubble
sync promo.

This patch does not address client views with layers, such as ExtensionPopup.

BUG=581600
==========

to

==========
Allow NonClientFrameView to define a clip for the client view. This fixes the
website settings bubble.

Also draw the border on top in BubbleFrameView. This fixes the bookmark bubble
sync promo.

This patch does not address client views with layers, such as ExtensionPopup.

BUG=581600
==========

[commit-bot: I haz the power, 2016-03-23 19:07:22]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-23 19:09:03]

Description was changed from

==========
Allow NonClientFrameView to define a clip for the client view. This fixes the
website settings bubble.

Also draw the border on top in BubbleFrameView. This fixes the bookmark bubble
sync promo.

This patch does not address client views with layers, such as ExtensionPopup.

BUG=581600
==========

to

==========
Allow NonClientFrameView to define a clip for the client view. This fixes the
website settings bubble.

Also draw the border on top in BubbleFrameView. This fixes the bookmark bubble
sync promo.

This patch does not address client views with layers, such as ExtensionPopup.

BUG=581600

Committed: https://crrev.com/9c5853b50cb270fabac8533c2c6844fd83f00158
Cr-Commit-Position: refs/heads/master@{#382895}
==========

[commit-bot: I haz the power, 2016-03-23 19:09:05]

Patchset 6 (id:??) landed as
https://crrev.com/9c5853b50cb270fabac8533c2c6844fd83f00158
Cr-Commit-Position: refs/heads/master@{#382895}