ElementAnimations should hold an ObservableList of AnimationPlayers.

ElementAnimations should hold an ObservableList of AnimationPlayers.

Before this, the AnimationPlayers were held in a linked list, and
modifying the list while iterating could cause undefined behavior.

BUG=631052
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/66cb1320242db9ce2954a539dc2d5410bf033dbd
Cr-Commit-Position: refs/heads/master@{#410152}

[ymalik, 2016-07-27 23:03:24]

Description was changed from

==========
ElementAnimations should hold an ObservableList of AnimationPlayers.

BUG=631052
==========

to

==========
ElementAnimations should hold an ObservableList of AnimationPlayers.

BUG=631052
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[ymalik, 2016-07-27 23:05:03]

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

[commit-bot: I haz the power, 2016-07-27 23:05:26]

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-07-27 23:05:27]

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

[commit-bot: I haz the power, 2016-07-27 23:05:27]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[ymalik, 2016-07-27 23:56:58]

Description was changed from

==========
ElementAnimations should hold an ObservableList of AnimationPlayers.

BUG=631052
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
ElementAnimations should hold an ObservableList of AnimationPlayers.

This patch fixes the crash that was exposed after 
https://codereview.chromium.org/1698093005.

Before this, the AnimationPlayers were held in a linked list, and
modifying the list while iterating caused undefined behavior.

BUG=631052
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[ymalik, 2016-07-27 23:56:58]

ymalik@chromium.org changed reviewers:
+ ajuma@chromium.org, alancutter@chromium.org

[jbroman, 2016-07-28 01:01:15]

https://codereview.chromium.org/2189813002/diff/20001/cc/animation/element_an...
File cc/animation/element_animations_unittest.cc (right):

https://codereview.chromium.org/2189813002/diff/20001/cc/animation/element_an...
cc/animation/element_animations_unittest.cc:300: class
TestGarbageCollectedAnimationPlayer : public AnimationPlayer {
drive-by: Please don't use the term "garbage collected" for ref-counted objects;
we generally use it to refer to a mark-and-sweep or similar collector, like
Oilpan or V8. "Destroyed", perhaps?

[ymalik, 2016-07-28 02:45:33]

https://codereview.chromium.org/2189813002/diff/20001/cc/animation/element_an...
File cc/animation/element_animations_unittest.cc (right):

https://codereview.chromium.org/2189813002/diff/20001/cc/animation/element_an...
cc/animation/element_animations_unittest.cc:300: class
TestGarbageCollectedAnimationPlayer : public AnimationPlayer {
On 2016/07/28 01:01:15, jbroman wrote:
> drive-by: Please don't use the term "garbage collected" for ref-counted
objects;
> we generally use it to refer to a mark-and-sweep or similar collector, like
> Oilpan or V8. "Destroyed", perhaps?

Thanks! Done.

[alancutter (OOO until 2018), 2016-07-28 07:37:13]

s/NotifyAnimationFinished/OnAnimationFinished/ in
third_party/WebKit/Source/platform/animation/CompositorAnimationPlayerTest.cpp
to make compile happy.

[ajuma, 2016-07-28 18:36:41]

lgtm

https://codereview.chromium.org/2189813002/diff/60001/cc/test/animation_timel...
File cc/test/animation_timelines_test_common.h (right):

https://codereview.chromium.org/2189813002/diff/60001/cc/test/animation_timel...
cc/test/animation_timelines_test_common.h:247: int started() { return started_;
}
Please rename the variables and getters so its clear that they represent a count
now (e.g. something like started_count_ or num_started_).

[loyso (OOO), 2016-08-03 03:45:06]

631052 is marked as already fixed. Could you add a comment, why we need this.
Thanks!

[alancutter (OOO until 2018), 2016-08-03 03:48:54]

On 2016/08/03 at 03:45:06, loyso wrote:
> 631052 is marked as already fixed. Could you add a comment, why we need this.
Thanks!

We should still implement this change even though the original bug has been
fixed, see the description for why.

[ymalik, 2016-08-03 22:16:43]

@alancutter, @loyso

PTAL :)

[alancutter (OOO until 2018), 2016-08-04 00:45:15]

lgtm, the description probably shouldn't mention fixing the crash as it didn't
actually do that.

https://codereview.chromium.org/2189813002/diff/80001/cc/animation/element_an...
File cc/animation/element_animations_unittest.cc (right):

https://codereview.chromium.org/2189813002/diff/80001/cc/animation/element_an...
cc/animation/element_animations_unittest.cc:215:
EXPECT_TRUE(timeline_->GetPlayerById(player->id()));
I wonder if it would be cleaner to replace the FOR_EACH_OBSERVER macro with
template<typename Callback> ObserverList::forEach(Callback). This would save you
from having to inline the macro here and there and also from using macros.
I'm not against the code that's already here but personally I'd swap this out
for a lambda.

[loyso (OOO), 2016-08-04 06:24:22]

On 2016/08/03 03:48:54, alancutter wrote:
> On 2016/08/03 at 03:45:06, loyso wrote:
> > 631052 is marked as already fixed. Could you add a comment, why we need
this.
> Thanks!
> We should still implement this change even though the original bug has been
> fixed, see the description for why.
Sorry, I don't understand the connection.
Also, there is a comment: "I'm still seeing the crash with
https://codereview.chromium.org/2189813002/"

We use the intrusive list here intentionally. We had no any AnimationPlayer
destructions/detachments in Notify* callbacks so far.
That's why we prefer the speed over the flexibility.
Am I missing something? Do we have some NotifyAnimationStarted with some
destruction/detachment inside?

[loyso (OOO), 2016-08-04 06:32:10]

https://codereview.chromium.org/2189813002/diff/80001/cc/animation/animation_...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/2189813002/diff/80001/cc/animation/animation_...
cc/animation/animation_player.h:77: virtual void
OnAnimationStarted(base::TimeTicks monotonic_time,
It's very sad to make it polymorphic only for the sake of testing purposes.

[loyso (OOO), 2016-08-04 06:37:31]

Yeah, I see. blink::Animation::notifyAnimationStarted may lead to 
destroyCompositorPlayer() call in Animation::setCompositorPending.

[loyso (OOO), 2016-08-04 06:50:37]

https://codereview.chromium.org/2189813002/diff/80001/cc/animation/animation_...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/2189813002/diff/80001/cc/animation/animation_...
cc/animation/animation_player.h:77: virtual void
OnAnimationStarted(base::TimeTicks monotonic_time,
On 2016/08/04 06:32:10, loyso wrote:
> It's very sad to make it polymorphic only for the sake of testing purposes.
I'm severely jet lagged but... Can we use
AnimationPlayer::set_animation_delegate in tests?

[loyso (OOO), 2016-08-04 07:00:13]

I mean, to provide your custom TestAnimationDelegate which combines the
detachment and counting.

[ymalik, 2016-08-04 15:31:47]

@loyso, PTAL :)

https://codereview.chromium.org/2189813002/diff/80001/cc/animation/animation_...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/2189813002/diff/80001/cc/animation/animation_...
cc/animation/animation_player.h:77: virtual void
OnAnimationStarted(base::TimeTicks monotonic_time,
On 2016/08/04 06:50:37, loyso wrote:
> On 2016/08/04 06:32:10, loyso wrote:
> > It's very sad to make it polymorphic only for the sake of testing purposes.
> I'm severely jet lagged but... Can we use
> AnimationPlayer::set_animation_delegate in tests?

Yes, I am detaching the player from the timeline from TestAnimationDelegate.
This way we don't have to make this polymorphic. Thanks.

https://codereview.chromium.org/2189813002/diff/80001/cc/animation/element_an...
File cc/animation/element_animations_unittest.cc (right):

https://codereview.chromium.org/2189813002/diff/80001/cc/animation/element_an...
cc/animation/element_animations_unittest.cc:215:
EXPECT_TRUE(timeline_->GetPlayerById(player->id()));
On 2016/08/04 00:45:15, alancutter wrote:
> I wonder if it would be cleaner to replace the FOR_EACH_OBSERVER macro with
> template<typename Callback> ObserverList::forEach(Callback). This would save
you
> from having to inline the macro here and there and also from using macros.
> I'm not against the code that's already here but personally I'd swap this out
> for a lambda.

Acknowledged.

[ymalik, 2016-08-04 15:31:59]

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

[commit-bot: I haz the power, 2016-08-04 15:32:27]

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-08-04 15:32:28]

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

[commit-bot: I haz the power, 2016-08-04 15:32:28]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[loyso (OOO), 2016-08-05 00:13:12]

On 2016/08/04 15:31:47, ymalik wrote:
> On 2016/08/04 00:45:15, alancutter wrote:
> > I wonder if it would be cleaner to replace the FOR_EACH_OBSERVER macro with
> > template<typename Callback> ObserverList::forEach(Callback). This would save
> you
> > from having to inline the macro here and there and also from using macros.
> > I'm not against the code that's already here but personally I'd swap this
out
> > for a lambda.
> Acknowledged.
I suspect that FOR_EACH_OBSERVER macro isn't supported by our code search and it
breaks all the xrefs where the method is actually called.
Am I right?

[loyso (OOO), 2016-08-05 00:13:25]

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/animation...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/animation...
cc/animation/animation_player.h:77: void OnAnimationStarted(base::TimeTicks
monotonic_time,
Any rationale for renaming? The comment "AnimationDelegate routing" means that
there is a chain of methods (ElementAnimations::NotifyPlayersAnimationStarted ->
AnimationPlayer::NotifyAnimationStarted -> AnimationDelegate::
NotifyAnimationStarted). No reason to introduce inconsistency in this chain
IMHO. Or you can rename it as a whole :)

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/animation...
cc/animation/animation_player.h:96: protected:
No reason for private section now. This is a final class.

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
File cc/animation/element_animations.cc (right):

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
cc/animation/element_animations.cc:1461: while ((player = it.GetNext()) !=
nullptr) {
Any comment, why this iteration is different and FOR_EACH_OBSERVER isn't used?

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
File cc/animation/element_animations.h (left):

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
cc/animation/element_animations.h:12: #include "base/containers/linked_list.h"
This is not needed anymore.

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
File cc/animation/element_animations_unittest.cc (right):

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
cc/animation/element_animations_unittest.cc:361:
animations->NotifyAnimationStarted(events->events_[0]);
Could you add the comment like "The actual detachment happens here, inside the
callback"

[loyso (OOO), 2016-08-05 00:18:48]

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
File cc/animation/element_animations.cc (right):

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
cc/animation/element_animations.cc:1459:
base::ObserverList<AnimationPlayer>::Iterator it(players_list_.get());
Use ElementAnimations::PlayersList type here, please.

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
File cc/animation/element_animations_unittest.cc (right):

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
cc/animation/element_animations_unittest.cc:194:
base::ObserverList<AnimationPlayer>::Iterator it(
ElementAnimations::PlayersList type here and everywhere?

https://codereview.chromium.org/2189813002/diff/100001/cc/test/animation_time...
File cc/test/animation_timelines_test_common.cc (right):

https://codereview.chromium.org/2189813002/diff/100001/cc/test/animation_time...
cc/test/animation_timelines_test_common.cc:499: ?
base::ObserverList<AnimationPlayer>::Iterator(
ElementAnimations::PlayersList type here and everywhere?

[loyso (OOO), 2016-08-05 00:19:51]

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/animation...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/animation...
cc/animation/animation_player.h:10: #include "base/containers/linked_list.h"
It becomes unused.

[loyso (OOO), 2016-08-05 00:21:13]

On 2016/08/04 00:45:15, alancutter wrote:
> the description probably shouldn't mention fixing the crash as it didn't
> actually do that.
+1

[ymalik, 2016-08-05 00:41:20]

Description was changed from

==========
ElementAnimations should hold an ObservableList of AnimationPlayers.

This patch fixes the crash that was exposed after 
https://codereview.chromium.org/1698093005.

Before this, the AnimationPlayers were held in a linked list, and
modifying the list while iterating caused undefined behavior.

BUG=631052
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
ElementAnimations should hold an ObservableList of AnimationPlayers.

Before this, the AnimationPlayers were held in a linked list, and
modifying the list while iterating could cause undefined behavior.

BUG=631052
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[ymalik, 2016-08-05 00:54:05]

@loyso, my apologies for missing the points you mentioned. Perhaps I was
jetlagged when I wrote this CL. PTAL :)

Updated description.

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/animation...
File cc/animation/animation_player.h (right):

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/animation...
cc/animation/animation_player.h:10: #include "base/containers/linked_list.h"
On 2016/08/05 00:19:51, loyso wrote:
> It becomes unused.

Done.

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/animation...
cc/animation/animation_player.h:77: void OnAnimationStarted(base::TimeTicks
monotonic_time,
On 2016/08/05 00:13:25, loyso wrote:
> Any rationale for renaming? The comment "AnimationDelegate routing" means that
> there is a chain of methods (ElementAnimations::NotifyPlayersAnimationStarted
->
> AnimationPlayer::NotifyAnimationStarted -> AnimationDelegate::
> NotifyAnimationStarted). No reason to introduce inconsistency in this chain
> IMHO. Or you can rename it as a whole :)

I did it because most uses of ObserverList had "on" as a prefix. No real reason,
you're right that it should probably remain notify.

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/animation...
cc/animation/animation_player.h:96: protected:
On 2016/08/05 00:13:25, loyso wrote:
> No reason for private section now. This is a final class.

You're right. Done.

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
File cc/animation/element_animations.cc (right):

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
cc/animation/element_animations.cc:1459:
base::ObserverList<AnimationPlayer>::Iterator it(players_list_.get());
On 2016/08/05 00:18:48, loyso wrote:
> Use ElementAnimations::PlayersList type here, please.

Totally, Done.

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
cc/animation/element_animations.cc:1461: while ((player = it.GetNext()) !=
nullptr) {
On 2016/08/05 00:13:25, loyso wrote:
> Any comment, why this iteration is different and FOR_EACH_OBSERVER isn't used?

We need to call player->OnAnimationTakeover with a new copy of the curve.

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
File cc/animation/element_animations.h (left):

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
cc/animation/element_animations.h:12: #include "base/containers/linked_list.h"
On 2016/08/05 00:13:25, loyso wrote:
> This is not needed anymore.

Done.

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
File cc/animation/element_animations_unittest.cc (right):

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
cc/animation/element_animations_unittest.cc:194:
base::ObserverList<AnimationPlayer>::Iterator it(
On 2016/08/05 00:18:48, loyso wrote:
> ElementAnimations::PlayersList type here and everywhere?

Done.

https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_a...
cc/animation/element_animations_unittest.cc:361:
animations->NotifyAnimationStarted(events->events_[0]);
On 2016/08/05 00:13:25, loyso wrote:
> Could you add the comment like "The actual detachment happens here, inside the
> callback"

Done.

https://codereview.chromium.org/2189813002/diff/100001/cc/test/animation_time...
File cc/test/animation_timelines_test_common.cc (right):

https://codereview.chromium.org/2189813002/diff/100001/cc/test/animation_time...
cc/test/animation_timelines_test_common.cc:499: ?
base::ObserverList<AnimationPlayer>::Iterator(
On 2016/08/05 00:18:48, loyso wrote:
> ElementAnimations::PlayersList type here and everywhere?

Done.

[ymalik, 2016-08-05 00:55:25]

On 2016/08/05 00:13:12, loyso wrote:
> On 2016/08/04 15:31:47, ymalik wrote:
> > On 2016/08/04 00:45:15, alancutter wrote:
> > > I wonder if it would be cleaner to replace the FOR_EACH_OBSERVER macro
with
> > > template<typename Callback> ObserverList::forEach(Callback). This would
save
> > you
> > > from having to inline the macro here and there and also from using macros.
> > > I'm not against the code that's already here but personally I'd swap this
> out
> > > for a lambda.
> > Acknowledged.
> I suspect that FOR_EACH_OBSERVER macro isn't supported by our code search and
it
> breaks all the xrefs where the method is actually called.
> Am I right?

That's right. We could add the proposed function in addition to
FOR_EACH_OBSERVER, but that's probably a separate CL.

[ymalik, 2016-08-05 01:00:52]

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

[commit-bot: I haz the power, 2016-08-05 01:01:10]

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-08-05 01:01:11]

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

[commit-bot: I haz the power, 2016-08-05 01:01:12]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[ymalik, 2016-08-05 01:01:18]

The CQ bit was unchecked by ymalik@chromium.org

[ymalik, 2016-08-05 01:01:26]

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

[ymalik, 2016-08-05 01:01:32]

The CQ bit was unchecked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-08-05 01:01:41]

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-08-05 01:01:42]

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

[commit-bot: I haz the power, 2016-08-05 01:01:43]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[loyso (OOO), 2016-08-05 01:11:18]

On 2016/08/05 00:55:25, ymalik wrote:
> On 2016/08/05 00:13:12, loyso wrote:
> > On 2016/08/04 15:31:47, ymalik wrote:
> > > On 2016/08/04 00:45:15, alancutter wrote:
> > > > I wonder if it would be cleaner to replace the FOR_EACH_OBSERVER macro
> with
> > > > template<typename Callback> ObserverList::forEach(Callback). This would
> save
> > > you
> > > > from having to inline the macro here and there and also from using
macros.
> > > > I'm not against the code that's already here but personally I'd swap
this
> > out
> > > > for a lambda.
> > > Acknowledged.
> > I suspect that FOR_EACH_OBSERVER macro isn't supported by our code search
and
> it
> > breaks all the xrefs where the method is actually called.
> > Am I right?
> That's right. We could add the proposed function in addition to
> FOR_EACH_OBSERVER, but that's probably a separate CL.
Can we avoid this macro then and use implicit iteration as in
ElementAnimations::NotifyPlayersAnimationTakeover?

[loyso (OOO), 2016-08-05 01:12:03]

On 2016/08/05 01:11:18, loyso wrote:
> On 2016/08/05 00:55:25, ymalik wrote:
> > On 2016/08/05 00:13:12, loyso wrote:
> > > On 2016/08/04 15:31:47, ymalik wrote:
> > > > On 2016/08/04 00:45:15, alancutter wrote:
> > > > > I wonder if it would be cleaner to replace the FOR_EACH_OBSERVER macro
> > with
> > > > > template<typename Callback> ObserverList::forEach(Callback). This
would
> > save
> > > > you
> > > > > from having to inline the macro here and there and also from using
> macros.
> > > > > I'm not against the code that's already here but personally I'd swap
> this
> > > out
> > > > > for a lambda.
> > > > Acknowledged.
> > > I suspect that FOR_EACH_OBSERVER macro isn't supported by our code search
> and
> > it
> > > breaks all the xrefs where the method is actually called.
> > > Am I right?
> > That's right. We could add the proposed function in addition to
> > FOR_EACH_OBSERVER, but that's probably a separate CL.
> Can we avoid this macro then and use implicit iteration as in
> ElementAnimations::NotifyPlayersAnimationTakeover?
explicit C++ iteration, I mean.

[ymalik, 2016-08-05 01:28:15]

On 2016/08/05 01:12:03, loyso wrote:
> On 2016/08/05 01:11:18, loyso wrote:
> > On 2016/08/05 00:55:25, ymalik wrote:
> > > On 2016/08/05 00:13:12, loyso wrote:
> > > > On 2016/08/04 15:31:47, ymalik wrote:
> > > > > On 2016/08/04 00:45:15, alancutter wrote:
> > > > > > I wonder if it would be cleaner to replace the FOR_EACH_OBSERVER
macro
> > > with
> > > > > > template<typename Callback> ObserverList::forEach(Callback). This
> would
> > > save
> > > > > you
> > > > > > from having to inline the macro here and there and also from using
> > macros.
> > > > > > I'm not against the code that's already here but personally I'd swap
> > this
> > > > out
> > > > > > for a lambda.
> > > > > Acknowledged.
> > > > I suspect that FOR_EACH_OBSERVER macro isn't supported by our code
search
> > and
> > > it
> > > > breaks all the xrefs where the method is actually called.
> > > > Am I right?
> > > That's right. We could add the proposed function in addition to
> > > FOR_EACH_OBSERVER, but that's probably a separate CL.
> > Can we avoid this macro then and use implicit iteration as in
> > ElementAnimations::NotifyPlayersAnimationTakeover?
> explicit C++ iteration, I mean.

Removed FOR_EACH_OBSERVER so codesearch doesn't break.

[ymalik, 2016-08-05 01:28:18]

On 2016/08/05 01:12:03, loyso wrote:
> On 2016/08/05 01:11:18, loyso wrote:
> > On 2016/08/05 00:55:25, ymalik wrote:
> > > On 2016/08/05 00:13:12, loyso wrote:
> > > > On 2016/08/04 15:31:47, ymalik wrote:
> > > > > On 2016/08/04 00:45:15, alancutter wrote:
> > > > > > I wonder if it would be cleaner to replace the FOR_EACH_OBSERVER
macro
> > > with
> > > > > > template<typename Callback> ObserverList::forEach(Callback). This
> would
> > > save
> > > > > you
> > > > > > from having to inline the macro here and there and also from using
> > macros.
> > > > > > I'm not against the code that's already here but personally I'd swap
> > this
> > > > out
> > > > > > for a lambda.
> > > > > Acknowledged.
> > > > I suspect that FOR_EACH_OBSERVER macro isn't supported by our code
search
> > and
> > > it
> > > > breaks all the xrefs where the method is actually called.
> > > > Am I right?
> > > That's right. We could add the proposed function in addition to
> > > FOR_EACH_OBSERVER, but that's probably a separate CL.
> > Can we avoid this macro then and use implicit iteration as in
> > ElementAnimations::NotifyPlayersAnimationTakeover?
> explicit C++ iteration, I mean.

Removed FOR_EACH_OBSERVER so codesearch doesn't break.

[loyso (OOO), 2016-08-05 01:33:28]

LGTM with nit.

https://codereview.chromium.org/2189813002/diff/160001/cc/animation/element_a...
File cc/animation/element_animations.cc (right):

https://codereview.chromium.org/2189813002/diff/160001/cc/animation/element_a...
cc/animation/element_animations.cc:1434: while ((player = it.GetNext()) !=
nullptr) {
Code style: No braces {} need for one line (here and everywhere)
Also, would be nice to add a TODO to rework that macro/iteration here and
globally.
(Just a single comment in NotifyPlayersAnimationStarted)

[ymalik, 2016-08-05 14:15:45]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-08-05 14:15:45]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
alancutter@chromium.org, loyso@chromium.org
Link to the patchset: https://codereview.chromium.org/2189813002/#ps180001
(title: "Apply loyso's nit")

[commit-bot: I haz the power, 2016-08-05 14:15:57]

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-08-05 14:15:58]

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

[commit-bot: I haz the power, 2016-08-05 14:15:59]

Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[ymalik, 2016-08-05 14:36:49]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-08-05 14:37:04]

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-08-05 14:37:06]

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

[commit-bot: I haz the power, 2016-08-05 14:37:07]

Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[ymalik, 2016-08-05 14:48:12]

Description was changed from

==========
ElementAnimations should hold an ObservableList of AnimationPlayers.

Before this, the AnimationPlayers were held in a linked list, and
modifying the list while iterating could cause undefined behavior.

BUG=631052
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
ElementAnimations should hold an ObservableList of AnimationPlayers.

Before this, the AnimationPlayers were held in a linked list, and
modifying the list while iterating could cause undefined behavior.

BUG=631052
==========

[ymalik, 2016-08-05 14:48:23]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-08-05 14:48:48]

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-08-05 15:28:31]

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

[commit-bot: I haz the power, 2016-08-05 15:28:33]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[ymalik, 2016-08-05 16:58:29]

Description was changed from

==========
ElementAnimations should hold an ObservableList of AnimationPlayers.

Before this, the AnimationPlayers were held in a linked list, and
modifying the list while iterating could cause undefined behavior.

BUG=631052
==========

to

==========
ElementAnimations should hold an ObservableList of AnimationPlayers.

Before this, the AnimationPlayers were held in a linked list, and
modifying the list while iterating could cause undefined behavior.

BUG=631052
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[ymalik, 2016-08-05 18:52:37]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-08-05 18:52:38]

The patchset sent to the CQ was uploaded after l-g-t-m from loyso@chromium.org,
ajuma@chromium.org, alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2189813002/#ps190001
(title: "fix windows bot")

[commit-bot: I haz the power, 2016-08-05 18:53: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-08-05 20:01:23]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2016-08-05 20:02:59]

Description was changed from

==========
ElementAnimations should hold an ObservableList of AnimationPlayers.

Before this, the AnimationPlayers were held in a linked list, and
modifying the list while iterating could cause undefined behavior.

BUG=631052
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
ElementAnimations should hold an ObservableList of AnimationPlayers.

Before this, the AnimationPlayers were held in a linked list, and
modifying the list while iterating could cause undefined behavior.

BUG=631052
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/66cb1320242db9ce2954a539dc2d5410bf033dbd
Cr-Commit-Position: refs/heads/master@{#410152}
==========

[commit-bot: I haz the power, 2016-08-05 20:03:01]

Patchset 11 (id:??) landed as
https://crrev.com/66cb1320242db9ce2954a539dc2d5410bf033dbd
Cr-Commit-Position: refs/heads/master@{#410152}