Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
304453002:
Prepare SMILTimeContainer for oilpan (Closed)
|
Created:
6 years, 7 months ago by kouhei (in TOK) Modified:
6 years, 6 months ago CC:
blink-reviews, Eric Willigers, Steve Block, krit, rjwright, Mike Lawther (Google), blink-reviews-animation_chromium.org, fs, Timothy Loh, dstockwell, ed+blinkwatch_opera.com, shans, f(malita), gyuyoung.kim_webkit.org, darktears, Stephen Chennney, kouhei+svg_chromium.org, rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionPrepare SMILTimeContainer for oilpan
This prepares |SMILTimeContainer::m_scheduledAnimations| for oilpan.
- Vector<SVGSMILElement*> is changed to LinkedHashSet, as oilpan do not support having weak members as vector element.
- Entries pointing to disposing elements are automatically rewritten by oilpan weak processing. They are explicitly removed at |updateAnimations()|
BUG=357163
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174933
Patch Set 1 #
Total comments: 12
Patch Set 2 : address comments (but HeapTest.cpp) #Patch Set 3 : change containers and evict invalid keys manually #
Total comments: 18
Patch Set 4 : address comments #
Total comments: 5
Patch Set 5 : fix_compile #Patch Set 6 : fix_compile2 #
Total comments: 1
Messages
Total messages: 30 (0 generated)
https://codereview.chromium.org/304453002/diff/1/Source/core/svg/SVGElementRa... File Source/core/svg/SVGElementRareData.h (right): https://codereview.chromium.org/304453002/diff/1/Source/core/svg/SVGElementRa... Source/core/svg/SVGElementRareData.h:154: RawPtrWillBeWeakMember<SVGElement> m_owner; This can be a WeakMember since it's only for oilpan builds. https://codereview.chromium.org/304453002/diff/1/Source/core/svg/SVGElementRa... Source/core/svg/SVGElementRareData.h:159: RawPtrWillBeMember<SVGElement> m_correspondingElement; Just to confirm: This should be a WeakMember conceptually, but doesn't need to be weak because it's explicitly cleared out. Right? https://codereview.chromium.org/304453002/diff/1/Source/core/svg/SVGSVGElemen... File Source/core/svg/SVGSVGElement.cpp (right): https://codereview.chromium.org/304453002/diff/1/Source/core/svg/SVGSVGElemen... Source/core/svg/SVGSVGElement.cpp:784: SVGGraphicsElement::trace(visitor); This should be a tail call. https://codereview.chromium.org/304453002/diff/1/Source/core/svg/animation/SM... File Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/304453002/diff/1/Source/core/svg/animation/SM... Source/core/svg/animation/SMILTimeContainer.h:74: SMILTimeContainer(SVGSVGElement& owner); Add explicit. https://codereview.chromium.org/304453002/diff/1/Source/core/svg/animation/SM... Source/core/svg/animation/SMILTimeContainer.h:114: typedef pair<RawPtrWillBeWeakMember<SVGElement>, QualifiedName> ElementAttributePair; - Did you confirm that weak processing for pairs works as expected? - In non-oilpan builds, we should currently have any destructors that remove entries from m_scheduledAnimations when the SVGElement dies. We should remove the code from oilpan builds, but where is it?
https://codereview.chromium.org/304453002/diff/1/Source/core/svg/SVGElementRa... File Source/core/svg/SVGElementRareData.h (right): https://codereview.chromium.org/304453002/diff/1/Source/core/svg/SVGElementRa... Source/core/svg/SVGElementRareData.h:159: RawPtrWillBeMember<SVGElement> m_correspondingElement; On 2014/05/26 08:27:27, haraken wrote: > > Just to confirm: This should be a WeakMember conceptually, but doesn't need to > be weak because it's explicitly cleared out. Right? > Discussed offline. This is a strong member both conceptually and practically. So using a member completely makes sense. What we don't still understand is why this is not a RefPtr in the pre-oilpan world, but this seems to be an actual bug (kouhei is saying that there are crash reports about it).
https://codereview.chromium.org/304453002/diff/1/Source/core/svg/SVGElementRa... File Source/core/svg/SVGElementRareData.h (right): https://codereview.chromium.org/304453002/diff/1/Source/core/svg/SVGElementRa... Source/core/svg/SVGElementRareData.h:154: RawPtrWillBeWeakMember<SVGElement> m_owner; On 2014/05/26 08:27:27, haraken wrote: > > This can be a WeakMember since it's only for oilpan builds. Done. https://codereview.chromium.org/304453002/diff/1/Source/core/svg/SVGSVGElemen... File Source/core/svg/SVGSVGElement.cpp (right): https://codereview.chromium.org/304453002/diff/1/Source/core/svg/SVGSVGElemen... Source/core/svg/SVGSVGElement.cpp:784: SVGGraphicsElement::trace(visitor); On 2014/05/26 08:27:27, haraken wrote: > > This should be a tail call. Done. https://codereview.chromium.org/304453002/diff/1/Source/core/svg/animation/SM... File Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/304453002/diff/1/Source/core/svg/animation/SM... Source/core/svg/animation/SMILTimeContainer.h:74: SMILTimeContainer(SVGSVGElement& owner); On 2014/05/26 08:27:27, haraken wrote: > > Add explicit. Done. https://codereview.chromium.org/304453002/diff/1/Source/core/svg/animation/SM... Source/core/svg/animation/SMILTimeContainer.h:114: typedef pair<RawPtrWillBeWeakMember<SVGElement>, QualifiedName> ElementAttributePair; On 2014/05/26 08:27:27, haraken wrote: > > - Did you confirm that weak processing for pairs works as expected? No. I'll add it on HeapTest.cpp > - In non-oilpan builds, we should currently have any destructors that remove > entries from m_scheduledAnimations when the SVGElement dies. We should remove > the code from oilpan builds, but where is it? They are handled by |document().accessSVGExtensions().removeAllElementReferencesForTarget|
https://codereview.chromium.org/304453002/diff/1/Source/core/svg/animation/SM... File Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/304453002/diff/1/Source/core/svg/animation/SM... Source/core/svg/animation/SMILTimeContainer.h:114: typedef pair<RawPtrWillBeWeakMember<SVGElement>, QualifiedName> ElementAttributePair; On 2014/05/26 08:55:28, kouhei wrote: > On 2014/05/26 08:27:27, haraken wrote: > > > > - Did you confirm that weak processing for pairs works as expected? > No. I'll add it on HeapTest.cpp Actually they are not deleted per HeapTest_HeapWeakPairs. What can we do in that case? Registering a weak processing hook to be called on every GC seems to have too much overhead...
https://codereview.chromium.org/304453002/diff/1/Source/core/svg/animation/SM... File Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/304453002/diff/1/Source/core/svg/animation/SM... Source/core/svg/animation/SMILTimeContainer.h:114: typedef pair<RawPtrWillBeWeakMember<SVGElement>, QualifiedName> ElementAttributePair; On 2014/05/26 09:12:48, kouhei wrote: > On 2014/05/26 08:55:28, kouhei wrote: > > On 2014/05/26 08:27:27, haraken wrote: > > > > > > - Did you confirm that weak processing for pairs works as expected? > > No. I'll add it on HeapTest.cpp > > Actually they are not deleted per HeapTest_HeapWeakPairs. > What can we do in that case? Registering a weak processing hook to be called on > every GC seems to have too much overhead... Erik@: Would you give us any advice? :)
There's not support for this right now. If updateAnimations is called frequently enough (is it on a timer that repeats?) then you could add a loop to the start that finds and removes entries with null elements (that have been zapped by weak processing). You may have to check for zapped elements other places too and ignore them.
PTAL. On 2014/05/26 14:19:25, Erik Corry wrote: > There's not support for this right now. > > If updateAnimations is called frequently enough (is it on a timer that repeats?) > then you could add a loop to the start that finds and removes entries with null > elements (that have been zapped by weak processing). You may have to check for > zapped elements other places too and ignore them. Thanks! I added that in |SMILTimeContainer::updateAnimations|
https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:119: m_scheduledAnimations.remove(it); Just help me understand: Having this remove() looks fine, but how was the previous code working without having the remove()? https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:386: if (!it->key.first || !it->value || it->value->isEmpty()) I don't know if we need the !it->key.first check. It would depend on how the weak processing for HeapHashMap<pair<WeakMember<X>, Y>, Z> works. When X dies, is the associated entry removed from the heap hash map? Also I wonder if we need the !it->value check. https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:426: m_scheduledAnimations.removeAll(invalidKeys); Ditto. I'm curious how the previous code was working without this removeAll(). https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.h:116: typedef WillBeHeapVector<RefPtrWillBeMember<SVGSMILElement> > AnimationsVector; Can we move this typedef to the cpp file? https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:210: m_timeContainer->unschedule(this, m_targetElement, m_attributeName); Just to confirm: This code is already removed in oilpan builds, but it is wrong to remove it before landing this CL. It becomes correct after landing this CL. Am I understanding correctly?
https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:387: invalidKeys.add(it->key); Should there not also be "continue;" here?
https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:386: if (!it->key.first || !it->value || it->value->isEmpty()) On 2014/05/27 08:25:56, haraken wrote: > > I don't know if we need the !it->key.first check. It would depend on how the > weak processing for HeapHashMap<pair<WeakMember<X>, Y>, Z> works. When X dies, > is the associated entry removed from the heap hash map? No, the associated entry is not removed, that's why we need this code. > Also I wonder if we need the !it->value check. I guess we don't
https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:119: m_scheduledAnimations.remove(it); On 2014/05/27 08:25:56, haraken wrote: > > Just help me understand: Having this remove() looks fine, but how was the > previous code working without having the remove()? I believe the invalid pointer was just there, but as |scheduled| was empty, there weren't any problem. https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:386: if (!it->key.first || !it->value || it->value->isEmpty()) On 2014/05/27 08:41:48, Erik Corry wrote: > On 2014/05/27 08:25:56, haraken wrote: > > > > I don't know if we need the !it->key.first check. It would depend on how the > > weak processing for HeapHashMap<pair<WeakMember<X>, Y>, Z> works. When X dies, > > is the associated entry removed from the heap hash map? > > No, the associated entry is not removed, that's why we need this code. > > > Also I wonder if we need the !it->value check. > > I guess we don't > Ack. https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:387: invalidKeys.add(it->key); On 2014/05/27 08:28:06, Erik Corry wrote: > Should there not also be "continue;" here? Yes! Ack. https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:426: m_scheduledAnimations.removeAll(invalidKeys); On 2014/05/27 08:25:56, haraken wrote: > > Ditto. I'm curious how the previous code was working without this removeAll(). if !ENABLE(OILPAN), |unschedule| is guaranteed to be called. if ENABLE(OILPAN) it is processed through weak processing in (HeapHashMap<pair<Weak, attr>, >), so invalid keys remain.
lgtm https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:242: for (GroupedAnimationsMap::iterator it = m_scheduledAnimations.begin(); it != end; ++it) { I think you should check for null keys here too. It may not be necessary to remove them.
+pdr
PTAL. https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:242: for (GroupedAnimationsMap::iterator it = m_scheduledAnimations.begin(); it != end; ++it) { On 2014/05/27 08:55:23, Erik Corry wrote: > I think you should check for null keys here too. It may not be necessary to > remove them. Done. https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:386: if (!it->key.first || !it->value || it->value->isEmpty()) On 2014/05/27 08:50:51, kouhei wrote: > On 2014/05/27 08:41:48, Erik Corry wrote: > > On 2014/05/27 08:25:56, haraken wrote: > > > > > > I don't know if we need the !it->key.first check. It would depend on how the > > > weak processing for HeapHashMap<pair<WeakMember<X>, Y>, Z> works. When X > dies, > > > is the associated entry removed from the heap hash map? > > > > No, the associated entry is not removed, that's why we need this code. > > > > > Also I wonder if we need the !it->value check. > > > > I guess we don't > > > > Ack. Done. https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:387: invalidKeys.add(it->key); On 2014/05/27 08:50:51, kouhei wrote: > On 2014/05/27 08:28:06, Erik Corry wrote: > > Should there not also be "continue;" here? > > Yes! Ack. Done. https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.h:116: typedef WillBeHeapVector<RefPtrWillBeMember<SVGSMILElement> > AnimationsVector; On 2014/05/27 08:25:56, haraken wrote: > > Can we move this typedef to the cpp file? Done. https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:210: m_timeContainer->unschedule(this, m_targetElement, m_attributeName); On 2014/05/27 08:25:56, haraken wrote: > > Just to confirm: This code is already removed in oilpan builds, but it is wrong > to remove it before landing this CL. It becomes correct after landing this CL. > Am I understanding correctly? For 209-210, yes. 208 is still wrong (added comment)
LGTM. Please explain non-trivial changes in the CL description. https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/304453002/diff/40001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:210: m_timeContainer->unschedule(this, m_targetElement, m_attributeName); On 2014/05/28 01:00:50, kouhei wrote: > On 2014/05/27 08:25:56, haraken wrote: > > > > Just to confirm: This code is already removed in oilpan builds, but it is > wrong > > to remove it before landing this CL. It becomes correct after landing this CL. > > Am I understanding correctly? > > For 209-210, yes. 208 is still wrong (added comment) Thanks. Recently we're doing a lot of complicated things, and it's hard to keep track of them without having a FIXME. It's a nice idea to have a FIXME even if you're planning to fix it in a follow-up. https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:244: continue; (If this code is not performance-sensitive,) rather than having the continue statement in the loop, I'd prefer calling a helper method that cleans up m_scheduledAnimations before starting the loop. cleanUpDeadEntries(); // Cleans up m_scheduledAnimations for (...) { ...; } https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:391: invalidKeys.add(it->key); Ditto. cleanUpDeadEntries(); for (...) { ...; }
https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:391: invalidKeys.add(it->key); On 2014/05/28 01:43:56, haraken wrote: > > Ditto. > > cleanUpDeadEntries(); > for (...) { > ...; > } I'd prefer current code, as |updateAnimations| is to be called on every frame.
LGTM with a question. https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.h (right): https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.h:115: typedef WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<SVGSMILElement> > AnimationsLinkedHashSet; Is there a reason for switching this from a vector to a linked hash set?
On 2014/05/28 01:56:34, pdr wrote: > LGTM with a question. > > https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... > File Source/core/svg/animation/SMILTimeContainer.h (right): > > https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... > Source/core/svg/animation/SMILTimeContainer.h:115: typedef > WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<SVGSMILElement> > > AnimationsLinkedHashSet; > Is there a reason for switching this from a vector to a linked hash set? Oilpan does not support Vector<WeakMember> so this had to be changed to LinkedHashSet.
On 2014/05/28 02:04:15, kouhei wrote: > On 2014/05/28 01:56:34, pdr wrote: > > LGTM with a question. > > > > > https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... > > File Source/core/svg/animation/SMILTimeContainer.h (right): > > > > > https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... > > Source/core/svg/animation/SMILTimeContainer.h:115: typedef > > WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<SVGSMILElement> > > > AnimationsLinkedHashSet; > > Is there a reason for switching this from a vector to a linked hash set? > > Oilpan does not support Vector<WeakMember> so this had to be changed to > LinkedHashSet. Possibly naive question: aren't there large performance differences between the two? I haven't profiled ours but I think of vectors as having better cache performance during iteration, and faster copying.
On 2014/05/28 02:11:37, pdr wrote: > On 2014/05/28 02:04:15, kouhei wrote: > > On 2014/05/28 01:56:34, pdr wrote: > > > LGTM with a question. > > > > > > > > > https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... > > > File Source/core/svg/animation/SMILTimeContainer.h (right): > > > > > > > > > https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... > > > Source/core/svg/animation/SMILTimeContainer.h:115: typedef > > > WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<SVGSMILElement> > > > > AnimationsLinkedHashSet; > > > Is there a reason for switching this from a vector to a linked hash set? > > > > Oilpan does not support Vector<WeakMember> so this had to be changed to > > LinkedHashSet. > > Possibly naive question: aren't there large performance differences between the > two? I haven't profiled ours but I think of vectors as having better cache > performance during iteration, and faster copying. It is possible, but I don't think it is observable in this case, as operations we do per element is heavy.
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kouhei@chromium.org/304453002/70001
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kouhei@chromium.org/304453002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/9922) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9733) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9189) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9398)
Message was sent while issue was closed.
Change committed as 174933
Message was sent while issue was closed.
On 2014/05/28 02:11:37, pdr wrote: > On 2014/05/28 02:04:15, kouhei wrote: > > On 2014/05/28 01:56:34, pdr wrote: > > > LGTM with a question. > > > > > > > > > https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... > > > File Source/core/svg/animation/SMILTimeContainer.h (right): > > > > > > > > > https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... > > > Source/core/svg/animation/SMILTimeContainer.h:115: typedef > > > WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<SVGSMILElement> > > > > AnimationsLinkedHashSet; > > > Is there a reason for switching this from a vector to a linked hash set? > > > > Oilpan does not support Vector<WeakMember> so this had to be changed to > > LinkedHashSet. > > Possibly naive question: aren't there large performance differences between the > two? I haven't profiled ours but I think of vectors as having better cache > performance during iteration, and faster copying. Cache performance of LinkedHashSet should be OK, it's just marching through a flat backing store.
Message was sent while issue was closed.
https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (left): https://codereview.chromium.org/304453002/diff/50001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:114: scheduled->remove(idx); This was a remove from a vector, which is an n squared operation if you do it n times. It's nice to get rid of this (disclaimer: I haven't measured whether it matters).
Message was sent while issue was closed.
https://codereview.chromium.org/304453002/diff/80001/Source/core/svg/animatio... File Source/core/svg/animation/SMILTimeContainer.cpp (right): https://codereview.chromium.org/304453002/diff/80001/Source/core/svg/animatio... Source/core/svg/animation/SMILTimeContainer.cpp:432: m_scheduledAnimations.removeAll(invalidKeys); I just realized that this will never remove anything. The remove(item) calls all do nothing because an item with a nulled key.first has a different hash code from the one it had when it was inserted. Therefore the lookup fails and nothing is removed. That probably leads to leaks. I'm working on fixing handling of pairs with weakness so that this is not an issue. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
