Mac: Well-behaved accelerated plugins, actual fix

Mac: Well-behaved accelerated plugins, actual fix

Configure the opengl view to draw below the window, draw the view with NSClearColor so that's transparent, and make the window non-opaque while plugins are showing, so that the opengl surface can show through.

This is a bit slower than previously without this patch, but it's about as fast as it was when we used CoreAnimation to show IOSurfaces.

Parts of this code are from Simon Fraser's MacTierra code (with permission).

BUG=44087, 51748
TEST=
* Go to youtube. Findbar should show up on top of video. Go fullscreen. Tab overlay should show up on top of video.
* Go to youtube, drag tab with video out and to a new window. Video should continue playing.
* Install https://chrome.google.com/extensions/detail/bdnkaenpadjoldiddfdidinjmjeagaji , click browser action. Should still work.
* Install a transparent theme such as https://tools.google.com/chrome/intl/cy/themes/theme_at_mecko.html or http://chromium.googlecode.com/issues/attachment?aid=3778755830122130591&name=theme-bug.crx&token=27908ca7e699a023c8446657b24c4696 . Browser window shouldn't become transparent while videos are playing. (This is actually not yet working)

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

[Nico, 2010-08-20 21:57:06]

The CL introduces a regression as-is: When a theme with transparent images is
installed, the transparent images actually make the window transparent while
coreanimation plugins are visible. I'd rather fix that in a follow-up CL (by
removing transparency from theme images on theme load).

(If you're ok with that, I will remove that point from the TEST= area and file a
new bug for that.)

[Nico, 2010-08-20 22:00:33]

Forgot to say: I added a flag which can be used to disable hole-punching, as a
debugging aid ("can you try running with --disable-hole-punching?") if this has
weird unwanted effects. But since this API has been around since 10.2 or so, I'm
hopeful that it won't be needed.

[stuartmorgan, 2010-08-20 23:31:41]

I don't see anything here that accounts for plugins that are marked as
transparent.

[Nico, 2010-08-20 23:33:30]

Transparent plugins depend on plugins being drawn by the compositor,
as discussed previously.

On Fri, Aug 20, 2010 at 4:31 PM,  <stuartmorgan@chromium.org> wrote:
> I don't see anything here that accounts for plugins that are marked as
> transparent.
>
> http://codereview.chromium.org/3176027/show
>

[stuartmorgan, 2010-08-20 23:45:43]

On 2010/08/20 23:33:30, Nico wrote:
> Transparent plugins depend on plugins being drawn by the compositor,
> as discussed previously.

To be completely correct, yes, but we also discussed having them draw over the
page instead of under it in the mean time. If I'm understanding correctly
transparent plugins would cut a hole all the way through the window with this
patch, with seems like a really bad interim state.

[Nico, 2010-08-20 23:47:03]

On Fri, Aug 20, 2010 at 4:45 PM,  <stuartmorgan@chromium.org> wrote:
> On 2010/08/20 23:33:30, Nico wrote:
>>
>> Transparent plugins depend on plugins being drawn by the compositor,
>> as discussed previously.
>
> To be completely correct, yes, but we also discussed having them draw over
> the
> page instead of under it in the mean time. If I'm understanding correctly
> transparent plugins would cut a hole all the way through the window with
> this
> patch, with seems like a really bad interim state.

Oh ok, I didn't realize we agreed to do this workaround. Will update
this patch :-)

>
> http://codereview.chromium.org/3176027/show
>

[Nico, 2010-08-23 22:34:21]

> >> Transparent plugins depend on plugins being drawn by the compositor,
> >> as discussed previously.
> >
> > To be completely correct, yes, but we also discussed having them draw over
> > the
> > page instead of under it in the mean time. If I'm understanding correctly
> > transparent plugins would cut a hole all the way through the window with
> > this
> > patch, with seems like a really bad interim state.
> 
> Oh ok, I didn't realize we agreed to do this workaround. Will update
> this patch :-)

I added this, but then we discovered that this will never be hit in practice,
because we can't tell in general if a plugin will be transparent or not.

We agreed on being broken for transparent CA plugins for m7, and if actual
plugins turn up that use this, we will add a check to set transparent_ for these
(if we can). And I'll try to get plugins into the compositor quickly.

[Nico, 2010-08-24 00:21:54]

After some more consideration, I think we shouldn't add all that
"can_draw_transparent" in this CL when it's never used at the moment anyway. If
you agree, review patch set 2.

[stuartmorgan, 2010-08-24 16:38:42]

LGTM

http://codereview.chromium.org/3176027/diff/2001/3003
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3176027/diff/2001/3003#newcode138
chrome/browser/renderer_host/render_widget_host_view_mac.mm:138: // Informat
protocol implemented by windows that need to be informed explicitly
s/Informat/Informal/

http://codereview.chromium.org/3176027/diff/2001/3003#newcode256
chrome/browser/renderer_host/render_widget_host_view_mac.mm:256: [self
getRectsBeingDrawn:&dirtyRects count:&dirtyRectCount];
Move this to before the blank line, since it's more closely associated with the
uninitialized values? (And uninitialized values that aren't glued to a call that
fills them in make me nervous.)

http://codereview.chromium.org/3176027/diff/2001/3003#newcode313
chrome/browser/renderer_host/render_widget_host_view_mac.mm:313: if ([self
window] &&
[self window] here and newWindow below are unnecessary checks; the nil-message
will explicitly return NO.

[stuartmorgan, 2010-08-24 17:08:26]

Sorry, I should have said specifically: Patch Set 2 LGTM

I agree that we should wait on all the additional complexity until we know if we
need it. Sorry again for sidetracking you.

[Nico, 2010-08-24 17:08:46]

thanks, committing

http://codereview.chromium.org/3176027/diff/2001/3003
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3176027/diff/2001/3003#newcode138
chrome/browser/renderer_host/render_widget_host_view_mac.mm:138: // Informat
protocol implemented by windows that need to be informed explicitly
On 2010/08/24 16:38:42, stuartmorgan wrote:
> s/Informat/Informal/

Done.

http://codereview.chromium.org/3176027/diff/2001/3003#newcode256
chrome/browser/renderer_host/render_widget_host_view_mac.mm:256: [self
getRectsBeingDrawn:&dirtyRects count:&dirtyRectCount];
On 2010/08/24 16:38:42, stuartmorgan wrote:
> Move this to before the blank line, since it's more closely associated with
the
> uninitialized values? (And uninitialized values that aren't glued to a call
that
> fills them in make me nervous.)

Done.

http://codereview.chromium.org/3176027/diff/2001/3003#newcode313
chrome/browser/renderer_host/render_widget_host_view_mac.mm:313: if ([self
window] &&
On 2010/08/24 16:38:42, stuartmorgan wrote:
> [self window] here and newWindow below are unnecessary checks; the nil-message
> will explicitly return NO.

i know, but i feel being explicit adds clarity in this case.

[pink (ping after 24hrs), 2010-08-24 17:18:54]

lgtm

http://codereview.chromium.org/3176027/diff/18001/19007
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3176027/diff/18001/19007#newcode140
chrome/browser/renderer_host/render_widget_host_view_mac.mm:140: @protocol
UnderlayableSurface
If it's a @protocol, then it's formal. s/Informal//

http://codereview.chromium.org/3176027/diff/18001/19007#newcode204
chrome/browser/renderer_host/render_widget_host_view_mac.mm:204: // then, we
have the coice of two evils:
choice

http://codereview.chromium.org/3176027/diff/18001/19007#newcode343
chrome/browser/renderer_host/render_widget_host_view_mac.mm:343: if ([self
window] &&
do you need the extra window null checks? [nil respondsToSelector:] is always
NO, right?

[Nico, 2010-08-24 17:27:55]

Filed http://crbug.com/53217 for the themes glitch this introduces

http://codereview.chromium.org/3176027/diff/18001/19007
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3176027/diff/18001/19007#newcode140
chrome/browser/renderer_host/render_widget_host_view_mac.mm:140: @protocol
UnderlayableSurface
On 2010/08/24 17:18:55, pink wrote:
> If it's a @protocol, then it's formal. s/Informal//

Not really, since nothing implements this protocol. It's just here to get rid of
the "unknow method -underlaySurfaceAdded, assuming int return type and ...
parameters" warning.

http://codereview.chromium.org/3176027/diff/18001/19007#newcode204
chrome/browser/renderer_host/render_widget_host_view_mac.mm:204: // then, we
have the coice of two evils:
On 2010/08/24 17:18:55, pink wrote:
> choice

will fix when i touch this file again (probably soon)

http://codereview.chromium.org/3176027/diff/18001/19007#newcode343
chrome/browser/renderer_host/render_widget_host_view_mac.mm:343: if ([self
window] &&
On 2010/08/24 17:18:55, pink wrote:
> do you need the extra window null checks? [nil respondsToSelector:] is always
> NO, right?

i don't need them, but i thought it's clearer.

since both stuart and you complained, i guess i'll remove them when i touch this
file the next time :-P

[pink (ping after 24hrs), 2010-08-24 17:39:57]

> Not really, since nothing implements this protocol. It's just here to
> get rid of the "unknow method -underlaySurfaceAdded, assuming int return
> type and ... parameters" warning.

We generally use @category for this type of thing, since @protocol
really has a meaning like a pure-virtual interface in C++. There's an
expectation that someone is implementing it, not just to make the
compiler happy.

-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com