Move the content-dependent RecreateLayer logic from aura::Window to RWHVA

Move the content-dependent RecreateLayer logic from aura::Window to RWHVA

aura::Window is the wrong place to reason about all the different types of
external data that can go into layer, its delegate (RWHVA) is better suited. So
move that logic there.

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

[piman, 2013-09-11 00:00:03]

danakj: please review
sky: OWNERS for ui/ and ash/

Note: the only "meat" is in ui/aura/window.cc and
content/browser/renderer_host/render_widget_host_view_aura.cc

[sky, 2013-09-11 01:57:23]

LGTM

https://codereview.chromium.org/23444051/diff/11001/ui/aura/window_delegate.h
File ui/aura/window_delegate.h (right):

https://codereview.chromium.org/23444051/diff/11001/ui/aura/window_delegate.h...
ui/aura/window_delegate.h:99: // Called from RecreateLayer() if the layer the
window is associated with has
nit: update comment as it appears to be wrong.

[danakj, 2013-09-11 17:55:05]

LGTM on the code moving. Can you clarify a couple things for me though for the
future?

https://codereview.chromium.org/23444051/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/23444051/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2507: //
crbug.com/175211.
This bug is marked as fixed, but has no context at all to help me understand why
we're swapping the old texture into the new layer instead of giving the new
layer a copy of the old texture. Can you fill me in?

https://codereview.chromium.org/23444051/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2512: if
(host_->is_accelerated_compositing_active() &&
Can you explain how do we have old_texture.get() but not have accelerated
compositing active?

https://codereview.chromium.org/23444051/diff/11001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (left):

https://codereview.chromium.org/23444051/diff/11001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:783: // The layer we
create doesn't have an external texture, so this should never
You kept this comment elsewhere, do yuo want to keep it here? Should we have it
in every empty function? Should we move this comment to the declaration of the
pure virtual method to say "do nothing if there's no external content"?

[piman, 2013-09-11 20:42:05]

On 2013/09/11 17:55:05, danakj wrote:
> LGTM on the code moving. Can you clarify a couple things for me though for the
> future?
> 
>
https://codereview.chromium.org/23444051/diff/11001/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/23444051/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2507: //
> crbug.com/175211.
> This bug is marked as fixed, but has no context at all to help me understand
why
> we're swapping the old texture into the new layer instead of giving the new
> layer a copy of the old texture. Can you fill me in?

So, for the ContentLayer case, what we do is we rely on the fact that we don't
lose the tiles when we remove the delegate, so the existing layer (that
supposedly has the tiles filled up already) becomes the "copy", and we move the
delegate to the newly created layer so that it can fill it, so the new layer
becomes the window's layer (hence, I suppose, why it's called "RecreateLayer"
not "DuplicateLayer").
Because of that, for the TextureLayer case, because the existing texture is the
one we want to return to the renderer, it has to go to the new layer, and we put
the copy on the old layer.

> 
>
https://codereview.chromium.org/23444051/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2512: if
> (host_->is_accelerated_compositing_active() &&
> Can you explain how do we have old_texture.get() but not have accelerated
> compositing active?

There's transient states when this can happen.
is_accelerated_compositing_active() is unfortunately changed independently of
when we receive the frames. When the renderer switches to non-composited, we
keep the texture around until we receive a non-composited frame, in case the
renderer switches back to composited, to avoid flashes. See
RenderWidgetHostViewAura::OnAcceleratedCompositingStateChanged

> 
>
https://codereview.chromium.org/23444051/diff/11001/ui/views/widget/desktop_a...
> File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (left):
> 
>
https://codereview.chromium.org/23444051/diff/11001/ui/views/widget/desktop_a...
> ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:783: // The layer
we
> create doesn't have an external texture, so this should never
> You kept this comment elsewhere, do yuo want to keep it here? Should we have
it
> in every empty function? Should we move this comment to the declaration of the
> pure virtual method to say "do nothing if there's no external content"?

I'll see if I can rationalize the comments.

[danakj, 2013-09-11 20:45:21]

On Wed, Sep 11, 2013 at 4:42 PM, <piman@chromium.org> wrote:

> On 2013/09/11 17:55:05, danakj wrote:
>
>> LGTM on the code moving. Can you clarify a couple things for me though
>> for the
>> future?
>>
>
>
> https://codereview.chromium.**org/23444051/diff/11001/**
>
content/browser/renderer_host/**render_widget_host_view_aura.**cc<https://codereview.chromium.org/23444051/diff/11001/content/browser/renderer_host/render_widget_host_view_aura.cc>
>
>> File content/browser/renderer_host/**render_widget_host_view_aura.**cc
>> (right):
>>
>
>
> https://codereview.chromium.**org/23444051/diff/11001/**
> content/browser/renderer_host/**render_widget_host_view_aura.**
>
cc#newcode2507<https://codereview.chromium.org/23444051/diff/11001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode2507>
>
>> content/browser/renderer_host/**render_widget_host_view_aura.**cc:2507:
>> //
>> crbug.com/175211.
>> This bug is marked as fixed, but has no context at all to help me
>> understand
>>
> why
>
>> we're swapping the old texture into the new layer instead of giving the
>> new
>> layer a copy of the old texture. Can you fill me in?
>>
>
> So, for the ContentLayer case, what we do is we rely on the fact that we
> don't
> lose the tiles when we remove the delegate, so the existing layer (that
> supposedly has the tiles filled up already) becomes the "copy", and we
> move the
> delegate to the newly created layer so that it can fill it, so the new
> layer
> becomes the window's layer (hence, I suppose, why it's called
> "RecreateLayer"
> not "DuplicateLayer").
> Because of that, for the TextureLayer case, because the existing texture
> is the
> one we want to return to the renderer, it has to go to the new layer, and
> we put
> the copy on the old layer.


Thanks! So it's about which resource we'll be returning. Think this could
replace/augment the current comment?


>
>
>
>
> https://codereview.chromium.**org/23444051/diff/11001/**
> content/browser/renderer_host/**render_widget_host_view_aura.**
>
cc#newcode2512<https://codereview.chromium.org/23444051/diff/11001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode2512>
>
>> content/browser/renderer_host/**render_widget_host_view_aura.**cc:2512:
>> if
>> (host_->is_accelerated_**compositing_active() &&
>> Can you explain how do we have old_texture.get() but not have accelerated
>> compositing active?
>>
>
> There's transient states when this can happen.
> is_accelerated_compositing_**active() is unfortunately changed
> independently of
> when we receive the frames. When the renderer switches to non-composited,
> we
> keep the texture around until we receive a non-composited frame, in case
> the
> renderer switches back to composited, to avoid flashes. See
> RenderWidgetHostViewAura::**OnAcceleratedCompositingStateC**hanged


Oof. I thought a renderer couldn't change back and forth, but I guess it
can change between being a WebView and a WebWidget within the same
renderer? That's kinda annoying. Can't wait to get rid of this.


>
>
>
>
> https://codereview.chromium.**org/23444051/diff/11001/ui/**
>
views/widget/desktop_aura/**desktop_native_widget_aura.cc<https://codereview.chromium.org/23444051/diff/11001/ui/views/widget/desktop_aura/desktop_native_widget_aura.cc>
>
>> File ui/views/widget/desktop_aura/**desktop_native_widget_aura.cc (left):
>>
>
>
> https://codereview.chromium.**org/23444051/diff/11001/ui/**
>
views/widget/desktop_aura/**desktop_native_widget_aura.cc#**oldcode783<https://codereview.chromium.org/23444051/diff/11001/ui/views/widget/desktop_aura/desktop_native_widget_aura.cc#oldcode783>
>
>> ui/views/widget/desktop_aura/**desktop_native_widget_aura.cc:**783: //
>> The layer
>>
> we
>
>> create doesn't have an external texture, so this should never
>> You kept this comment elsewhere, do yuo want to keep it here? Should we
>> have
>>
> it
>
>> in every empty function? Should we move this comment to the declaration
>> of the
>> pure virtual method to say "do nothing if there's no external content"?
>>
>
> I'll see if I can rationalize the comments.
>
>
>
https://codereview.chromium.**org/23444051/<https://codereview.chromium.org/2...
>

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

[commit-bot: I haz the power, 2013-09-12 00:49:14]

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

[piman, 2013-09-12 01:02:33]

https://codereview.chromium.org/23444051/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/23444051/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:2507: //
crbug.com/175211.
On 2013/09/11 17:55:05, danakj wrote:
> This bug is marked as fixed, but has no context at all to help me understand
why
> we're swapping the old texture into the new layer instead of giving the new
> layer a copy of the old texture. Can you fill me in?

Done.

https://codereview.chromium.org/23444051/diff/11001/ui/aura/window_delegate.h
File ui/aura/window_delegate.h (right):

https://codereview.chromium.org/23444051/diff/11001/ui/aura/window_delegate.h...
ui/aura/window_delegate.h:99: // Called from RecreateLayer() if the layer the
window is associated with has
On 2013/09/11 01:57:23, sky wrote:
> nit: update comment as it appears to be wrong.

Done.

https://codereview.chromium.org/23444051/diff/11001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (left):

https://codereview.chromium.org/23444051/diff/11001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:783: // The layer we
create doesn't have an external texture, so this should never
On 2013/09/11 17:55:05, danakj wrote:
> You kept this comment elsewhere, do yuo want to keep it here? Should we have
it
> in every empty function? Should we move this comment to the declaration of the
> pure virtual method to say "do nothing if there's no external content"?

Added larger comments to WindowDelegate::DidRecreateLayer (the interface), and
removed all redundant comments in implementation.

[commit-bot: I haz the power, 2013-09-12 06:23:19]

Change committed as 222735