Fix window shadows.

Fix window shadows.

For accelerated content, switch to a less scary method of fixing window shadows that works better.

BUG=53382, 56154, 120772
TEST=go to accelerated page, notice no square corners


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132206

[Avi (use Gerrit), 2012-04-10 16:49:41]

Yes, three reviewers. Please let me know what you think.

My main concern here is speed. We're overlaying lines over the left and right of
the window that are invisible to the eye but still there. If we've punched out
the entire window, this is no problem. If we haven't, then we are compositing
these on top of the RWHVs, and that's my worry.

Do we have a slow Mac to test scroll speed with? Something with a GMA950?

The alternative could be to turn on and off the lines. We could have the same
system we do now of the RWHV telling the window when holes are punched. However,
instead of how we have it today when the window responds by flipping opacity
(which screws things up) we could have it turn on and off the bars. This is a
lot less clean a solution and is therefore less preferable.

[jbates, 2012-04-10 17:54:00]

LGTM, I think this looks pretty good. If performance is a problem, another
option is to have RenderWidgetHostViewCocoa::drawRect draw these lines when it
blits the IOSurface.

https://chromiumcodereview.appspot.com/10037008/diff/1/ui/base/cocoa/underlay...
File ui/base/cocoa/underlay_opengl_hosting_window.mm (right):

https://chromiumcodereview.appspot.com/10037008/diff/1/ui/base/cocoa/underlay...
ui/base/cocoa/underlay_opengl_hosting_window.mm:40: NSComparisonResult
OpaqueViewsOnTop(id view1, id view2, void* context) {
If view1 and view2 are both OpaqueView, then:
  OpaqueViewsOnTop(view1, view2) == NSOrderedDescending
  OpaqueViewsOnTop(view2, view1) == NSOrderedDescending
Could check for equality first?

https://chromiumcodereview.appspot.com/10037008/diff/1/ui/base/cocoa/underlay...
ui/base/cocoa/underlay_opengl_hosting_window.mm:105: [leftOpaque
setAlphaValue:0.002f];
Is this chosen on purpose to round to zero when converted to uint8? Assuming OSX
is drawing windows via 8-bit-per-color shaders, 0.002 => (uint8)(255*(0.002)) =>
0

Would be interesting to verify that with a screenshot using the old code and the
new (but a thicker line so it overlaps with the tab contents a bit) on an
accelerated tab with a white background (and compare the white pixel values).

[Avi (use Gerrit), 2012-04-10 18:34:45]

https://chromiumcodereview.appspot.com/10037008/diff/1/ui/base/cocoa/underlay...
File ui/base/cocoa/underlay_opengl_hosting_window.mm (right):

https://chromiumcodereview.appspot.com/10037008/diff/1/ui/base/cocoa/underlay...
ui/base/cocoa/underlay_opengl_hosting_window.mm:40: NSComparisonResult
OpaqueViewsOnTop(id view1, id view2, void* context) {
On 2012/04/10 17:54:00, jbates wrote:
> Could check for equality first?

Probably a good idea.

https://chromiumcodereview.appspot.com/10037008/diff/1/ui/base/cocoa/underlay...
ui/base/cocoa/underlay_opengl_hosting_window.mm:105: [leftOpaque
setAlphaValue:0.002f];
You ascribe to me far more cleverness than is deserved. I selected 0.002 because
that was the smallest value (with three digits) that still had the effect of
preserving the window background.

[Mark Mentovai, 2012-04-10 19:08:29]

Cool. LGTM.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
File ui/base/cocoa/underlay_opengl_hosting_window.h (right):

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.h:16: -
(id)initWithContentRect:(NSRect)contentRect
You don’t need to declare this here. It’s already part of NSWindow.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
File ui/base/cocoa/underlay_opengl_hosting_window.mm (right):

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:41: BOOL view1IsOpaqueView =
[view1 isKindOfClass:[OpaqueView class]];
Style nit: use_c_plus_plus_naming outside of @interface/@implementation blocks.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:72: Method m =
class_getInstanceMethod([NSView class], @selector(didAddSubview:));
You can use borderViewClass here instead of [NSView class]. But…

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:78: method_getTypeEncoding(m));
If this is all you’re using m for, it seems like you don’t even really need it.
You can build up the encoding with

  @encode(void), @encode(id), @encode(SEL), @encode(NSView*)

but this also sucks because @encode isn’t treated as a string literal during
preprocessing so you can’t use the preprocessor’s string paste.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:107: [rootView
addSubview:leftOpaque];
Can you wait to add it ’til after the resize mask and alpha are set up? 114 too.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:108: [leftOpaque
setAutoresizingMask:NSViewMaxXMargin|NSViewHeightSizable];
Tiny bit more readable with space around the “or” operator, and also on line
115.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:109: [leftOpaque
setAlphaValue:0.002f];
Want a const CGFloat to store this so you can be sure you’re using the same
value for rightOpaque?

I’d say “get rid of the f” because CGFloat isn’t always gonna be a float.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:113: 1, NSHeight(rootBounds) - 2
* kEdgeInset)]);
Similarly, you can also keep consts for y and h.

[Avi (use Gerrit), 2012-04-10 19:42:49]

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
File ui/base/cocoa/underlay_opengl_hosting_window.h (right):

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.h:16: -
(id)initWithContentRect:(NSRect)contentRect
On 2012/04/10 19:08:30, Mark Mentovai wrote:
> You don’t need to declare this here. It’s already part of NSWindow.

Done.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
File ui/base/cocoa/underlay_opengl_hosting_window.mm (right):

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:41: BOOL view1IsOpaqueView =
[view1 isKindOfClass:[OpaqueView class]];
On 2012/04/10 19:08:30, Mark Mentovai wrote:
> Style nit: use_c_plus_plus_naming outside of @interface/@implementation
blocks.

Done.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:78: method_getTypeEncoding(m));
I'm having trouble; how do you paste together the strings generated by @encode
into the one big string wanted by class_addMethod? I'm completely happy cheating
and grabbing a ready-to-go type encoding string.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:107: [rootView
addSubview:leftOpaque];
On 2012/04/10 19:08:30, Mark Mentovai wrote:
> Can you wait to add it ’til after the resize mask and alpha are set up? 114
too.

Done.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:108: [leftOpaque
setAutoresizingMask:NSViewMaxXMargin|NSViewHeightSizable];
On 2012/04/10 19:08:30, Mark Mentovai wrote:
> Tiny bit more readable with space around the “or” operator, and also on line
> 115.

Done.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:109: [leftOpaque
setAlphaValue:0.002f];
On 2012/04/10 19:08:30, Mark Mentovai wrote:
> Want a const CGFloat to store this so you can be sure you’re using the same
> value for rightOpaque?
> 
> I’d say “get rid of the f” because CGFloat isn’t always gonna be a float.

Done.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:113: 1, NSHeight(rootBounds) - 2
* kEdgeInset)]);
On 2012/04/10 19:08:30, Mark Mentovai wrote:
> Similarly, you can also keep consts for y and h.

It's weird to have some of the values from the root bounds be consts and not
others.

[Mark Mentovai, 2012-04-10 19:48:28]

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
File ui/base/cocoa/underlay_opengl_hosting_window.mm (right):

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:78: method_getTypeEncoding(m));
Avi wrote:
> I'm having trouble; how do you paste together the strings generated by @encode
> into the one big string wanted by class_addMethod? I'm completely happy
cheating
> and grabbing a ready-to-go type encoding string.

You’d have to build it at runtime with snprintf.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:113: 1, NSHeight(rootBounds) - 2
* kEdgeInset)]);
Avi wrote:
> On 2012/04/10 19:08:30, Mark Mentovai wrote:
> > Similarly, you can also keep consts for y and h.
> 
> It's weird to have some of the values from the root bounds be consts and not
> others.

They’re all consts in that you can declare rootBounds const too…

[Nico, 2012-04-10 19:52:48]

lgtm

That's a lot simpler :-) Can you check this does the right thing for an
extension bubble with composited contents? (sail@ has such an extension afaik)

https://chromiumcodereview.appspot.com/10037008/diff/1011/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://chromiumcodereview.appspot.com/10037008/diff/1011/chrome/browser/ui/c...
chrome/browser/ui/cocoa/browser_window_controller.mm:241: [window
respondsToSelector:@selector(setBottomCornerRounded:)])
Is this part necessary? (just curious)

[Avi (use Gerrit), 2012-04-10 20:11:22]

> Can you check this does the right thing for an
extension bubble with composited contents? (sail@ has such an extension afaik)

fffuuuu

lemme look into that.

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
File ui/base/cocoa/underlay_opengl_hosting_window.mm (right):

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:78: method_getTypeEncoding(m));
Well, sure, but I'm not sure I see the advantage to grabbing it off the class
we're overriding.

https://chromiumcodereview.appspot.com/10037008/diff/1011/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://chromiumcodereview.appspot.com/10037008/diff/1011/chrome/browser/ui/c...
chrome/browser/ui/cocoa/browser_window_controller.mm:241: [window
respondsToSelector:@selector(setBottomCornerRounded:)])
On 2012/04/10 19:52:48, Nico wrote:
> Is this part necessary? (just curious)

I thought that it was and I changed it, but I believe it isn't (and I left it in
anyway).

[Mark Mentovai, 2012-04-10 20:19:10]

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
File ui/base/cocoa/underlay_opengl_hosting_window.mm (right):

https://chromiumcodereview.appspot.com/10037008/diff/5001/ui/base/cocoa/under...
ui/base/cocoa/underlay_opengl_hosting_window.mm:78: method_getTypeEncoding(m));
Avi wrote:
> Well, sure, but I'm not sure I see the advantage to grabbing it off the class
> we're overriding.

That’s why I said it sucks too.

[Avi (use Gerrit), 2012-04-12 20:11:00]

On 2012/04/10 19:52:48, Nico wrote:
> That's a lot simpler :-) Can you check this does the right thing for an
> extension bubble with composited contents? (sail@ has such an extension afaik)

This seems to do the wrong thing with bubbles in general, though I don't know
why. Looking into it.

[Avi (use Gerrit), 2012-04-12 20:17:01]

fffffuuuuu

Bubbles are typically InfoBubbleWindows, which are ChromeEventProcessingWindows,
which are UnderlayOpenGLHostingWindows. And in that case, you _do_ want
transparency to follow the content shape.

fffffuuuuu

[Avi (use Gerrit), 2012-04-12 20:38:32]

OK, fixed so we only do magic for titled windows. WDYT?

[commit-bot: I haz the power, 2012-04-13 15:45:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/10037008/15001

[commit-bot: I haz the power, 2012-04-13 17:18:43]

Change committed as 132206