MacViews: Clip contents for non-rectangular windows.

MacViews: Clip contents for non-rectangular windows.

Currently, default rectangular clipping is done on non-rectangular windows.
Hence for these windows, there is no way to prevent subviews from painting
outside the window boundary. To resolve this, this CL adds a mask layer to the
compositor_superview's layer in BridgedContentView. The path of the mask layer
corresponds to the window mask of the non-client frame view attached to the
widget.

BUG=581600

[karandeepb, 2016-02-24 03:18:41]

Patchset #1 (id:1) has been deleted

[karandeepb, 2016-02-24 03:18:51]

Patchset #1 (id:20001) has been deleted

[karandeepb, 2016-02-24 03:18:58]

Patchset #1 (id:40001) has been deleted

[karandeepb, 2016-02-24 03:44:07]

Description was changed from

==========
Formatted code


Initial commit. Check edit bookmark dialog and macviewsbrowser

BUG=
==========

to

==========
MacViews: Clip contents for non-rectangular windows.

Currently, default rectangular clipping is done on non-rectangular windows.
Hence for these windows, there is no way to prevent subviews from painting
outside the window boundary. To resolve this, this CL adds a mask layer to the
compositor_superview's layer in BridgedContentView. The path of the mask layer
corresponds to the window mask of the non-client frame view attached to the
widget.

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

[karandeepb, 2016-02-24 03:46:35]

Patchset #1 (id:60001) has been deleted

[karandeepb, 2016-02-24 03:50:15]

Patchset #1 (id:80001) has been deleted

[karandeepb, 2016-02-24 04:12:07]

Patchset #1 (id:100001) has been deleted

[karandeepb, 2016-02-24 04:18:46]

Patchset #1 (id:120001) has been deleted

[karandeepb, 2016-02-24 04:27:17]

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

[karandeepb, 2016-02-24 04:27:18]

PTAL Trent. I have attached screenshots on the corresponding bug.

https://codereview.chromium.org/1718043003/diff/140001/ui/gfx/path_mac.mm
File ui/gfx/path_mac.mm (right):

https://codereview.chromium.org/1718043003/diff/140001/ui/gfx/path_mac.mm#new...
ui/gfx/path_mac.mm:123: base::ScopedCFTypeRef<CGPathRef>
CreateCGPathFromNSBezierPath(
This is slightly inspired from
https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/CocoaD....
Is that ok?

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:1260: // If mask layer does not exist
yet, create one and set it as the background
Another place to initialize the mask layer could be through
NativeWidgetMac::OnWidgetInitDone since by then the non client frame view would
have been attached to the widget.

[tapted, 2016-02-24 05:05:03]

https://codereview.chromium.org/1718043003/diff/140001/ui/gfx/path_mac.mm
File ui/gfx/path_mac.mm (right):

https://codereview.chromium.org/1718043003/diff/140001/ui/gfx/path_mac.mm#new...
ui/gfx/path_mac.mm:123: base::ScopedCFTypeRef<CGPathRef>
CreateCGPathFromNSBezierPath(
On 2016/02/24 04:27:18, karandeepb wrote:
> This is slightly inspired from
>
https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/CocoaD....
> Is that ok?

I think you can do

#import "third_party/google_toolbox_for_mac/src/AppKit/GTMNSBezierPath+CGPath.h"

in bridged_native_widget.mm then just call -[NSBezierPath gtm_CGPath]


(however, it seems to be missing the bit about the path being closed, so maybe a
helper function in an anonymous namespace is good for that)

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.h (right):

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.h:83: - (void)onSizeChanged;
perhaps just expose updateWindowMask?

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.h:86: - (NSBezierPath*)windowMask;
@property(readonly, nonatomic) NSBezierPath* windowMask;

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:7: #import <Quartz/Quartz.h>
this can be grouped with the other <> includes, since they are all "operating
system headers" rather than STL/standard-library headers

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:689: [bridged_view_ onSizeChanged];
Yeah, I'd call the method updateWindowMask and invoke it from UpdateMaskPath()
instead of here

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:1249: if (![bridged_view_ windowMask])
perhaps after this, do

{
  // Removing a mask after adding one isn't currently supported.
  DCHECK(![[compositor_superview_ layer] mask]);
  return;
}

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:1260: // If mask layer does not exist
yet, create one and set it as the background
On 2016/02/24 04:27:18, karandeepb wrote:
> Another place to initialize the mask layer could be through
> NativeWidgetMac::OnWidgetInitDone since by then the non client frame view
would
> have been attached to the widget.

I think this here is OK. E.g. it's feasible that we may want to support removing
the mask later on if the client frame view changes its mind about things after
it gets shown.

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:1265: [mask setBackgroundColor:[[NSColor
clearColor] CGColor]];
There's a CGColor constant for this (also I think this is default? So might not
need to be specified)

[karandeepb, 2016-02-25 03:27:04]

Patchset #2 (id:160001) has been deleted

[karandeepb, 2016-02-25 03:31:06]

PTAL Trent. Thanks.

https://codereview.chromium.org/1718043003/diff/140001/ui/gfx/path_mac.mm
File ui/gfx/path_mac.mm (right):

https://codereview.chromium.org/1718043003/diff/140001/ui/gfx/path_mac.mm#new...
ui/gfx/path_mac.mm:123: base::ScopedCFTypeRef<CGPathRef>
CreateCGPathFromNSBezierPath(
On 2016/02/24 05:05:03, tapted wrote:
> On 2016/02/24 04:27:18, karandeepb wrote:
> > This is slightly inspired from
> >
>
https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/CocoaD....
> > Is that ok?
> 
> I think you can do
> 
> #import
"third_party/google_toolbox_for_mac/src/AppKit/GTMNSBezierPath+CGPath.h"
> 
> in bridged_native_widget.mm then just call -[NSBezierPath gtm_CGPath]
> 
> 
> (however, it seems to be missing the bit about the path being closed, so maybe
a
> helper function in an anonymous namespace is good for that)

Done. Weird that the src folder doesn't show up on code search -
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/google....

Don't think closing the path is necessary. That is only required during hit
testing. Here we are using the path to fill the mask layer, thereby changing its
alpha channel. And while filling, Quartz treats each subpath as closed.

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.h (right):

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.h:83: - (void)onSizeChanged;
On 2016/02/24 05:05:03, tapted wrote:
> perhaps just expose updateWindowMask?
> 

Done. Wasn't it better encapsulation though, since bridged content view managed
its own state.

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.h:86: - (NSBezierPath*)windowMask;
On 2016/02/24 05:05:03, tapted wrote:
> @property(readonly, nonatomic) NSBezierPath* windowMask;

Done.

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:7: #import <Quartz/Quartz.h>
On 2016/02/24 05:05:03, tapted wrote:
> this can be grouped with the other <> includes, since they are all "operating
> system headers" rather than STL/standard-library headers

Done. Aren't stdint.h and stddef.h standard library headers?

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:689: [bridged_view_ onSizeChanged];
On 2016/02/24 05:05:03, tapted wrote:
> Yeah, I'd call the method updateWindowMask and invoke it from UpdateMaskPath()
> instead of here

Done.

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:1249: if (![bridged_view_ windowMask])
On 2016/02/24 05:05:03, tapted wrote:
> perhaps after this, do
> 
> {
>   // Removing a mask after adding one isn't currently supported.
>   DCHECK(![[compositor_superview_ layer] mask]);
>   return;
> }

Done.

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:1260: // If mask layer does not exist
yet, create one and set it as the background
On 2016/02/24 05:05:03, tapted wrote:
> On 2016/02/24 04:27:18, karandeepb wrote:
> > Another place to initialize the mask layer could be through
> > NativeWidgetMac::OnWidgetInitDone since by then the non client frame view
> would
> > have been attached to the widget.
> 
> I think this here is OK. E.g. it's feasible that we may want to support
removing
> the mask later on if the client frame view changes its mind about things after
> it gets shown.

Done.

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:1265: [mask setBackgroundColor:[[NSColor
clearColor] CGColor]];
On 2016/02/24 05:05:03, tapted wrote:
> There's a CGColor constant for this (also I think this is default? So might
not
> need to be specified)

Done.

[tapted, 2016-02-25 08:07:21]

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.h (right):

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.h:83: - (void)onSizeChanged;
On 2016/02/25 03:31:06, karandeepb wrote:
> On 2016/02/24 05:05:03, tapted wrote:
> > perhaps just expose updateWindowMask?
> > 
> 
> Done. Wasn't it better encapsulation though, since bridged content view
managed
> its own state.

I think what I didn't like was that an NSView shouldn't need to be told that its
size changed -- it should be able to get that from Cocoa APIs.

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1718043003/diff/140001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:7: #import <Quartz/Quartz.h>
On 2016/02/25 03:31:06, karandeepb wrote:
> On 2016/02/24 05:05:03, tapted wrote:
> > this can be grouped with the other <> includes, since they are all
"operating
> > system headers" rather than STL/standard-library headers
> 
> Done. Aren't stdint.h and stddef.h standard library headers?

I guess it's a bit arbitrary, but I generally see it applied as "group stuff
with a '.' in the header, and group stuff like <vector><string>,etc."

https://codereview.chromium.org/1718043003/diff/180001/ui/views/DEPS
File ui/views/DEPS (right):

https://codereview.chromium.org/1718043003/diff/180001/ui/views/DEPS#newcode26
ui/views/DEPS:26: "+third_party/google_toolbox_for_mac/src/AppKit"
I think we can put this in ui/views/cocoa/DEPS, making it

include_rules = [
  "+ui/accelerated_widget_mac",
  +third_party/google_toolbox_for_mac/src",
]


And make sure all the builders are happy with this, including a component
build... 

deps += [ "//third_party/google_toolbox_for_mac" ]

should have to go into a BUILD.gn file somewhere. also a .gyp.  But also it's
ObjectiveC, which has runtime linking, so stuff can still compile without this
but fail at compile time with an `unrecognized selector` error

https://codereview.chromium.org/1718043003/diff/180001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/1718043003/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.mm:231: windowMask_.reset();
is this needed here? (i.e. was it to fix an error?)

https://codereview.chromium.org/1718043003/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.mm:293: - (NSBezierPath*)windowMask {
nit: move up beneath clearView, since it roughly matches the declaration order
(initializers/dealloc should still always be before properties that aren't
synthesized)

[karandeepb, 2016-02-26 05:23:04]

Patchset #4 (id:220001) has been deleted

[karandeepb, 2016-03-03 07:27:39]

Patchset #5 (id:260001) has been deleted

[karandeepb, 2016-03-03 07:27:53]

Patchset #7 (id:320001) has been deleted

[karandeepb, 2016-03-04 03:01:56]

PTAL Trent. Sorry for the delay. The trybots pass as can be seen from Patch Set
7.

https://codereview.chromium.org/1718043003/diff/180001/ui/views/DEPS
File ui/views/DEPS (right):

https://codereview.chromium.org/1718043003/diff/180001/ui/views/DEPS#newcode26
ui/views/DEPS:26: "+third_party/google_toolbox_for_mac/src/AppKit"
On 2016/02/25 08:07:21, tapted wrote:
> I think we can put this in ui/views/cocoa/DEPS, making it
> 
> include_rules = [
>   "+ui/accelerated_widget_mac",
>   +third_party/google_toolbox_for_mac/src",
> ]
> 
> 
> And make sure all the builders are happy with this, including a component
> build... 
> 
> deps += [ "//third_party/google_toolbox_for_mac" ]
> 
> should have to go into a BUILD.gn file somewhere. also a .gyp.  But also it's
> ObjectiveC, which has runtime linking, so stuff can still compile without this
> but fail at compile time with an `unrecognized selector` error

Done.

https://codereview.chromium.org/1718043003/diff/180001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/1718043003/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.mm:231: windowMask_.reset();
On 2016/02/25 08:07:21, tapted wrote:
> is this needed here? (i.e. was it to fix an error?)

Not really. This is called here -
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/cocoa/bri...
and it has a comment to the effect that Cocoa may still have references to the
bridged_view_ object. Hence I thought we should free up windowMask_.

https://codereview.chromium.org/1718043003/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.mm:293: - (NSBezierPath*)windowMask {
On 2016/02/25 08:07:21, tapted wrote:
> nit: move up beneath clearView, since it roughly matches the declaration order
> (initializers/dealloc should still always be before properties that aren't
> synthesized)

Done.

https://codereview.chromium.org/1718043003/diff/360001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/1718043003/diff/360001/ui/views/views.gyp#new...
ui/views/views.gyp:801: # Needed to link to Obj-C static libraries.
I am not sure if this needs to be added to a gn file as well. The trybots pass
without it. Also, I can't seem to find a direct analogous of link_settings in
gn.

[tapted, 2016-03-04 04:15:01]

https://codereview.chromium.org/1718043003/diff/180001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/1718043003/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.mm:231: windowMask_.reset();
On 2016/03/04 03:01:55, karandeepb wrote:
> On 2016/02/25 08:07:21, tapted wrote:
> > is this needed here? (i.e. was it to fix an error?)
> 
> Not really. This is called here -
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/cocoa/bri...
> and it has a comment to the effect that Cocoa may still have references to the
> bridged_view_ object. Hence I thought we should free up windowMask_.

That shouldn't be necessary - if windowMask_ referred to some C++ object that
was about to be `delete`d by the caller we would, but since it's on ObjC object
we can just rely on the regular ObjC lifetime. So let's delete this line,
otherwise we're implying there's a more complicated lifetime issue than there
is.

https://codereview.chromium.org/1718043003/diff/360001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1718043003/diff/360001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:1243: // TODO(karandeepb): Add tests to
verify mask layer is correctly set up.
we don't usually have TODO: add tests in code comments :). You can make a bug
and assign it to yourself, but if it's not urgent we can usually just write the
tests and land them with the CL.

https://codereview.chromium.org/1718043003/diff/360001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/1718043003/diff/360001/ui/views/views.gyp#new...
ui/views/views.gyp:801: # Needed to link to Obj-C static libraries.
On 2016/03/04 03:01:56, karandeepb wrote:
> I am not sure if this needs to be added to a gn file as well. The trybots pass
> without it. Also, I can't seem to find a direct analogous of link_settings in
> gn. 

ldflags += [ "-Wl,-ObjC" ]

would probably do it.

I think the Mac-GN trybot [assuming there is one] currently just builds, and
doesn't try to run anything, which is why this probably hasn't come up in the
past.

Can you try running components_unittests in GN build locally on Mac, to see if
it's needed? 

(there might be an explosion of other problems, or components_unittests might
not even be built at all, but we should try not to leave problems for us later
on, since these errors are really hard to diagnose)

[karandeepb, 2016-03-07 04:58:50]

Patchset #9 (id:380001) has been deleted

[karandeepb, 2016-03-07 06:34:11]

Patchset #9 (id:400001) has been deleted