mac: Use a full-size content view (reland 1).

mac: Use a full-size content view (reland 1).

The original attempt to land the CL incorrectly interacted with another
recently landed CL that added the tabStripBackgroundView
(https://codereview.chromium.org/611453004). This CL was causing that view to
become layer backed, which caused it to draw on top of non-layer backed window
content, including the window controls.

This reland reworks the logic so that the tabStripBackgroundView is added to
the root view and does not become layer backed. This reland also updates
browser_tests to catch similar such errors in the future.

--------------Original CL Description---------------
Make the window's contentView have the same size as the window itself. Adjust
the view hierarchy so that Chrome does not add subviews directly to the root
view.

By default, the contentView does not occupy the full size of a framed window.
Chrome still wants to draw in the title bar. Historically, Chrome has done this
by adding subviews directly to the root view. This causes several problems. The
most egregious is related to layer ordering when the root view does not have a
layer. By making the window's contentView full-sized, there is no longer any
need to add subviews to the root view.

I deleted the tests in presentation_mode_controller_unittest.mm since they were
layout tests for the browser_window_controller, but the tests in
browser_window_layout_unittest.mm are both more robust and more comprehensive.

I fixed a bug in moveViewsForImmersiveFullscreen:... where the tabStripView was
being added to the source window rather than the destination window.

This CL is mostly a reland of https://codereview.chromium.org/399803002/.

Original CL Committed: https://crrev.com/746dbb9cfefaac243704956db431ff9bb9b0485b
Original CL Cr-Commit-Position: refs/heads/master@{#298584}

BUG=417923
Committed: https://crrev.com/9fa914507cbcce4fb7f05501106d0a5183d3dff9
Cr-Commit-Position: refs/heads/master@{#299168}

[erikchen, 2014-10-09 21:16:06]

Patchset #2 (id:160001) has been deleted

[erikchen, 2014-10-09 21:24:38]

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

[erikchen, 2014-10-09 21:24:39]

andre: Please review.

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:322: [self
insertTabStripBackgroundViewIntoWindow:destWindow];
I assume this was an oversight in the CL that introduced tabStripBackgroundView.

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_window_controller.mm (right):

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:91: [self
insertTabStripBackgroundViewIntoWindow:window];
It's unclear that tabStripBackgroundView should be added if hasTabStrip is NO.
However, changing the logic might have ramifications, which I'd like to keep out
of this CL.

[Andre, 2014-10-09 23:15:49]

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right):

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:255:
std::vector<NSView*> exceptions;
How about something like this,
NSArray* exceptions = @[ [[view window] contentView], ....
if ([exceptions containsObject:view])
  EXPECT_FALSE([view isOpaque]);
else
  EXPECT_FALSE([view layer]);

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:302: // There
is no zoom button on OSX 10.10+.
I may have misremembered this, but I thought in 10.10 ZoomButton returns the
green fullscreen button while FullscreenButton returns nil.

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:317: BOOL
belowViewBeingVerified_;
Can we pass these as parameters and return values instead of using these
instance variables?
I think it will read better.

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:322: [self
insertTabStripBackgroundViewIntoWindow:destWindow];
On 2014/10/09 21:24:39, erikchen wrote:
> I assume this was an oversight in the CL that introduced
tabStripBackgroundView.
> 

tabStripBackgroundView should not be moved here.
The fullscreen window uses FloatingBarBackgroundView to draw the theme.

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_window_controller.h (right):

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_window_controller.h:175: -
(void)insertTabStripBackgroundViewIntoWindow:(NSWindow*)window;
Make this private, since the external caller is not needed anymore.

[erikchen, 2014-10-10 00:47:34]

Patchset #3 (id:370001) has been deleted

[erikchen, 2014-10-10 00:47:46]

andresantoso: PTAL

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right):

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:255:
std::vector<NSView*> exceptions;
On 2014/10/09 23:15:48, Andre wrote:
> How about something like this,
> NSArray* exceptions = @[ [[view window] contentView], ....
> if ([exceptions containsObject:view])
>   EXPECT_FALSE([view isOpaque]);
> else
>   EXPECT_FALSE([view layer]);

I used your suggestion of an array literal, although I decided to go with an
early return rather than an if/else.

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:302: // There
is no zoom button on OSX 10.10+.
On 2014/10/09 23:15:48, Andre wrote:
> I may have misremembered this, but I thought in 10.10 ZoomButton returns the
> green fullscreen button while FullscreenButton returns nil.

You're right. I was mistaken. Although the new nomenclature is really
confusing... :(

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:317: BOOL
belowViewBeingVerified_;
On 2014/10/09 23:15:48, Andre wrote:
> Can we pass these as parameters and return values instead of using these
> instance variables?
> I think it will read better.

I think that passing the parameters would reduce the readability. Instead, I
performed a significant refactor on all of the names. I also added many
comments.

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:322: [self
insertTabStripBackgroundViewIntoWindow:destWindow];
On 2014/10/09 23:15:48, Andre wrote:
> On 2014/10/09 21:24:39, erikchen wrote:
> > I assume this was an oversight in the CL that introduced
> tabStripBackgroundView.
> > 
> 
> tabStripBackgroundView should not be moved here.
> The fullscreen window uses FloatingBarBackgroundView to draw the theme.

Done.

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_window_controller.h (right):

https://codereview.chromium.org/646703002/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_window_controller.h:175: -
(void)insertTabStripBackgroundViewIntoWindow:(NSWindow*)window;
On 2014/10/09 23:15:48, Andre wrote:
> Make this private, since the external caller is not needed anymore.

Done.

[Andre, 2014-10-10 04:37:14]

https://codereview.chromium.org/646703002/diff/420001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right):

https://codereview.chromium.org/646703002/diff/420001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:292:
CheckViewsDoNotObscure([[[view window] contentView] superview]);
I still think it is not ideal to use instance variables that are only meaningful
when the recursion is in progress.
How about making a checker class and put the states there.
Something like,
  ViewsDoNotObscureChecker checker;  // needs a better class name
  checker.check(view);

Then,
  ViewsDoNotObscureChecker::Check(NSView* view) {
    belowExposedView_ = false;
    exposedView_ = view;
    CheckViewsDoNotObscure([[[view window] contentView] superview]);
  }

[erikchen, 2014-10-10 17:38:08]

andresantoso: PTAL

https://codereview.chromium.org/646703002/diff/420001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right):

https://codereview.chromium.org/646703002/diff/420001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:292:
CheckViewsDoNotObscure([[[view window] contentView] superview]);
On 2014/10/10 04:37:13, Andre wrote:
> I still think it is not ideal to use instance variables that are only
meaningful
> when the recursion is in progress.
> How about making a checker class and put the states there.
> Something like,
>   ViewsDoNotObscureChecker checker;  // needs a better class name
>   checker.check(view);
> 
> Then,
>   ViewsDoNotObscureChecker::Check(NSView* view) {
>     belowExposedView_ = false;
>     exposedView_ = view;
>     CheckViewsDoNotObscure([[[view window] contentView] superview]);
>   }

I moved all of the logic into a new class.

[Andre, 2014-10-10 18:08:50]

lgtm

https://codereview.chromium.org/646703002/diff/520001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right):

https://codereview.chromium.org/646703002/diff/520001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:89: private:
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/646703002/diff/520001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:133: if (view
== exposed_view_) {
I think you may be able to move the code from above (If view is parent of
exposed_view) here, and skip ahead to the next view without recursing if you hit
a superview of exposed_view.
But either way is fine.

https://codereview.chromium.org/646703002/diff/520001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:153: BOOL
below_exposed_view_;
This should be initialized in constructor to avoid random uninitialized value,
even if not used until Check().

[Andre, 2014-10-10 18:08:52]

lgtm

[erikchen, 2014-10-10 18:11:00]

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

[erikchen, 2014-10-10 18:11:00]

avi: Please review.

[erikchen, 2014-10-10 18:11:41]

avi: Oops, failed to see andre's first lgtm. Need 5 min to respond to his
comments.

[erikchen, 2014-10-10 18:19:11]

https://codereview.chromium.org/646703002/diff/520001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right):

https://codereview.chromium.org/646703002/diff/520001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:89: private:
On 2014/10/10 18:08:49, Andre wrote:
> DISALLOW_COPY_AND_ASSIGN

Done. (Moved to end of class, since that's the common location for it.)

https://codereview.chromium.org/646703002/diff/520001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:133: if (view
== exposed_view_) {
On 2014/10/10 18:08:49, Andre wrote:
> I think you may be able to move the code from above (If view is parent of
> exposed_view) here, and skip ahead to the next view without recursing if you
hit
> a superview of exposed_view.
> But either way is fine.

That doesn't sound accurate. The first view passed here is the root view, which
is guaranteed to be an ancestor of exposed_view. If we don't recurse, then we
skip all of the views. Keeping the logic as is.

https://codereview.chromium.org/646703002/diff/520001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:153: BOOL
below_exposed_view_;
On 2014/10/10 18:08:49, Andre wrote:
> This should be initialized in constructor to avoid random uninitialized value,
> even if not used until Check().

Done.

[erikchen, 2014-10-10 18:21:49]

avi: Please review.

[Andre, 2014-10-10 18:22:20]

https://codereview.chromium.org/646703002/diff/520001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right):

https://codereview.chromium.org/646703002/diff/520001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:133: if (view
== exposed_view_) {
On 2014/10/10 18:19:11, erikchen wrote:
> On 2014/10/10 18:08:49, Andre wrote:
> > I think you may be able to move the code from above (If view is parent of
> > exposed_view) here, and skip ahead to the next view without recursing if you
> hit
> > a superview of exposed_view.
> > But either way is fine.
> 
> That doesn't sound accurate. The first view passed here is the root view,
which
> is guaranteed to be an ancestor of exposed_view. If we don't recurse, then we
> skip all of the views. Keeping the logic as is.

Got it.

[Avi (use Gerrit), 2014-10-10 18:30:29]

lgtm

https://codereview.chromium.org/646703002/diff/610001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right):

https://codereview.chromium.org/646703002/diff/610001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:152: // flag is
set to false.
Weird wrapping.

[erikchen, 2014-10-10 18:33:15]

https://codereview.chromium.org/646703002/diff/610001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right):

https://codereview.chromium.org/646703002/diff/610001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:152: // flag is
set to false.
On 2014/10/10 18:30:29, Avi wrote:
> Weird wrapping.

Fixed

[erikchen, 2014-10-10 18:33:47]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-10 18:34:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/646703002/690001

[commit-bot: I haz the power, 2014-10-10 19:56:51]

Committed patchset #6 (id:690001)

[commit-bot: I haz the power, 2014-10-10 19:57:29]

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