Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
338773002:
[Mac] Make app window backgrounds white. (Closed)
|
Created:
6 years, 6 months ago by jackhou1 Modified:
6 years, 6 months ago Reviewers:
tapted CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Mac] Make app window backgrounds white.
Native title bars make the background color grey. This changes it back
to white.
BUG=382829
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277732
Patch Set 1 #
Total comments: 3
Patch Set 2 : Clip the native implementation to only draw the title bar. #
Total comments: 7
Patch Set 3 : Address comments #Patch Set 4 : Sync and rebase #Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/338773002/diff/1/chrome/browser/ui/cocoa/apps... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/338773002/diff/1/chrome/browser/ui/cocoa/apps... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:355: would it make sense to just call [window setBackgroundColor:[NSColor whiteColor]] here?
https://codereview.chromium.org/338773002/diff/1/chrome/browser/ui/cocoa/apps... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/338773002/diff/1/chrome/browser/ui/cocoa/apps... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:355: On 2014/06/17 00:21:53, tapted wrote: > would it make sense to just call [window setBackgroundColor:[NSColor > whiteColor]] here? That seems to make the entire background white (including the title bar).
I looked into changing the background color that the native implementation draws, but I can't see an obvious way to override it. I've changed it so that the native implementation only draws the title bar area. Drawing the conent area rect doesn't affect rounded corners at the bottom.
So.. after clicking Publish+Mail, I realised this isn't reproducible with chrome://flags/#enable-apps-show-on-first-paint which will soon become default :/. but! There's a horrible "other" bug that comes up in that case. https://codereview.chromium.org/338773002/diff/1/chrome/browser/ui/cocoa/apps... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/338773002/diff/1/chrome/browser/ui/cocoa/apps... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:355: On 2014/06/17 00:37:15, jackhou1 wrote: > On 2014/06/17 00:21:53, tapted wrote: > > would it make sense to just call [window setBackgroundColor:[NSColor > > whiteColor]] here? > > That seems to make the entire background white (including the title bar). Ah - yeah that would suck. https://codereview.chromium.org/338773002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/338773002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:224: NSRect contentRect = [NSWindow contentRectForFrameRect:rect I think you can use [self contentRectForFrameRect:rect]; https://codereview.chromium.org/338773002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:230: // implementation draws a grey background. nit: grey -> gray to be consistent with color :p https://codereview.chromium.org/338773002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:232: NSMakeRect(NSMinX(contentRect), I stared at the second argument for a while before realising what it was doing (I was reading NSMaxY as NSMinY). Pity there's no NSSubtractRect (I think). Maybe rename: rect -> frameRect contentRect -> rect Then do rect.origin.y = NSMaxY(rect); rect.size.height = CGFLOAT_MAX; rect = NSIntersectionRect(rect, frameRect); I think this makes it clearer what's changing. https://codereview.chromium.org/338773002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:236: [NSBezierPath clipRect:titleBarRect]; It's possible clipping will be more expensive than just painting the full area twice (assuming that actually works) (but I'm guessing, so probably not worth worrying about)
On 2014/06/17 06:09:52, tapted wrote: > So.. after clicking Publish+Mail, I realised this isn't reproducible with > chrome://flags/#enable-apps-show-on-first-paint which will soon become default > :/. I don't think it will be terribly soon, there are a lot of tests to address. Maybe M38. > > but! There's a horrible "other" bug that comes up in that case. > > https://codereview.chromium.org/338773002/diff/1/chrome/browser/ui/cocoa/apps... > File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): > > https://codereview.chromium.org/338773002/diff/1/chrome/browser/ui/cocoa/apps... > chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:355: > On 2014/06/17 00:37:15, jackhou1 wrote: > > On 2014/06/17 00:21:53, tapted wrote: > > > would it make sense to just call [window setBackgroundColor:[NSColor > > > whiteColor]] here? > > > > That seems to make the entire background white (including the title bar). > > Ah - yeah that would suck. > > https://codereview.chromium.org/338773002/diff/20001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): > > https://codereview.chromium.org/338773002/diff/20001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:224: NSRect contentRect > = [NSWindow contentRectForFrameRect:rect > I think you can use [self contentRectForFrameRect:rect]; > > https://codereview.chromium.org/338773002/diff/20001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:230: // implementation > draws a grey background. > nit: grey -> gray to be consistent with color :p > > https://codereview.chromium.org/338773002/diff/20001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:232: > NSMakeRect(NSMinX(contentRect), > I stared at the second argument for a while before realising what it was doing > (I was reading NSMaxY as NSMinY). Pity there's no NSSubtractRect (I think). > > Maybe rename: > rect -> frameRect > contentRect -> rect > > Then do > > rect.origin.y = NSMaxY(rect); > rect.size.height = CGFLOAT_MAX; > rect = NSIntersectionRect(rect, frameRect); > > > I think this makes it clearer what's changing. > > https://codereview.chromium.org/338773002/diff/20001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:236: [NSBezierPath > clipRect:titleBarRect]; > It's possible clipping will be more expensive than just painting the full area > twice (assuming that actually works) (but I'm guessing, so probably not worth > worrying about)
On 2014/06/17 06:15:00, benwells wrote: > On 2014/06/17 06:09:52, tapted wrote: > > So.. after clicking Publish+Mail, I realised this isn't reproducible with > > chrome://flags/#enable-apps-show-on-first-paint which will soon become default > > :/. > > I don't think it will be terribly soon, there are a lot of tests to address. > Maybe M38. Ah, lgtm then with those changes, in the interests of removing a stable blocker (and assuming enabling that flag with this patch doesn't have badness worse than http://crbug.com/382829#c6). (But, IMO, a bit of gray while maximising is OK too).
> (But, IMO, a bit of gray while maximising is OK too). Yeah, I don't think it's a big deal either, but it seemed like an easy thing to fix. https://codereview.chromium.org/338773002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/338773002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:224: NSRect contentRect = [NSWindow contentRectForFrameRect:rect On 2014/06/17 06:09:51, tapted wrote: > I think you can use [self contentRectForFrameRect:rect]; Done. https://codereview.chromium.org/338773002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:230: // implementation draws a grey background. On 2014/06/17 06:09:52, tapted wrote: > nit: grey -> gray to be consistent with color :p Done. https://codereview.chromium.org/338773002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:232: NSMakeRect(NSMinX(contentRect), On 2014/06/17 06:09:51, tapted wrote: > I stared at the second argument for a while before realising what it was doing > (I was reading NSMaxY as NSMinY). Pity there's no NSSubtractRect (I think). > > Maybe rename: > rect -> frameRect > contentRect -> rect > > Then do > > rect.origin.y = NSMaxY(rect); > rect.size.height = CGFLOAT_MAX; > rect = NSIntersectionRect(rect, frameRect); > > > I think this makes it clearer what's changing. Done.
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/338773002/40001
The CQ bit was unchecked by jackhou@chromium.org
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/338773002/60001
Message was sent while issue was closed.
Change committed as 277732 |
