mac: Toolbar renders incorrectly when downloads bar is open.

mac: Toolbar renders incorrectly when downloads bar is open.

The toolbar's superview chromeContentView was incorrectly sized and was too
small. This happened because chromeContentView's layout was not being updated
in -[BrowserWindowController layoutSubviews].

I updated the unit tests in browser_window_controller_unittest.mm with 2 changes:
- All layout checking should use window coordinates, to ensure consistency.
- Layout checking should ensure that the frame of each view is within the
bounds of the superview.

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

[erikchen, 2014-07-25 20:59:42]

shess: Please review.

[Scott Hess - ex-Googler, 2014-07-25 22:26:53]

https://codereview.chromium.org/419683008/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_unittest.mm (right):

https://codereview.chromium.org/419683008/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:261: BOOL
ViewHierarchyContainmentValid(NSView* view) {
What, not implementing this as a private category?  Sigh :-).

https://codereview.chromium.org/419683008/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:298:
EXPECT_EQ(NSMinY(contentView), NSMinY(download));
Nobody ever checks:
  EXPECT_EQ(NSMaxY(contentView), something);
I see the comment down there - should it at least be _LE or _GE, or even within
epsilon?

https://codereview.chromium.org/419683008/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:301:
EXPECT_EQ(NSMaxY(toolbar), NSMinY(tabstrip));
Isn't this identical to the last line of the test?

Though that last line should follow the pattern of the comparisons.

[erikchen, 2014-07-25 23:22:07]

shess: PTAL

https://codereview.chromium.org/419683008/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_unittest.mm (right):

https://codereview.chromium.org/419683008/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:298:
EXPECT_EQ(NSMinY(contentView), NSMinY(download));
On 2014/07/25 22:26:53, Scott Hess wrote:
> Nobody ever checks:
>   EXPECT_EQ(NSMaxY(contentView), something);
> I see the comment down there - should it at least be _LE or _GE, or even
within
> epsilon? 

I added a comment. The height of the contentView varies across OS version, so we
can't check NSMaxY. ViewHierarchyContainmentValid performs the equivalent of
LE/GE bounds checking.

https://codereview.chromium.org/419683008/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:301:
EXPECT_EQ(NSMaxY(toolbar), NSMinY(tabstrip));
On 2014/07/25 22:26:53, Scott Hess wrote:
> Isn't this identical to the last line of the test?
> 
> Though that last line should follow the pattern of the comparisons.

yeah. I removed the last line, and moved the comment up.

[Scott Hess - ex-Googler, 2014-07-25 23:59:05]

LGTM, but I think the include doesn't belong.

https://codereview.chromium.org/419683008/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_unittest.mm (right):

https://codereview.chromium.org/419683008/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:301:
EXPECT_EQ(NSMaxY(toolbar), NSMinY(tabstrip));
On 2014/07/25 23:22:06, erikchen wrote:
> On 2014/07/25 22:26:53, Scott Hess wrote:
> > Isn't this identical to the last line of the test?
> > 
> > Though that last line should follow the pattern of the comparisons.
> 
> yeah. I removed the last line, and moved the comment up.

Hmm, I was more wondering why this was added here, when there was a version that
was in the order of the progression from bottom to top.  But ... *shrug*.

https://codereview.chromium.org/419683008/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_unittest.mm (right):

https://codereview.chromium.org/419683008/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:23: #import
"chrome/browser/ui/cocoa/fast_resize_view.h"
This seems unlikely, now that I look at it.

https://codereview.chromium.org/419683008/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:300: // against
NSMaxY of contentView.
I don't think this comment helps, since this code isn't checking the window
frame at all.  I think the comment about the tabstrip check will have to
represent the concept.

[erikchen, 2014-07-26 00:12:16]

https://codereview.chromium.org/419683008/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_unittest.mm (right):

https://codereview.chromium.org/419683008/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:301:
EXPECT_EQ(NSMaxY(toolbar), NSMinY(tabstrip));
On 2014/07/25 23:59:04, Scott Hess wrote:
> On 2014/07/25 23:22:06, erikchen wrote:
> > On 2014/07/25 22:26:53, Scott Hess wrote:
> > > Isn't this identical to the last line of the test?
> > > 
> > > Though that last line should follow the pattern of the comparisons.
> > 
> > yeah. I removed the last line, and moved the comment up.
> 
> Hmm, I was more wondering why this was added here, when there was a version
that
> was in the order of the progression from bottom to top.  But ... *shrug*.

Moved it back down.

https://codereview.chromium.org/419683008/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_unittest.mm (right):

https://codereview.chromium.org/419683008/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:23: #import
"chrome/browser/ui/cocoa/fast_resize_view.h"
On 2014/07/25 23:59:05, Scott Hess wrote:
> This seems unlikely, now that I look at it.

Won't compile without it. FastResizeView is forward declared, and can't be
casted to NSView.

https://codereview.chromium.org/419683008/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:300: // against
NSMaxY of contentView.
On 2014/07/25 23:59:05, Scott Hess wrote:
> I don't think this comment helps, since this code isn't checking the window
> frame at all.  I think the comment about the tabstrip check will have to
> represent the concept.

Removed the comment.

[erikchen, 2014-07-26 00:12:19]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-07-26 00:12:55]

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

[Scott Hess - ex-Googler, 2014-07-26 00:21:27]

https://codereview.chromium.org/419683008/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_unittest.mm (right):

https://codereview.chromium.org/419683008/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:23: #import
"chrome/browser/ui/cocoa/fast_resize_view.h"
On 2014/07/26 00:12:16, erikchen wrote:
> On 2014/07/25 23:59:05, Scott Hess wrote:
> > This seems unlikely, now that I look at it.
> 
> Won't compile without it. FastResizeView is forward declared, and can't be
> casted to NSView.

Weird.  So I'm guessing what you mean is that [controller tabContentArea]'s
result can't be passed to FrameInWindowForView() because it doesn't know that
it's a subclass of NSView, but it is able to figure out to call
objc_msgSend_stret because of the NSRect assignment?  Weirdo.  I wonder if
there's a warning we could be turning on to flush out that kind of ambiguity.

[erikchen, 2014-07-26 00:49:08]

I played around with it. The compiler is treating FastResizeView like an
object with type id.


On Fri, Jul 25, 2014 at 5:21 PM, <shess@chromium.org> wrote:

>
> https://codereview.chromium.org/419683008/diff/60001/
> chrome/browser/ui/cocoa/browser_window_controller_unittest.mm
> File chrome/browser/ui/cocoa/browser_window_controller_unittest.mm
> (right):
>
> https://codereview.chromium.org/419683008/diff/60001/
> chrome/browser/ui/cocoa/browser_window_controller_unittest.mm#newcode23
> chrome/browser/ui/cocoa/browser_window_controller_unittest.mm:23:
> #import "chrome/browser/ui/cocoa/fast_resize_view.h"
> On 2014/07/26 00:12:16, erikchen wrote:
>
>> On 2014/07/25 23:59:05, Scott Hess wrote:
>> > This seems unlikely, now that I look at it.
>>
>
>  Won't compile without it. FastResizeView is forward declared, and
>>
> can't be
>
>> casted to NSView.
>>
>
> Weird.  So I'm guessing what you mean is that [controller
> tabContentArea]'s result can't be passed to FrameInWindowForView()
> because it doesn't know that it's a subclass of NSView, but it is able
> to figure out to call objc_msgSend_stret because of the NSRect
> assignment?  Weirdo.  I wonder if there's a warning we could be turning
> on to flush out that kind of ambiguity.
>
> https://codereview.chromium.org/419683008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-07-26 19:57:30]

Change committed as 285796

[erikchen, 2014-07-29 01:22:35]

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

The reason for reverting is:  NSInvalidArgumentException reason Unlocking Focus
on wrong view (<NSView: 0x#>), expected <FastResizeView: 0x#>

https://code.google.com/p/chromium/issues/detail?id=397089.