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

Issue 1989033003: Move clearAnimatedType() up the stack (Closed)

Created:
4 years, 7 months ago by davve
Modified:
4 years, 7 months ago
Reviewers:
fs
CC:
fs, 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

Move clearAnimatedType() up the stack This is in preparation for locking the animated property type for the resulting animation while the accumulated animation is computed. There seems to be a crash due to the animated property type being cleared too early. To catch this crash at the point of error, the plan is to lock the animated property type of the result animation during processing, and guard for clearing the animation property while the lock is held (in clearAnimatedType()). For SMILTimeContainer::updateAnimations() to have a chance of unlocking the animated property _before_ clearAnimatedType() is called, we need to move the call up to SMILTimeContainer::updateAnimations(). (The assumption is that moving the call shouldn't make a difference since SMILTimeContainer::updateAnimations() is the only call-site for SVGSMILElement::progress() and the intermediate code shouldn't depend on the animated property.) Since the clearing of property type and nullifying of resultElement seems tied together, grouping them makes sense regardless of the crash chase. BUG=581546 Committed: https://crrev.com/8ac7017db3685604db2d79d8e667ba1d7cf463a2 Cr-Commit-Position: refs/heads/master@{#394698}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -4 lines) Patch
M third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp View 1 chunk +4 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp View 1 chunk +0 lines, -2 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 12 (5 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1989033003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1989033003/1
4 years, 7 months ago (2016-05-18 14:27:08 UTC) #2
davve
4 years, 7 months ago (2016-05-18 14:27:14 UTC) #4
fs
Make sense. LGTM.
4 years, 7 months ago (2016-05-18 15:59:59 UTC) #5
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 7 months ago (2016-05-18 16:16:45 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1989033003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1989033003/1
4 years, 7 months ago (2016-05-19 06:31:32 UTC) #9
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 7 months ago (2016-05-19 06:35:48 UTC) #10
commit-bot: I haz the power
4 years, 7 months ago (2016-05-19 06:37:16 UTC) #12
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/8ac7017db3685604db2d79d8e667ba1d7cf463a2
Cr-Commit-Position: refs/heads/master@{#394698}

Powered by Google App Engine
This is Rietveld 408576698