Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2248643003: Simplify time tracking in SMILTimeContainer (Closed)

Created:
1 year, 3 months ago by fs
Modified:
1 year, 3 months ago
Reviewers:
pdr.
CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, chromium-reviews, krit, Eric Willigers, f(malita), gyuyoung2, kouhei+svg_chromium.org, pdr+svgwatchlist_chromium.org, rjwright, rwlbuis, Stephen Chennney, shans
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Simplify time tracking in SMILTimeContainer Instead of 5 difference time fields, use two - one to track the last seek/pause time in the container ("presentation time"), and one to track the document time corresponding to that. Use two bool flags for tracking 'paused' and 'started' state. Also straighten out code-flow in SMILTimeContainer::begin() to make it a bit more obvious that we're essentially mirroring the contents of updateAnimationsAndScheduleFrameIfNeeded. begin() is also renamed into start(). Pass double to SMILTimeContainer::scheduleAnimationFrame, do some ASSERT->DCHECK transformations when touching code and touch up some comments. BUG=631879 Committed: https://crrev.com/0b3f0bda7fd496aa15541eedf44d1fe660c730dc Cr-Commit-Position: refs/heads/master@{#413283}

Patch Set 1 #

Patch Set 2 : Move sampling of currentTime to a method (synchronizeToDocumentTimeline) #

Patch Set 3 : Test for the timeline sequence causing the bug/assert #

Patch Set 4 : Fix some comments #

Total comments: 5

Patch Set 5 : begin() -> start(); touch-up comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+105 lines, -82 lines) Patch
A third_party/WebKit/LayoutTests/svg/animations/pause-setcurrenttime-unpause-before-timeline-start.html View 1 2 1 chunk +22 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/svg/SVGDocumentExtensions.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/svg/SVGSVGElement.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.h View 1 2 3 4 3 chunks +11 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp View 1 2 3 4 6 chunks +70 lines, -71 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 42 (28 generated)
fs
I was only going to fix the assert, but... (╯°□°)╯︵ ┻━┻ ...so I just rewrote ...
1 year, 3 months ago (2016-08-18 12:29:37 UTC) #13
pdr.
Nice! Two minor suggestions. LGTM with or without them. https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp File third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp#newcode240 third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp:240: ...
1 year, 3 months ago (2016-08-18 19:50:07 UTC) #18
fs
https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.h File third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.h#newcode126 third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.h:126: bool m_started; // The timeline has been started. On ...
1 year, 3 months ago (2016-08-18 20:34:32 UTC) #19
fs
https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp File third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp#newcode240 third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp:240: // If the document didn't begin yet, record a ...
1 year, 3 months ago (2016-08-19 11:11:24 UTC) #23
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/2248643003/80001
1 year, 3 months ago (2016-08-19 11:51:01 UTC) #28
commit-bot: I haz the power
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_ng/builds/277037)
1 year, 3 months ago (2016-08-19 14:59:24 UTC) #30
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/2248643003/80001
1 year, 3 months ago (2016-08-19 17:25:52 UTC) #32
commit-bot: I haz the power
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_ng/builds/277320)
1 year, 3 months ago (2016-08-19 21:45:59 UTC) #34
pdr.
On 2016/08/19 at 21:45:59, commit-bot wrote: > Try jobs failed on following builders: > win_chromium_rel_ng ...
1 year, 3 months ago (2016-08-19 21:46:44 UTC) #35
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/2248643003/80001
1 year, 3 months ago (2016-08-19 21:47:35 UTC) #37
fs
On 2016/08/19 at 21:46:44, pdr wrote: > On 2016/08/19 at 21:45:59, commit-bot wrote: > > ...
1 year, 3 months ago (2016-08-19 21:50:24 UTC) #38
pdr.
On 2016/08/19 at 21:50:24, fs wrote: > On 2016/08/19 at 21:46:44, pdr wrote: > > ...
1 year, 3 months ago (2016-08-19 21:52:57 UTC) #39
commit-bot: I haz the power
Committed patchset #5 (id:80001)
1 year, 3 months ago (2016-08-19 23:29:57 UTC) #40
commit-bot: I haz the power
1 year, 3 months ago (2016-08-19 23:31:33 UTC) #42
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/0b3f0bda7fd496aa15541eedf44d1fe660c730dc
Cr-Commit-Position: refs/heads/master@{#413283}

Powered by Google App Engine
This is Rietveld efc10ee0f