[MacViews] Implement colored window frames.

[MacViews] Implement colored window frames.

This works the same way as NativeAppWindowCocoa by inserting a colored
NSView into the window's contentView's superview.

This also adds a test that checks for correct frame colors when active and
inactive.

BUG=459877
Committed: https://crrev.com/2ebfcc03fc85c4578008cfed368097950f9d6348
Cr-Commit-Position: refs/heads/master@{#329999}

[jackhou1, 2015-04-24 07:05:02]

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

[jackhou1, 2015-04-24 07:05:03]

tapted, what do you think?

https://codereview.chromium.org/1053303003/diff/1/ui/gfx/mac/nswindow_frame_c...
File ui/gfx/mac/nswindow_frame_controls.h (right):

https://codereview.chromium.org/1053303003/diff/1/ui/gfx/mac/nswindow_frame_c...
ui/gfx/mac/nswindow_frame_controls.h:40: GFX_EXPORT void
AddColoredTitlebarToNSWindow(
Should we merge TitlebarBackgroundView and TabStripBackgroundView?

[tapted, 2015-05-06 02:58:29]

Don't forget a brief summary in the description too

https://codereview.chromium.org/1053303003/diff/1/ui/gfx/mac/nswindow_frame_c...
File ui/gfx/mac/nswindow_frame_controls.h (right):

https://codereview.chromium.org/1053303003/diff/1/ui/gfx/mac/nswindow_frame_c...
ui/gfx/mac/nswindow_frame_controls.h:40: GFX_EXPORT void
AddColoredTitlebarToNSWindow(
On 2015/04/24 07:05:03, jackhou1 (OOO until May 11) wrote:
> Should we merge TitlebarBackgroundView and TabStripBackgroundView?

I'd lean towards "nah" - probably not enough in common - just an NSBezierPath.

The common bit might be
`gfx::ClipCurrentDrawingContextToRoundedWindow(NSWindow*)`, but there's too many
temporaries needed later

https://codereview.chromium.org/1053303003/diff/1/ui/gfx/mac/nswindow_frame_c...
File ui/gfx/mac/nswindow_frame_controls.mm (right):

https://codereview.chromium.org/1053303003/diff/1/ui/gfx/mac/nswindow_frame_c...
ui/gfx/mac/nswindow_frame_controls.mm:90: void
AddColoredTitlebarToNSWindow(NSWindow* window,
Can this go in chrome/browser/ui/apps ? I don't think anything but app windows
will ever want it.

[tapted, 2015-05-06 03:15:42]

some comments on the test

(The CL description should say you're adding a test too, since that's a cool
thing to do :)

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm
(right):

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:5: //
This file tests whichever implementation of NativeAppWindow is used.
btw - have you been able to try out the views version? I think I struggled to
get browser_tests/interactive_ui_tests to link with mac_views_browser=1. So it
would be good to verify that.

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:11:
#include "base/mac/mac_util.h"
is this used?

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:12:
#include "base/mac/scoped_nsobject.h"
nit: import

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:13:
#include "base/mac/sdk_forward_declarations.h"
import

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:15:
#include "content/public/test/test_utils.h"
is this used?

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:21:
class NativeAppWindowCocoaInteractiveTest
If the harness is empty, you can instead do

`using NativeAppWindowCocoaInteractiveTest = extensions::PlatformAppBrowserTest;

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:32:
IN_PROC_BROWSER_TEST_F(NativeAppWindowCocoaInteractiveTest, FrameColor) {
needs a comment saying what the test is for

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:44:
NSColor* blueColor = gfx::SkColorToSRGBNSColor(SK_ColorBLUE);
blueColor -> blue_color

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:46:
NSColor* color = [bitmap colorAtX:(bounds.size.width / 2)y:5];
bounds.size.width/2 -> NSMidX(bounds)

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:46:
NSColor* color = [bitmap colorAtX:(bounds.size.width / 2)y:5];
space before `y:`, also below

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:49:
EXPECT_EQ([blueColor blueComponent], [color blueComponent]);
I don't find these checks as convincing as just removing `blueColor/redColor`
completely and checking against a constant 0xff / 0x0. Especially since that
will then match the manifest in the string constant at the start of the test.

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:51:
[ns_window makeKeyAndOrderFront:nil];
I think this updates isMain/Key asynchronously, so you'll need something like
`ScopedFakeWindowMainStatus` from app_shim_menu_controller_mac_browsertest or
makePretendKeyWindowAndSetFirstResponder from ui/gfx/test/ui_cocoa_test_helper.h
or CocoaActivationWaiter from
chrome/browser/ui/cocoa/panels/panel_cocoa_browsertest.mm or
WidgetActivationWaiter from widget_interactive_uitest.cc or
ShowAndFocusNativeWindow from interactive_test_utils_mac.mm

(some of those will work in a browser_test -- it would be nice if this wasn't
another interactive_ui_test :)

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:53:
NSColor* redColor = gfx::SkColorToSRGBNSColor(SK_ColorRED);
red_color

[jackhou1, 2015-05-12 05:39:05]

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm
(right):

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:5: //
This file tests whichever implementation of NativeAppWindow is used.
On 2015/05/06 03:15:42, tapted wrote:
> btw - have you been able to try out the views version? I think I struggled to
> get browser_tests/interactive_ui_tests to link with mac_views_browser=1. So it
> would be good to verify that.

Yeah I had to comment out a bunch of test files in chrome_tests.gypi to get it
to link. But this test works.

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:11:
#include "base/mac/mac_util.h"
On 2015/05/06 03:15:42, tapted wrote:
> is this used?

Moved back to browser_test.

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:12:
#include "base/mac/scoped_nsobject.h"
On 2015/05/06 03:15:42, tapted wrote:
> nit: import

Done.

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:13:
#include "base/mac/sdk_forward_declarations.h"
On 2015/05/06 03:15:42, tapted wrote:
> import

Done.

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:15:
#include "content/public/test/test_utils.h"
On 2015/05/06 03:15:42, tapted wrote:
> is this used?

Moved back to browser_test.

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:21:
class NativeAppWindowCocoaInteractiveTest
On 2015/05/06 03:15:41, tapted wrote:
> If the harness is empty, you can instead do
> 
> `using NativeAppWindowCocoaInteractiveTest =
extensions::PlatformAppBrowserTest;

Moved back to browser_test.

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:32:
IN_PROC_BROWSER_TEST_F(NativeAppWindowCocoaInteractiveTest, FrameColor) {
On 2015/05/06 03:15:42, tapted wrote:
> needs a comment saying what the test is for

Done.

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:44:
NSColor* blueColor = gfx::SkColorToSRGBNSColor(SK_ColorBLUE);
On 2015/05/06 03:15:42, tapted wrote:
> blueColor -> blue_color

Done.

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:46:
NSColor* color = [bitmap colorAtX:(bounds.size.width / 2)y:5];
On 2015/05/06 03:15:42, tapted wrote:
> space before `y:`, also below

Done.

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:46:
NSColor* color = [bitmap colorAtX:(bounds.size.width / 2)y:5];
On 2015/05/06 03:15:42, tapted wrote:
> bounds.size.width/2 -> NSMidX(bounds)

Done.

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:49:
EXPECT_EQ([blueColor blueComponent], [color blueComponent]);
On 2015/05/06 03:15:42, tapted wrote:
> I don't find these checks as convincing as just removing `blueColor/redColor`
> completely and checking against a constant 0xff / 0x0. Especially since that
> will then match the manifest in the string constant at the start of the test.

Done.

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:51:
[ns_window makeKeyAndOrderFront:nil];
On 2015/05/06 03:15:42, tapted wrote:
> I think this updates isMain/Key asynchronously, so you'll need something like
> `ScopedFakeWindowMainStatus` from app_shim_menu_controller_mac_browsertest or
> makePretendKeyWindowAndSetFirstResponder from
ui/gfx/test/ui_cocoa_test_helper.h
> or CocoaActivationWaiter from
> chrome/browser/ui/cocoa/panels/panel_cocoa_browsertest.mm or
> WidgetActivationWaiter from widget_interactive_uitest.cc or
> ShowAndFocusNativeWindow from interactive_test_utils_mac.mm
> 
> (some of those will work in a browser_test -- it would be nice if this wasn't
> another interactive_ui_test :)

ScopedFakeWindowMainStatus works. Moved to its own file.

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:53:
NSColor* redColor = gfx::SkColorToSRGBNSColor(SK_ColorRED);
On 2015/05/06 03:15:42, tapted wrote:
> red_color

Done.

https://codereview.chromium.org/1053303003/diff/1/ui/gfx/mac/nswindow_frame_c...
File ui/gfx/mac/nswindow_frame_controls.mm (right):

https://codereview.chromium.org/1053303003/diff/1/ui/gfx/mac/nswindow_frame_c...
ui/gfx/mac/nswindow_frame_controls.mm:90: void
AddColoredTitlebarToNSWindow(NSWindow* window,
On 2015/05/06 02:58:29, tapted wrote:
> Can this go in chrome/browser/ui/apps ? I don't think anything but app windows
> will ever want it.

Moved to titlebar_background_view.h|mm

[tapted, 2015-05-12 06:15:44]

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm
(right):

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:49:
EXPECT_EQ([blueColor blueComponent], [color blueComponent]);
On 2015/05/12 05:39:05, jackhou1 wrote:
> On 2015/05/06 03:15:42, tapted wrote:
> > I don't find these checks as convincing as just removing
`blueColor/redColor`
> > completely and checking against a constant 0xff / 0x0. Especially since that
> > will then match the manifest in the string constant at the start of the
test.
> 
> Done.

You missed this i think

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (left):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:791:
[titlebar_background_view_ setNeedsDisplay:YES];
why isn't this needed any more (also in *Resign)

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:14: #import
"base/mac/scoped_objc_class_swizzler.h"
is this used?

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/scoped_fake_nswindow_main_status.h (right):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/scoped_fake_nswindow_main_status.h:5: #ifndef
CHROME_BROWSER_UI_COCOA_APPS_SCOPED_FAKE_NSWINDOW_MAIN_STATUS_H_
This needs to be in a /test folder, or at the very least have "_test_[helper]"
in its name.

Probably chrome/browser/ui/test [or ui/gfx/test]

There are also test_helper.* files in chrome/browser/ui/cocoa (IMO they should
be moved into a test folder, but they also set a precedent for _test_helper

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/scoped_fake_nswindow_main_status.h:23: static
NSWindow* window_;
can this be moved to an anonymous namespace like g_fake_main_window_;? I think
you can get rid of GetMainWindow() then as well.

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/titlebar_background_view.h (right):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/titlebar_background_view.h:10: #include
"base/mac/scoped_nsobject.h"
nit:import

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/titlebar_background_view.mm (right):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/titlebar_background_view.mm:7: #import
<Cocoa/Cocoa.h>
nit: don't need this since it's in the header.

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.h (right):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.h:28:
extensions::NativeAppWindow* native_app_window_;
ownership comment?

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.mm (right):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:13: #import
"ui/gfx/mac/nswindow_frame_controls.h"
unused?

[jackhou1, 2015-05-14 02:59:14]

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm
(right):

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:49:
EXPECT_EQ([blueColor blueComponent], [color blueComponent]);
On 2015/05/12 06:15:43, tapted wrote:
> On 2015/05/12 05:39:05, jackhou1 wrote:
> > On 2015/05/06 03:15:42, tapted wrote:
> > > I don't find these checks as convincing as just removing
> `blueColor/redColor`
> > > completely and checking against a constant 0xff / 0x0. Especially since
that
> > > will then match the manifest in the string constant at the start of the
> test.
> > 
> > Done.
> 
> You missed this i think

NSColor components are CGFloats, so it's 1 instead of 255.

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (left):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:791:
[titlebar_background_view_ setNeedsDisplay:YES];
On 2015/05/12 06:15:43, tapted wrote:
> why isn't this needed any more (also in *Resign)

I think it was redundant. The -[NSView drawRect:] gets called when the window
changes focus.

2   Chromium Framework                  0x00000001121660b3
-[TitlebarBackgroundView drawRect:] + 179
3   AppKit                              0x00007fff87728b19 -[NSView
_drawRect:clip:] + 4238
4   AppKit                              0x00007fff8772717a -[NSView
_recursiveDisplayAllDirtyWithLockFocus:visRect:] + 1875
5   AppKit                              0x00007fff8772757e -[NSView
_recursiveDisplayAllDirtyWithLockFocus:visRect:] + 2903
6   AppKit                              0x00007fff87725016 -[NSView
_recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
+ 913
7   AppKit                              0x00007fff87724773 -[NSThemeFrame
_recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
+ 333
8   AppKit                              0x00007fff877212cb -[NSView
_displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:] + 2761
9   AppKit                              0x00007fff876ffe2d -[NSView
displayIfNeeded] + 1876
10  AppKit                              0x00007fff8771d1c5 -[NSWindow
displayIfNeeded] + 232

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:14: #import
"base/mac/scoped_objc_class_swizzler.h"
On 2015/05/12 06:15:43, tapted wrote:
> is this used?

Nope, removed.

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/scoped_fake_nswindow_main_status.h (right):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/scoped_fake_nswindow_main_status.h:5: #ifndef
CHROME_BROWSER_UI_COCOA_APPS_SCOPED_FAKE_NSWINDOW_MAIN_STATUS_H_
On 2015/05/12 06:15:43, tapted wrote:
> This needs to be in a /test folder, or at the very least have "_test_[helper]"
> in its name.
> 
> Probably chrome/browser/ui/test [or ui/gfx/test]
> 
> There are also test_helper.* files in chrome/browser/ui/cocoa (IMO they should
> be moved into a test folder, but they also set a precedent for _test_helper

Done. Moved to chrome/browser/ui/test.

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/scoped_fake_nswindow_main_status.h:23: static
NSWindow* window_;
On 2015/05/12 06:15:43, tapted wrote:
> can this be moved to an anonymous namespace like g_fake_main_window_;? I think
> you can get rid of GetMainWindow() then as well.

Done.

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/titlebar_background_view.h (right):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/titlebar_background_view.h:10: #include
"base/mac/scoped_nsobject.h"
On 2015/05/12 06:15:43, tapted wrote:
> nit:import

Done.

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/titlebar_background_view.mm (right):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/titlebar_background_view.mm:7: #import
<Cocoa/Cocoa.h>
On 2015/05/12 06:15:43, tapted wrote:
> nit: don't need this since it's in the header.

Done.

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.h (right):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.h:28:
extensions::NativeAppWindow* native_app_window_;
On 2015/05/12 06:15:43, tapted wrote:
> ownership comment? 

Done.

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.mm (right):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:13: #import
"ui/gfx/mac/nswindow_frame_controls.h"
On 2015/05/12 06:15:43, tapted wrote:
> unused?

Done.

[tapted, 2015-05-14 04:23:51]

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm
(right):

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:49:
EXPECT_EQ([blueColor blueComponent], [color blueComponent]);
On 2015/05/14 02:59:13, jackhou1 wrote:
> On 2015/05/12 06:15:43, tapted wrote:
> > On 2015/05/12 05:39:05, jackhou1 wrote:
> > > On 2015/05/06 03:15:42, tapted wrote:
> > > > I don't find these checks as convincing as just removing
> > `blueColor/redColor`
> > > > completely and checking against a constant 0xff / 0x0. Especially since
> that
> > > > will then match the manifest in the string constant at the start of the
> > test.
> > > 
> > > Done.
> > 
> > You missed this i think
> 
> NSColor components are CGFloats, so it's 1 instead of 255.

Ah, can you comment - it should say the values correspond to the hexvalues in
the string constant. (maybe you could use rgb(100%, 0%, 0%) to make them seem
realted, but that's probably more confusing.)

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (left):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:791:
[titlebar_background_view_ setNeedsDisplay:YES];
On 2015/05/14 02:59:13, jackhou1 wrote:
> On 2015/05/12 06:15:43, tapted wrote:
> > why isn't this needed any more (also in *Resign)
> 
> I think it was redundant. The -[NSView drawRect:] gets called when the window
> changes focus.
> 
> 2   Chromium Framework                  0x00000001121660b3
> -[TitlebarBackgroundView drawRect:] + 179
> 3   AppKit                              0x00007fff87728b19 -[NSView
> _drawRect:clip:] + 4238
> 4   AppKit                              0x00007fff8772717a -[NSView
> _recursiveDisplayAllDirtyWithLockFocus:visRect:] + 1875
> 5   AppKit                              0x00007fff8772757e -[NSView
> _recursiveDisplayAllDirtyWithLockFocus:visRect:] + 2903
> 6   AppKit                              0x00007fff87725016 -[NSView
>
_recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
> + 913
> 7   AppKit                              0x00007fff87724773 -[NSThemeFrame
>
_recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
> + 333
> 8   AppKit                              0x00007fff877212cb -[NSView
> _displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:] + 2761
> 9   AppKit                              0x00007fff876ffe2d -[NSView
> displayIfNeeded] + 1876

So is something in NSWindow already setting setNeedsDisplay:YES for the code to
proceed at this point? (it's fine if it is - just want to make sure there's not
something more subtle... i.e. were those lines originally added for a reason?

> 10  AppKit                              0x00007fff8771d1c5 -[NSWindow
> displayIfNeeded] + 232

https://codereview.chromium.org/1053303003/diff/90001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/titlebar_background_view.h (right):

https://codereview.chromium.org/1053303003/diff/90001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/titlebar_background_view.h:19: // Adds a
TitlebarBackgroundView to the [[window contentView] superView].
nit: empty line before

https://codereview.chromium.org/1053303003/diff/90001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.h (right):

https://codereview.chromium.org/1053303003/diff/90001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.h:28: // Weak.
NativeAppWindowViews owns a Widget.
The Widget in NativeAppWindowViews is also weak.

I think technically the Widget owner is either "itself" or the WebContents in
AppWindowContentsImpl, since extensions::AppWindow is its WebContentsDelegate,
and it guarantees to call native_app_window_->Close() along with the
WebContents.

So I'd say "Weak. Owned by extensions::AppWindow (which manages our Widget via
its WebContents)."

https://codereview.chromium.org/1053303003/diff/90001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc
(right):

https://codereview.chromium.org/1053303003/diff/90001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc:179:
apps::AppWindowFrameView* frame = static_cast<apps::AppWindowFrameView*>(
This isn't so nice.. What about making
ChromeNativeAppWindowViews::CreateNonStandardAppFrame() a pure virtual function,
and moving its definition in here?

[jackhou1, 2015-05-14 06:20:41]

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm
(right):

https://codereview.chromium.org/1053303003/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_interactive_uitest.mm:49:
EXPECT_EQ([blueColor blueComponent], [color blueComponent]);
On 2015/05/14 04:23:50, tapted wrote:
> On 2015/05/14 02:59:13, jackhou1 wrote:
> > On 2015/05/12 06:15:43, tapted wrote:
> > > On 2015/05/12 05:39:05, jackhou1 wrote:
> > > > On 2015/05/06 03:15:42, tapted wrote:
> > > > > I don't find these checks as convincing as just removing
> > > `blueColor/redColor`
> > > > > completely and checking against a constant 0xff / 0x0. Especially
since
> > that
> > > > > will then match the manifest in the string constant at the start of
the
> > > test.
> > > > 
> > > > Done.
> > > 
> > > You missed this i think
> > 
> > NSColor components are CGFloats, so it's 1 instead of 255.
> 
> Ah, can you comment - it should say the values correspond to the hexvalues in
> the string constant. (maybe you could use rgb(100%, 0%, 0%) to make them seem
> realted, but that's probably more confusing.)

Done.

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (left):

https://codereview.chromium.org/1053303003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:791:
[titlebar_background_view_ setNeedsDisplay:YES];
On 2015/05/14 04:23:50, tapted wrote:
> On 2015/05/14 02:59:13, jackhou1 wrote:
> > On 2015/05/12 06:15:43, tapted wrote:
> > > why isn't this needed any more (also in *Resign)
> > 
> > I think it was redundant. The -[NSView drawRect:] gets called when the
window
> > changes focus.
> > 
> > 2   Chromium Framework                  0x00000001121660b3
> > -[TitlebarBackgroundView drawRect:] + 179
> > 3   AppKit                              0x00007fff87728b19 -[NSView
> > _drawRect:clip:] + 4238
> > 4   AppKit                              0x00007fff8772717a -[NSView
> > _recursiveDisplayAllDirtyWithLockFocus:visRect:] + 1875
> > 5   AppKit                              0x00007fff8772757e -[NSView
> > _recursiveDisplayAllDirtyWithLockFocus:visRect:] + 2903
> > 6   AppKit                              0x00007fff87725016 -[NSView
> >
>
_recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
> > + 913
> > 7   AppKit                              0x00007fff87724773 -[NSThemeFrame
> >
>
_recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:]
> > + 333
> > 8   AppKit                              0x00007fff877212cb -[NSView
> > _displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:] + 2761
> > 9   AppKit                              0x00007fff876ffe2d -[NSView
> > displayIfNeeded] + 1876
> 
> So is something in NSWindow already setting setNeedsDisplay:YES for the code
to
> proceed at this point? (it's fine if it is - just want to make sure there's
not
> something more subtle... i.e. were those lines originally added for a reason?
> 
> > 10  AppKit                              0x00007fff8771d1c5 -[NSWindow
> > displayIfNeeded] + 232
> 

Yeah, it happens whenever the window changes focus, e.g. I think it's
responsible for changing the color of the buttons.

I added these lines when changing the colored frame implementation last time. We
need to call setNeedsDisplay for the corresponding NSView in browser windows
because the theme changes at other times. I think that's the only reason, so
it's not needed for app windows.

https://codereview.chromium.org/1053303003/diff/90001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/titlebar_background_view.h (right):

https://codereview.chromium.org/1053303003/diff/90001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/titlebar_background_view.h:19: // Adds a
TitlebarBackgroundView to the [[window contentView] superView].
On 2015/05/14 04:23:50, tapted wrote:
> nit: empty line before

Done.

https://codereview.chromium.org/1053303003/diff/90001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.h (right):

https://codereview.chromium.org/1053303003/diff/90001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.h:28: // Weak.
NativeAppWindowViews owns a Widget.
On 2015/05/14 04:23:50, tapted wrote:
> The Widget in NativeAppWindowViews is also weak.
> 
> I think technically the Widget owner is either "itself" or the WebContents in
> AppWindowContentsImpl, since extensions::AppWindow is its WebContentsDelegate,
> and it guarantees to call native_app_window_->Close() along with the
> WebContents.
> 
> So I'd say "Weak. Owned by extensions::AppWindow (which manages our Widget via
> its WebContents)."

Done.

https://codereview.chromium.org/1053303003/diff/90001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc
(right):

https://codereview.chromium.org/1053303003/diff/90001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_aura.cc:179:
apps::AppWindowFrameView* frame = static_cast<apps::AppWindowFrameView*>(
On 2015/05/14 04:23:50, tapted wrote:
> This isn't so nice.. What about making
> ChromeNativeAppWindowViews::CreateNonStandardAppFrame() a pure virtual
function,
> and moving its definition in here?

Done.

[tapted, 2015-05-14 06:32:12]

sweet! lgtm

[jackhou1, 2015-05-14 06:45:24]

jackhou@chromium.org changed reviewers:
+ sky@chromium.org

[jackhou1, 2015-05-14 06:45:24]

sky, please review for OWNERS in:

chrome/browser/ui/test/

[sky, 2015-05-14 15:51:35]

LGTM

[jackhou1, 2015-05-15 00:42:15]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2015-05-15 00:42:43]

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

[commit-bot: I haz the power, 2015-05-15 00:51:19]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2015-05-15 00:52:35]

Patchset 8 (id:??) landed as
https://crrev.com/2ebfcc03fc85c4578008cfed368097950f9d6348
Cr-Commit-Position: refs/heads/master@{#329999}