Cleanups to well-behaved accelerated plugin CL requested by...

chrome/browser/renderer_host/render_widget_host_view_mac.mm

Index: chrome/browser/renderer_host/render_widget_host_view_mac.mm
===================================================================
--- chrome/browser/renderer_host/render_widget_host_view_mac.mm	(revision 56221)
+++ chrome/browser/renderer_host/render_widget_host_view_mac.mm	(working copy)
@@ -173,7 +173,7 @@
 }
 
 // This is the renderer output callback function
-static CVReturn MyDisplayLinkCallback(
+static CVReturn DrawOneAcceleratedPluginCallback(
     CVDisplayLinkRef displayLink,
     const CVTimeStamp* now,
     const CVTimeStamp* outputTime,
@@ -211,7 +211,8 @@
 
     // Set up a display link to do OpenGL rendering on a background thread.
     CVDisplayLinkCreateWithActiveCGDisplays(&displayLink_);
-    CVDisplayLinkSetOutputCallback(displayLink_, &MyDisplayLinkCallback, self);
+    CVDisplayLinkSetOutputCallback(displayLink_,
+        &DrawOneAcceleratedPluginCallback, self);
     CVDisplayLinkSetCurrentCGDisplayFromOpenGLContext(
         displayLink_, cglContext_, cglPixelFormat_);
     CVDisplayLinkStart(displayLink_);
@@ -433,12 +434,15 @@
       }
 
       PluginViewMap::iterator it = plugin_views_.find(geom.window);
-      CHECK(plugin_views_.end() != it);
+      DCHECK(plugin_views_.end() != it);

[Nico, 2010/08/19 22:24:22]

what's the rationale for replacing these CHECKs with DCHECKs and early returns?
If this is ever hit, this is a serious problem.

See
http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

[Ken Russell (switch to Gerrit), 2010/08/19 22:28:47]

Stuart was concerned about the use of CHECK. I could go either way. There might
be slight race conditions during page shutdown that we might want to handle more
gracefully. From my standpoint, feel free to strengthen these if stress testing
indicates that CHECKs don't crash.

[stuartmorgan, 2010/08/19 22:35:59]

From that document:

"Sometimes it is preferable to force a crash to happen via CHECK. Those cases
include situations where you are dealing with untrusted input (provided there is
no reasonable recovery path) and the consequences of continuing execution could
result in a security vulnerability."

What is the security vulnerability of just ignoring an attempt to move a plugin
that doesn't exist? I can't see what the problem is here that is so bad that
crashing the user's browser is our most graceful option.

I'm fine with DLOG instead of DCHECK, but either seems far, far better than
CHECK.

+      if (plugin_views_.end() == it) {
+        continue;
+      }
       NSRect new_rect([cocoa_view_ RectToNSRect:rect]);
       [it->second setFrame:new_rect];
       [it->second setNeedsDisplay:YES];
 
-      plugin_container_manager_.MovePluginContainer(geom);
+      plugin_container_manager_.SetPluginContainerGeometry(geom);
     }
   }
 }
@@ -589,7 +593,7 @@
     // deepest-first ordering.
     for (NSView* subview in [cocoa_view_ subviews]) {
       if (![subview isKindOfClass:[RenderWidgetHostViewCocoa class]])
-        continue;  // Skip plugin views.
+        continue;  // Skip accelerated views.
 
       [static_cast<RenderWidgetHostViewCocoa*>(subview)
           renderWidgetHostViewMac]->ShutdownHost();
@@ -754,10 +758,12 @@
 void RenderWidgetHostViewMac::DestroyFakePluginWindowHandle(
     gfx::PluginWindowHandle window) {
   PluginViewMap::iterator it = plugin_views_.find(window);
-  CHECK(plugin_views_.end() != it);
+  DCHECK(plugin_views_.end() != it);
+  if (plugin_views_.end() == it) {
+    return;
+  }
   [it->second removeFromSuperview];
   plugin_views_.erase(it);
-
   plugin_container_manager_.DestroyFakePluginWindowHandle(window);
 }
 
@@ -766,9 +772,6 @@
     int32 width,
     int32 height,
     uint64 io_surface_identifier) {
-  PluginViewMap::iterator it = plugin_views_.find(window);
-  CHECK(plugin_views_.end() != it);
-
   plugin_container_manager_.SetSizeAndIOSurface(window,
                                                 width,
                                                 height,
@@ -793,9 +796,6 @@
     int32 width,
     int32 height,
     TransportDIB::Handle transport_dib) {
-  PluginViewMap::iterator it = plugin_views_.find(window);
-  CHECK(plugin_views_.end() != it);
-
   plugin_container_manager_.SetSizeAndTransportDIB(window,
                                                    width,
                                                    height,
@@ -805,10 +805,14 @@
 void RenderWidgetHostViewMac::AcceleratedSurfaceBuffersSwapped(
     gfx::PluginWindowHandle window) {
   PluginViewMap::iterator it = plugin_views_.find(window);
-  CHECK(plugin_views_.end() != it);
-  CHECK([it->second isKindOfClass:[AcceleratedPluginView class]]);
+  DCHECK(plugin_views_.end() != it);
+  if (plugin_views_.end() == it) {
+    return;
+  }
+  DCHECK([it->second isKindOfClass:[AcceleratedPluginView class]]);
 
-  AcceleratedPluginView* view = static_cast<AcceleratedPluginView*>(it->second);
+  AcceleratedPluginView* view =
+      static_cast<AcceleratedPluginView*>(it->second);
   [view setHidden:NO];
   [view setSurfaceWasSwapped:YES];
 }
@@ -817,8 +821,10 @@
       CGLContextObj context, gfx::PluginWindowHandle plugin_handle) {
   // Called on the display link thread.
   PluginViewMap::iterator it = plugin_views_.find(plugin_handle);
-  CHECK(plugin_views_.end() != it);
-
+  DCHECK(plugin_views_.end() != it);
+  if (plugin_views_.end() == it) {
+    return;
+  }
   CGLSetCurrentContext(context);
   // TODO(thakis): Pixel or view coordinates?
   NSSize size = [it->second frame].size;
@@ -832,7 +838,9 @@
   glLoadIdentity();
 
   plugin_container_manager_.Draw(
-      context, plugin_handle, GetRenderWidgetHost()->is_gpu_rendering_active());
+      context,
+      plugin_handle,
+      GetRenderWidgetHost()->is_gpu_rendering_active());
 }
 
 void RenderWidgetHostViewMac::ForceTextureReload() {
@@ -1338,11 +1346,6 @@
   }
 
   if (renderWidgetHostView_->render_widget_host_->is_gpu_rendering_active()) {
-    // In this mode the accelerated plugin layer is considered to be
-    // opaque. We do not want its contents to be blended with anything
-    // underneath it.
-    acceleratedPluginLayer_.get().opaque = YES;
-    [acceleratedPluginLayer_.get() setNeedsDisplay];
     return;
   }
 
@@ -1459,15 +1462,6 @@
     if (renderWidgetHostView_->whiteout_start_time_.is_null())
       renderWidgetHostView_->whiteout_start_time_ = base::TimeTicks::Now();
   }
-
-  // If we get here then the accelerated plugin layer is not supposed
-  // to be considered opaque -- plugins overlay the browser's normal
-  // painting.
-  acceleratedPluginLayer_.get().opaque = NO;
-
-  // This helps keep accelerated plugins' output in better sync with the
-  // window as it resizes.
-  [acceleratedPluginLayer_.get() setNeedsDisplay];
 }
 
 - (BOOL)canBecomeKeyView {
@@ -2137,15 +2131,15 @@
 }
 
 - (void)viewDidMoveToWindow {
-  // If we move into a new window, refresh the frame information. We don't need
-  // to do it if it was the same window as it used to be in, since that case
-  // is covered by DidBecomeSelected.
-  // We only want to do this for real browser views, not popups.
   if (canBeKeyView_) {
     NSWindow* newWindow = [self window];
     // Pointer comparison only, since we don't know if lastWindow_ is still
     // valid.
     if (newWindow) {
+      // If we move into a new window, refresh the frame information. We
+      // don't need to do it if it was the same window as it used to be in,
+      // since that case is covered by DidBecomeSelected. We only want to
+      // do this for real browser views, not popups.
       if (newWindow != lastWindow_) {
         lastWindow_ = newWindow;
         renderWidgetHostView_->WindowFrameChanged();
@@ -2216,10 +2210,6 @@
   }
 }
 
-- (void)drawAcceleratedPluginLayer {
-  [acceleratedPluginLayer_.get() setNeedsDisplay];
-}
-
 - (void)cancelComposition {
   if (!hasMarkedText_)
     return;