Enable browser plugin compositing by default, but add a flag to disable it.

Enable browser plugin compositing by default, but add a flag to disable it.


BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186794

[alexst (slow to review), 2013-03-07 15:13:33]

Fady brought up enabling webview compositing by default in Dev with a switch to
turn it off.

[Fady Samuel, 2013-03-07 15:21:43]

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_process_host_impl.cc (left):

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_process_host_impl.cc:767:
switches::kEnableBrowserPluginCompositing,
Plumbing DisableBrowserPluginCompositing is not necessary?

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_guest.cc (left):

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_guest.cc:125: if
(enable_compositing_) {
This is no longer necessary?

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_guest.cc:272: return
platform_view_->HasAcceleratedSurface(desired_size);
ditto.

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_guest.cc:175: return true;
Hmm, so this changes the behavior with both compositing enabled and
disabled...presumably this simply isn't called if compositing is disabled?

[alexst (slow to review), 2013-03-07 15:30:00]

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_process_host_impl.cc (left):

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_process_host_impl.cc:767:
switches::kEnableBrowserPluginCompositing,
On 2013/03/07 15:21:44, Fady Samuel wrote:
> Plumbing DisableBrowserPluginCompositing is not necessary?

I am pretty sure nobody is using it in the renderer any more.

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_guest.cc (left):

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_guest.cc:125: if
(enable_compositing_) {
On 2013/03/07 15:21:44, Fady Samuel wrote:
> This is no longer necessary?

I was working off the mistaken assumption that the whole buffer was there, and
just the damage rect was different. Right now, this won't work, so I am removing
it until we can find a fix.

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_guest.cc:272: return
platform_view_->HasAcceleratedSurface(desired_size);
On 2013/03/07 15:21:44, Fady Samuel wrote:
> ditto.

Shouldn't be called.

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_guest.cc:175: return true;
On 2013/03/07 15:21:44, Fady Samuel wrote:
> Hmm, so this changes the behavior with both compositing enabled and
> disabled...presumably this simply isn't called if compositing is disabled?

This shouldn't be called at all because I think it deals with the packing store.
In fact, I think this should be false.

[Fady Samuel, 2013-03-07 15:32:20]

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/12547016/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_guest.cc:175: return true;
On 2013/03/07 15:30:00, alexst wrote:
> On 2013/03/07 15:21:44, Fady Samuel wrote:
> > Hmm, so this changes the behavior with both compositing enabled and
> > disabled...presumably this simply isn't called if compositing is disabled?
> 
> This shouldn't be called at all because I think it deals with the packing
store.
> In fact, I think this should be false.
> 

If you're confident something shouldn't be called perhaps add a NOTREACHED() so
we catch it if things regress or there's a bug?

[alexst (slow to review), 2013-03-07 15:54:54]

> If you're confident something shouldn't be called perhaps add a NOTREACHED()
so
> we catch it if things regress or there's a bug?

95% confident. It's used in the thumbnailer, which should only deal with tabs.
I'll return false and put NOTIMPLEMENTED() as a lesser check.

[piman, 2013-03-07 16:53:59]

https://codereview.chromium.org/12547016/diff/7001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/chrome_restart_request.cc (left):

https://codereview.chromium.org/12547016/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/chrome_restart_request.cc:86:
::switches::kEnableBrowserPluginCompositing,
We want the disable flag here now.

[alexst (slow to review), 2013-03-07 17:55:49]

patch.

https://codereview.chromium.org/12547016/diff/7001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/chrome_restart_request.cc (left):

https://codereview.chromium.org/12547016/diff/7001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/chrome_restart_request.cc:86:
::switches::kEnableBrowserPluginCompositing,
On 2013/03/07 16:53:59, piman wrote:
> We want the disable flag here now.

Right, thank you.

[piman, 2013-03-07 17:59:41]

lgtm

https://codereview.chromium.org/12547016/diff/10001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/12547016/diff/10001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/chrome_restart_request.cc:86:
::switches::kDisableBrowserPluginCompositing,
nit: alpha order.

[zel, 2013-03-07 18:06:06]

lgtm

[Fady Samuel, 2013-03-07 18:06:19]

LGTM

[commit-bot: I haz the power, 2013-03-07 18:07:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexst@chromium.org/12547016/16001

[commit-bot: I haz the power, 2013-03-07 22:19:24]

Change committed as 186794