Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
305783004:
Prepare SVGSMILElement for oilpan (Closed)
|
Created:
6 years, 6 months ago by kouhei (in TOK) Modified:
6 years, 6 months ago CC:
blink-reviews, ed+blinkwatch_opera.com, shans, rjwright, Mike Lawther (Google), blink-reviews-animation_chromium.org, rwlbuis, fs, kouhei+svg_chromium.org, dstockwell, Timothy Loh, krit, f(malita), gyuyoung.kim_webkit.org, darktears, Stephen Chennney, Steve Block, Eric Willigers Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionPrepare SVGSMILElement for oilpan.
In oilpan builds, |m_syncBaseDependents| and |m_syncBase| are changed to weak references from manual detach in d-tors.
|m_syncBase| is changed from Element* -> SVGSMILElement* as its only valid value is SVGSMILElement*.
BUG=357163
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175088
Patch Set 1 #
Total comments: 7
Patch Set 2 : strong refs #
Total comments: 1
Messages
Total messages: 11 (0 generated)
On 2014/05/29 04:37:42, kouhei wrote: LGTM
Please write what this CL is doing in the CL description so that others can understand what it's doing. Also the title should be more specific. https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... Source/core/svg/animation/SVGSMILElement.cpp:223: disconnectEventBaseConditions(); Making m_syncBaseDependents weak will allow us to remove disconnectSyncBaseConditions(), but is it OK to remove disconnectEventBaseConditions() as well? https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... File Source/core/svg/animation/SVGSMILElement.h (right): https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... Source/core/svg/animation/SVGSMILElement.h:201: RefPtrWillBeWeakMember<SVGSMILElement> m_syncBase; It looks strange that RefPtr becomes WeakMember. Shouldn't the Condition and the SVGSMILElement live & die together? In what situation do we want to let the Condition outlive the SVGSMILElement?
https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... Source/core/svg/animation/SVGSMILElement.cpp:223: disconnectEventBaseConditions(); On 2014/05/29 04:58:58, haraken wrote: > > Making m_syncBaseDependents weak will allow us to remove > disconnectSyncBaseConditions(), but is it OK to remove > disconnectEventBaseConditions() as well? |disconnectEventBaseConditions()| can also be removed as ~RefPtr will be handled by |~Condition| which is now GCFinalized. https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... File Source/core/svg/animation/SVGSMILElement.h (right): https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... Source/core/svg/animation/SVGSMILElement.h:201: RefPtrWillBeWeakMember<SVGSMILElement> m_syncBase; On 2014/05/29 04:58:58, haraken wrote: > It looks strange that RefPtr becomes WeakMember. Shouldn't the Condition and the > SVGSMILElement live & die together? In what situation do we want to let the > Condition outlive the SVGSMILElement? syncBase was manually unregistered via d-tor, however we are removing this in oilpan builds, so this need to be weak.
https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... Source/core/svg/animation/SVGSMILElement.cpp:665: SVGElement* eventBase = eventBaseFor(*condition); Is it guaranteed that the eventBase exists in the same document as the SVGSMILElement? If the eventBase and the SVGSMILElement are in the same tree, they die together, so I agree that we don't need to clean up the eventBase when the SVGSMILElement dies. https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... Source/core/svg/animation/SVGSMILElement.cpp:669: condition->setEventListener(nullptr); I don't fully understand this. Would you elaborate on why you don't need to run line 668 and 669 when the SVGSMILElement dies? https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... File Source/core/svg/animation/SVGSMILElement.h (right): https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... Source/core/svg/animation/SVGSMILElement.h:201: RefPtrWillBeWeakMember<SVGSMILElement> m_syncBase; On 2014/05/29 05:08:10, kouhei wrote: > On 2014/05/29 04:58:58, haraken wrote: > > It looks strange that RefPtr becomes WeakMember. Shouldn't the Condition and > the > > SVGSMILElement live & die together? In what situation do we want to let the > > Condition outlive the SVGSMILElement? > > syncBase was manually unregistered via d-tor, however we are removing this in > oilpan builds, so this need to be weak. Even if "X was manually unregistered via d-tor", it doesn't necessarily mean that we want to make it weak. It's possible that the pointer was a back pointer (not a weak pointer) and thus was manually unregistered when X died. The point is whether the Condition and the SVGSMILElement should die together or not. So I want to understand in what situation we want to let the Condition outlive the SVGSMILElement.
On 2014/05/29 06:14:09, haraken wrote: > https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... > File Source/core/svg/animation/SVGSMILElement.cpp (right): > > https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... > Source/core/svg/animation/SVGSMILElement.cpp:665: SVGElement* eventBase = > eventBaseFor(*condition); > > Is it guaranteed that the eventBase exists in the same document as the > SVGSMILElement? If the eventBase and the SVGSMILElement are in the same tree, > they die together, so I agree that we don't need to clean up the eventBase when > the SVGSMILElement dies. > > https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... > Source/core/svg/animation/SVGSMILElement.cpp:669: > condition->setEventListener(nullptr); > > I don't fully understand this. Would you elaborate on why you don't need to run > line 668 and 669 when the SVGSMILElement dies? > > https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... > File Source/core/svg/animation/SVGSMILElement.h (right): > > https://codereview.chromium.org/305783004/diff/1/Source/core/svg/animation/SV... > Source/core/svg/animation/SVGSMILElement.h:201: > RefPtrWillBeWeakMember<SVGSMILElement> m_syncBase; > On 2014/05/29 05:08:10, kouhei wrote: > > On 2014/05/29 04:58:58, haraken wrote: > > > It looks strange that RefPtr becomes WeakMember. Shouldn't the Condition and > > the > > > SVGSMILElement live & die together? In what situation do we want to let the > > > Condition outlive the SVGSMILElement? > > > > syncBase was manually unregistered via d-tor, however we are removing this in > > oilpan builds, so this need to be weak. > > Even if "X was manually unregistered via d-tor", it doesn't necessarily mean > that we want to make it weak. It's possible that the pointer was a back pointer > (not a weak pointer) and thus was manually unregistered when X died. > > The point is whether the Condition and the SVGSMILElement should die together or > not. So I want to understand in what situation we want to let the Condition > outlive the SVGSMILElement. Discussed offline. Changed all to strong references as the tree removal case I was worrying is explicitly handled in |removedFrom|.
LGTM. https://codereview.chromium.org/305783004/diff/20001/Source/core/svg/animatio... File Source/core/svg/animation/SVGSMILElement.cpp (right): https://codereview.chromium.org/305783004/diff/20001/Source/core/svg/animatio... Source/core/svg/animation/SVGSMILElement.cpp:1250: DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<WillBeHeapHashSet<RawPtrWillBeMember<SVGSMILElement> > >, loopBreaker, (adoptPtrWillBeNoop(new WillBeHeapHashSet<RawPtrWillBeMember<SVGSMILElement> >()))); Discussed offline. I wonder if this Persistent<Member> might cause leaks, but it won't happen because the entry is removed when we finish the recursion of notifyDependentsIntervalChanged. Would you add a comment about why we need the loopBreaker hack?
On 2014/05/29 08:11:52, haraken wrote: > LGTM. > > https://codereview.chromium.org/305783004/diff/20001/Source/core/svg/animatio... > File Source/core/svg/animation/SVGSMILElement.cpp (right): > > https://codereview.chromium.org/305783004/diff/20001/Source/core/svg/animatio... > Source/core/svg/animation/SVGSMILElement.cpp:1250: > DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<WillBeHeapHashSet<RawPtrWillBeMember<SVGSMILElement> > > >, loopBreaker, (adoptPtrWillBeNoop(new > WillBeHeapHashSet<RawPtrWillBeMember<SVGSMILElement> >()))); > > Discussed offline. I wonder if this Persistent<Member> might cause leaks, but it > won't happen because the entry is removed when we finish the recursion of > notifyDependentsIntervalChanged. > > Would you add a comment about why we need the loopBreaker hack? Ack. Will do on a followup CL.
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/305783004/20001
Message was sent while issue was closed.
Change committed as 175088 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
