Mac: Well-behaved accelerated plugins, preparation

Mac: Well-behaved accelerated plugins, preparation

This moves IOSurfaces into NSOpenGLViews instead of CALayers.

The advantage of this is that we can use hole-punching (which I'm leaving for a separate CL, but which is easy to add: see diff from patch set 6 to 5), which fixes the appearance of find bar etc. Also, we don't have to hack around coreanimation behavior we don't want, like appkit destroying and recreating our layers, and CA animating everything implicitly.

Even though this CL doesn't have hole punching yet, it fixes the find bar in most cases already, because now only the area actually covered by a plugin is drawn via opengl – so if the findbar doesn't overlap the accelerated plugin, it will be visible.

I didn't test what happens if a page uses both accelerated plugins and the compositor. One view covering the whole tab is created for the compositor, and one view for every plugin. It might even work, but I couldn't find a page to test this with, and accelerated plugins will be handled by the compositor anyway.

The opengl views don't handle any events: Events fall through to the parent view (RWHVMac) and are handled as before.

BUG=51748
TEST=
* Go to youtube, video should still paint correctly (even when scrolling, switching tabs, resizing the window rapidly, etc). Mouse clicks and keyboard events in the player should still work.
* Build with `GYP_DEFINES="use_accelerated_compositing=1" gclient runhooks`, run with --enable-accelerated-compositing. Demos at http://webkit.org/blog/386/3d-transforms/ should still look fine (the color shift is not a regression caused by this CL, it was that way before)

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

[Ken Russell (switch to Gerrit), 2010-08-11 01:20:18]

This all looks good. While there is more mechanism involved, it sounds like this
approach will work better at least in the short and medium term.

Thanks for taking care of this.

Minor nits: in the CL description, "if a page uses both accelerated plugins" ->
"if a page uses both accelerated plugins and the compositor"; also ideally need
a bug ID.

[Nico, 2010-08-11 01:42:57]

On 2010/08/11 01:20:18, kbr wrote:
> This all looks good. While there is more mechanism involved, it sounds like
this
> approach will work better at least in the short and medium term.

It's not _that_ much more code :-)

Conceptually, I'd argue it's even simpler since we no longer use the whole
coreanimation machinery.

> Minor nits: in the CL description, "if a page uses both accelerated plugins"
->
> "if a page uses both accelerated plugins and the compositor"; also ideally
need
> a bug ID.

Both done.

I will land this now, and then leave for a week of vacation. If there are any
problems with this, back it out again.

[stuartmorgan, 2010-08-11 16:40:44]

Some cleanup for when you get back.

Was there some reason it was critical to land this before you left? It would
have been nice if the change had been polished beyond the proof-of-concept level
before landing it.

http://codereview.chromium.org/3010054/diff/15001/16001
File chrome/browser/renderer_host/accelerated_surface_container_mac.cc (left):

http://codereview.chromium.org/3010054/diff/15001/16001#oldcode65
chrome/browser/renderer_host/accelerated_surface_container_mac.cc:65: void
AcceleratedSurfaceContainerMac::MoveTo(
MoveTo is a poor name for this function at this point, since it has no concept
of position any more. SetGeometry?

http://codereview.chromium.org/3010054/diff/15001/16003
File chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc
(right):

http://codereview.chromium.org/3010054/diff/15001/16003#newcode80
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc:80: //
context associated with the CAOpenGLLayer is destroyed. However,
Comment is no longer right.

http://codereview.chromium.org/3010054/diff/15001/16003#newcode96
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc:96:
glColorMask(true, true, true, true);
The division between this method and Draw seems pretty arbitrary now that it's
only painting one thing. Is there a reason not to push all this GL code into
Draw()?

http://codereview.chromium.org/3010054/diff/15001/16004
File chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h
(right):

http://codereview.chromium.org/3010054/diff/15001/16004#newcode39
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h:39: //
short-circuit the normal drawing process.
This comment pretty clearly doesn't match the function any more.

http://codereview.chromium.org/3010054/diff/15001/16004#newcode60
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h:60: //
context, which must already be current.
Again, the comment is now completely wrong.

http://codereview.chromium.org/3010054/diff/15001/16006
File chrome/browser/renderer_host/render_widget_host_view_mac.h (left):

http://codereview.chromium.org/3010054/diff/15001/16006#oldcode141
chrome/browser/renderer_host/render_widget_host_view_mac.h:141: -
(void)drawAcceleratedPluginLayer;
This method is now completely unused, and needs to be removed.

http://codereview.chromium.org/3010054/diff/15001/16006
File chrome/browser/renderer_host/render_widget_host_view_mac.h (right):

http://codereview.chromium.org/3010054/diff/15001/16006#newcode245
chrome/browser/renderer_host/render_widget_host_view_mac.h:245: // Informs the
plug-in instances that their drawing context has changed.
Comment and function are now mismatched.

http://codereview.chromium.org/3010054/diff/15001/16007
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3010054/diff/15001/16007#newcode176
chrome/browser/renderer_host/render_widget_host_view_mac.mm:176: static CVReturn
MyDisplayLinkCallback(
Can we give this a better name please?

http://codereview.chromium.org/3010054/diff/15001/16007#newcode436
chrome/browser/renderer_host/render_widget_host_view_mac.mm:436:
CHECK(plugin_views_.end() != it);
CHECK? Really? Crashing end-users doesn't seem like a great failure mode (as
opposed to say, DCHECK + |continue;|)

http://codereview.chromium.org/3010054/diff/15001/16007#newcode592
chrome/browser/renderer_host/render_widget_host_view_mac.mm:592: continue;  //
Skip plugin views.
s/plugin/accelerated/

http://codereview.chromium.org/3010054/diff/15001/16007#newcode797
chrome/browser/renderer_host/render_widget_host_view_mac.mm:797:
CHECK(plugin_views_.end() != it);
Again, why CHECK?

http://codereview.chromium.org/3010054/diff/15001/16007#newcode808
chrome/browser/renderer_host/render_widget_host_view_mac.mm:808:
CHECK(plugin_views_.end() != it);
And here, and below, and in DrawAcceleratedSurfaceInstance.

http://codereview.chromium.org/3010054/diff/15001/16007#newcode2143
chrome/browser/renderer_host/render_widget_host_view_mac.mm:2143: // We only
want to do this for real browser views, not popups.
Move all this comment to just above the |if (newWindow != lastWindow_) {| so
that it is associated with the code it's actually describing.

[Ken Russell (switch to Gerrit), 2010-08-14 02:12:11]

I'll take care of these cleanups for Nico while he's away under
http://codereview.chromium.org/3185004 .

http://codereview.chromium.org/3010054/diff/15001/16001
File chrome/browser/renderer_host/accelerated_surface_container_mac.cc (left):

http://codereview.chromium.org/3010054/diff/15001/16001#oldcode65
chrome/browser/renderer_host/accelerated_surface_container_mac.cc:65: void
AcceleratedSurfaceContainerMac::MoveTo(
On 2010/08/11 16:40:44, stuartmorgan wrote:
> MoveTo is a poor name for this function at this point, since it has no concept
> of position any more. SetGeometry?

Will rename.

http://codereview.chromium.org/3010054/diff/15001/16003
File chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc
(right):

http://codereview.chromium.org/3010054/diff/15001/16003#newcode80
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc:80: //
context associated with the CAOpenGLLayer is destroyed. However,
On 2010/08/11 16:40:44, stuartmorgan wrote:
> Comment is no longer right.

Actually, because each container is rendered into its own OpenGL context, the
logic isn't right any more either. Will fix this.

http://codereview.chromium.org/3010054/diff/15001/16003#newcode96
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc:96:
glColorMask(true, true, true, true);
On 2010/08/11 16:40:44, stuartmorgan wrote:
> The division between this method and Draw seems pretty arbitrary now that it's
> only painting one thing. Is there a reason not to push all this GL code into
> Draw()?

I'd rather leave it structured like this in case we ever get back to a situation
where more than one accelerated plugin at a time. More likely, though, we'll
move to a model where the compositor handles everything, and this entire class
will go away.

http://codereview.chromium.org/3010054/diff/15001/16004
File chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h
(right):

http://codereview.chromium.org/3010054/diff/15001/16004#newcode39
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h:39: //
short-circuit the normal drawing process.
On 2010/08/11 16:40:44, stuartmorgan wrote:
> This comment pretty clearly doesn't match the function any more.

Will fix.

http://codereview.chromium.org/3010054/diff/15001/16004#newcode60
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h:60: //
context, which must already be current.
On 2010/08/11 16:40:44, stuartmorgan wrote:
> Again, the comment is now completely wrong.

Will fix.

http://codereview.chromium.org/3010054/diff/15001/16006
File chrome/browser/renderer_host/render_widget_host_view_mac.h (left):

http://codereview.chromium.org/3010054/diff/15001/16006#oldcode141
chrome/browser/renderer_host/render_widget_host_view_mac.h:141: -
(void)drawAcceleratedPluginLayer;
On 2010/08/11 16:40:44, stuartmorgan wrote:
> This method is now completely unused, and needs to be removed.

Will delete this and other related code.

http://codereview.chromium.org/3010054/diff/15001/16006
File chrome/browser/renderer_host/render_widget_host_view_mac.h (right):

http://codereview.chromium.org/3010054/diff/15001/16006#newcode245
chrome/browser/renderer_host/render_widget_host_view_mac.h:245: // Informs the
plug-in instances that their drawing context has changed.
On 2010/08/11 16:40:44, stuartmorgan wrote:
> Comment and function are now mismatched.

Will fix.

http://codereview.chromium.org/3010054/diff/15001/16007
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3010054/diff/15001/16007#newcode176
chrome/browser/renderer_host/render_widget_host_view_mac.mm:176: static CVReturn
MyDisplayLinkCallback(
On 2010/08/11 16:40:44, stuartmorgan wrote:
> Can we give this a better name please?

Will change to "DrawOneAcceleratedPluginCallback".

http://codereview.chromium.org/3010054/diff/15001/16007#newcode436
chrome/browser/renderer_host/render_widget_host_view_mac.mm:436:
CHECK(plugin_views_.end() != it);
On 2010/08/11 16:40:44, stuartmorgan wrote:
> CHECK? Really? Crashing end-users doesn't seem like a great failure mode (as
> opposed to say, DCHECK + |continue;|)

Will change these to DCHECKs and add if tests for release builds.

http://codereview.chromium.org/3010054/diff/15001/16007#newcode592
chrome/browser/renderer_host/render_widget_host_view_mac.mm:592: continue;  //
Skip plugin views.
On 2010/08/11 16:40:44, stuartmorgan wrote:
> s/plugin/accelerated/

Will fix.

http://codereview.chromium.org/3010054/diff/15001/16007#newcode797
chrome/browser/renderer_host/render_widget_host_view_mac.mm:797:
CHECK(plugin_views_.end() != it);
On 2010/08/11 16:40:44, stuartmorgan wrote:
> Again, why CHECK?

Will change.

http://codereview.chromium.org/3010054/diff/15001/16007#newcode808
chrome/browser/renderer_host/render_widget_host_view_mac.mm:808:
CHECK(plugin_views_.end() != it);
On 2010/08/11 16:40:44, stuartmorgan wrote:
> And here, and below, and in DrawAcceleratedSurfaceInstance.

Will change.

http://codereview.chromium.org/3010054/diff/15001/16007#newcode2143
chrome/browser/renderer_host/render_widget_host_view_mac.mm:2143: // We only
want to do this for real browser views, not popups.
On 2010/08/11 16:40:44, stuartmorgan wrote:
> Move all this comment to just above the |if (newWindow != lastWindow_) {| so
> that it is associated with the code it's actually describing.

Will do.

[Nico, 2010-08-19 15:41:50]

On Wed, Aug 11, 2010 at 9:40 AM,  <stuartmorgan@chromium.org> wrote:
> Some cleanup for when you get back.
>
> Was there some reason it was critical to land this before you left? It would
> have been nice if the change had been polished beyond the proof-of-concept
> level before landing it.

I wanted to make sure that there's a dev channel release that has this
change but not the hole-cutting stuff (which I'm hoping to land today
or tomorrow).

Sorry for the mess; thanks Ken for cleaning up after me.

>
>
> http://codereview.chromium.org/3010054/diff/15001/16001
> File chrome/browser/renderer_host/accelerated_surface_container_mac.cc
> (left):
>
> http://codereview.chromium.org/3010054/diff/15001/16001#oldcode65
> chrome/browser/renderer_host/accelerated_surface_container_mac.cc:65:
> void AcceleratedSurfaceContainerMac::MoveTo(
> MoveTo is a poor name for this function at this point, since it has no
> concept of position any more. SetGeometry?
>
> http://codereview.chromium.org/3010054/diff/15001/16003
> File
> chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc
> (right):
>
> http://codereview.chromium.org/3010054/diff/15001/16003#newcode80
> chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc:80:
> // context associated with the CAOpenGLLayer is destroyed. However,
> Comment is no longer right.
>
> http://codereview.chromium.org/3010054/diff/15001/16003#newcode96
> chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc:96:
> glColorMask(true, true, true, true);
> The division between this method and Draw seems pretty arbitrary now
> that it's only painting one thing. Is there a reason not to push all
> this GL code into Draw()?
>
> http://codereview.chromium.org/3010054/diff/15001/16004
> File
> chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h
> (right):
>
> http://codereview.chromium.org/3010054/diff/15001/16004#newcode39
> chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h:39:
> // short-circuit the normal drawing process.
> This comment pretty clearly doesn't match the function any more.
>
> http://codereview.chromium.org/3010054/diff/15001/16004#newcode60
> chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h:60:
> // context, which must already be current.
> Again, the comment is now completely wrong.
>
> http://codereview.chromium.org/3010054/diff/15001/16006
> File chrome/browser/renderer_host/render_widget_host_view_mac.h (left):
>
> http://codereview.chromium.org/3010054/diff/15001/16006#oldcode141
> chrome/browser/renderer_host/render_widget_host_view_mac.h:141: -
> (void)drawAcceleratedPluginLayer;
> This method is now completely unused, and needs to be removed.
>
> http://codereview.chromium.org/3010054/diff/15001/16006
> File chrome/browser/renderer_host/render_widget_host_view_mac.h (right):
>
> http://codereview.chromium.org/3010054/diff/15001/16006#newcode245
> chrome/browser/renderer_host/render_widget_host_view_mac.h:245: //
> Informs the plug-in instances that their drawing context has changed.
> Comment and function are now mismatched.
>
> http://codereview.chromium.org/3010054/diff/15001/16007
> File chrome/browser/renderer_host/render_widget_host_view_mac.mm
> (right):
>
> http://codereview.chromium.org/3010054/diff/15001/16007#newcode176
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:176: static
> CVReturn MyDisplayLinkCallback(
> Can we give this a better name please?
>
> http://codereview.chromium.org/3010054/diff/15001/16007#newcode436
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:436:
> CHECK(plugin_views_.end() != it);
> CHECK? Really? Crashing end-users doesn't seem like a great failure mode
> (as opposed to say, DCHECK + |continue;|)
>
> http://codereview.chromium.org/3010054/diff/15001/16007#newcode592
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:592:
> continue;  // Skip plugin views.
> s/plugin/accelerated/
>
> http://codereview.chromium.org/3010054/diff/15001/16007#newcode797
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:797:
> CHECK(plugin_views_.end() != it);
> Again, why CHECK?
>
> http://codereview.chromium.org/3010054/diff/15001/16007#newcode808
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:808:
> CHECK(plugin_views_.end() != it);
> And here, and below, and in DrawAcceleratedSurfaceInstance.
>
> http://codereview.chromium.org/3010054/diff/15001/16007#newcode2143
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:2143: // We
> only want to do this for real browser views, not popups.
> Move all this comment to just above the |if (newWindow != lastWindow_)
> {| so that it is associated with the code it's actually describing.
>
> http://codereview.chromium.org/3010054/show
>