Disable partial swaps for webview guest renderer until we can figure out how to do that properly.

Disable partial swaps for webview guest renderer until we can figure out how to do that properly.


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

[Fady Samuel, 2013-03-05 19:00:21]

browser_plugin LGTM

[piman, 2013-03-05 20:32:37]

https://codereview.chromium.org/12463007/diff/1/ui/gfx/native_widget_types.h
File ui/gfx/native_widget_types.h (right):

https://codereview.chromium.org/12463007/diff/1/ui/gfx/native_widget_types.h#...
ui/gfx/native_widget_types.h:275: TEXTURE_TRANSPORT_NO_PARTIAL_SWAP
I'm really uneasy with passing feature flags through this type enum. How about
disabling partial swaps in the guest, by changing the LayerTreeSettings in
RenderWidgetCompositor::Create ?

[alexst (slow to review), 2013-03-05 21:09:28]

https://codereview.chromium.org/12463007/diff/1/ui/gfx/native_widget_types.h
File ui/gfx/native_widget_types.h (right):

https://codereview.chromium.org/12463007/diff/1/ui/gfx/native_widget_types.h#...
ui/gfx/native_widget_types.h:275: TEXTURE_TRANSPORT_NO_PARTIAL_SWAP
On 2013/03/05 20:32:37, piman wrote:
> I'm really uneasy with passing feature flags through this type enum. How about
> disabling partial swaps in the guest, by changing the LayerTreeSettings in


> RenderWidgetCompositor::Create ?


I'll need to go do some digging to find a good way to do that. I've written via
SurfaceType because it's one place I know something is a guest.

Currently a renderer itself doesn't know it's a webview, so I'll need to plumb
that information through, somehow.

I'll need a flag along the lines of do_partial_swap_but_render_everything_anyway
if we want to forward the damage rect to the embedder.

[Fady Samuel, 2013-03-05 21:53:10]

On 2013/03/05 21:09:28, alexst wrote:
> https://codereview.chromium.org/12463007/diff/1/ui/gfx/native_widget_types.h
> File ui/gfx/native_widget_types.h (right):
> 
>
https://codereview.chromium.org/12463007/diff/1/ui/gfx/native_widget_types.h#...
> ui/gfx/native_widget_types.h:275: TEXTURE_TRANSPORT_NO_PARTIAL_SWAP
> On 2013/03/05 20:32:37, piman wrote:
> > I'm really uneasy with passing feature flags through this type enum. How
about
> > disabling partial swaps in the guest, by changing the LayerTreeSettings in
> 
> 
> > RenderWidgetCompositor::Create ?
> 
> 
> I'll need to go do some digging to find a good way to do that. I've written
via
> SurfaceType because it's one place I know something is a guest.
> 
> Currently a renderer itself doesn't know it's a webview, so I'll need to plumb
> that information through, somehow.
> 
> I'll need a flag along the lines of
do_partial_swap_but_render_everything_anyway
> if we want to forward the damage rect to the embedder.

Generally, we've been trying to avoid writing guest-renderer-specific code to
avoid accidentally writing code that gives guest renderers more privileges than
they should have. Generally, a guest process should not know it's a guest
process. Only the browser process's treatment of it is different.

[piman, 2013-03-05 22:05:24]

On Tue, Mar 5, 2013 at 1:53 PM, <fsamuel@chromium.org> wrote:

> On 2013/03/05 21:09:28, alexst wrote:
>
>> https://codereview.chromium.**org/12463007/diff/1/ui/gfx/**
>>
native_widget_types.h<https://codereview.chromium.org/12463007/diff/1/ui/gfx/native_widget_types.h>
>> File ui/gfx/native_widget_types.h (right):
>>
>
>
> https://codereview.chromium.**org/12463007/diff/1/ui/gfx/**
>
native_widget_types.h#**newcode275<https://codereview.chromium.org/12463007/diff/1/ui/gfx/native_widget_types.h#newcode275>
>
>> ui/gfx/native_widget_types.h:**275: TEXTURE_TRANSPORT_NO_PARTIAL_**SWAP
>> On 2013/03/05 20:32:37, piman wrote:
>> > I'm really uneasy with passing feature flags through this type enum. How
>>
> about
>
>> > disabling partial swaps in the guest, by changing the LayerTreeSettings
>> in
>>
>
>
>  > RenderWidgetCompositor::Create ?
>>
>
>
>  I'll need to go do some digging to find a good way to do that. I've
>> written
>>
> via
>
>> SurfaceType because it's one place I know something is a guest.
>>
>
>  Currently a renderer itself doesn't know it's a webview, so I'll need to
>> plumb
>> that information through, somehow.
>>
>
>  I'll need a flag along the lines of
>>
> do_partial_swap_but_render_**everything_anyway
>
>> if we want to forward the damage rect to the embedder.
>>
>
> Generally, we've been trying to avoid writing guest-renderer-specific code
> to
> avoid accidentally writing code that gives guest renderers more privileges
> than
> they should have. Generally, a guest process should not know it's a guest
> process. Only the browser process's treatment of it is different.
>

In this case a guest needs to behave differently compared to a non-guest
(it isn't reasonable to disable partial swaps across the board). One way or
another some information has to flow to there.

Antoine


>
https://codereview.chromium.**org/12463007/<https://codereview.chromium.org/1...
>

[Fady Samuel, 2013-03-05 22:22:53]

On 2013/03/05 22:05:24, piman wrote:
> On Tue, Mar 5, 2013 at 1:53 PM, <mailto:fsamuel@chromium.org> wrote:
> 
> > On 2013/03/05 21:09:28, alexst wrote:
> >
> >> https://codereview.chromium.**org/12463007/diff/1/ui/gfx/**
> >>
>
native_widget_types.h<https://codereview.chromium.org/12463007/diff/1/ui/gfx/native_widget_types.h>
> >> File ui/gfx/native_widget_types.h (right):
> >>
> >
> >
> > https://codereview.chromium.**org/12463007/diff/1/ui/gfx/**
> >
>
native_widget_types.h#**newcode275<https://codereview.chromium.org/12463007/diff/1/ui/gfx/native_widget_types.h#newcode275>
> >
> >> ui/gfx/native_widget_types.h:**275: TEXTURE_TRANSPORT_NO_PARTIAL_**SWAP
> >> On 2013/03/05 20:32:37, piman wrote:
> >> > I'm really uneasy with passing feature flags through this type enum. How
> >>
> > about
> >
> >> > disabling partial swaps in the guest, by changing the LayerTreeSettings
> >> in
> >>
> >
> >
> >  > RenderWidgetCompositor::Create ?
> >>
> >
> >
> >  I'll need to go do some digging to find a good way to do that. I've
> >> written
> >>
> > via
> >
> >> SurfaceType because it's one place I know something is a guest.
> >>
> >
> >  Currently a renderer itself doesn't know it's a webview, so I'll need to
> >> plumb
> >> that information through, somehow.
> >>
> >
> >  I'll need a flag along the lines of
> >>
> > do_partial_swap_but_render_**everything_anyway
> >
> >> if we want to forward the damage rect to the embedder.
> >>
> >
> > Generally, we've been trying to avoid writing guest-renderer-specific code
> > to
> > avoid accidentally writing code that gives guest renderers more privileges
> > than
> > they should have. Generally, a guest process should not know it's a guest
> > process. Only the browser process's treatment of it is different.
> >
> 
> In this case a guest needs to behave differently compared to a non-guest
> (it isn't reasonable to disable partial swaps across the board). One way or
> another some information has to flow to there.
> 
> Antoine
> 
> 
> >
>
https://codereview.chromium.**org/12463007/%3Chttps://codereview.chromium.org...>
> >

That's fine as long as it's a flag that's specific to disabling partial swaps
for the renderer to avoid a precedent for adding more guest-specific code in the
future in the renderer where it might not be necessary.

[alexst (slow to review), 2013-03-06 17:56:12]

Here is version 2, plumbed through the renderer settings.

[Fady Samuel, 2013-03-06 18:01:26]

On 2013/03/06 17:56:12, alexst wrote:
> Here is version 2, plumbed through the renderer settings.

I'm happy with this plumbing from BrowserPlugin's perspective. Antoine,
thoughts?

[piman, 2013-03-06 18:05:16]

LGTM from my pov, adding James who just removed a similar flag from
RendererPreferences.

https://codereview.chromium.org/12463007/diff/10001/content/public/common/ren...
File content/public/common/renderer_preferences.h (right):

https://codereview.chromium.org/12463007/diff/10001/content/public/common/ren...
content/public/common/renderer_preferences.h:124: // Specifies whether patially
swapping composited buffers is
typo: partially

https://codereview.chromium.org/12463007/diff/10001/content/public/common/ren...
content/public/common/renderer_preferences.h:126: // in on if supported.
"turns in on" typo?

[alexst (slow to review), 2013-03-06 18:24:12]

https://codereview.chromium.org/12463007/diff/10001/content/public/common/ren...
File content/public/common/renderer_preferences.h (right):

https://codereview.chromium.org/12463007/diff/10001/content/public/common/ren...
content/public/common/renderer_preferences.h:126: // in on if supported.
On 2013/03/06 18:05:16, piman wrote:
> "turns in on" typo?

Fixed and rephrased.

[alexst (slow to review), 2013-03-06 18:43:03]

On 2013/03/06 18:05:16, piman wrote:
> LGTM from my pov, adding James who just removed a similar flag from
> RendererPreferences.
> 

Antoine, you mentioned adding James. Which James are you talking about? I don't
think I saw any new names added to the CL.

[alexst (slow to review), 2013-03-06 18:55:24]

+cdn for IPC.

[Fady Samuel, 2013-03-06 18:59:38]

Ohh, one minor comment.

https://codereview.chromium.org/12463007/diff/10001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/12463007/diff/10001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:93: // Webview
compositing does not currently work with partial swaps.
Please avoid using the term webview in BrowserPlugin if not necessary as the
BrowserPlugin infrastructure will be used by other elements shortly.

// BrowserPlugin compositing does not currently work with partial swaps.

[piman, 2013-03-06 19:08:01]

+jamesr
oops, missed adding him somehow

[jamesr, 2013-03-06 19:30:27]

Please don't do it this way.

https://codereview.chromium.org/12463007/diff/11010/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/12463007/diff/11010/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:221:
texture_layer_->setNeedsDisplay();
Is this an unrelated change?

https://codereview.chromium.org/12463007/diff/11010/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/12463007/diff/11010/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:174:
settings.partialSwapEnabled = web_settings.partialSwapEnabled &&
No, please don't add more dependencies on web_settings.

https://codereview.chromium.org/12463007/diff/11010/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/12463007/diff/11010/content/renderer/render_w...
content/renderer/render_widget.cc:1353: override_settings.partialSwapEnabled =
allow_partial_swap_;
Yuck yuck yuck!  RenderWidgetCompositor has a RenderWidget*, can't it just ask
the RenderWidget questions like this directly instead of bouncing through a
WebKit struct?

[alexst (slow to review), 2013-03-06 20:34:07]

Patch

https://codereview.chromium.org/12463007/diff/11010/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/12463007/diff/11010/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:221:
texture_layer_->setNeedsDisplay();
On 2013/03/06 19:30:28, jamesr wrote:
> Is this an unrelated change?

Related, when partial swaps are turned on, the embedder compositor didn't have
the right invalidation rect, which resulted in corruption in the plugin
container area.

https://codereview.chromium.org/12463007/diff/11010/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/12463007/diff/11010/content/renderer/render_w...
content/renderer/render_widget.cc:1353: override_settings.partialSwapEnabled =
allow_partial_swap_;
On 2013/03/06 19:30:28, jamesr wrote:
> Yuck yuck yuck!  RenderWidgetCompositor has a RenderWidget*, can't it just ask
> the RenderWidget questions like this directly instead of bouncing through a
> WebKit struct?

This seemed like a related field in the struct, I didn't realize dependency on
it was undesirable. I'll re-plumb.

[jamesr, 2013-03-06 22:37:59]

https://codereview.chromium.org/12463007/diff/17002/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12463007/diff/17002/content/renderer/render_v...
content/renderer/render_view_impl.cc:737: allow_partial_swap_ =
params->renderer_prefs.allow_partial_swap;
We store the entire renderer_prefs struct in renderer_prefs_.  Why spend extra
memory keeping an additional copy of the bool?

[Cris Neckar, 2013-03-06 23:11:25]

IPC changes LGTM for now. I'll look again after you refactor for james.

[alexst (slow to review), 2013-03-06 23:12:34]

https://codereview.chromium.org/12463007/diff/17002/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12463007/diff/17002/content/renderer/render_v...
content/renderer/render_view_impl.cc:737: allow_partial_swap_ =
params->renderer_prefs.allow_partial_swap;
On 2013/03/06 22:37:59, jamesr wrote:
> We store the entire renderer_prefs struct in renderer_prefs_.  Why spend extra
> memory keeping an additional copy of the bool?

renderer_preferences_ is stored on RenderViewImpl, and RenderWIdgetCompositor
has RenderWidget. I added the bool as an alternative to making
allow_partial_swap()  virtual.

[jamesr, 2013-03-06 23:13:45]

On 2013/03/06 23:12:34, alexst wrote:
>
https://codereview.chromium.org/12463007/diff/17002/content/renderer/render_v...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/12463007/diff/17002/content/renderer/render_v...
> content/renderer/render_view_impl.cc:737: allow_partial_swap_ =
> params->renderer_prefs.allow_partial_swap;
> On 2013/03/06 22:37:59, jamesr wrote:
> > We store the entire renderer_prefs struct in renderer_prefs_.  Why spend
extra
> > memory keeping an additional copy of the bool?
> 
> renderer_preferences_ is stored on RenderViewImpl, and RenderWIdgetCompositor
> has RenderWidget. I added the bool as an alternative to making
> allow_partial_swap()  virtual.

We pay for the memory always.  We would pay for a virtual call once, when
setting up a new compositor instance.

[alexst (slow to review), 2013-03-07 00:33:41]

> We pay for the memory always.  We would pay for a virtual call once, when
> setting up a new compositor instance.

That's fair. New patch is up.

https://codereview.chromium.org/12463007/diff/12019/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12463007/diff/12019/content/renderer/render_v...
content/renderer/render_view_impl.cc:689:
params->renderer_prefs.allow_partial_swap;
This needs to be set here because OnSetRendererPrefs relies on webwidget_ being
initialized, and we need to know if we are partially swapping during webwidget_
initialization.

[jamesr, 2013-03-07 00:48:05]

https://codereview.chromium.org/12463007/diff/12019/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12463007/diff/12019/content/renderer/render_v...
content/renderer/render_view_impl.cc:689:
params->renderer_prefs.allow_partial_swap;
On 2013/03/07 00:33:41, alexst wrote:
> This needs to be set here because OnSetRendererPrefs relies on webwidget_
being
> initialized, and we need to know if we are partially swapping during
webwidget_
> initialization.

Hmmmm, this is getting quite tricky.  Why don't you just set this in the
constructor?

Rather than being a RendererPreference that's actually slightly magical, what
about making this a CreateWindow_Params member and putting it explicitly in the
RenderViewImplParams struct?

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

> Hmmmm, this is getting quite tricky.  Why don't you just set this in the
> constructor?
> 
> Rather than being a RendererPreference that's actually slightly magical, what
> about making this a CreateWindow_Params member and putting it explicitly in
the
> RenderViewImplParams struct?

New patch is up.

[jamesr, 2013-03-07 19:16:05]

lgtm

[Cris Neckar, 2013-03-07 20:08:49]

ipc lgtm

[commit-bot: I haz the power, 2013-03-07 20:16:49]

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

[commit-bot: I haz the power, 2013-03-07 20:39:51]

Failed to trigger a try job on win7_aura
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-03-07 20:40:00]

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

[commit-bot: I haz the power, 2013-03-08 02:41:32]

Change committed as 186863