cc : Add IsInIdtoIndexMap to property tres

cc : Add IsInIdtoIndexMap to property tres

And use that to update during animations.

BUG=609208
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/f2778b2fb94176efcbaab7502cc65279ee8e64e8
Cr-Commit-Position: refs/heads/master@{#395109}

[jaydasika, 2016-05-19 22:11:26]

Description was changed from

==========
cc : Add IsInIdtoIndexMap to property tres

And use that to update dusring animations.

BUG=609208
==========

to

==========
cc : Add IsInIdtoIndexMap to property tres

And use that to update dusring animations.

BUG=609208
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-05-19 22:16:21]

jaydasika@chromium.org changed reviewers:
+ ajuma@chromium.org, vollick@chromium.org, weiliangc@chromium.org

[jaydasika, 2016-05-19 22:16:22]

PTAL

https://codereview.chromium.org/1994333002/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1994333002/diff/1/cc/layers/layer_impl.cc#new...
cc/layers/layer_impl.cc:659: property_trees->transform_id_to_index_map[id()]);
There is unit tests for which transform_tree_index = -1, but
property_trees->transform_id_to_index_map[id()] isn't, which is weird! That's
the reason we were having the check for -1. I will investigate that in a
different CL.

[jaydasika, 2016-05-19 22:16:45]

Description was changed from

==========
cc : Add IsInIdtoIndexMap to property tres

And use that to update dusring animations.

BUG=609208
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc : Add IsInIdtoIndexMap to property tres

And use that to update during animations.

BUG=609208
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-05-20 04:03:16]

On 2016/05/19 22:16:22, jaydasika wrote:
> PTAL
> 
> https://codereview.chromium.org/1994333002/diff/1/cc/layers/layer_impl.cc
> File cc/layers/layer_impl.cc (right):
> 
>
https://codereview.chromium.org/1994333002/diff/1/cc/layers/layer_impl.cc#new...
> cc/layers/layer_impl.cc:659: property_trees->transform_id_to_index_map[id()]);
> There is unit tests for which transform_tree_index = -1, but
> property_trees->transform_id_to_index_map[id()] isn't, which is weird! That's
> the reason we were having the check for -1. I will investigate that in a
> different CL.

All bots are are happy after I have removed an element out of the map when it
changes its layer tree host. But I still don't understand the crashes. I
initially thought, maybe a layer is getting deleted and a new layer gets added
with the deleted layer's id and then if we call something like SetPosition we
will have old value in the map,  if all this happens before we could rebuild the
property trees. But that wouldn't explain the crash because if we still haven't
rebuild the property trees, we should still have a valid property tree node ? Is
there any place other than property tree builder where we clear the property
trees(I mean the individual transform/effect/scroll/clip trees) ?

[ajuma, 2016-05-20 15:34:38]

On 2016/05/20 04:03:16, jaydasika wrote:
> On 2016/05/19 22:16:22, jaydasika wrote:
> > PTAL
> > 
> > https://codereview.chromium.org/1994333002/diff/1/cc/layers/layer_impl.cc
> > File cc/layers/layer_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1994333002/diff/1/cc/layers/layer_impl.cc#new...
> > cc/layers/layer_impl.cc:659:
property_trees->transform_id_to_index_map[id()]);
> > There is unit tests for which transform_tree_index = -1, but
> > property_trees->transform_id_to_index_map[id()] isn't, which is weird!
That's
> > the reason we were having the check for -1. I will investigate that in a
> > different CL.
> 
> All bots are are happy after I have removed an element out of the map when it
> changes its layer tree host. But I still don't understand the crashes. I
> initially thought, maybe a layer is getting deleted and a new layer gets added
> with the deleted layer's id and then if we call something like SetPosition we
> will have old value in the map,  if all this happens before we could rebuild
the
> property trees. But that wouldn't explain the crash because if we still
haven't
> rebuild the property trees, we should still have a valid property tree node ?
Is
> there any place other than property tree builder where we clear the property
> trees(I mean the individual transform/effect/scroll/clip trees) ?

Were these tests crashing in Layer or in LayerImpl? Removing a Layer from its
LayerTreeHost causes the layer's property tree indices to get reset to -1, so it
makes sense that if a layer is removed and then re-added to the tree and the if
layer wasn't removed from the map, then the layer's id would be in the map but
the layer would have transform tree index -1 (and we'd crash if we tried to use
the layer's transform node).

[ajuma, 2016-05-20 15:48:14]

lgtm

https://codereview.chromium.org/1994333002/diff/40001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1994333002/diff/40001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1166: }
Nit: this has extra indentation

[chromium-reviews, 2016-05-20 15:50:46]

The tests are crashing in Layer. My question was why would they crash when
we try to use the transform node(even if transform tree index is -1, but
the entry is in map)? I can understand the crash if we have cleared the
transform tree but as far as I know transform tree gets cleared only in
property tree building.

On Fri, 20 May 2016, 8:34 a.m. , <ajuma@chromium.org> wrote:

> On 2016/05/20 04:03:16, jaydasika wrote:
> > On 2016/05/19 22:16:22, jaydasika wrote:
> > > PTAL
> > >
> > >
> https://codereview.chromium.org/1994333002/diff/1/cc/layers/layer_impl.cc
> > > File cc/layers/layer_impl.cc (right):
> > >
> > >
> >
>
>
https://codereview.chromium.org/1994333002/diff/1/cc/layers/layer_impl.cc#new...
> > > cc/layers/layer_impl.cc:659:
> property_trees->transform_id_to_index_map[id()]);
> > > There is unit tests for which transform_tree_index = -1, but
> > > property_trees->transform_id_to_index_map[id()] isn't, which is weird!
> That's
> > > the reason we were having the check for -1. I will investigate that in
> a
> > > different CL.
> >
> > All bots are are happy after I have removed an element out of the map
> when it
> > changes its layer tree host. But I still don't understand the crashes. I
> > initially thought, maybe a layer is getting deleted and a new layer gets
> added
> > with the deleted layer's id and then if we call something like
> SetPosition we
> > will have old value in the map, if all this happens before we could
> rebuild
> the
> > property trees. But that wouldn't explain the crash because if we still
> haven't
> > rebuild the property trees, we should still have a valid property tree
> node ?
> Is
> > there any place other than property tree builder where we clear the
> property
> > trees(I mean the individual transform/effect/scroll/clip trees) ?
>
> Were these tests crashing in Layer or in LayerImpl? Removing a Layer from
> its
> LayerTreeHost causes the layer's property tree indices to get reset to -1,
> so it
> makes sense that if a layer is removed and then re-added to the tree and
> the if
> layer wasn't removed from the map, then the layer's id would be in the map
> but
> the layer would have transform tree index -1 (and we'd crash if we tried
> to use
> the layer's transform node).
>
> https://codereview.chromium.org/1994333002/
>

-- 
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.

[ajuma, 2016-05-20 15:56:09]

On 2016/05/20 15:50:46, chromium-reviews wrote:
> The tests are crashing in Layer. My question was why would they crash when
> we try to use the transform node(even if transform tree index is -1, but
> the entry is in map)? I can understand the crash if we have cleared the
> transform tree but as far as I know transform tree gets cleared only in
> property tree building.

After checking whether the entry is the map, the transform_tree_index (rather
than the value in the map) is being used to get the transform node. So my guess
is the sequence is:
1) Remove Layer from LayerTreeHost. At this point, the property tree indexes get
reset to -1.
2) Add the Layer back to the same LayerTreeHost.
3) Check if the layer is in the id-to-index map (it is, since the map wasn't
cleared), then use the layer's transform_tree_index to get a transform node
(this gets null), and then try to use the node (crash).

[jaydasika, 2016-05-20 16:05:06]

https://codereview.chromium.org/1994333002/diff/40001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1994333002/diff/40001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1166: }
On 2016/05/20 15:48:14, ajuma wrote:
> Nit: this has extra indentation

Done.

[jaydasika, 2016-05-20 16:05:11]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2016-05-20 16:05:12]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1994333002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-05-20 16:05:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1994333002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1994333002/60001

[jaydasika, 2016-05-20 16:16:08]

On 2016/05/20 15:56:09, ajuma wrote:
> On 2016/05/20 15:50:46, chromium-reviews wrote:
> > The tests are crashing in Layer. My question was why would they crash when
> > we try to use the transform node(even if transform tree index is -1, but
> > the entry is in map)? I can understand the crash if we have cleared the
> > transform tree but as far as I know transform tree gets cleared only in
> > property tree building.
> 
> After checking whether the entry is the map, the transform_tree_index (rather
> than the value in the map) is being used to get the transform node. So my
guess
> is the sequence is:
> 1) Remove Layer from LayerTreeHost. At this point, the property tree indexes
get
> reset to -1.
> 2) Add the Layer back to the same LayerTreeHost.
> 3) Check if the layer is in the id-to-index map (it is, since the map wasn't
> cleared), then use the layer's transform_tree_index to get a transform node
> (this gets null), and then try to use the node (crash).

Ah, I forgot that we were using the transform tree index there. Thanks for the
clarification, the crashes make sense now.

[commit-bot: I haz the power, 2016-05-20 17:52:29]

Description was changed from

==========
cc : Add IsInIdtoIndexMap to property tres

And use that to update during animations.

BUG=609208
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc : Add IsInIdtoIndexMap to property tres

And use that to update during animations.

BUG=609208
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-05-20 17:52:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-20 17:53:47]

Description was changed from

==========
cc : Add IsInIdtoIndexMap to property tres

And use that to update during animations.

BUG=609208
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc : Add IsInIdtoIndexMap to property tres

And use that to update during animations.

BUG=609208
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/f2778b2fb94176efcbaab7502cc65279ee8e64e8
Cr-Commit-Position: refs/heads/master@{#395109}
==========

[commit-bot: I haz the power, 2016-05-20 17:53:48]

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

[rnephew (Reviews Here), 2016-05-23 18:20:08]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2000253003/ by rnephew@chromium.org.

The reason for reverting is: 
Breaking android perf test:
crbug.com/614022.