mac: Fix tab dragging visual bug in Yosemite.

mac: Fix tab dragging visual bug in Yosemite.

In OSX 10.10+, all views must be added to the NSWindow's contentView. Some
views (like the tab strip and the profile icon) are placed on top of the title
bar and require special treatment. All other views are added as subviews of
'chromeContentView' in TabWindowController.  This allows tab dragging and
fullscreen logic to easily move the views that don't need special treatment.

This CL also removes the instances where a VersionIndependentWindow's
contentView gets replaced by setContentView:. Instead, the 'chromeContentView'
gets passed around as a subview. This allows VersionIndependentWindow to remove
another of its internal hacks.

BUG=392239
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283166

[erikchen, 2014-07-09 23:52:51]

andresantoso: Please review.

[erikchen, 2014-07-10 23:30:52]

andresantoso: I've rewritten this CL. Please review.

[Andre, 2014-07-11 00:57:11]

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

https://codereview.chromium.org/379293003/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:981: if
(floatingBarBackingView_) {
I think this floatingBarBackingView_ is placed in the title bar area, so it may
have
to be a subview of window.contentView instead of chromeContentView.

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

https://codereview.chromium.org/379293003/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:67: NSMakeRect(0, 0, 750,
600)]);
initWithFrame:[chromeContentView_ bounds]
to avoid repeating magic numbers.

https://codereview.chromium.org/379293003/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:70:
[self.chromeContentView addSubview:tabContentArea_];
nit: self.chromeContentView -> chromeContentView_ to be consistent with
surrounding code.

[erikchen, 2014-07-11 01:17:43]

andresantoso: PTAL

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

https://codereview.chromium.org/379293003/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:981: if
(floatingBarBackingView_) {
On 2014/07/11 00:57:11, Andre wrote:
> I think this floatingBarBackingView_ is placed in the title bar area, so it
may
> have
> to be a subview of window.contentView instead of chromeContentView.

The floatingBarBackingView gets used in presentation mode to handle the dropdown
of the omnibox/bookmarksbar/tabstrip when the mouse is moved to the top of the
screen. Since its in fullscreen mode, the contentView takes up the entirety of
the screen anyways.

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

https://codereview.chromium.org/379293003/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:67: NSMakeRect(0, 0, 750,
600)]);
On 2014/07/11 00:57:11, Andre wrote:
> initWithFrame:[chromeContentView_ bounds]
> to avoid repeating magic numbers.

Done.

https://codereview.chromium.org/379293003/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:70:
[self.chromeContentView addSubview:tabContentArea_];
On 2014/07/11 00:57:11, Andre wrote:
> nit: self.chromeContentView -> chromeContentView_ to be consistent with
> surrounding code.

Done.

[Andre, 2014-07-11 01:34:00]

lgtm

[erikchen, 2014-07-11 01:37:05]

shess: Looking for an OWNER review.

[Scott Hess - ex-Googler, 2014-07-11 18:08:34]

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

https://codereview.chromium.org/379293003/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:70:
[self.chromeContentView addSubview:tabContentArea_];
On 2014/07/11 01:17:43, erikchen1 wrote:
> On 2014/07/11 00:57:11, Andre wrote:
> > nit: self.chromeContentView -> chromeContentView_ to be consistent with
> > surrounding code.
> 
> Done.

There are a couple other places in the patch which are using
self.chromeContentView and should be converted to chromeContentView_.

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

https://codereview.chromium.org/379293003/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:570:
base::scoped_nsobject<NSView> contentView([self.chromeContentView retain]);
Previously, |contentView| was essential because -removeFromSuperview would
remove the last reference.  Now chromeContentView_ should still contain a ref (I
think), so could the below just s/contentView/chromeContentView_/ ?

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

https://codereview.chromium.org/379293003/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:61: [[NSView alloc]
initWithFrame:NSMakeRect(0, 0, 750, 600)]);
Suggest NSWidth(contentRect) and NSHeight(contentRect) rather than duplicating
magic numbers, as long as you're here.

https://codereview.chromium.org/379293003/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:73: NSMakeRect(0, 0, 750,
37)]);
Might as well NSWidth() here, too.  I'm sure 37 is a principled choice someone
made at some point :-).

[erikchen, 2014-07-11 20:48:05]

shess: PTAL

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

https://codereview.chromium.org/379293003/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:70:
[self.chromeContentView addSubview:tabContentArea_];
On 2014/07/11 18:08:34, Scott Hess wrote:
> On 2014/07/11 01:17:43, erikchen1 wrote:
> > On 2014/07/11 00:57:11, Andre wrote:
> > > nit: self.chromeContentView -> chromeContentView_ to be consistent with
> > > surrounding code.
> > 
> > Done.
> 
> There are a couple other places in the patch which are using
> self.chromeContentView and should be converted to chromeContentView_.

That's not possible for most of the classes, since chromeContentView_ is a
private property. I also prefer to use self.'property' notation for properties,
since that's the entire point of having the property abstraction. I changed it
here only because there's a light taboo against using 'self' in the designated
initializer.

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

https://codereview.chromium.org/379293003/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:570:
base::scoped_nsobject<NSView> contentView([self.chromeContentView retain]);
On 2014/07/11 18:08:34, Scott Hess wrote:
> Previously, |contentView| was essential because -removeFromSuperview would
> remove the last reference.  Now chromeContentView_ should still contain a ref
(I
> think), so could the below just s/contentView/chromeContentView_/ ?

I removed the local variable.

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

https://codereview.chromium.org/379293003/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:61: [[NSView alloc]
initWithFrame:NSMakeRect(0, 0, 750, 600)]);
On 2014/07/11 18:08:34, Scott Hess wrote:
> Suggest NSWidth(contentRect) and NSHeight(contentRect) rather than duplicating
> magic numbers, as long as you're here.

I pulled out the numbers into a local variable.

https://codereview.chromium.org/379293003/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:73: NSMakeRect(0, 0, 750,
37)]);
On 2014/07/11 18:08:34, Scott Hess wrote:
> Might as well NSWidth() here, too.  I'm sure 37 is a principled choice someone
> made at some point :-).

I pulled out the numbers into a local variable.

[Scott Hess - ex-Googler, 2014-07-11 21:10:52]

LGTM, except that the constants need to be constant and k-prefixed.  Because
pedantic.

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

https://codereview.chromium.org/379293003/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:70:
[self.chromeContentView addSubview:tabContentArea_];
On 2014/07/11 20:48:04, erikchen1 wrote:
> On 2014/07/11 18:08:34, Scott Hess wrote:
> > On 2014/07/11 01:17:43, erikchen1 wrote:
> > > On 2014/07/11 00:57:11, Andre wrote:
> > > > nit: self.chromeContentView -> chromeContentView_ to be consistent with
> > > > surrounding code.
> > > 
> > > Done.
> > 
> > There are a couple other places in the patch which are using
> > self.chromeContentView and should be converted to chromeContentView_.
> 
> That's not possible for most of the classes, since chromeContentView_ is a
> private property. I also prefer to use self.'property' notation for
properties,
> since that's the entire point of having the property abstraction. I changed it
> here only because there's a light taboo against using 'self' in the designated
> initializer.

There are so few cases of self.accessor in the codebase that I'm inclined to
think they are mistakes rather than intentional style allowances - but at this
point I think it doesn't matter.  As far as your preferences come into play ...
it's nice to have preferences, but given that we have a billion Chromium
developers, please use style A if the code you're working in uses style A,
unless there's been a style-overlord ruling that things should be converted to
style B.

The style guide says "Whenever practical" WRT using accessors in the designated
initializer or -dealloc, for good reason.  I wouldn't say it's a light taboo.

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

https://codereview.chromium.org/379293003/diff/110001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:51: CGFloat defaultHeight
= 600;
In this case - const, and kDefaultWidth/kDefaultHeight.

[erikchen, 2014-07-11 21:18:19]

shess: PTAL

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

https://codereview.chromium.org/379293003/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:70:
[self.chromeContentView addSubview:tabContentArea_];
On 2014/07/11 21:10:52, Scott Hess wrote:
> On 2014/07/11 20:48:04, erikchen1 wrote:
> > On 2014/07/11 18:08:34, Scott Hess wrote:
> > > On 2014/07/11 01:17:43, erikchen1 wrote:
> > > > On 2014/07/11 00:57:11, Andre wrote:
> > > > > nit: self.chromeContentView -> chromeContentView_ to be consistent
with
> > > > > surrounding code.
> > > > 
> > > > Done.
> > > 
> > > There are a couple other places in the patch which are using
> > > self.chromeContentView and should be converted to chromeContentView_.
> > 
> > That's not possible for most of the classes, since chromeContentView_ is a
> > private property. I also prefer to use self.'property' notation for
> properties,
> > since that's the entire point of having the property abstraction. I changed
it
> > here only because there's a light taboo against using 'self' in the
designated
> > initializer.
> 
> There are so few cases of self.accessor in the codebase that I'm inclined to
> think they are mistakes rather than intentional style allowances - but at this
> point I think it doesn't matter.  As far as your preferences come into play
...
> it's nice to have preferences, but given that we have a billion Chromium
> developers, please use style A if the code you're working in uses style A,
> unless there's been a style-overlord ruling that things should be converted to
> style B.
> 
> The style guide says "Whenever practical" WRT using accessors in the
designated
> initializer or -dealloc, for good reason.  I wouldn't say it's a light taboo.

Fair points. Got it.

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

https://codereview.chromium.org/379293003/diff/110001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_window_controller.mm:51: CGFloat defaultHeight
= 600;
On 2014/07/11 21:10:52, Scott Hess wrote:
> In this case - const, and kDefaultWidth/kDefaultHeight.

Done.

[Scott Hess - ex-Googler, 2014-07-11 21:19:54]

lgtm

[erikchen, 2014-07-11 21:30:11]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-07-11 21:30:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikchen@chromium.org/379293003/130001

[commit-bot: I haz the power, 2014-07-12 02:38:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-12 04:22:50]

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

[commit-bot: I haz the power, 2014-07-12 04:22:50]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[erikchen, 2014-07-14 18:39:40]

shess: PTAL

I added 2 more lines. See the diff between patch set 7 and patch set 6. I was
playing around with the build, and I realized that each time chromeContentView
gets added as a subview, its frame needs to be reset.

[Scott Hess - ex-Googler, 2014-07-14 19:11:49]

On 2014/07/14 18:39:40, erikchen1 wrote:
> shess: PTAL
> 
> I added 2 more lines. See the diff between patch set 7 and patch set 6. I was
> playing around with the build, and I realized that each time chromeContentView
> gets added as a subview, its frame needs to be reset.

LGTM.

Unfortunately, I have no idea whether it's really right or wrong to do this. 
Might be worth considering whether there's a long-term solution to be had
involving a custom NSView subclass to encapsulate relationships like this in one
place.

[erikchen, 2014-07-14 19:35:01]

On 2014/07/14 19:11:49, Scott Hess wrote:
> On 2014/07/14 18:39:40, erikchen1 wrote:
> > shess: PTAL
> > 
> > I added 2 more lines. See the diff between patch set 7 and patch set 6. I
was
> > playing around with the build, and I realized that each time
chromeContentView
> > gets added as a subview, its frame needs to be reset.
> 
> LGTM.
> 
> Unfortunately, I have no idea whether it's really right or wrong to do this. 
> Might be worth considering whether there's a long-term solution to be had
> involving a custom NSView subclass to encapsulate relationships like this in
one
> place.

[erikchen, 2014-07-14 19:36:37]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-07-14 19:37:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikchen@chromium.org/379293003/170001

[commit-bot: I haz the power, 2014-07-15 08:46:17]

Change committed as 283166

[Michael Achenbach, 2014-07-15 09:25:26]

A revert of this CL has been created in
https://codereview.chromium.org/390313002/ by machenbach@chromium.org.

The reason for reverting is: This possibly broke interactive_ui_tests on mac:
http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac10.6%20Tests%....

[danakj, 2014-07-16 21:12:50]

A revert of this CL has been created in
https://codereview.chromium.org/394333002/ by danakj@chromium.org.

The reason for reverting is: Causing failures on the main waterfall.

http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac%2010.6%20Tes...

BrowserWindowFullScreenControllerTest.TestActivate and
BrowserWindowFullScreenControllerTest.TestFullscreen both crash..