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

Issue 2248643003: Simplify time tracking in SMILTimeContainer (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 1 month ago by fs
Modified:
1 year, 1 month 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
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 42 (28 generated)
fs
I was only going to fix the assert, but... (╯°□°)╯︵ ┻━┻ ...so I just rewrote ...
1 year, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month ago (2016-08-19 21:52:57 UTC) #39
commit-bot: I haz the power
Committed patchset #5 (id:80001)
1 year, 1 month ago (2016-08-19 23:29:57 UTC) #40
commit-bot: I haz the power
1 year, 1 month 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}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b