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

Cleanups to well-behaved accelerated plugin CL requested by
stuartmorgan in http://codereview.chromium.org/3010054 .

BUG=51748
TEST=visited YouTube and ensured video shows up; 3D CSS demos; WebGL demos


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

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

Please review. Thanks.

[stuartmorgan, 2010-08-16 15:22:35]

Just a few small things... thanks for doing this!

http://codereview.chromium.org/3185004/diff/6001/7004
File chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h
(right):

http://codereview.chromium.org/3185004/diff/6001/7004#newcode37
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h:37: //
Indicates whether the given PluginWindowHandle is a "root" one, which
s/is a "root" one"/is "root"/ ?

http://codereview.chromium.org/3185004/diff/6001/7005
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (left):

http://codereview.chromium.org/3185004/diff/6001/7005#oldcode1345
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1345:
[acceleratedPluginLayer_.get() setNeedsDisplay];
Why is this being removed?

http://codereview.chromium.org/3185004/diff/6001/7005#oldcode1470
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1470:
[acceleratedPluginLayer_.get() setNeedsDisplay];
Same question here.

http://codereview.chromium.org/3185004/diff/6001/7005
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3185004/diff/6001/7005#newcode806
chrome/browser/renderer_host/render_widget_host_view_mac.mm:806: if
(plugin_views_.end() != it) {
Probably cleaner to flip the logic and do an early return.

http://codereview.chromium.org/3185004/diff/6001/7005#newcode821
chrome/browser/renderer_host/render_widget_host_view_mac.mm:821: if
(plugin_views_.end() != it) {
Doubly so here.

[Ken Russell (switch to Gerrit), 2010-08-16 21:59:22]

http://codereview.chromium.org/3185004/diff/6001/7004
File chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h
(right):

http://codereview.chromium.org/3185004/diff/6001/7004#newcode37
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h:37: //
Indicates whether the given PluginWindowHandle is a "root" one, which
On 2010/08/16 15:22:35, stuartmorgan wrote:
> s/is a "root" one"/is "root"/ ?

Done.

http://codereview.chromium.org/3185004/diff/6001/7005
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (left):

http://codereview.chromium.org/3185004/diff/6001/7005#oldcode1345
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1345:
[acceleratedPluginLayer_.get() setNeedsDisplay];
On 2010/08/16 15:22:35, stuartmorgan wrote:
> Why is this being removed?

The CALayer which was previously used for accelerated surface rendering has been
removed. These sends are going to nil. I've removed the acceleratedPluginLayer_
member as well. (I just realized that the CL didn't include my changes to
render_widget_host_view_mac.h; now fixed.)

http://codereview.chromium.org/3185004/diff/6001/7005#oldcode1470
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1470:
[acceleratedPluginLayer_.get() setNeedsDisplay];
On 2010/08/16 15:22:35, stuartmorgan wrote:
> Same question here.

See above.

http://codereview.chromium.org/3185004/diff/6001/7005
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3185004/diff/6001/7005#newcode806
chrome/browser/renderer_host/render_widget_host_view_mac.mm:806: if
(plugin_views_.end() != it) {
On 2010/08/16 15:22:35, stuartmorgan wrote:
> Probably cleaner to flip the logic and do an early return.

Done.

http://codereview.chromium.org/3185004/diff/6001/7005#newcode821
chrome/browser/renderer_host/render_widget_host_view_mac.mm:821: if
(plugin_views_.end() != it) {
On 2010/08/16 15:22:35, stuartmorgan wrote:
> Doubly so here.

Done.

[stuartmorgan, 2010-08-16 22:38:18]

LGTM

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

Thanks for doing this!

http://codereview.chromium.org/3185004/diff/13002/17006
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3185004/diff/13002/17006#newcode437
chrome/browser/renderer_host/render_widget_host_view_mac.mm:437:
DCHECK(plugin_views_.end() != it);
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]

http://codereview.chromium.org/3185004/diff/13002/17006
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3185004/diff/13002/17006#newcode437
chrome/browser/renderer_host/render_widget_host_view_mac.mm:437:
DCHECK(plugin_views_.end() != it);
On 2010/08/19 22:24:22, Nico wrote:
> 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...

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]

http://codereview.chromium.org/3185004/diff/13002/17006
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3185004/diff/13002/17006#newcode437
chrome/browser/renderer_host/render_widget_host_view_mac.mm:437:
DCHECK(plugin_views_.end() != it);
On 2010/08/19 22:24:22, Nico wrote:
> 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...

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.