Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2248643003:
Simplify time tracking in SMILTimeContainer (Closed)
|
Created:
4 years, 4 months ago by fs Modified:
4 years, 4 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. |
DescriptionSimplify 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 #
Dependent Patchsets: Messages
Total messages: 42 (28 generated)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Simplify time tracking in SMILTimeContainer
Instead of 5 difference time fields, use two - one to track the last
seek/pause time in the container, 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. Pass double to
SMILTimeContainer::scheduleAnimationFrame, do some ASSERT->DCHECK
transformations when touching code and touch up some comments.
BUG=631879
==========
to
==========
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. Pass double to
SMILTimeContainer::scheduleAnimationFrame, do some ASSERT->DCHECK
transformations when touching code and touch up some comments.
BUG=631879
==========
fs@opera.com changed reviewers: + pdr@chromium.org
I was only going to fix the assert, but... (╯°□°)╯︵ ┻━┻ ...so I just rewrote into something better resembling sanity. (_And_ fixed the assert issue!) ┬─┬ ノ( ゜-゜ノ) (order restored)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice! Two minor suggestions. LGTM with or without them. https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp:240: // If the document didn't begin yet, record a new start time we'll seek to Maybe a little cleaner? // If the document hasn't finished loading, |m_presentationTime| will be used // as the start time to seek to once it's possible. https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.h:126: bool m_started; // The timeline has been started. We mix "start" and "begin" here. Wdyt about m_hasBegun/hasBegun(), or start() instead of begin()?
https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.h:126: bool m_started; // The timeline has been started. On 2016/08/18 at 19:50:06, pdr. wrote: > We mix "start" and "begin" here. Wdyt about m_hasBegun/hasBegun(), or start() instead of begin()? I guess begin, and variants thereof, would be more SMILy (it refers to this as "document begin"). I think I prefer 'start' a little bit though (and that's probably a smaller incremental change), so unless you have a strong opinion I might go with that. Will decide tomorrow though =)
Description was changed from
==========
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. Pass double to
SMILTimeContainer::scheduleAnimationFrame, do some ASSERT->DCHECK
transformations when touching code and touch up some comments.
BUG=631879
==========
to
==========
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
==========
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp:240: // If the document didn't begin yet, record a new start time we'll seek to On 2016/08/18 at 19:50:06, pdr. wrote: > Maybe a little cleaner? > // If the document hasn't finished loading, |m_presentationTime| will be used > // as the start time to seek to once it's possible. Changed. https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/2248643003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.h:126: bool m_started; // The timeline has been started. On 2016/08/18 at 20:34:32, fs wrote: > On 2016/08/18 at 19:50:06, pdr. wrote: > > We mix "start" and "begin" here. Wdyt about m_hasBegun/hasBegun(), or start() instead of begin()? > > I guess begin, and variants thereof, would be more SMILy (it refers to this as "document begin"). I think I prefer 'start' a little bit though (and that's probably a smaller incremental change), so unless you have a strong opinion I might go with that. Will decide tomorrow though =) I ended up going with begin() -> start().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by fs@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2248643003/#ps80001 (title: "begin() -> start(); touch-up comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
On 2016/08/19 at 21:45:59, commit-bot wrote: > 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_...) Windows!! >:( Lets try again
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/19 at 21:46:44, pdr wrote: > On 2016/08/19 at 21:45:59, commit-bot wrote: > > 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_...) > > Windows!! >:( Now, who would've thought this would come in handy again... (╯°□°)╯︵ ┻━┻
On 2016/08/19 at 21:50:24, fs wrote: > On 2016/08/19 at 21:46:44, pdr wrote: > > On 2016/08/19 at 21:45:59, commit-bot wrote: > > > 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_...) > > > > Windows!! >:( > > Now, who would've thought this would come in handy again... > > (╯°□°)╯︵ ┻━┻ (╯°□°)╯︵ sʍopuᴉʍ
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from
==========
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
==========
to
==========
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}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0b3f0bda7fd496aa15541eedf44d1fe660c730dc Cr-Commit-Position: refs/heads/master@{#413283} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
