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

Issue 642773004: Clear baseRenderStyle on detach. (Closed)

Created:
6 years, 1 month ago by rune
Modified:
6 years, 1 month ago
Reviewers:
dstockwell, esprehn
CC:
dstockwell, darktears, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, Eric Willigers, Mike Lawther (Google), rjwright, rwlbuis, shans, sof, Steve Block, Timothy Loh
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Clear baseRenderStyle on detach. ActiveAnimations::baseRenderStyle was only nulled correctly from styleForElement, but that method is not always called when clearing the styleChangeType(). For instance when the we have detached the element, but don't need to create a renderer for it when re-attaching. This bug caused the baseRenderStyle to survive moving an element between documents triggering the assert checking that the baseRenderStyle was correct. Clearing the baseRenderStyle unconditionally on detach() should fix this. R=dstockwell@chromium.org,esprehn@chromium.org BUG=427433 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184515

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -0 lines) Patch
A LayoutTests/animations/base-render-style-crash.html View 1 chunk +12 lines, -0 lines 0 comments Download
A LayoutTests/animations/base-render-style-crash-expected.html View 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/animation/ActiveAnimations.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/animation/ActiveAnimations.cpp View 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/dom/Element.cpp View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 5 (1 generated)
rune
6 years, 1 month ago (2014-10-28 10:16:28 UTC) #1
dstockwell
lgtm
6 years, 1 month ago (2014-10-28 10:37:38 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/642773004/1
6 years, 1 month ago (2014-10-28 11:55:29 UTC) #4
commit-bot: I haz the power
6 years, 1 month ago (2014-10-28 13:01:30 UTC) #5
Message was sent while issue was closed.
Committed patchset #1 (id:1) as 184515

Powered by Google App Engine
This is Rietveld 408576698