Remove browser-side tracking of accelerated compositing

Remove browser-side tracking of accelerated compositing

From the browser-side of things, compositing is always on, except for layout
tests.  The places that do test the compositing state on the browser side are
irrelevant for layout tests and/or redundant with existing state (e.g. whether
we received a frame).

BUG=None

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

[piman, 2014-05-09 23:13:43]

danakj: please review
tsepez: security for content/common/view_messages.h (removing one IPC).

[Tom Sepez, 2014-05-10 17:31:31]

Rubberstamp LGTM on deleting messages.

[danakj, 2014-05-12 17:25:07]

While compositing is always on, one thing is, when moving from one page to
another, Blink tells RenderWidget that it is "deactivating compositing" and does
SetDeferCommits to stop the compositor until the next page is loaded so we don't
paint and waste cycles during the load.

This "deactivating compositing" ends up in sending this message to the browser.
Maybe the browser doesn't need to care tho because it just means the renderer
isn't sending a frame, and it'll know that by not getting a frame. :)

But just wanted to mention that. *back to reading cl*

[danakj, 2014-05-12 17:37:58]

Super happy if we can do this, a few more direct questions/concerns:

https://codereview.chromium.org/274163004/diff/20001/content/browser/composit...
File content/browser/compositor/delegated_frame_host.cc (left):

https://codereview.chromium.org/274163004/diff/20001/content/browser/composit...
content/browser/compositor/delegated_frame_host.cc:103: if
(!host->is_accelerated_compositing_active())
Does this mean if you tried to resize during a slow page load, you'd resize lock
until the new site loaded while we're showing blank? Or are there any
implications to this wrt page load that we need to consider?

https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:1236:
is_accelerated_compositing_active_ = false;
Should we be dropping the frame on the RWHV here to make sure we don't try to do
a readback of those resources when the renderer is gone and the resources may be
destroyed? This bool used to guard that case, at least, sorta?

https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:249: //
Using accelerated compositing, but a compositing surface might not be
Update this comment?

https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:1214:
callback.Run(false, SkBitmap());
should we NOTREACHED? or is this a valid case

[piman, 2014-05-13 20:25:40]

https://codereview.chromium.org/274163004/diff/20001/content/browser/composit...
File content/browser/compositor/delegated_frame_host.cc (left):

https://codereview.chromium.org/274163004/diff/20001/content/browser/composit...
content/browser/compositor/delegated_frame_host.cc:103: if
(!host->is_accelerated_compositing_active())
On 2014/05/12 17:37:58, danakj wrote:
> Does this mean if you tried to resize during a slow page load, you'd resize
lock
> until the new site loaded while we're showing blank? Or are there any
> implications to this wrt page load that we need to consider?

It is actually a desirable outcome. The resize lock is on a (short) timeout. If
the page finishes loading during that time, and is able to produce a frame of
the right size, we'd want to give it a chance.

https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:1236:
is_accelerated_compositing_active_ = false;
On 2014/05/12 17:37:58, danakj wrote:
> Should we be dropping the frame on the RWHV here to make sure we don't try to
do
> a readback of those resources when the renderer is gone and the resources may
be
> destroyed? This bool used to guard that case, at least, sorta?

RenderProcessGone below will destroy the RWHV anyway.

https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_browsertest.cc
(right):

https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_browsertest.cc:249: //
Using accelerated compositing, but a compositing surface might not be
On 2014/05/12 17:37:58, danakj wrote:
> Update this comment?

Done.

https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:1214:
callback.Run(false, SkBitmap());
On 2014/05/12 17:37:58, danakj wrote:
> should we NOTREACHED? or is this a valid case

Won't we get here if we CopyFromCompositingSurface before we received any frame?

[danakj, 2014-05-13 21:23:43]

LGTM

https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:1214:
callback.Run(false, SkBitmap());
On 2014/05/13 20:25:40, piman wrote:
> On 2014/05/12 17:37:58, danakj wrote:
> > should we NOTREACHED? or is this a valid case
> 
> Won't we get here if we CopyFromCompositingSurface before we received any
frame?

Only if you didn't check IsSurfaceAvailableForCopy() first, which I would expect
us to.

But maybe not, RendererOverridesHandler::InnerSwapCompositorFrame seems to not
check first. Tho maybe it doesn't need to.

And I guess RenderWidgetHostImpl::CopyFromBackingStore doesn't check first. Huh.

[piman, 2014-05-14 23:52:46]

On 2014/05/13 21:23:43, danakj wrote:
> LGTM
> 
>
https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_view_mac.mm (right):
> 
>
https://codereview.chromium.org/274163004/diff/20001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_mac.mm:1214:
> callback.Run(false, SkBitmap());
> On 2014/05/13 20:25:40, piman wrote:
> > On 2014/05/12 17:37:58, danakj wrote:
> > > should we NOTREACHED? or is this a valid case
> > 
> > Won't we get here if we CopyFromCompositingSurface before we received any
> frame?
> 
> Only if you didn't check IsSurfaceAvailableForCopy() first, which I would
expect
> us to.
> 
> But maybe not, RendererOverridesHandler::InnerSwapCompositorFrame seems to not
> check first. Tho maybe it doesn't need to.
> 
> And I guess RenderWidgetHostImpl::CopyFromBackingStore doesn't check first.
Huh.

Yeah, actually, I think we can move towards the opposite, and remove
IsSurfaceAvailableForCopy. I think it's not needed any more, since we deliver
failure synchronously from CopyFromCompositingSurface (and we don't have a
fallback anyway).

[piman, 2014-05-14 23:53:42]

BTW, this needs https://codereview.chromium.org/286953002/ to avoid test
flakiness that was masked by the previous behavior.

[piman, 2014-05-15 21:39:45]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-05-15 21:40:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/piman@chromium.org/274163004/140001

[commit-bot: I haz the power, 2014-05-15 23:42:21]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-05-16 00:44:16]

Change committed as 270878