Generalize layer mirroring for phantom windows

Generalize layer mirroring for phantom windows

Previously, the layers of a phantom window were recreated when the
surface changed in the corresponding layers of the original window.
The code also assumed that layers with surfaces are leaves in the
layer tree, but Exosphere surfaces are nested. Recursive layer
cloning would be inefficient for surfaces changing in each frame,
so this CL adds an alternative way to mirror layers. In addition,
the bounds of Exosphere surfaces relative to their parent window
are updated whenever the window moves, so this CL provides a way
to synchronize relative positions of layers in phantom windows.

In addition to implementing phantom windows for ARC apps, this CL
fixes three bugs related to mirroring:

  1) Memory corruption due to the non-recursive layer cloning.
  2) Dangling pointers due to observers sticking around.
  3) Black Alt+Tab previews for ARC windows.

BUG=642894
BUG=649452
BUG=653113
TEST=ARC apps that render continuously (e.g. games, video players)
     can be dragged to external displays, and their content is
     mirrored in phantom windows.
TEST=ARC apps are mirrored in Alt+Tab previews.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/4b33bfeda856f7ae20ffa43176e5bd529d5a100a
Cr-Commit-Position: refs/heads/master@{#427891}

[Dominik Laskowski, 2016-09-30 20:23:32]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 20:24:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dominik Laskowski, 2016-09-30 20:25:51]

domlaskowski@chromium.org changed reviewers:
+ oshima@chromium.org, reveman@chromium.org

[Dominik Laskowski, 2016-09-30 20:25:52]

PTAL.

[commit-bot: I haz the power, 2016-09-30 21:26:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 21:26:51]

Dry run: This issue passed the CQ dry run.

[reveman, 2016-10-03 22:17:50]

components/exo and general approach lgtm

[oshima, 2016-10-04 21:06:31]

lgtm

https://codereview.chromium.org/2383263002/diff/1/ash/wm/drag_window_controll...
File ash/wm/drag_window_controller.cc (right):

https://codereview.chromium.org/2383263002/diff/1/ash/wm/drag_window_controll...
ash/wm/drag_window_controller.cc:61: delegates_.clear();  // Stop observing
layers.
thanks for catching this. i believe this fixes the crash I noticed yesterday.

https://codereview.chromium.org/2383263002/diff/1/ui/compositor/layer_owner.h
File ui/compositor/layer_owner.h (right):

https://codereview.chromium.org/2383263002/diff/1/ui/compositor/layer_owner.h...
ui/compositor/layer_owner.h:40: // Requests a |mirror| of the owned layer from
the LayerOwnerDelegate.
how about "Requests to mirror the content of owning layer onto |mirror| layer"?

[oshima, 2016-10-04 21:15:05]

hmm, i just tried the patch, and seems like crbug.com/636594 is regressed. can
you look into it?

[oshima, 2016-10-04 21:15:42]

not lgtm (sorry)

[Dominik Laskowski, 2016-10-04 23:18:52]

domlaskowski@chromium.org changed reviewers:
+ estade@chromium.org

[Dominik Laskowski, 2016-10-04 23:18:52]

+estade

I was aware of that bug from the Git history, and briefly looked into it while
testing this patch. At first, I thought it was a regression caused by my patch,
but I was also able to reproduce it on ToT. My wild guess is that a layer
subtree of the original window is recreated on page reload. Neither this patch
nor the original code handles that case.

https://codereview.chromium.org/2383263002/diff/1/ui/compositor/layer_owner.h
File ui/compositor/layer_owner.h (right):

https://codereview.chromium.org/2383263002/diff/1/ui/compositor/layer_owner.h...
ui/compositor/layer_owner.h:40: // Requests a |mirror| of the owned layer from
the LayerOwnerDelegate.
On 2016/10/04 21:06:31, oshima wrote:
> how about "Requests to mirror the content of owning layer onto |mirror|
layer"?

Done.

[oshima, 2016-10-05 20:23:20]

On 2016/10/04 23:18:52, Dominik Laskowski wrote:
> +estade
> 
> I was aware of that bug from the Git history, and briefly looked into it while
> testing this patch. At first, I thought it was a regression caused by my
patch,
> but I was also able to reproduce it on ToT. My wild guess is that a layer
> subtree of the original window is recreated on page reload. Neither this patch
> nor the original code handles that case.

I could repro easily with this patch, but couldn't repro w/o easily. I'm fine to
land if it's not regression.
evan, can you confirm?

> 
> https://codereview.chromium.org/2383263002/diff/1/ui/compositor/layer_owner.h
> File ui/compositor/layer_owner.h (right):
> 
>
https://codereview.chromium.org/2383263002/diff/1/ui/compositor/layer_owner.h...
> ui/compositor/layer_owner.h:40: // Requests a |mirror| of the owned layer from
> the LayerOwnerDelegate.
> On 2016/10/04 21:06:31, oshima wrote:
> > how about "Requests to mirror the content of owning layer onto |mirror|
> layer"?
> 
> Done.

[Evan Stade, 2016-10-06 00:12:02]

On 2016/10/05 20:23:20, oshima wrote:
> On 2016/10/04 23:18:52, Dominik Laskowski wrote:
> > +estade
> > 
> > I was aware of that bug from the Git history, and briefly looked into it
while
> > testing this patch. At first, I thought it was a regression caused by my
> patch,
> > but I was also able to reproduce it on ToT. My wild guess is that a layer
> > subtree of the original window is recreated on page reload. Neither this
patch
> > nor the original code handles that case.
> 
> I could repro easily with this patch, but couldn't repro w/o easily. I'm fine
to
> land if it's not regression.
> evan, can you confirm?

this is also my experience. It went from hard to repro to easy to repro.

[Dominik Laskowski, 2016-10-06 21:05:17]

On 2016/10/06 00:12:02, Evan Stade wrote:
> On 2016/10/05 20:23:20, oshima wrote:
> > On 2016/10/04 23:18:52, Dominik Laskowski wrote:
> > > +estade
> > > 
> > > I was aware of that bug from the Git history, and briefly looked into it
> while
> > > testing this patch. At first, I thought it was a regression caused by my
> > patch,
> > > but I was also able to reproduce it on ToT. My wild guess is that a layer
> > > subtree of the original window is recreated on page reload. Neither this
> patch
> > > nor the original code handles that case.
> > 
> > I could repro easily with this patch, but couldn't repro w/o easily. I'm
fine
> to
> > land if it's not regression.
> > evan, can you confirm?
> 
> this is also my experience. It went from hard to repro to easy to repro.

Could we fix that bug in a separate CL? It seems to be a general problem that
affects both Alt+Tab and phantom windows. For example, if you drag a Chrome
window to an external display and, while keeping the mouse button down, open
DevTools using the keyboard shortcut, the phantom window is stale because its
layer tree is out of sync. We need to clone the layer tree recursively when
layers are added or removed, but not when a surface changes.

[oshima, 2016-10-06 21:24:54]

On 2016/10/06 21:05:17, Dominik Laskowski wrote:
> On 2016/10/06 00:12:02, Evan Stade wrote:
> > On 2016/10/05 20:23:20, oshima wrote:
> > > On 2016/10/04 23:18:52, Dominik Laskowski wrote:
> > > > +estade
> > > > 
> > > > I was aware of that bug from the Git history, and briefly looked into it
> > while
> > > > testing this patch. At first, I thought it was a regression caused by my
> > > patch,
> > > > but I was also able to reproduce it on ToT. My wild guess is that a
layer
> > > > subtree of the original window is recreated on page reload. Neither this
> > patch
> > > > nor the original code handles that case.
> > > 
> > > I could repro easily with this patch, but couldn't repro w/o easily. I'm
> fine
> > to
> > > land if it's not regression.
> > > evan, can you confirm?
> > 
> > this is also my experience. It went from hard to repro to easy to repro.
> 
> Could we fix that bug in a separate CL? It seems to be a general problem that
> affects both Alt+Tab and phantom windows. For example, if you drag a Chrome
> window to an external display and, while keeping the mouse button down, open
> DevTools using the keyboard shortcut, the phantom window is stale because its
> layer tree is out of sync. We need to clone the layer tree recursively when
> layers are added or removed, but not when a surface changes.

I was thinking about that, given that this fixes crashes.

lgtm

and I can look into that issue.

[Evan Stade, 2016-10-06 21:29:11]

On 2016/10/06 21:24:54, oshima wrote:
> On 2016/10/06 21:05:17, Dominik Laskowski wrote:
> > On 2016/10/06 00:12:02, Evan Stade wrote:
> > > On 2016/10/05 20:23:20, oshima wrote:
> > > > On 2016/10/04 23:18:52, Dominik Laskowski wrote:
> > > > > +estade
> > > > > 
> > > > > I was aware of that bug from the Git history, and briefly looked into
it
> > > while
> > > > > testing this patch. At first, I thought it was a regression caused by
my
> > > > patch,
> > > > > but I was also able to reproduce it on ToT. My wild guess is that a
> layer
> > > > > subtree of the original window is recreated on page reload. Neither
this
> > > patch
> > > > > nor the original code handles that case.
> > > > 
> > > > I could repro easily with this patch, but couldn't repro w/o easily. I'm
> > fine
> > > to
> > > > land if it's not regression.
> > > > evan, can you confirm?
> > > 
> > > this is also my experience. It went from hard to repro to easy to repro.
> > 
> > Could we fix that bug in a separate CL? It seems to be a general problem
that
> > affects both Alt+Tab and phantom windows. For example, if you drag a Chrome
> > window to an external display and, while keeping the mouse button down, open
> > DevTools using the keyboard shortcut, the phantom window is stale because
its
> > layer tree is out of sync. We need to clone the layer tree recursively when
> > layers are added or removed, but not when a surface changes.
> 
> I was thinking about that, given that this fixes crashes.
> 
> lgtm
> 
> and I can look into that issue.

I don't object

[Dominik Laskowski, 2016-10-06 21:33:53]

Description was changed from

==========
Generalize layer mirroring for phantom windows

Previously, the layers of a phantom window were recreated when the
surface changed in the corresponding layers of the original window.
The code also assumed that layers with surfaces are leaves in the
layer tree, but Exosphere surfaces are nested. Recursive layer
cloning would be inefficient for surfaces changing in each frame,
so this CL adds an alternative way to mirror layers. In addition,
the bounds of Exosphere surfaces relative to their parent window
are updated whenever the window moves, so this CL provides a way
to synchronize relative positions of layers in phantom windows.

BUG=642894
TEST=ARC apps that render continuously (e.g. games, video players)
     can be dragged to external displays, and their content is
     mirrored in phantom windows.
==========

to

==========
Generalize layer mirroring for phantom windows

Previously, the layers of a phantom window were recreated when the
surface changed in the corresponding layers of the original window.
The code also assumed that layers with surfaces are leaves in the
layer tree, but Exosphere surfaces are nested. Recursive layer
cloning would be inefficient for surfaces changing in each frame,
so this CL adds an alternative way to mirror layers. In addition,
the bounds of Exosphere surfaces relative to their parent window
are updated whenever the window moves, so this CL provides a way
to synchronize relative positions of layers in phantom windows.

BUG=642894
BUG=649452
BUG=653113
TEST=ARC apps that render continuously (e.g. games, video players)
     can be dragged to external displays, and their content is
     mirrored in phantom windows.
==========

[Dominik Laskowski, 2016-10-06 22:20:15]

domlaskowski@chromium.org changed reviewers:
+ danakj@chromium.org, pkotwicz@chromium.org, sadrul@chromium.org,
sky@chromium.org

[Dominik Laskowski, 2016-10-06 22:21:13]

+pkotwicz for ash/{common,}/wm/
+sky for chrome/browser/ui/views/frame/
+sadrul for content/browser/renderer_host/
+danakj for ui/compositor/

[Dominik Laskowski, 2016-10-06 22:24:55]

On 2016/10/06 22:21:13, Dominik Laskowski wrote:
> +pkotwicz for ash/{common,}/wm/
> +sky for chrome/browser/ui/views/frame/
> +sadrul for content/browser/renderer_host/
> +danakj for ui/compositor/

pkotwicz: Never mind, oshima already covered ash/

[danakj, 2016-10-06 22:42:34]

It seems like piman's proposal over here
https://codereview.chromium.org/2254733003#msg19 is relevant now also, that we
replace RecreateLayer with a simpler layer-clone-that-hears-about-changes.

Did you consider doing that instead? I don't understand why we want two ways to
clone/mirror layers.

[oshima, 2016-10-07 13:01:29]

On 2016/10/06 22:42:34, danakj wrote:
> It seems like piman's proposal over here
> https://codereview.chromium.org/2254733003#msg19 is relevant now also, that we
> replace RecreateLayer with a simpler layer-clone-that-hears-about-changes.
> 
> Did you consider doing that instead? I don't understand why we want two ways
to
> clone/mirror layers.

Recreate layer is designed this way to implement close/hide animation where the
original layer won't be painted.

[pkotwicz, 2016-10-07 13:45:38]

I have not looked at the ChromeOS code for over a year. Oshima is probably a
better reviewer than I. I will look at your CL Monday (I am OOO today)

[sky, 2016-10-07 16:27:22]

On 2016/10/07 13:01:29, oshima wrote:
> On 2016/10/06 22:42:34, danakj wrote:
> > It seems like piman's proposal over here
> > https://codereview.chromium.org/2254733003#msg19 is relevant now also, that
we
> > replace RecreateLayer with a simpler layer-clone-that-hears-about-changes.
> > 
> > Did you consider doing that instead? I don't understand why we want two ways
> to
> > clone/mirror layers.
> 
> Recreate layer is designed this way to implement close/hide animation where
the
> original layer won't be painted.

Indeed, but RecreateLayer has problems in so far as it doesn't track changes to
the original layer. This is important in use cases like alt-tab. I agree with
Dana and Antoine, we should go with 'simpler
layer-clone-that-hears-about-changes'.

  -Scott

[Dominik Laskowski, 2016-10-07 22:22:28]

On 2016/10/07 16:27:22, sky wrote:
> On 2016/10/07 13:01:29, oshima wrote:
> > On 2016/10/06 22:42:34, danakj wrote:
> > > It seems like piman's proposal over here
> > > https://codereview.chromium.org/2254733003#msg19 is relevant now also,
that
> we
> > > replace RecreateLayer with a simpler layer-clone-that-hears-about-changes.
> > > 
> > > Did you consider doing that instead? I don't understand why we want two
ways
> > to
> > > clone/mirror layers.
> > 
> > Recreate layer is designed this way to implement close/hide animation where
> the
> > original layer won't be painted.
> 
> Indeed, but RecreateLayer has problems in so far as it doesn't track changes
to
> the original layer. This is important in use cases like alt-tab. I agree with
> Dana and Antoine, we should go with 'simpler
> layer-clone-that-hears-about-changes'.
> 
>   -Scott

While we could shift the responsibility for synchronizing state (e.g. surface,
bounds) between a layer and its mirror(s) from ForwardingLayerDelegate to Layer,
we would still need ForwardingLayerDelegate for layers that are not backed by a
surface. I figured that keeping related functionality there is simpler and
clearer.

However, I agree that the above proposal is a better approach, and is likely a
prerequisite to fixing the bug mentioned earlier, i.e. synchronizing changes to
the layer tree.

We would still need cloning and mirroring to be distinct operations though,
because sometimes we just want a snapshot rather than keeping the layers
synchronized.

I'd be happy to take a stab at refactoring, but I'd prefer to land this
relatively simple CL first for a few reasons:

1) It fixes memory corruption due to the non-recursive layer cloning.
2) It fixes dangling pointers due to observers sticking around after
destruction.
3) It fixes Alt+Tab and phantom windows for ARC windows.

[danakj, 2016-10-07 22:25:32]

On Fri, Oct 7, 2016 at 3:22 PM, <domlaskowski@chromium.org> wrote:

> On 2016/10/07 16:27:22, sky wrote:
> > On 2016/10/07 13:01:29, oshima wrote:
> > > On 2016/10/06 22:42:34, danakj wrote:
> > > > It seems like piman's proposal over here
> > > > https://codereview.chromium.org/2254733003#msg19 is relevant now
> also,
> that
> > we
> > > > replace RecreateLayer with a simpler layer-clone-that-hears-about-
> changes.
> > > >
> > > > Did you consider doing that instead? I don't understand why we want
> two
> ways
> > > to
> > > > clone/mirror layers.
> > >
> > > Recreate layer is designed this way to implement close/hide animation
> where
> > the
> > > original layer won't be painted.
> >
> > Indeed, but RecreateLayer has problems in so far as it doesn't track
> changes
> to
> > the original layer. This is important in use cases like alt-tab. I agree
> with
> > Dana and Antoine, we should go with 'simpler
> > layer-clone-that-hears-about-changes'.
> >
> > -Scott
>
> While we could shift the responsibility for synchronizing state (e.g.
> surface,
> bounds) between a layer and its mirror(s) from ForwardingLayerDelegate to
> Layer,
> we would still need ForwardingLayerDelegate for layers that are not backed
> by a
> surface. I figured that keeping related functionality there is simpler and
> clearer.
>
> However, I agree that the above proposal is a better approach, and is
> likely a
> prerequisite to fixing the bug mentioned earlier, i.e. synchronizing
> changes to
> the layer tree.
>
> We would still need cloning and mirroring to be distinct operations though,
> because sometimes we just want a snapshot rather than keeping the layers
> synchronized.
>
> I'd be happy to take a stab at refactoring, but I'd prefer to land this
> relatively simple CL first for a few reasons:
>
> 1) It fixes memory corruption due to the non-recursive layer cloning.
> 2) It fixes dangling pointers due to observers sticking around after
> destruction.
> 3) It fixes Alt+Tab and phantom windows for ARC windows.
>

Removing code is a lot harder than adding it, so I'd like for you to try
out the desired approach first. Cloning without mirroring should look about
the same but just not add itself as an observer of the layers.


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

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2016-10-07 22:53:58]

+1 to what Dana said.

Also, when do we want a snapshot vs keeping the layers in sync?

  -Scott

On Fri, Oct 7, 2016 at 3:25 PM,  <danakj@chromium.org> wrote:
> On Fri, Oct 7, 2016 at 3:22 PM, <domlaskowski@chromium.org> wrote:
>>
>> On 2016/10/07 16:27:22, sky wrote:
>> > On 2016/10/07 13:01:29, oshima wrote:
>> > > On 2016/10/06 22:42:34, danakj wrote:
>> > > > It seems like piman's proposal over here
>> > > > https://codereview.chromium.org/2254733003#msg19 is relevant now
>> > > > also,
>> that
>> > we
>> > > > replace RecreateLayer with a simpler
>> > > > layer-clone-that-hears-about-changes.
>> > > >
>> > > > Did you consider doing that instead? I don't understand why we want
>> > > > two
>> ways
>> > > to
>> > > > clone/mirror layers.
>> > >
>> > > Recreate layer is designed this way to implement close/hide animation
>> > > where
>> > the
>> > > original layer won't be painted.
>> >
>> > Indeed, but RecreateLayer has problems in so far as it doesn't track
>> > changes
>> to
>> > the original layer. This is important in use cases like alt-tab. I agree
>> > with
>> > Dana and Antoine, we should go with 'simpler
>> > layer-clone-that-hears-about-changes'.
>> >
>> > -Scott
>>
>> While we could shift the responsibility for synchronizing state (e.g.
>> surface,
>> bounds) between a layer and its mirror(s) from ForwardingLayerDelegate to
>> Layer,
>> we would still need ForwardingLayerDelegate for layers that are not backed
>> by a
>> surface. I figured that keeping related functionality there is simpler and
>> clearer.
>>
>> However, I agree that the above proposal is a better approach, and is
>> likely a
>> prerequisite to fixing the bug mentioned earlier, i.e. synchronizing
>> changes to
>> the layer tree.
>>
>> We would still need cloning and mirroring to be distinct operations
>> though,
>> because sometimes we just want a snapshot rather than keeping the layers
>> synchronized.
>>
>> I'd be happy to take a stab at refactoring, but I'd prefer to land this
>> relatively simple CL first for a few reasons:
>>
>> 1) It fixes memory corruption due to the non-recursive layer cloning.
>> 2) It fixes dangling pointers due to observers sticking around after
>> destruction.
>> 3) It fixes Alt+Tab and phantom windows for ARC windows.
>
>
> Removing code is a lot harder than adding it, so I'd like for you to try out
> the desired approach first. Cloning without mirroring should look about the
> same but just not add itself as an observer of the layers.
>
>>
>>
>> https://codereview.chromium.org/2383263002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oshima, 2016-10-08 01:13:49]

On 2016/10/07 16:27:22, sky wrote:
> On 2016/10/07 13:01:29, oshima wrote:
> > On 2016/10/06 22:42:34, danakj wrote:
> > > It seems like piman's proposal over here
> > > https://codereview.chromium.org/2254733003#msg19 is relevant now also,
that
> we
> > > replace RecreateLayer with a simpler layer-clone-that-hears-about-changes.
> > > 
> > > Did you consider doing that instead? I don't understand why we want two
ways
> > to
> > > clone/mirror layers.
> > 
> > Recreate layer is designed this way to implement close/hide animation where
> the
> > original layer won't be painted.
> 
> Indeed, but RecreateLayer has problems in so far as it doesn't track changes
to
> the original layer. This is important in use cases like alt-tab. I agree with
> Dana and Antoine, we should go with 'simpler
> layer-clone-that-hears-about-changes'.
> 
>   -Scott

I'm fine to add that. Just want to make sure that we don't simply replace it.

[Dominik Laskowski, 2016-10-11 22:51:42]

Description was changed from

==========
Generalize layer mirroring for phantom windows

Previously, the layers of a phantom window were recreated when the
surface changed in the corresponding layers of the original window.
The code also assumed that layers with surfaces are leaves in the
layer tree, but Exosphere surfaces are nested. Recursive layer
cloning would be inefficient for surfaces changing in each frame,
so this CL adds an alternative way to mirror layers. In addition,
the bounds of Exosphere surfaces relative to their parent window
are updated whenever the window moves, so this CL provides a way
to synchronize relative positions of layers in phantom windows.

BUG=642894
BUG=649452
BUG=653113
TEST=ARC apps that render continuously (e.g. games, video players)
     can be dragged to external displays, and their content is
     mirrored in phantom windows.
==========

to

==========
Generalize layer mirroring for phantom windows

Previously, the layers of a phantom window were recreated when the
surface changed in the corresponding layers of the original window.
The code also assumed that layers with surfaces are leaves in the
layer tree, but Exosphere surfaces are nested. Recursive layer
cloning would be inefficient for surfaces changing in each frame,
so this CL adds an alternative way to mirror layers. In addition,
the bounds of Exosphere surfaces relative to their parent window
are updated whenever the window moves, so this CL provides a way
to synchronize relative positions of layers in phantom windows.

BUG=642894
BUG=649452
BUG=653113
TEST=ARC apps that render continuously (e.g. games, video players)
     can be dragged to external displays, and their content is
     mirrored in phantom windows.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Dominik Laskowski, 2016-10-11 22:54:40]

Description was changed from

==========
Generalize layer mirroring for phantom windows

Previously, the layers of a phantom window were recreated when the
surface changed in the corresponding layers of the original window.
The code also assumed that layers with surfaces are leaves in the
layer tree, but Exosphere surfaces are nested. Recursive layer
cloning would be inefficient for surfaces changing in each frame,
so this CL adds an alternative way to mirror layers. In addition,
the bounds of Exosphere surfaces relative to their parent window
are updated whenever the window moves, so this CL provides a way
to synchronize relative positions of layers in phantom windows.

BUG=642894
BUG=649452
BUG=653113
TEST=ARC apps that render continuously (e.g. games, video players)
     can be dragged to external displays, and their content is
     mirrored in phantom windows.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Generalize layer mirroring for phantom windows

Previously, the layers of a phantom window were recreated when the
surface changed in the corresponding layers of the original window.
The code also assumed that layers with surfaces are leaves in the
layer tree, but Exosphere surfaces are nested. Recursive layer
cloning would be inefficient for surfaces changing in each frame,
so this CL adds an alternative way to mirror layers. In addition,
the bounds of Exosphere surfaces relative to their parent window
are updated whenever the window moves, so this CL provides a way
to synchronize relative positions of layers in phantom windows.

In addition to implementing phantom windows for ARC apps, this CL
fixes three bugs related to mirroring:

  1) Memory corruption due to the non-recursive layer cloning.
  2) Dangling pointers due to observers sticking around.
  3) Black Alt+Tab previews for ARC windows.

BUG=642894
BUG=649452
BUG=653113
TEST=ARC apps that render continuously (e.g. games, video players)
     can be dragged to external displays, and their content is
     mirrored in phantom windows.
TEST=ARC apps are mirrored in Alt+Tab previews.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Dominik Laskowski, 2016-10-11 22:55:43]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 22:56:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 23:20:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-11 23:20:46]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Dominik Laskowski, 2016-10-12 02:59:48]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 02:59:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 04:33:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 04:33:08]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Dominik Laskowski, 2016-10-12 17:50:13]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 17:50:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dominik Laskowski, 2016-10-12 18:36:45]

On 2016/10/08 01:13:49, oshima wrote:
> On 2016/10/07 16:27:22, sky wrote:
> > On 2016/10/07 13:01:29, oshima wrote:
> > > On 2016/10/06 22:42:34, danakj wrote:
> > > > It seems like piman's proposal over here
> > > > https://codereview.chromium.org/2254733003#msg19 is relevant now also,
> that
> > we
> > > > replace RecreateLayer with a simpler
layer-clone-that-hears-about-changes.
> > > > 
> > > > Did you consider doing that instead? I don't understand why we want two
> ways
> > > to
> > > > clone/mirror layers.
> > > 
> > > Recreate layer is designed this way to implement close/hide animation
where
> > the
> > > original layer won't be painted.
> > 
> > Indeed, but RecreateLayer has problems in so far as it doesn't track changes
> to
> > the original layer. This is important in use cases like alt-tab. I agree
with
> > Dana and Antoine, we should go with 'simpler
> > layer-clone-that-hears-about-changes'.
> > 
> >   -Scott
> 
> I'm fine to add that. Just want to make sure that we don't simply replace it.

PTAL. I've moved mirroring to the Layer class, and added MirrorLayers as an
alternative to RecreateLayers. The former is used for Alt+Tab and phantom
windows, while the latter is used for animations, e.g. window fading, screen
rotation. MirrorLayers still depends on a LayerDelegateFactory for non-surface
layers, notably Chrome's chrome.

[commit-bot: I haz the power, 2016-10-12 19:03:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 19:03:08]

Dry run: This issue passed the CQ dry run.

[oshima, 2016-10-12 19:31:40]

ash lgtm with nits

https://codereview.chromium.org/2383263002/diff/100001/cc/layers/surface_layer.h
File cc/layers/surface_layer.h (right):

https://codereview.chromium.org/2383263002/diff/100001/cc/layers/surface_laye...
cc/layers/surface_layer.h:45: gfx::Size surface_size() const { return
surface_size_; }
nit: const gfx::Size&

https://codereview.chromium.org/2383263002/diff/100001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/100001/ui/compositor/layer.cc...
ui/compositor/layer.cc:624: for (auto* mirror : mirrors_)
you need {} for this case

https://codereview.chromium.org/2383263002/diff/100001/ui/compositor/layer.cc...
ui/compositor/layer.cc:959: if (sync_bounds_)
nit {}

or early exit

https://codereview.chromium.org/2383263002/diff/100001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/2383263002/diff/100001/ui/compositor/layer.h#...
ui/compositor/layer.h:78: Layer(const Layer&);
explicit

https://codereview.chromium.org/2383263002/diff/100001/ui/compositor/layer.h#...
ui/compositor/layer.h:426: // LayerObserver overrides:
nit: // LayerObserver:

https://codereview.chromium.org/2383263002/diff/100001/ui/wm/core/window_util.h
File ui/wm/core/window_util.h (right):

https://codereview.chromium.org/2383263002/diff/100001/ui/wm/core/window_util...
ui/wm/core/window_util.h:63: LayerDelegateFactory* delegate_factory);
we ma want to remove delegate_factory because this was added to implement
mirroring. I'll leave this to owners though.

[Dominik Laskowski, 2016-10-13 17:58:56]

https://codereview.chromium.org/2383263002/diff/100001/cc/layers/surface_layer.h
File cc/layers/surface_layer.h (right):

https://codereview.chromium.org/2383263002/diff/100001/cc/layers/surface_laye...
cc/layers/surface_layer.h:45: gfx::Size surface_size() const { return
surface_size_; }
On 2016/10/12 19:31:41, oshima wrote:
> nit: const gfx::Size&

Done.

https://codereview.chromium.org/2383263002/diff/100001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/100001/ui/compositor/layer.cc...
ui/compositor/layer.cc:624: for (auto* mirror : mirrors_)
On 2016/10/12 19:31:41, oshima wrote:
> you need {} for this case

Done.

https://codereview.chromium.org/2383263002/diff/100001/ui/compositor/layer.cc...
ui/compositor/layer.cc:959: if (sync_bounds_)
On 2016/10/12 19:31:41, oshima wrote:
> nit {}
> 
> or early exit

Done.

https://codereview.chromium.org/2383263002/diff/100001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/2383263002/diff/100001/ui/compositor/layer.h#...
ui/compositor/layer.h:78: Layer(const Layer&);
On 2016/10/12 19:31:41, oshima wrote:
> explicit

Done.

https://codereview.chromium.org/2383263002/diff/100001/ui/compositor/layer.h#...
ui/compositor/layer.h:426: // LayerObserver overrides:
On 2016/10/12 19:31:41, oshima wrote:
> nit: // LayerObserver:

Done.

https://codereview.chromium.org/2383263002/diff/100001/ui/wm/core/window_util.h
File ui/wm/core/window_util.h (right):

https://codereview.chromium.org/2383263002/diff/100001/ui/wm/core/window_util...
ui/wm/core/window_util.h:63: LayerDelegateFactory* delegate_factory);
On 2016/10/12 19:31:41, oshima wrote:
> we ma want to remove delegate_factory because this was added to implement
> mirroring. I'll leave this to owners though.

True, all callers pass nullptr. sky, sadrul: WDYT?

[Dominik Laskowski, 2016-10-13 18:05:12]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 18:05:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 18:08:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 18:08:25]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Dominik Laskowski, 2016-10-13 18:19:14]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 18:19:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 19:15:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 19:15:50]

Dry run: This issue passed the CQ dry run.

[sky, 2016-10-13 19:21:33]

https://codereview.chromium.org/2383263002/diff/140001/ash/common/wm/forwardi...
File ash/common/wm/forwarding_layer_delegate.h (left):

https://codereview.chromium.org/2383263002/diff/140001/ash/common/wm/forwardi...
ash/common/wm/forwarding_layer_delegate.h:29: ForwardingLayerDelegate(ui::Layer*
new_layer, ui::Layer* original_layer);
In the context of the constructor client_layer doesn't mean anything. Please
leave it as new_layer. In the context of the class I would be tempted to rename
client_layer_ to layer_.

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.cc...
ui/compositor/layer.cc:960: mirror->SetBounds(bounds);
I don't think it's always the case that the mirror wants the same bounds as the
original. For example, in alt-tab I don't think it wants the bounds mirrored.

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.cc...
ui/compositor/layer.cc:1052: const auto it = std::find(mirrors_.begin(),
mirrors_.end(), mirror);
mirror->RemoveObserver(this).

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.h#...
ui/compositor/layer.h:78: explicit Layer(const Layer&);
The style guide generally frowns on this, especially for such a complex type,
and it isn't really clear what is copied and what isn't. I think it better to
inline what you need in Mirror().

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.h#...
ui/compositor/layer.h:457: std::vector<Layer*> mirrors_;  // Not owned.
std::vector->std::set as you don't need the order.

https://codereview.chromium.org/2383263002/diff/140001/ui/wm/core/window_util.cc
File ui/wm/core/window_util.cc (right):

https://codereview.chromium.org/2383263002/diff/140001/ui/wm/core/window_util...
ui/wm/core/window_util.cc:124: new
ui::LayerTreeOwner(root->layer()->Mirror().release()));
MakeUnique.
optional: using the release here is awkward. Would be nice if LayerTreeOwner
took a unique_ptr.

[Dominik Laskowski, 2016-10-13 22:19:39]

https://codereview.chromium.org/2383263002/diff/140001/ash/common/wm/forwardi...
File ash/common/wm/forwarding_layer_delegate.h (left):

https://codereview.chromium.org/2383263002/diff/140001/ash/common/wm/forwardi...
ash/common/wm/forwarding_layer_delegate.h:29: ForwardingLayerDelegate(ui::Layer*
new_layer, ui::Layer* original_layer);
On 2016/10/13 19:21:33, sky wrote:
> In the context of the constructor client_layer doesn't mean anything. Please
> leave it as new_layer. In the context of the class I would be tempted to
rename
> client_layer_ to layer_.

I was going for consistency, but I agree. new_layer and layer_ it is.

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.cc...
ui/compositor/layer.cc:960: mirror->SetBounds(bounds);
On 2016/10/13 19:21:33, sky wrote:
> I don't think it's always the case that the mirror wants the same bounds as
the
> original. For example, in alt-tab I don't think it wants the bounds mirrored.

Good point. I've added an option to disable bounds syncing in MirrorLayers.

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.cc...
ui/compositor/layer.cc:1052: const auto it = std::find(mirrors_.begin(),
mirrors_.end(), mirror);
On 2016/10/13 19:21:33, sky wrote:
> mirror->RemoveObserver(this).

The caller is the destructor of |mirror|, so that'd be redundant.

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.h#...
ui/compositor/layer.h:78: explicit Layer(const Layer&);
On 2016/10/13 19:21:33, sky wrote:
> The style guide generally frowns on this, especially for such a complex type,
> and it isn't really clear what is copied and what isn't. I think it better to
> inline what you need in Mirror().

It's also called from LayerOwner::RecreateLayer. Would a Layer::Clone be
preferable?

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.h#...
ui/compositor/layer.h:457: std::vector<Layer*> mirrors_;  // Not owned.
On 2016/10/13 19:21:33, sky wrote:
> std::vector->std::set as you don't need the order.

I started with std::unordered_set, but figured std::vector is more appropriate
given that iteration is more frequent than removal/insertion and lookup isn't
needed.

https://codereview.chromium.org/2383263002/diff/140001/ui/wm/core/window_util.cc
File ui/wm/core/window_util.cc (right):

https://codereview.chromium.org/2383263002/diff/140001/ui/wm/core/window_util...
ui/wm/core/window_util.cc:124: new
ui::LayerTreeOwner(root->layer()->Mirror().release()));
On 2016/10/13 19:21:33, sky wrote:
> MakeUnique.
> optional: using the release here is awkward. Would be nice if LayerTreeOwner
> took a unique_ptr.

Done.

[Dominik Laskowski, 2016-10-13 22:28:41]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 22:29:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 22:56:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 22:56:08]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Dominik Laskowski, 2016-10-13 23:06:02]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 23:06:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dominik Laskowski, 2016-10-13 23:45:55]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 23:46:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 00:38:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 00:38:20]

Dry run: This issue passed the CQ dry run.

[sky, 2016-10-14 15:18:41]

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.h#...
ui/compositor/layer.h:78: explicit Layer(const Layer&);
On 2016/10/13 22:19:39, Dominik Laskowski wrote:
> On 2016/10/13 19:21:33, sky wrote:
> > The style guide generally frowns on this, especially for such a complex
type,
> > and it isn't really clear what is copied and what isn't. I think it better
to
> > inline what you need in Mirror().
> 
> It's also called from LayerOwner::RecreateLayer. Would a Layer::Clone be
> preferable?

Yes please.

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.h#...
ui/compositor/layer.h:457: std::vector<Layer*> mirrors_;  // Not owned.
On 2016/10/13 22:19:39, Dominik Laskowski wrote:
> On 2016/10/13 19:21:33, sky wrote:
> > std::vector->std::set as you don't need the order.
> 
> I started with std::unordered_set, but figured std::vector is more appropriate
> given that iteration is more frequent than removal/insertion and lookup isn't
> needed.

I like set to convey it's unordered, but ok.

[Dominik Laskowski, 2016-10-14 19:14:00]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 19:14:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dominik Laskowski, 2016-10-14 19:15:02]

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/2383263002/diff/140001/ui/compositor/layer.h#...
ui/compositor/layer.h:78: explicit Layer(const Layer&);
On 2016/10/14 15:18:41, sky wrote:
> On 2016/10/13 22:19:39, Dominik Laskowski wrote:
> > On 2016/10/13 19:21:33, sky wrote:
> > > The style guide generally frowns on this, especially for such a complex
> type,
> > > and it isn't really clear what is copied and what isn't. I think it better
> to
> > > inline what you need in Mirror().
> > 
> > It's also called from LayerOwner::RecreateLayer. Would a Layer::Clone be
> > preferable?
> 
> Yes please.

Done.

[commit-bot: I haz the power, 2016-10-14 19:17:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 19:17:53]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Dominik Laskowski, 2016-10-14 19:36:39]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 19:37:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 20:41:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 20:41:22]

Dry run: This issue passed the CQ dry run.

[sky, 2016-10-14 22:36:37]

https://codereview.chromium.org/2383263002/diff/260001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/260001/ui/compositor/layer.cc...
ui/compositor/layer.cc:128: clone->SetOpacity(GetTargetOpacity());
What about
int background_blur_radius_, satuations, brightness, grayscale, inverted, mask,
zoom...?

ALso, how about changing the name of the clone to be name_ + "clone" ?

https://codereview.chromium.org/2383263002/diff/260001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/2383263002/diff/260001/ui/compositor/layer.h#...
ui/compositor/layer.h:459: bool sync_bounds_ = false;     // Synchronize bounds
to mirrors.
Please add a more descriptive comment. Maybe something like "If true, any
changes made to the bounds of this layer are propagated to mirrors."

[Dominik Laskowski, 2016-10-17 19:53:21]

PTAL at the additional refactoring. The latest patch removes
LayerDelegateFactory and ForwardingLayerDelegate in favor of integrating that
functionality into the Layer class. Layer::Mirror can now mirror both surface
content and content painted by a delegate. The previous code forced users of
RecreateLayers to manage LayerDelegate lifetime for the mirrors, which caused
the dangling pointer bug. The delegate is now owned by the mirrored layer.

https://codereview.chromium.org/2383263002/diff/260001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/260001/ui/compositor/layer.cc...
ui/compositor/layer.cc:128: clone->SetOpacity(GetTargetOpacity());
On 2016/10/14 22:36:37, sky wrote:
> What about
> int background_blur_radius_, satuations, brightness, grayscale, inverted,
mask,
> zoom...?
> 
> ALso, how about changing the name of the clone to be name_ + "clone" ?

This code was pulled from LayerOwner::RecreateLayer, so this subset was
apparently sufficient. Anyway, I've updated it to copy the remaining Layer
state. The cc::Layer content isn't copied for all layer types, but this patch is
only intended for surface mirroring, so I've added a cautionary comment for now.

Based on a quick code search, the convention is to delimit layer names with
colons, so I've changed the name accordingly.

https://codereview.chromium.org/2383263002/diff/260001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/2383263002/diff/260001/ui/compositor/layer.h#...
ui/compositor/layer.h:459: bool sync_bounds_ = false;     // Synchronize bounds
to mirrors.
On 2016/10/14 22:36:37, sky wrote:
> Please add a more descriptive comment. Maybe something like "If true, any
> changes made to the bounds of this layer are propagated to mirrors."

Done.

[Dominik Laskowski, 2016-10-17 22:22:30]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 22:23:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 22:45:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-17 22:45:59]

Dry run: Exceeded global retry quota

[Dominik Laskowski, 2016-10-17 23:19:53]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 23:20:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 00:17:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 00:17:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dominik Laskowski, 2016-10-18 01:02:46]

On 2016/10/17 19:53:21, Dominik Laskowski wrote:
> PTAL at the additional refactoring. The latest patch removes
> LayerDelegateFactory and ForwardingLayerDelegate in favor of integrating that
> functionality into the Layer class. Layer::Mirror can now mirror both surface
> content and content painted by a delegate. The previous code forced users of
> RecreateLayers to manage LayerDelegate lifetime for the mirrors, which caused
> the dangling pointer bug. The delegate is now owned by the mirrored layer.
> 
> https://codereview.chromium.org/2383263002/diff/260001/ui/compositor/layer.cc
> File ui/compositor/layer.cc (right):
> 
>
https://codereview.chromium.org/2383263002/diff/260001/ui/compositor/layer.cc...
> ui/compositor/layer.cc:128: clone->SetOpacity(GetTargetOpacity());
> On 2016/10/14 22:36:37, sky wrote:
> > What about
> > int background_blur_radius_, satuations, brightness, grayscale, inverted,
> mask,
> > zoom...?
> > 
> > ALso, how about changing the name of the clone to be name_ + "clone" ?
> 
> This code was pulled from LayerOwner::RecreateLayer, so this subset was
> apparently sufficient. Anyway, I've updated it to copy the remaining Layer
> state. The cc::Layer content isn't copied for all layer types, but this patch
is
> only intended for surface mirroring, so I've added a cautionary comment for
now.
> 
> Based on a quick code search, the convention is to delimit layer names with
> colons, so I've changed the name accordingly.
> 
> https://codereview.chromium.org/2383263002/diff/260001/ui/compositor/layer.h
> File ui/compositor/layer.h (right):
> 
>
https://codereview.chromium.org/2383263002/diff/260001/ui/compositor/layer.h#...
> ui/compositor/layer.h:459: bool sync_bounds_ = false;     // Synchronize
bounds
> to mirrors.
> On 2016/10/14 22:36:37, sky wrote:
> > Please add a more descriptive comment. Maybe something like "If true, any
> > changes made to the bounds of this layer are propagated to mirrors."
> 
> Done.

Never mind about layer renaming. The name of LayerOwner::layer() should be the
same before and after RecreateLayer.

[Dominik Laskowski, 2016-10-18 01:02:48]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 01:03:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 03:37:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 03:37:18]

Dry run: This issue passed the CQ dry run.

[sky, 2016-10-19 19:27:14]

Oshima should look at this again too.

https://codereview.chromium.org/2383263002/diff/340001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/340001/ui/compositor/layer.cc...
ui/compositor/layer.cc:52: class Layer::LayerMirror : public ui::LayerOwner,
I find this code confusing. In particular it's a LayerOwner, yet the layer is
immediately released in AcquireLayer. What do you gain by making this class a
LayerOwner? Additionally why not make this class directly be the LayerObserver?
That way Layer itself isn't a LayerObserver only for some rare cases and
additionally Layer::LayerDestroyed() wouldn't need to lookup the LayerMirror.

https://codereview.chromium.org/2383263002/diff/340001/ui/compositor/layer_ow...
File ui/compositor/layer_owner.h (right):

https://codereview.chromium.org/2383263002/diff/340001/ui/compositor/layer_ow...
ui/compositor/layer_owner.h:47: bool OwnsLayer() const;
Why does this need to be public?

https://codereview.chromium.org/2383263002/diff/340001/ui/wm/core/window_util.h
File ui/wm/core/window_util.h (right):

https://codereview.chromium.org/2383263002/diff/340001/ui/wm/core/window_util...
ui/wm/core/window_util.h:50: ui::LayerOwner* root, bool sync_bounds);
Document what sync_bounds means (you can just say look at Layer::sync_bounds.

[Dominik Laskowski, 2016-10-19 22:46:38]

Thanks for the review so far.

https://codereview.chromium.org/2383263002/diff/340001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/340001/ui/compositor/layer.cc...
ui/compositor/layer.cc:52: class Layer::LayerMirror : public ui::LayerOwner,
On 2016/10/19 19:27:14, sky wrote:
> I find this code confusing. In particular it's a LayerOwner, yet the layer is
> immediately released in AcquireLayer. What do you gain by making this class a
> LayerOwner? Additionally why not make this class directly be the
LayerObserver?
> That way Layer itself isn't a LayerObserver only for some rare cases and
> additionally Layer::LayerDestroyed() wouldn't need to lookup the LayerMirror.

The LayerOwner inheritance was just to reduce boilerplate, but I've removed it
for clarity. The Layer class must be the observer, because it must remove
destroyed LayerMirror instances from its list of mirrors.

https://codereview.chromium.org/2383263002/diff/340001/ui/compositor/layer_ow...
File ui/compositor/layer_owner.h (right):

https://codereview.chromium.org/2383263002/diff/340001/ui/compositor/layer_ow...
ui/compositor/layer_owner.h:47: bool OwnsLayer() const;
On 2016/10/19 19:27:14, sky wrote:
> Why does this need to be public?

For the DCHECK in wm::RecreateLayers, which I think makes more sense than
returning a LayerTreeOwner pointing to null if the input LayerOwner does not own
a layer.

https://codereview.chromium.org/2383263002/diff/340001/ui/wm/core/window_util.h
File ui/wm/core/window_util.h (right):

https://codereview.chromium.org/2383263002/diff/340001/ui/wm/core/window_util...
ui/wm/core/window_util.h:50: ui::LayerOwner* root, bool sync_bounds);
On 2016/10/19 19:27:14, sky wrote:
> Document what sync_bounds means (you can just say look at Layer::sync_bounds.

Done.

[sky, 2016-10-19 23:44:26]

https://codereview.chromium.org/2383263002/diff/340001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/340001/ui/compositor/layer.cc...
ui/compositor/layer.cc:52: class Layer::LayerMirror : public ui::LayerOwner,
On 2016/10/19 22:46:38, Dominik Laskowski wrote:
> On 2016/10/19 19:27:14, sky wrote:
> > I find this code confusing. In particular it's a LayerOwner, yet the layer
is
> > immediately released in AcquireLayer. What do you gain by making this class
a
> > LayerOwner? Additionally why not make this class directly be the
> LayerObserver?
> > That way Layer itself isn't a LayerObserver only for some rare cases and
> > additionally Layer::LayerDestroyed() wouldn't need to lookup the
LayerMirror.
> 
> The LayerOwner inheritance was just to reduce boilerplate, but I've removed it
> for clarity. The Layer class must be the observer, because it must remove
> destroyed LayerMirror instances from its list of mirrors.

I was suggesting you make this class the LayerObserver and in OnLayerDestroyed
call a private function in the host layer, e.g. OnMirroredLayerDestroyed(this).

[Dominik Laskowski, 2016-10-20 02:10:47]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 02:11:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dominik Laskowski, 2016-10-20 02:12:30]

https://codereview.chromium.org/2383263002/diff/340001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/340001/ui/compositor/layer.cc...
ui/compositor/layer.cc:52: class Layer::LayerMirror : public ui::LayerOwner,
On 2016/10/19 23:44:26, sky wrote:
> On 2016/10/19 22:46:38, Dominik Laskowski wrote:
> > On 2016/10/19 19:27:14, sky wrote:
> > > I find this code confusing. In particular it's a LayerOwner, yet the layer
> is
> > > immediately released in AcquireLayer. What do you gain by making this
class
> a
> > > LayerOwner? Additionally why not make this class directly be the
> > LayerObserver?
> > > That way Layer itself isn't a LayerObserver only for some rare cases and
> > > additionally Layer::LayerDestroyed() wouldn't need to lookup the
> LayerMirror.
> > 
> > The LayerOwner inheritance was just to reduce boilerplate, but I've removed
it
> > for clarity. The Layer class must be the observer, because it must remove
> > destroyed LayerMirror instances from its list of mirrors.
> 
> I was suggesting you make this class the LayerObserver and in OnLayerDestroyed
> call a private function in the host layer, e.g.
OnMirroredLayerDestroyed(this).

Done.

[commit-bot: I haz the power, 2016-10-20 02:15:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 02:15:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Dominik Laskowski, 2016-10-20 02:23:17]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 02:24:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 04:12:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 04:12:37]

Dry run: This issue passed the CQ dry run.

[sky, 2016-10-20 15:36:39]

LGTM - Oshima should take another look too though.

[Dominik Laskowski, 2016-10-20 23:10:35]

On 2016/10/20 15:36:39, sky wrote:
> LGTM - Oshima should take another look too though.

The latest patch addresses offline feedback from oshima, specifically that
mirrors of a layer painted by a delegate were not invalidated. Another possible
refactoring pointed out by oshima is that LayerObserver is only used for
mirroring, so it can be removed and replaced by a private equivalent in the
Layer class. However, since this CL fixes release blockers, I would like to
address this (and add unit tests) in a separate patch.

danakj, sadrul: PTAL

[Dominik Laskowski, 2016-10-20 23:39:25]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 23:39:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 02:37:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 02:37:23]

Dry run: This issue passed the CQ dry run.

[oshima, 2016-10-21 13:35:09]

lgtm with one nit.

cleaning up in a separate CL sgtm.

https://codereview.chromium.org/2383263002/diff/380001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/380001/ui/compositor/layer.cc...
ui/compositor/layer.cc:65: Layer* layer() const { return mirror_; }
remote const since you're returning (exposing) internal object as non const.

you'd need to remove const in for loop, but that's correct thing.

[Dominik Laskowski, 2016-10-21 18:22:54]

danakj, sadrul: Friendly ping. The release is scheduled for Monday, so this CL
would have to land on Sunday at the latest.

https://codereview.chromium.org/2383263002/diff/380001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/380001/ui/compositor/layer.cc...
ui/compositor/layer.cc:65: Layer* layer() const { return mirror_; }
On 2016/10/21 13:35:09, oshima wrote:
> remote const since you're returning (exposing) internal object as non const.
> 
> you'd need to remove const in for loop, but that's correct thing.

Done. No need to update the loops, because the "const" applies to the pointer.

[Dominik Laskowski, 2016-10-21 18:24:47]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 18:25:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 20:15:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 20:15:34]

Dry run: This issue passed the CQ dry run.

[bhthompson1, 2016-10-24 17:32:30]

bhthompson@google.com changed reviewers:
+ bhthompson@google.com

[bhthompson1, 2016-10-24 17:32:32]

Ping for review, we need to get this in to unblock R55 beta.

[danakj, 2016-10-24 20:28:01]

ui/compositor LG overall

Can you add layer unit test that mirrors a painted layer, paints the original
layer, and verifies the observing layer is updated too within the specified
damage region?

Also it seems like at least some of the LayerOwner tests should be Layer tests
now like LayerOwnerTest.RecreateLayerWithTransform which is really testing
Clone, but now from a distance.

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:54: LayerMirror(Layer* mirrored, Layer* mirror)
nit: mirror_source and mirror_dest? or just source and dest?

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:65: Layer* layer() { return mirror_; }
This method's name is ambiguous can it specify its the source/dest layer in the
relationship?

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:69: if (const auto delegate = mirrored_->delegate())
auto*

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:740: if (animator_.get() &&
animator_->IsAnimatingProperty(
no .get() needed

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:887: mirror->layer()->SchedulePaint(invalidation);
I would expect SchedulePaint for the whole layers bounds when Mirror() happens,
and then propogate SchedulePaint() calls from SchedulePaint(), and do nothing
here. Would that work?

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.h#...
ui/compositor/layer.h:80: std::unique_ptr<Layer> Clone() const;
Can you group Clone() and Mirror() together? People will find one but not the
other. Also can you make it more clear that Mirror is a clone that continues to
update with these names? MirrorOnce, MirrorForever or Clone, CloneAndMirror, or
something?

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer_ow...
File ui/compositor/layer_owner.cc (right):

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer_ow...
ui/compositor/layer_owner.cc:41: SetLayer(old_layer->Clone());
This method could be a lot simpler, just return old_layer->Clone(), but
OnLayerRecreated implementations might need to be looked at, so maybe not for
this CL.

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer_ow...
File ui/compositor/layer_owner.h (right):

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer_ow...
ui/compositor/layer_owner.h:47: bool OwnsLayer() const;
Can this remain not public and you can DCHECK it in LayerOwner::RecreateLayer()
instead of in LayerTreeOwner?

[Dominik Laskowski, 2016-10-24 22:58:05]

Thanks for the review. Good points about unit tests, but would you mind if they
landed in a follow-up CL along with the remaining refactoring?

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:54: LayerMirror(Layer* mirrored, Layer* mirror)
On 2016/10/24 20:28:02, danakj wrote:
> nit: mirror_source and mirror_dest? or just source and dest?

Done.

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:65: Layer* layer() { return mirror_; }
On 2016/10/24 20:28:02, danakj wrote:
> This method's name is ambiguous can it specify its the source/dest layer in
the
> relationship?

Done.

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:69: if (const auto delegate = mirrored_->delegate())
On 2016/10/24 20:28:02, danakj wrote:
> auto*

OnPaintLayer isn't const.

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:740: if (animator_.get() &&
animator_->IsAnimatingProperty(
On 2016/10/24 20:28:02, danakj wrote:
> no .get() needed

Done, and elsewhere too.

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:887: mirror->layer()->SchedulePaint(invalidation);
On 2016/10/24 20:28:02, danakj wrote:
> I would expect SchedulePaint for the whole layers bounds when Mirror()
happens,
> and then propogate SchedulePaint() calls from SchedulePaint(), and do nothing
> here. Would that work?

I was also wondering why DidPaintLayer was called here and not from
SchedulePaint, but it's not a trivial refactoring so I'd rather not attempt it
in this CL.

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.h#...
ui/compositor/layer.h:80: std::unique_ptr<Layer> Clone() const;
On 2016/10/24 20:28:02, danakj wrote:
> Can you group Clone() and Mirror() together? People will find one but not the
> other. Also can you make it more clear that Mirror is a clone that continues
to
> update with these names? MirrorOnce, MirrorForever or Clone, CloneAndMirror,
or
> something?

Done, but I think "Clone" and "Mirror" are self-explanatory given their meaning
in the real world.

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer_ow...
File ui/compositor/layer_owner.cc (right):

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer_ow...
ui/compositor/layer_owner.cc:41: SetLayer(old_layer->Clone());
On 2016/10/24 20:28:02, danakj wrote:
> This method could be a lot simpler, just return old_layer->Clone(), but
> OnLayerRecreated implementations might need to be looked at, so maybe not for
> this CL.

Right, some callers would have to be modified because they rely on the ownership
transfer.

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer_ow...
File ui/compositor/layer_owner.h (right):

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer_ow...
ui/compositor/layer_owner.h:47: bool OwnsLayer() const;
On 2016/10/24 20:28:02, danakj wrote:
> Can this remain not public and you can DCHECK it in
LayerOwner::RecreateLayer()
> instead of in LayerTreeOwner?

I tried this in patch 16, but reverted it because some callers expect
RecreateLayer to return null if OwnsLayer returns false.

[Dominik Laskowski, 2016-10-24 23:02:53]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 23:03:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 23:09:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-24 23:09:19]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dominik Laskowski, 2016-10-24 23:15:07]

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:69: if (const auto delegate = mirrored_->delegate())
On 2016/10/24 22:58:05, Dominik Laskowski wrote:
> On 2016/10/24 20:28:02, danakj wrote:
> > auto*
> 
> OnPaintLayer isn't const.

...or did you mean:

if (auto* delegate = ...

[Dominik Laskowski, 2016-10-24 23:32:27]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 23:33:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-10-24 23:57:57]

> Thanks for the review. Good points about unit tests, but 
> would you mind if they landed in a follow-up CL along with
> the remaining refactoring?

TODOs for the refactoring sound ok. I'd prefer that tests land with the code. It
makes history more clear and it's good practice.

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:69: if (const auto delegate = mirrored_->delegate())
On 2016/10/24 23:15:07, Dominik Laskowski wrote:
> On 2016/10/24 22:58:05, Dominik Laskowski wrote:
> > On 2016/10/24 20:28:02, danakj wrote:
> > > auto*
> > 
> > OnPaintLayer isn't const.
> 
> ...or did you mean:
> 
> if (auto* delegate = ...

That'd be fine yes. It's a raw pointer so denote it with a *

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:887: mirror->layer()->SchedulePaint(invalidation);
On 2016/10/24 22:58:04, Dominik Laskowski wrote:
> On 2016/10/24 20:28:02, danakj wrote:
> > I would expect SchedulePaint for the whole layers bounds when Mirror()
> happens,
> > and then propogate SchedulePaint() calls from SchedulePaint(), and do
nothing
> > here. Would that work?
> 
> I was also wondering why DidPaintLayer was called here and not from
> SchedulePaint, but it's not a trivial refactoring so I'd rather not attempt it
> in this CL.

Would you mind leaving a TODO describing this all then? And maybe file and point
it at a bug?

[commit-bot: I haz the power, 2016-10-25 00:59:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 00:59:09]

Dry run: This issue passed the CQ dry run.

[Dominik Laskowski, 2016-10-25 22:25:40]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 22:26:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dominik Laskowski, 2016-10-25 22:44:57]

PTAL. I've added tests for mirroring, and moved/merged LayerOwner tests related
to cloning.

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:69: if (const auto delegate = mirrored_->delegate())
On 2016/10/24 23:57:57, danakj wrote:
> On 2016/10/24 23:15:07, Dominik Laskowski wrote:
> > On 2016/10/24 22:58:05, Dominik Laskowski wrote:
> > > On 2016/10/24 20:28:02, danakj wrote:
> > > > auto*
> > > 
> > > OnPaintLayer isn't const.
> > 
> > ...or did you mean:
> > 
> > if (auto* delegate = ...
> 
> That'd be fine yes. It's a raw pointer so denote it with a *

Done.

https://codereview.chromium.org/2383263002/diff/440001/ui/compositor/layer.cc...
ui/compositor/layer.cc:887: mirror->layer()->SchedulePaint(invalidation);
On 2016/10/24 23:57:57, danakj wrote:
> On 2016/10/24 22:58:04, Dominik Laskowski wrote:
> > On 2016/10/24 20:28:02, danakj wrote:
> > > I would expect SchedulePaint for the whole layers bounds when Mirror()
> > happens,
> > > and then propogate SchedulePaint() calls from SchedulePaint(), and do
> nothing
> > > here. Would that work?
> > 
> > I was also wondering why DidPaintLayer was called here and not from
> > SchedulePaint, but it's not a trivial refactoring so I'd rather not attempt
it
> > in this CL.
> 
> Would you mind leaving a TODO describing this all then? And maybe file and
point
> it at a bug?

Done, but a bug seems overkill.

[danakj, 2016-10-25 23:08:14]

Thanks LGTM. Still think Clone() and Mirror() don't describe their relationship
to eachother well, but idk how to convince you of that.

[commit-bot: I haz the power, 2016-10-25 23:26:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 23:26:11]

Dry run: This issue passed the CQ dry run.

[Dominik Laskowski, 2016-10-26 00:03:10]

sadrul: PTAL at the minor cleanup in content/browser/renderer_host/

[sadrul, 2016-10-26 18:59:27]

lgtm

[Dominik Laskowski, 2016-10-26 19:21:07]

The CQ bit was checked by domlaskowski@chromium.org

[Dominik Laskowski, 2016-10-26 19:21:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, sky@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2383263002/#ps480001
(title: "Add unit tests and TODOs")

[commit-bot: I haz the power, 2016-10-26 19:22:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 19:33:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 19:33:52]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Dominik Laskowski, 2016-10-26 19:50:12]

On 2016/10/26 19:33:52, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Committing after trivial rebase.

[Dominik Laskowski, 2016-10-26 19:50:23]

The CQ bit was checked by domlaskowski@chromium.org

[Dominik Laskowski, 2016-10-26 19:50:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, sky@chromium.org, oshima@chromium.org,
danakj@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2383263002/#ps500001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-26 19:50:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 20:04:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 20:04:30]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Dominik Laskowski, 2016-10-26 21:36:52]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 21:37:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 23:33:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 23:33:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dominik Laskowski, 2016-10-26 23:44:53]

The CQ bit was checked by domlaskowski@chromium.org

[commit-bot: I haz the power, 2016-10-26 23:45:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 00:35:07]

Description was changed from

==========
Generalize layer mirroring for phantom windows

Previously, the layers of a phantom window were recreated when the
surface changed in the corresponding layers of the original window.
The code also assumed that layers with surfaces are leaves in the
layer tree, but Exosphere surfaces are nested. Recursive layer
cloning would be inefficient for surfaces changing in each frame,
so this CL adds an alternative way to mirror layers. In addition,
the bounds of Exosphere surfaces relative to their parent window
are updated whenever the window moves, so this CL provides a way
to synchronize relative positions of layers in phantom windows.

In addition to implementing phantom windows for ARC apps, this CL
fixes three bugs related to mirroring:

  1) Memory corruption due to the non-recursive layer cloning.
  2) Dangling pointers due to observers sticking around.
  3) Black Alt+Tab previews for ARC windows.

BUG=642894
BUG=649452
BUG=653113
TEST=ARC apps that render continuously (e.g. games, video players)
     can be dragged to external displays, and their content is
     mirrored in phantom windows.
TEST=ARC apps are mirrored in Alt+Tab previews.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Generalize layer mirroring for phantom windows

Previously, the layers of a phantom window were recreated when the
surface changed in the corresponding layers of the original window.
The code also assumed that layers with surfaces are leaves in the
layer tree, but Exosphere surfaces are nested. Recursive layer
cloning would be inefficient for surfaces changing in each frame,
so this CL adds an alternative way to mirror layers. In addition,
the bounds of Exosphere surfaces relative to their parent window
are updated whenever the window moves, so this CL provides a way
to synchronize relative positions of layers in phantom windows.

In addition to implementing phantom windows for ARC apps, this CL
fixes three bugs related to mirroring:

  1) Memory corruption due to the non-recursive layer cloning.
  2) Dangling pointers due to observers sticking around.
  3) Black Alt+Tab previews for ARC windows.

BUG=642894
BUG=649452
BUG=653113
TEST=ARC apps that render continuously (e.g. games, video players)
     can be dragged to external displays, and their content is
     mirrored in phantom windows.
TEST=ARC apps are mirrored in Alt+Tab previews.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-10-27 00:35:09]

Committed patchset #26 (id:500001)

[commit-bot: I haz the power, 2016-10-27 00:38:23]

Description was changed from

==========
Generalize layer mirroring for phantom windows

Previously, the layers of a phantom window were recreated when the
surface changed in the corresponding layers of the original window.
The code also assumed that layers with surfaces are leaves in the
layer tree, but Exosphere surfaces are nested. Recursive layer
cloning would be inefficient for surfaces changing in each frame,
so this CL adds an alternative way to mirror layers. In addition,
the bounds of Exosphere surfaces relative to their parent window
are updated whenever the window moves, so this CL provides a way
to synchronize relative positions of layers in phantom windows.

In addition to implementing phantom windows for ARC apps, this CL
fixes three bugs related to mirroring:

  1) Memory corruption due to the non-recursive layer cloning.
  2) Dangling pointers due to observers sticking around.
  3) Black Alt+Tab previews for ARC windows.

BUG=642894
BUG=649452
BUG=653113
TEST=ARC apps that render continuously (e.g. games, video players)
     can be dragged to external displays, and their content is
     mirrored in phantom windows.
TEST=ARC apps are mirrored in Alt+Tab previews.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Generalize layer mirroring for phantom windows

Previously, the layers of a phantom window were recreated when the
surface changed in the corresponding layers of the original window.
The code also assumed that layers with surfaces are leaves in the
layer tree, but Exosphere surfaces are nested. Recursive layer
cloning would be inefficient for surfaces changing in each frame,
so this CL adds an alternative way to mirror layers. In addition,
the bounds of Exosphere surfaces relative to their parent window
are updated whenever the window moves, so this CL provides a way
to synchronize relative positions of layers in phantom windows.

In addition to implementing phantom windows for ARC apps, this CL
fixes three bugs related to mirroring:

  1) Memory corruption due to the non-recursive layer cloning.
  2) Dangling pointers due to observers sticking around.
  3) Black Alt+Tab previews for ARC windows.

BUG=642894
BUG=649452
BUG=653113
TEST=ARC apps that render continuously (e.g. games, video players)
     can be dragged to external displays, and their content is
     mirrored in phantom windows.
TEST=ARC apps are mirrored in Alt+Tab previews.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/4b33bfeda856f7ae20ffa43176e5bd529d5a100a
Cr-Commit-Position: refs/heads/master@{#427891}
==========

[commit-bot: I haz the power, 2016-10-27 00:38:25]

Patchset 26 (id:??) landed as
https://crrev.com/4b33bfeda856f7ae20ffa43176e5bd529d5a100a
Cr-Commit-Position: refs/heads/master@{#427891}