Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
422273002:
Oilpan: Prepare moving AsyncCallStackTracker to Oilpan (Closed)
|
Created:
6 years, 4 months ago by keishi Modified:
6 years, 4 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionOilpan: Prepare moving AsyncCallStackTracker to Oilpan
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180088
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : Merged 413113003 #
Total comments: 4
Patch Set 4 : Fixed #
Total comments: 6
Patch Set 5 : #
Total comments: 2
Patch Set 6 : break long line #Patch Set 7 : #Patch Set 8 : #
Total comments: 1
Messages
Total messages: 24 (0 generated)
Merged CL from https://codereview.chromium.org/413113003/
+aandrey - author of the async call stacks implementation.
https://codereview.chromium.org/422273002/diff/20001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/422273002/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:75: delete self; This would be wrong. We cannot explicitly destroy on-heap objects. https://codereview.chromium.org/422273002/diff/20001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.h (right): https://codereview.chromium.org/422273002/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.h:137: typedef WillBeHeapHashMap<ExecutionContext*, OwnPtrWillBeMember<ExecutionContextData> > ExecutionContextDataMap; ExecutionContext is on-heap, so you can use a Member.
https://codereview.chromium.org/422273002/diff/20001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/422273002/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:72: ExecutionContextData* self = m_tracker->m_executionContextDataMap.take(executionContext()); this should have been: OwnPtr<ExecutionContextData> self = adoptPtr(...); https://codereview.chromium.org/422273002/diff/40001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.h (right): https://codereview.chromium.org/422273002/diff/40001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.h:52: class AsyncCallStackTracker : public NoBaseWillBeGarbageCollectedFinalized<AsyncCallStackTracker> { FINAL https://codereview.chromium.org/422273002/diff/40001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.h:69: class AsyncCallChain : public RefCountedWillBeGarbageCollected<AsyncCallChain> { FINAL, RefCountedWillBeGarbageCollectedFinalized
https://codereview.chromium.org/422273002/diff/20001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/422273002/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:72: ExecutionContextData* self = m_tracker->m_executionContextDataMap.take(executionContext()); On 2014/08/11 11:11:04, aandrey wrote: > this should have been: OwnPtr<ExecutionContextData> self = adoptPtr(...); Done. https://codereview.chromium.org/422273002/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:75: delete self; On 2014/08/11 10:59:59, haraken wrote: > > This would be wrong. We cannot explicitly destroy on-heap objects. Done. https://codereview.chromium.org/422273002/diff/20001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.h (right): https://codereview.chromium.org/422273002/diff/20001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.h:137: typedef WillBeHeapHashMap<ExecutionContext*, OwnPtrWillBeMember<ExecutionContextData> > ExecutionContextDataMap; On 2014/08/11 10:59:59, haraken wrote: > > ExecutionContext is on-heap, so you can use a Member. Done. https://codereview.chromium.org/422273002/diff/40001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.h (right): https://codereview.chromium.org/422273002/diff/40001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.h:52: class AsyncCallStackTracker : public NoBaseWillBeGarbageCollectedFinalized<AsyncCallStackTracker> { On 2014/08/11 11:11:04, aandrey wrote: > FINAL Done. https://codereview.chromium.org/422273002/diff/40001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.h:69: class AsyncCallChain : public RefCountedWillBeGarbageCollected<AsyncCallChain> { On 2014/08/11 11:11:04, aandrey wrote: > FINAL, RefCountedWillBeGarbageCollectedFinalized Added FINAL. AsyncCallChain doesn't seem to require finalization.
https://codereview.chromium.org/422273002/diff/60001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/422273002/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:72: OwnPtrWillBeRawPtr<ExecutionContextData> self = adoptPtrWillBeNoop(m_tracker->m_executionContextDataMap.take(executionContext())); I think adoptPtrWillBeNoop() is not needed, once the value type is OwnPtrWillBeMember<ExecutionContextData> https://codereview.chromium.org/422273002/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:479: data = new AsyncCallStackTracker::ExecutionContextData(this, context); adoptPtrWillBeNoop?
https://codereview.chromium.org/422273002/diff/60001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/422273002/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:492: delete it->value; This would be problematic as well. You cannot explicitly destruct on-heap objects.
https://codereview.chromium.org/422273002/diff/60001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/422273002/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:72: OwnPtrWillBeRawPtr<ExecutionContextData> self = adoptPtrWillBeNoop(m_tracker->m_executionContextDataMap.take(executionContext())); On 2014/08/11 13:35:49, aandrey wrote: > I think adoptPtrWillBeNoop() is not needed, once the value type is > OwnPtrWillBeMember<ExecutionContextData> Done. https://codereview.chromium.org/422273002/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:479: data = new AsyncCallStackTracker::ExecutionContextData(this, context); On 2014/08/11 13:35:49, aandrey wrote: > adoptPtrWillBeNoop? Done. https://codereview.chromium.org/422273002/diff/60001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:492: delete it->value; On 2014/08/11 13:47:06, haraken wrote: > > This would be problematic as well. You cannot explicitly destruct on-heap > objects. > Done.
lgtm
LGTM https://codereview.chromium.org/422273002/diff/80001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/422273002/diff/80001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:479: data = m_executionContextDataMap.set(context, adoptPtrWillBeNoop(new AsyncCallStackTracker::ExecutionContextData(this, context))).storedValue->value.get(); Break up this line. It looks a bit too complex.
lgtm
Thanks, everyone. https://codereview.chromium.org/422273002/diff/80001/Source/core/inspector/As... File Source/core/inspector/AsyncCallStackTracker.cpp (right): https://codereview.chromium.org/422273002/diff/80001/Source/core/inspector/As... Source/core/inspector/AsyncCallStackTracker.cpp:479: data = m_executionContextDataMap.set(context, adoptPtrWillBeNoop(new AsyncCallStackTracker::ExecutionContextData(this, context))).storedValue->value.get(); On 2014/08/12 08:38:17, haraken wrote: > > Break up this line. It looks a bit too complex. Done.
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/422273002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...) android_blink_compile_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...) linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...) linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...) mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19434) win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22237) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/40463) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/45703)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19443)
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/422273002/120001
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/422273002/140001
Message was sent while issue was closed.
Change committed as 180088
Message was sent while issue was closed.
https://codereview.chromium.org/422273002/diff/140001/Source/core/inspector/A... File Source/core/inspector/AsyncCallStackTracker.h (right): https://codereview.chromium.org/422273002/diff/140001/Source/core/inspector/A... Source/core/inspector/AsyncCallStackTracker.h:81: class ExecutionContextData FINAL : public NoBaseWillBeGarbageCollectedFinalized<ExecutionContextData>, public ContextLifecycleObserver { It is very unfortunate that you had to move a private implementation class to public API. Can we avoid this? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
