[Mac] Avoid "adding unknown subview" warning.

[Mac] Avoid "adding unknown subview" warning.

Chrome Mac has added a subview above the window's content view, which
the Appkit warns about at runtime. Presumably, doing so may break in a
future macOS release. This cl adds Chrome's top-level subview to the
window's content view, which is what the Appkit wants.

BUG=605219

[erikchen, 2016-09-20 22:35:47]

erikchen@chromium.org changed reviewers:
+ erikchen@chromium.org

[erikchen, 2016-09-20 22:35:47]

why don't you need NSFullSizeContentViewWindowMask? Does that get set
automatically? Does this mean that
https://bugs.chromium.org/p/chromium/issues/detail?id=380412#c25 is fixed? Have
you tried those steps?

[shrike, 2016-09-20 22:43:19]

On 2016/09/20 22:35:47, erikchen wrote:
> why don't you need NSFullSizeContentViewWindowMask? Does that get set
> automatically?

I don't know. I wasn't thinking about (or really aware of)
NSFullSizeContentViewWindowMask. Perhaps setting the titlebar to be transparent
is the key. But it seems like there are no special hoops to jump through to get
the tabstrip tabs, etc. where they need to be, and the stoplight buttons still
work.

[erikchen, 2016-09-20 22:53:36]

On 2016/09/20 22:43:19, shrike wrote:
> On 2016/09/20 22:35:47, erikchen wrote:
> > why don't you need NSFullSizeContentViewWindowMask? Does that get set
> > automatically?
> 
> I don't know. I wasn't thinking about (or really aware of)
> NSFullSizeContentViewWindowMask. Perhaps setting the titlebar to be
transparent
> is the key. But it seems like there are no special hoops to jump through to
get
> the tabstrip tabs, etc. where they need to be, and the stoplight buttons still
> work.

Can you try the steps in
https://bugs.chromium.org/p/chromium/issues/detail?id=380412#c25 to see if it
still works?

[shrike, 2016-09-20 23:00:25]

On 2016/09/20 22:53:36, erikchen wrote:
> Can you try the steps in
> https://bugs.chromium.org/p/chromium/issues/detail?id=380412#c25 to see if it
> still works?

Hi erikchen@,

I apologize - you asked about the steps in that comment previously, but the
steps did not jump out at me when I first looked.

If I

1. Click in the top portion of the New Tab button
2. While still holding down the mouse, drag the mouse out of the New Tab button
to abort the press

the New Tab button unhighlights when I drag out, and the press aborts. The
window does not move as I drag the mouse.

[erikchen, 2016-09-20 23:07:42]

On 2016/09/20 23:00:25, shrike wrote:
> On 2016/09/20 22:53:36, erikchen wrote:
> > Can you try the steps in
> > https://bugs.chromium.org/p/chromium/issues/detail?id=380412#c25 to see if
it
> > still works?
> 
> Hi erikchen@,
> 
> I apologize - you asked about the steps in that comment previously, but the
> steps did not jump out at me when I first looked.
> 
> If I
> 
> 1. Click in the top portion of the New Tab button
> 2. While still holding down the mouse, drag the mouse out of the New Tab
button
> to abort the press
> 
> the New Tab button unhighlights when I drag out, and the press aborts. The
> window does not move as I drag the mouse.

If it "just works", I won't stop you from landing it. :) That being said, I
think you should only change the behavior of OSes that you've tested on. Did you
test on 10.11 or 10.10?

[erikchen, 2016-09-20 23:07:47]

https://codereview.chromium.org/2351183003/diff/1/chrome/browser/ui/cocoa/fra...
File chrome/browser/ui/cocoa/framed_browser_window.h (right):

https://codereview.chromium.org/2351183003/diff/1/chrome/browser/ui/cocoa/fra...
chrome/browser/ui/cocoa/framed_browser_window.h:24: const CGFloat
kBrowserFrameViewPaintHeight = 37.0;
I imagine we'll need two variables, one for 10.11+, and one for 10.10-?

https://codereview.chromium.org/2351183003/diff/1/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/full_size_content_window.mm (right):

https://codereview.chromium.org/2351183003/diff/1/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/full_size_content_window.mm:145: // ensureit is
positioned below the buttons.
s/ensureit/ensure it/

https://codereview.chromium.org/2351183003/diff/1/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right):

https://codereview.chromium.org/2351183003/diff/1/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:398: if
(base::mac::IsAtLeastOS10_10()) {
This CL contains references to both IsAtMost 10.10 and IsAtLeast10.10, for which
10.10 overlaps. It's not clear to me whether you're trying to change 10.11+ or
10.10+

[shrike, 2016-09-20 23:14:57]

On 2016/09/20 23:07:42, erikchen wrote:
> If it "just works", I won't stop you from landing it. :) That being said, I
> think you should only change the behavior of OSes that you've tested on. Did
you
> test on 10.11 or 10.10?

Agreed. My intent was to only change behavior on 10.11 and above. I'm working on
a 10.11 machine, and we'll know if it works or not on 10.12.

[shrike, 2016-09-20 23:27:17]

PTAL (but note that I still need to address the issue with the 37.0 frame paint
height)

https://codereview.chromium.org/2351183003/diff/1/chrome/browser/ui/cocoa/fra...
File chrome/browser/ui/cocoa/framed_browser_window.h (right):

https://codereview.chromium.org/2351183003/diff/1/chrome/browser/ui/cocoa/fra...
chrome/browser/ui/cocoa/framed_browser_window.h:24: const CGFloat
kBrowserFrameViewPaintHeight = 37.0;
On 2016/09/20 23:07:47, erikchen wrote:
> I imagine we'll need two variables, one for 10.11+, and one for 10.10-?

Ugh. My assumption is that the frame paint height is larger than it needs to be
for Material Design and so it's OK to make this change, but you may be right
about needing to special case this for 10.10 and below.

https://codereview.chromium.org/2351183003/diff/1/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/full_size_content_window.mm (right):

https://codereview.chromium.org/2351183003/diff/1/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/full_size_content_window.mm:145: // ensureit is
positioned below the buttons.
On 2016/09/20 23:07:47, erikchen wrote:
> s/ensureit/ensure it/

Done.

https://codereview.chromium.org/2351183003/diff/1/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right):

https://codereview.chromium.org/2351183003/diff/1/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:398: if
(base::mac::IsAtLeastOS10_10()) {
On 2016/09/20 23:07:47, erikchen wrote:
> This CL contains references to both IsAtMost 10.10 and IsAtLeast10.10, for
which
> 10.10 overlaps. It's not clear to me whether you're trying to change 10.11+ or
> 10.10+

I'm finding this isAtLeast/IsAtMost confusing. I believe I have used the correct
logic in this and the other location you mention, but I have changed the logic
here so that it's also IsAtMost10.10().

[erikchen, 2016-09-20 23:32:50]

lgtm % frame paint height

[Eugene But (OOO till 7-30), 2016-09-21 03:01:14]

eugenebut@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2016-09-21 03:01:15]

I tested this CL on 10.11.6. It moves close/minimize/maximize buttons to the top
and breaks RTL :(. I will attach screenshots to the bug.

[shrike, 2016-10-07 20:33:16]

Description was changed from

==========
[Mac] Avoid "adding unknown subview" warning.

Chrome Mac has added a subview above the window's content view, which
the Appkit warns about at runtime. Presumably, doing so may break in a
future macOS release. This cl adds Chrome's top-level subview to the
window's content view, which is what the Appkit wants.

BUG=605219
==========

to

==========
[Mac] Avoid "adding unknown subview" warning.

Chrome Mac has added a subview above the window's content view, which
the Appkit warns about at runtime. Presumably, doing so may break in a
future macOS release. This cl adds Chrome's top-level subview to the
window's content view, which is what the Appkit wants.

BUG=605219
==========