Stop disabling force_compositing_mode for background RenderViews.

Stop disabling force_compositing_mode for background RenderViews.

When a RenderView's WebContents are a VIEW_TYPE_BACKGROUND_CONTENTS or
a VIEW_TYPE_EXTENSION_BACKGROUND_PAGE, the RenderView is in the
background and never visible.

Currently we disable force_compositing_mode to prevent allocating a
GPU context for the view. Since force_compositing_mode is the standard
and we are deleting other code paths, we should stop trying to disable
the setting.

Instead, add a new parameter to the ViewMsg_New IPC for |background|,
and for a RenderView that |is_background|, when the RenderWidget makes
an OutputSurface for the compositor, it creates a new NullOutputSurface
which declares itself as a delegated compositor without a GPU context.
This puts the compositor into a similar state to software compositing,
but since the renderer never presents frames, the OutputSurface
provides no way of getting frames to the browser process.

R=jam@chromium.org, piman@chromium.org
BUG=362165
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264075

[danakj, 2014-04-10 19:55:40]

Hi, please tell me if I've done anything crazy/wrong here :)

[jam, 2014-04-10 20:27:48]

just a nit regarding not bringing in the "background" concept to src\content.
also do this for all the parameters now that you added (never_visible or
not_visible instead of background)

other than that, i defer to piman who's an owner in content

https://codereview.chromium.org/233093006/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/233093006/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:524: virtual bool
IsRenderViewHostForBackground(RenderViewHost* render_view_host);
nit: content doesn't know about "background" concept, that's an extensions
feature. so maybe call this IsRenderViewHostNeverVisible or something similar?

https://codereview.chromium.org/233093006/diff/20001/content/renderer/render_...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/233093006/diff/20001/content/renderer/render_...
content/renderer/render_widget.h:624: // Indeicates that we run in the
background and are never visible.
nit: spelling

[danakj, 2014-04-10 20:37:12]

Thanks, never_visible is nice, and doesn't confuse things with background tabs
:)

https://codereview.chromium.org/233093006/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/233093006/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:524: virtual bool
IsRenderViewHostForBackground(RenderViewHost* render_view_host);
On 2014/04/10 20:27:48, jam wrote:
> nit: content doesn't know about "background" concept, that's an extensions
> feature. so maybe call this IsRenderViewHostNeverVisible or something similar?

Done.

https://codereview.chromium.org/233093006/diff/20001/content/renderer/render_...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/233093006/diff/20001/content/renderer/render_...
content/renderer/render_widget.h:624: // Indeicates that we run in the
background and are never visible.
On 2014/04/10 20:27:48, jam wrote:
> nit: spelling

Done.

[piman, 2014-04-10 21:27:18]

LGTM.
Does this prevent the compositor from allocating shm for tiles and stuff? Or is
it a side effect of us never trying to commit/draw (is that true?)

[danakj, 2014-04-10 22:01:47]

On 2014/04/10 21:27:18, piman wrote:
> LGTM.
> Does this prevent the compositor from allocating shm for tiles and stuff? Or
is
> it a side effect of us never trying to commit/draw (is that true?)

Good question.. I am not sure what background pages do, if they have a 0,0
viewport then they'd not do anything like that. If they don't I think we'd
allocate and everything during the commit/activate stages, and scheduler would
be trying to draw at 60hz, which would be a no-op.

I'm not sure if this is okay or not.

[danakj, 2014-04-10 22:12:39]

On 2014/04/10 22:01:47, danakj wrote:
> On 2014/04/10 21:27:18, piman wrote:
> > LGTM.
> > Does this prevent the compositor from allocating shm for tiles and stuff? Or
> is
> > it a side effect of us never trying to commit/draw (is that true?)
> 
> Good question.. I am not sure what background pages do, if they have a 0,0
> viewport then they'd not do anything like that. If they don't I think we'd
> allocate and everything during the commit/activate stages, and scheduler would
> be trying to draw at 60hz, which would be a no-op.

I guess the lack of swap acks would throttle the drawing to 0, but we'd have
painted things if there's a non-empty viewport, I think.

> I'm not sure if this is okay or not.

[danakj, 2014-04-10 22:20:53]

On 2014/04/10 22:12:39, danakj wrote:
> On 2014/04/10 22:01:47, danakj wrote:
> > On 2014/04/10 21:27:18, piman wrote:
> > > LGTM.
> > > Does this prevent the compositor from allocating shm for tiles and stuff?
Or
> > is
> > > it a side effect of us never trying to commit/draw (is that true?)
> > 
> > Good question.. I am not sure what background pages do.

Offline Gmail background pages do not call SetViewportSize, leaving it 0x0. It
has a single layer that is 0x0 also, it ends up doing 3 commits, but we won't be
painting anything for it.

WDYT?

[vangelis, 2014-04-10 23:13:29]

On 2014/04/10 22:20:53, danakj wrote:
> On 2014/04/10 22:12:39, danakj wrote:
> > On 2014/04/10 22:01:47, danakj wrote:
> > > On 2014/04/10 21:27:18, piman wrote:
> > > > LGTM.
> > > > Does this prevent the compositor from allocating shm for tiles and
stuff?
> Or
> > > is
> > > > it a side effect of us never trying to commit/draw (is that true?)
> > > 
> > > Good question.. I am not sure what background pages do.
> 
> Offline Gmail background pages do not call SetViewportSize, leaving it 0x0. It
> has a single layer that is 0x0 also, it ends up doing 3 commits, but we won't
be
> painting anything for it.
> 
> WDYT?

Would we preemptively create a compositor context for them, even if they do no
drawing?

[piman, 2014-04-10 23:20:59]

On Thu, Apr 10, 2014 at 4:13 PM, <vangelis@google.com> wrote:

> On 2014/04/10 22:20:53, danakj wrote:
>
>> On 2014/04/10 22:12:39, danakj wrote:
>> > On 2014/04/10 22:01:47, danakj wrote:
>> > > On 2014/04/10 21:27:18, piman wrote:
>> > > > LGTM.
>> > > > Does this prevent the compositor from allocating shm for tiles and
>>
> stuff?
>
>> Or
>> > > is
>> > > > it a side effect of us never trying to commit/draw (is that true?)
>> > >
>> > > Good question.. I am not sure what background pages do.
>>
>
>  Offline Gmail background pages do not call SetViewportSize, leaving it
>> 0x0. It
>> has a single layer that is 0x0 also, it ends up doing 3 commits, but we
>> won't
>>
> be
>
>> painting anything for it.
>>
>
>  WDYT?
>>
>
> Would we preemptively create a compositor context for them, even if they
> do no
> drawing?
>

This patch makes it that no (NullOutputSurface).

>
> https://codereview.chromium.org/233093006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-04-11 18:06:39]

sorry should have mentioned this earlier, but it strikes me now that this should
be a callback on WebContentsDelegate instead of ContentBrowserClient. There are
two reasons:
-this is really a property of the webcontents
-since we're moving away from RenderViewHost, this will make that transition
invisible to chrome for this API

[danakj, 2014-04-14 22:19:54]

Ok patch set 6 does two things:

- DCHECKs and ensures background WebContents (NeverVisible) also start out
initially hidden.
- Moves the check for IsNeverVisible to the WebContentsDelegate (and the
ExtensionHost and BackgroundContents implementation classes).

PTAL jam@/piman@

[danakj, 2014-04-14 22:22:44]

+cevans for view_messages.h (adding param to tell renderer it will never be
shown, allow it to drop code/infrastructure needed for SwapBuffers)

[danakj, 2014-04-14 22:22:56]

+mek for extensions code

[Marijn Kruisselbrink, 2014-04-14 22:28:05]

On 2014/04/14 22:22:56, danakj wrote:
> +mek for extensions code
extensions code lgtm

[danakj, 2014-04-14 22:36:54]

+avi for chrome/browser/tab_contents/

[piman, 2014-04-14 22:40:26]

lgtm

[jam, 2014-04-14 22:43:01]

lgtm with comments

https://codereview.chromium.org/233093006/diff/100001/chrome/browser/tab_cont...
File chrome/browser/tab_contents/background_contents.cc (right):

https://codereview.chromium.org/233093006/diff/100001/chrome/browser/tab_cont...
chrome/browser/tab_contents/background_contents.cc:130:
extensions::GetViewType(web_contents));
this seems like a redundant dcheck. i would prefer we take it out. i.e. if we
can't trust that random code isn't changing the view type of webcontents in
chrome, then all hope is lost.

https://codereview.chromium.org/233093006/diff/100001/chrome/browser/tab_cont...
File chrome/browser/tab_contents/background_contents.h (right):

https://codereview.chromium.org/233093006/diff/100001/chrome/browser/tab_cont...
chrome/browser/tab_contents/background_contents.h:69: OVERRIDE;
nit: don't start a line with OVERRIDE. see recent chromium-dev discussion. i
think we're waiting on a new version of clang format which fixes this

https://codereview.chromium.org/233093006/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.cc (right):

https://codereview.chromium.org/233093006/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.cc:42: bool
RenderViewHostDelegate::IsNeverVisible() { return false; }
nit: put on a new line (see same cr-dev thread)

https://codereview.chromium.org/233093006/diff/100001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/233093006/diff/100001/content/public/browser/...
content/public/browser/web_contents_delegate.h:465: virtual bool
IsWebContentsNeverVisible(WebContents* web_contents);
nit: since this is on WebContentsDelegate, the "WebContents" in the method name
is redundant. just call it IsNeverVisible

[danakj, 2014-04-14 22:47:29]

https://codereview.chromium.org/233093006/diff/100001/chrome/browser/tab_cont...
File chrome/browser/tab_contents/background_contents.cc (right):

https://codereview.chromium.org/233093006/diff/100001/chrome/browser/tab_cont...
chrome/browser/tab_contents/background_contents.cc:130:
extensions::GetViewType(web_contents));
On 2014/04/14 22:43:01, jam wrote:
> this seems like a redundant dcheck. i would prefer we take it out. i.e. if we
> can't trust that random code isn't changing the view type of webcontents in
> chrome, then all hope is lost.

To me it's not so much about trust but to document the expectations of this
function and why it can return true, in a way that will have to be updated if
things changed in the future. I'd prefer to keep it if you don't feel strongly
about removing it.

https://codereview.chromium.org/233093006/diff/100001/chrome/browser/tab_cont...
File chrome/browser/tab_contents/background_contents.h (right):

https://codereview.chromium.org/233093006/diff/100001/chrome/browser/tab_cont...
chrome/browser/tab_contents/background_contents.h:69: OVERRIDE;
On 2014/04/14 22:43:01, jam wrote:
> nit: don't start a line with OVERRIDE. see recent chromium-dev discussion. i
> think we're waiting on a new version of clang format which fixes this

Yup. Will fix.

https://codereview.chromium.org/233093006/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.cc (right):

https://codereview.chromium.org/233093006/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.cc:42: bool
RenderViewHostDelegate::IsNeverVisible() { return false; }
On 2014/04/14 22:43:01, jam wrote:
> nit: put on a new line (see same cr-dev thread)

Done.

https://codereview.chromium.org/233093006/diff/100001/content/public/browser/...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/233093006/diff/100001/content/public/browser/...
content/public/browser/web_contents_delegate.h:465: virtual bool
IsWebContentsNeverVisible(WebContents* web_contents);
On 2014/04/14 22:43:01, jam wrote:
> nit: since this is on WebContentsDelegate, the "WebContents" in the method
name
> is redundant. just call it IsNeverVisible

Done.

[kenrb, 2014-04-15 15:28:29]

lgtm for IPC security review.

[danakj, 2014-04-15 17:55:16]

This change appears to break the
EphemeralAppBrowserTest.ReceiveMessagesWhenLaunched because the background
renderer now thinking it is not visible seems to mean that setTimeout callbacks
are not firing.

This scares me that it will break all the background apps ever.

[danakj, 2014-04-15 17:57:37]

On 2014/04/15 17:55:16, danakj wrote:
> This change appears to break the
> EphemeralAppBrowserTest.ReceiveMessagesWhenLaunched because the background
> renderer now thinking it is not visible seems to mean that setTimeout
callbacks
> are not firing.
> 
> This scares me that it will break all the background apps ever.

Supposedly chrome runs timers at 1second intervals on background pages, but that
doesn't seem to be working here. I'll continue to dig as I can.

[danakj, 2014-04-15 19:57:58]

So, IsNeverVisible and IsHidden need to be orthogonal things (at least for this
patch set where we're just enabling FCM).

What I've done here is removed the changes to set NeverVisible views as
initially_hidden, preserving their current behaviour.

But in RenderWidget, when NeverVisible, I avoid calling
compositor_->setSurfaceReady(), which means that cc's scheduler never starts up,
and we never try to produce frames, or call Layout().

[danakj, 2014-04-15 20:50:50]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-04-15 20:51:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/233093006/210001

[piman, 2014-04-15 21:21:33]

lgtm

[commit-bot: I haz the power, 2014-04-16 01:25:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/233093006/210001

[commit-bot: I haz the power, 2014-04-16 02:37:07]

Change committed as 264075