Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(353)

Issue 2189813002: ElementAnimations should hold an ObservableList of AnimationPlayers. (Closed)

Created:
4 years, 4 months ago by ymalik
Modified:
4 years, 4 months ago
CC:
cc-bugs_chromium.org, chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : Remove unused file #

Total comments: 2

Patch Set 3 : Fix bot + address jbroman's comment #

Patch Set 4 : Rebase + fix blink_platform_unittests compile error #

Total comments: 1

Patch Set 5 : Rebase + fix blink_platform_unittests compile error #

Total comments: 5

Patch Set 6 : apply loyso's comment #

Total comments: 18

Patch Set 7 : More review feedback #

Patch Set 8 : update tests after rename #

Patch Set 9 : Remove FOR_EACH_OBSERVER #

Total comments: 1

Patch Set 10 : Apply loyso's nit #

Patch Set 11 : fix windows bot #

Unified diffs Side-by-side diffs Delta from patch set Stats (+107 lines, -48 lines) Patch
M cc/animation/animation_player.h View 1 2 3 4 5 6 2 chunks +1 line, -3 lines 0 comments Download
M cc/animation/element_animations.h View 1 2 3 4 5 6 2 chunks +3 lines, -4 lines 0 comments Download
M cc/animation/element_animations.cc View 1 2 3 4 5 6 7 8 9 4 chunks +18 lines, -25 lines 0 comments Download
M cc/animation/element_animations_unittest.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +75 lines, -12 lines 0 comments Download
M cc/test/animation_timelines_test_common.cc View 1 2 3 4 5 6 1 chunk +10 lines, -4 lines 0 comments Download

Messages

Total messages: 70 (34 generated)
ymalik
4 years, 4 months ago (2016-07-27 23:57:07 UTC) #8
jbroman
https://codereview.chromium.org/2189813002/diff/20001/cc/animation/element_animations_unittest.cc File cc/animation/element_animations_unittest.cc (right): https://codereview.chromium.org/2189813002/diff/20001/cc/animation/element_animations_unittest.cc#newcode300 cc/animation/element_animations_unittest.cc:300: class TestGarbageCollectedAnimationPlayer : public AnimationPlayer { drive-by: Please don't ...
4 years, 4 months ago (2016-07-28 01:01:15 UTC) #9
ymalik
https://codereview.chromium.org/2189813002/diff/20001/cc/animation/element_animations_unittest.cc File cc/animation/element_animations_unittest.cc (right): https://codereview.chromium.org/2189813002/diff/20001/cc/animation/element_animations_unittest.cc#newcode300 cc/animation/element_animations_unittest.cc:300: class TestGarbageCollectedAnimationPlayer : public AnimationPlayer { On 2016/07/28 01:01:15, ...
4 years, 4 months ago (2016-07-28 02:45:33 UTC) #10
alancutter (OOO until 2018)
s/NotifyAnimationFinished/OnAnimationFinished/ in third_party/WebKit/Source/platform/animation/CompositorAnimationPlayerTest.cpp to make compile happy.
4 years, 4 months ago (2016-07-28 07:37:13 UTC) #11
ajuma
lgtm https://codereview.chromium.org/2189813002/diff/60001/cc/test/animation_timelines_test_common.h File cc/test/animation_timelines_test_common.h (right): https://codereview.chromium.org/2189813002/diff/60001/cc/test/animation_timelines_test_common.h#newcode247 cc/test/animation_timelines_test_common.h:247: int started() { return started_; } Please rename ...
4 years, 4 months ago (2016-07-28 18:36:41 UTC) #12
loyso (OOO)
631052 is marked as already fixed. Could you add a comment, why we need this. ...
4 years, 4 months ago (2016-08-03 03:45:06 UTC) #13
alancutter (OOO until 2018)
On 2016/08/03 at 03:45:06, loyso wrote: > 631052 is marked as already fixed. Could you ...
4 years, 4 months ago (2016-08-03 03:48:54 UTC) #14
ymalik
@alancutter, @loyso PTAL :)
4 years, 4 months ago (2016-08-03 22:16:43 UTC) #15
alancutter (OOO until 2018)
lgtm, the description probably shouldn't mention fixing the crash as it didn't actually do that. ...
4 years, 4 months ago (2016-08-04 00:45:15 UTC) #16
loyso (OOO)
On 2016/08/03 03:48:54, alancutter wrote: > On 2016/08/03 at 03:45:06, loyso wrote: > > 631052 ...
4 years, 4 months ago (2016-08-04 06:24:22 UTC) #17
loyso (OOO)
https://codereview.chromium.org/2189813002/diff/80001/cc/animation/animation_player.h File cc/animation/animation_player.h (right): https://codereview.chromium.org/2189813002/diff/80001/cc/animation/animation_player.h#newcode77 cc/animation/animation_player.h:77: virtual void OnAnimationStarted(base::TimeTicks monotonic_time, It's very sad to make ...
4 years, 4 months ago (2016-08-04 06:32:10 UTC) #18
loyso (OOO)
Yeah, I see. blink::Animation::notifyAnimationStarted may lead to destroyCompositorPlayer() call in Animation::setCompositorPending.
4 years, 4 months ago (2016-08-04 06:37:31 UTC) #19
loyso (OOO)
https://codereview.chromium.org/2189813002/diff/80001/cc/animation/animation_player.h File cc/animation/animation_player.h (right): https://codereview.chromium.org/2189813002/diff/80001/cc/animation/animation_player.h#newcode77 cc/animation/animation_player.h:77: virtual void OnAnimationStarted(base::TimeTicks monotonic_time, On 2016/08/04 06:32:10, loyso wrote: ...
4 years, 4 months ago (2016-08-04 06:50:37 UTC) #20
loyso (OOO)
I mean, to provide your custom TestAnimationDelegate which combines the detachment and counting.
4 years, 4 months ago (2016-08-04 07:00:13 UTC) #21
ymalik
@loyso, PTAL :) https://codereview.chromium.org/2189813002/diff/80001/cc/animation/animation_player.h File cc/animation/animation_player.h (right): https://codereview.chromium.org/2189813002/diff/80001/cc/animation/animation_player.h#newcode77 cc/animation/animation_player.h:77: virtual void OnAnimationStarted(base::TimeTicks monotonic_time, On 2016/08/04 ...
4 years, 4 months ago (2016-08-04 15:31:47 UTC) #22
loyso (OOO)
On 2016/08/04 15:31:47, ymalik wrote: > On 2016/08/04 00:45:15, alancutter wrote: > > I wonder ...
4 years, 4 months ago (2016-08-05 00:13:12 UTC) #27
loyso (OOO)
https://codereview.chromium.org/2189813002/diff/100001/cc/animation/animation_player.h File cc/animation/animation_player.h (right): https://codereview.chromium.org/2189813002/diff/100001/cc/animation/animation_player.h#newcode77 cc/animation/animation_player.h:77: void OnAnimationStarted(base::TimeTicks monotonic_time, Any rationale for renaming? The comment ...
4 years, 4 months ago (2016-08-05 00:13:25 UTC) #28
loyso (OOO)
https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_animations.cc File cc/animation/element_animations.cc (right): https://codereview.chromium.org/2189813002/diff/100001/cc/animation/element_animations.cc#newcode1459 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_animations_unittest.cc File ...
4 years, 4 months ago (2016-08-05 00:18:48 UTC) #29
loyso (OOO)
https://codereview.chromium.org/2189813002/diff/100001/cc/animation/animation_player.h File cc/animation/animation_player.h (right): https://codereview.chromium.org/2189813002/diff/100001/cc/animation/animation_player.h#newcode10 cc/animation/animation_player.h:10: #include "base/containers/linked_list.h" It becomes unused.
4 years, 4 months ago (2016-08-05 00:19:51 UTC) #30
loyso (OOO)
On 2016/08/04 00:45:15, alancutter wrote: > the description probably shouldn't mention fixing the crash as ...
4 years, 4 months ago (2016-08-05 00:21:13 UTC) #31
ymalik
@loyso, my apologies for missing the points you mentioned. Perhaps I was jetlagged when I ...
4 years, 4 months ago (2016-08-05 00:54:05 UTC) #33
ymalik
On 2016/08/05 00:13:12, loyso wrote: > On 2016/08/04 15:31:47, ymalik wrote: > > On 2016/08/04 ...
4 years, 4 months ago (2016-08-05 00:55:25 UTC) #34
loyso (OOO)
On 2016/08/05 00:55:25, ymalik wrote: > On 2016/08/05 00:13:12, loyso wrote: > > On 2016/08/04 ...
4 years, 4 months ago (2016-08-05 01:11:18 UTC) #45
loyso (OOO)
On 2016/08/05 01:11:18, loyso wrote: > On 2016/08/05 00:55:25, ymalik wrote: > > On 2016/08/05 ...
4 years, 4 months ago (2016-08-05 01:12:03 UTC) #46
ymalik
On 2016/08/05 01:12:03, loyso wrote: > On 2016/08/05 01:11:18, loyso wrote: > > On 2016/08/05 ...
4 years, 4 months ago (2016-08-05 01:28:15 UTC) #47
ymalik
On 2016/08/05 01:12:03, loyso wrote: > On 2016/08/05 01:11:18, loyso wrote: > > On 2016/08/05 ...
4 years, 4 months ago (2016-08-05 01:28:18 UTC) #48
loyso (OOO)
LGTM with nit. https://codereview.chromium.org/2189813002/diff/160001/cc/animation/element_animations.cc File cc/animation/element_animations.cc (right): https://codereview.chromium.org/2189813002/diff/160001/cc/animation/element_animations.cc#newcode1434 cc/animation/element_animations.cc:1434: while ((player = it.GetNext()) != nullptr) ...
4 years, 4 months ago (2016-08-05 01:33:28 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2189813002/180001
4 years, 4 months ago (2016-08-05 14:15:57 UTC) #52
commit-bot: I haz the power
Your CL was about to rely on recently removed CQ feature(s): * master.tryserver.blink builder linux_blink_rel ...
4 years, 4 months ago (2016-08-05 14:15:59 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2189813002/180001
4 years, 4 months ago (2016-08-05 14:37:04 UTC) #56
commit-bot: I haz the power
Your CL was about to rely on recently removed CQ feature(s): * master.tryserver.blink builder linux_blink_rel ...
4 years, 4 months ago (2016-08-05 14:37:07 UTC) #58
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2189813002/180001
4 years, 4 months ago (2016-08-05 14:48:48 UTC) #61
commit-bot: I haz the power
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_compile_dbg_ng/builds/233724)
4 years, 4 months ago (2016-08-05 15:28:33 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2189813002/190001
4 years, 4 months ago (2016-08-05 18:53:10 UTC) #67
commit-bot: I haz the power
Committed patchset #11 (id:190001)
4 years, 4 months ago (2016-08-05 20:01:23 UTC) #68
commit-bot: I haz the power
4 years, 4 months ago (2016-08-05 20:03:01 UTC) #70
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/66cb1320242db9ce2954a539dc2d5410bf033dbd
Cr-Commit-Position: refs/heads/master@{#410152}

Powered by Google App Engine
This is Rietveld 408576698