Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
433653003:
Support Promises event-based instrumentation on backend. (Closed)
|
Created:
6 years, 4 months ago by Alexandra Mikhaylova Modified:
6 years, 3 months ago CC:
blink-reviews, vsevik+blink_chromium.org, caseq+blink_chromium.org, arv+blink, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, abarth-chromium, loislo+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, blink-reviews-bindings_chromium.org, devtools-reviews_chromium.org, apavlov+blink_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionSupport Promises event-based instrumentation on backend.
BUG=348919
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181047
Patch Set 1 #Patch Set 2 : Remove TODO #
Total comments: 28
Patch Set 3 : Do not enable PromiseTracker by default #
Total comments: 2
Patch Set 4 : Address comments + REBASE #
Total comments: 23
Patch Set 5 : Address comments #
Total comments: 14
Patch Set 6 : Address comments #
Total comments: 22
Patch Set 7 : Address comments #Patch Set 8 : Disable PromiseTracker and REBASE #
Total comments: 6
Patch Set 9 : Address comments + REBASE #
Total comments: 6
Patch Set 10 : Address comments #
Total comments: 10
Patch Set 11 : Address review comments + REBASE #
Total comments: 1
Messages
Total messages: 27 (0 generated)
https://codereview.chromium.org/433653003/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/ScriptDebugServer.cpp (right): https://codereview.chromium.org/433653003/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/ScriptDebugServer.cpp:542: listener->didReceiveV8NewPromiseEvent(context, promise); merge them all into didReceiveV8PromiseEvent, do this logic in PromiseTracker https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:2: * Copyright (C) 2014 Google Inc. All rights reserved. use new https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:43: , m_functionDetails(*(new ScriptCallFrame("", "", "", 0, 0))) memory leak. just remove https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:49: if (getStack) { captureStack https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:50: RefPtrWillBeRawPtr<ScriptCallStack> stack = createScriptCallStack(1, true); should create stack on the given isolate https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:153: { memory leak for PromiseDataWrapper. use OwnPtr https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:155: PromiseData* promiseData = data.GetParameter()->m_data.get(); use WeakPtr<PromiseData> https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.h (right): https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:2: * Copyright (C) 2014 Google Inc. All rights reserved. use new copyright header https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:50: PromiseTracker() : m_isEnabled(true) { } default: false https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:58: void didCreatePromise(ExecutionContext*, v8::Handle<v8::Object> promise); dont use v8::* here https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:69: ScopedPersistent<v8::Object> m_promise; should not be public https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:70: ScriptCallFrame m_functionDetails; function details? https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:78: struct PromiseDataWrapper { remove from .h file https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:95: PromiseDataVector* getPromiseVector(v8::Handle<v8::Object> promise); we dont use "get" prefix https://codereview.chromium.org/433653003/diff/40001/Source/core/inspector/Sc... File Source/core/inspector/ScriptDebugListener.h (right): https://codereview.chromium.org/433653003/diff/40001/Source/core/inspector/Sc... Source/core/inspector/ScriptDebugListener.h:83: virtual void didReceiveV8NewPromiseEvent(ExecutionContext*, v8::Handle<v8::Object> promise) = 0; dont use v8::* here
https://codereview.chromium.org/433653003/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/ScriptDebugServer.cpp (right): https://codereview.chromium.org/433653003/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/ScriptDebugServer.cpp:542: listener->didReceiveV8NewPromiseEvent(context, promise); On 2014/08/01 08:28:12, aandrey wrote: > merge them all into didReceiveV8PromiseEvent, do this logic in PromiseTracker Done. https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:2: * Copyright (C) 2014 Google Inc. All rights reserved. On 2014/08/01 08:28:12, aandrey wrote: > use new Done. https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:43: , m_functionDetails(*(new ScriptCallFrame("", "", "", 0, 0))) On 2014/08/01 08:28:12, aandrey wrote: > memory leak. just remove Done. https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:49: if (getStack) { On 2014/08/01 08:28:12, aandrey wrote: > captureStack Done. https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:50: RefPtrWillBeRawPtr<ScriptCallStack> stack = createScriptCallStack(1, true); On 2014/08/01 08:28:12, aandrey wrote: > should create stack on the given isolate Done. https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:153: { On 2014/08/01 08:28:12, aandrey wrote: > memory leak for PromiseDataWrapper. use OwnPtr Thanks, done. https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:155: PromiseData* promiseData = data.GetParameter()->m_data.get(); On 2014/08/01 08:28:12, aandrey wrote: > use WeakPtr<PromiseData> Done. https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.h (right): https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:2: * Copyright (C) 2014 Google Inc. All rights reserved. On 2014/08/01 08:28:12, aandrey wrote: > use new copyright header Done. https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:50: PromiseTracker() : m_isEnabled(true) { } On 2014/08/01 08:28:12, aandrey wrote: > default: false Done. https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:58: void didCreatePromise(ExecutionContext*, v8::Handle<v8::Object> promise); On 2014/08/01 08:28:12, aandrey wrote: > dont use v8::* here Done. https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:69: ScopedPersistent<v8::Object> m_promise; On 2014/08/01 08:28:12, aandrey wrote: > should not be public Done. https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:70: ScriptCallFrame m_functionDetails; On 2014/08/01 08:28:13, aandrey wrote: > function details? Renamed to m_callFrame. https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:78: struct PromiseDataWrapper { On 2014/08/01 08:28:12, aandrey wrote: > remove from .h file Done. https://codereview.chromium.org/433653003/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:95: PromiseDataVector* getPromiseVector(v8::Handle<v8::Object> promise); On 2014/08/01 08:28:12, aandrey wrote: > we dont use "get" prefix Done. https://codereview.chromium.org/433653003/diff/40001/Source/core/inspector/Sc... File Source/core/inspector/ScriptDebugListener.h (right): https://codereview.chromium.org/433653003/diff/40001/Source/core/inspector/Sc... Source/core/inspector/ScriptDebugListener.h:83: virtual void didReceiveV8NewPromiseEvent(ExecutionContext*, v8::Handle<v8::Object> promise) = 0; On 2014/08/01 08:28:13, aandrey wrote: > dont use v8::* here Done.
https://codereview.chromium.org/433653003/diff/60001/Source/bindings/core/v8/... File Source/bindings/core/v8/ScriptDebugServer.cpp (right): https://codereview.chromium.org/433653003/diff/60001/Source/bindings/core/v8/... Source/bindings/core/v8/ScriptDebugServer.cpp:539: int status = static_cast<int>(promiseDetails->Get(v8AtomicString(m_isolate, "status"))->ToInteger()->Value()); why static_cast<int>? https://codereview.chromium.org/433653003/diff/60001/Source/bindings/core/v8/... Source/bindings/core/v8/ScriptDebugServer.cpp:541: ScopedPersistent<v8::Object> parentPromise(isolate, parentPromiseHandle->IsUndefined() ? v8::Handle<v8::Object>() : parentPromiseHandle->ToObject()); use IsObject() instead of IsUndefined() https://codereview.chromium.org/433653003/diff/60001/Source/bindings/core/v8/... Source/bindings/core/v8/ScriptDebugServer.cpp:542: listener->didReceiveV8PromiseEvent(pausedScriptState, promise, parentPromise, status); nit: line break before, assign m_pausedScriptState and m_executionState just before this line, as in lines 523-528 https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorDebuggerAgent.h (right): https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorDebuggerAgent.h:229: virtual void didReceiveV8PromiseEvent(ScriptState*, const ScopedPersistent<v8::Object>& promise, const ScopedPersistent<v8::Object>& parentPromise, int status) OVERRIDE FINAL; v8::* not allowed here https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:13: struct PromiseTracker::PromiseDataWrapper { wrap into an anonymous namespace https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:27: if (!(promiseData.get() && promiseData->m_promiseTracker->isEnabled())) if (!promiseData) return; PromiseTracker* tracker = promiseData->m_promiseTracker; if (!tracker->isEnabled()) return; https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:37: , m_promise(scriptState->isolate(), promise.newLocal(scriptState->isolate())) make copy constructor on ScopedPersistent https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:104: RefPtr<PromiseData> data = (*vector)[index]; vector->at(index) https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:108: data->m_parentPromise.set(isolate, parentPromise.newLocal(isolate)); should be no newLocal() call https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:136: int promiseHash = promise.newLocal(scriptState->isolate())->GetIdentityHash(); we can remove m_scriptState if we cache the identity hash https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:145: vector->remove(index); too much code duplication https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.h (right): https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:54: void didRemovePromise(ScriptState*, ScopedPersistent<v8::Object>& promise); should not be public
https://codereview.chromium.org/433653003/diff/60001/Source/bindings/core/v8/... File Source/bindings/core/v8/ScriptDebugServer.cpp (right): https://codereview.chromium.org/433653003/diff/60001/Source/bindings/core/v8/... Source/bindings/core/v8/ScriptDebugServer.cpp:539: int status = static_cast<int>(promiseDetails->Get(v8AtomicString(m_isolate, "status"))->ToInteger()->Value()); On 2014/08/04 16:58:47, aandrey wrote: > why static_cast<int>? Removed it. https://codereview.chromium.org/433653003/diff/60001/Source/bindings/core/v8/... Source/bindings/core/v8/ScriptDebugServer.cpp:541: ScopedPersistent<v8::Object> parentPromise(isolate, parentPromiseHandle->IsUndefined() ? v8::Handle<v8::Object>() : parentPromiseHandle->ToObject()); On 2014/08/04 16:58:47, aandrey wrote: > use IsObject() instead of IsUndefined() Done. https://codereview.chromium.org/433653003/diff/60001/Source/bindings/core/v8/... Source/bindings/core/v8/ScriptDebugServer.cpp:542: listener->didReceiveV8PromiseEvent(pausedScriptState, promise, parentPromise, status); On 2014/08/04 16:58:47, aandrey wrote: > nit: line break before, assign m_pausedScriptState and m_executionState just > before this line, as in lines 523-528 Done. https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorDebuggerAgent.h (right): https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorDebuggerAgent.h:229: virtual void didReceiveV8PromiseEvent(ScriptState*, const ScopedPersistent<v8::Object>& promise, const ScopedPersistent<v8::Object>& parentPromise, int status) OVERRIDE FINAL; On 2014/08/04 16:58:47, aandrey wrote: > v8::* not allowed here We'll keep it here for now as there's no quick alternative. https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:13: struct PromiseTracker::PromiseDataWrapper { On 2014/08/04 16:58:47, aandrey wrote: > wrap into an anonymous namespace How can it be compliant with the private method PromiseTracker::didRemovePromise then? https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:27: if (!(promiseData.get() && promiseData->m_promiseTracker->isEnabled())) On 2014/08/04 16:58:48, aandrey wrote: > if (!promiseData) > return; > PromiseTracker* tracker = promiseData->m_promiseTracker; > if (!tracker->isEnabled()) > return; Done. https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:37: , m_promise(scriptState->isolate(), promise.newLocal(scriptState->isolate())) On 2014/08/04 16:58:48, aandrey wrote: > make copy constructor on ScopedPersistent Switched back to v8::Handle instead. https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:104: RefPtr<PromiseData> data = (*vector)[index]; On 2014/08/04 16:58:48, aandrey wrote: > vector->at(index) Done. https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:108: data->m_parentPromise.set(isolate, parentPromise.newLocal(isolate)); On 2014/08/04 16:58:48, aandrey wrote: > should be no newLocal() call Done. https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:136: int promiseHash = promise.newLocal(scriptState->isolate())->GetIdentityHash(); On 2014/08/04 16:58:48, aandrey wrote: > we can remove m_scriptState if we cache the identity hash Done. https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.h (right): https://codereview.chromium.org/433653003/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:54: void didRemovePromise(ScriptState*, ScopedPersistent<v8::Object>& promise); On 2014/08/04 16:58:48, aandrey wrote: > should not be public Done.
https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:13: struct PromiseTracker::PromiseDataWrapper { should be just PromiseDataWrapper, wrapped into namespace { ... } https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:72: ASSERT(isEnabled()); redundant? https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:131: void PromiseTracker::didRemovePromise(WeakPtr<PromiseData>& promiseData) don't use WeakPtr here https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:133: ASSERT(isEnabled()); remove https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:142: found = true; too much copy-pasted code duplication, this is a no-go. this var is unused. https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.h (right): https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:53: struct PromiseDataWrapper; remove https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:56: friend struct PromiseDataWrapper; remove
https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:13: struct PromiseTracker::PromiseDataWrapper { On 2014/08/06 16:31:31, aandrey wrote: > should be just PromiseDataWrapper, wrapped into namespace { ... } Done. https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:72: ASSERT(isEnabled()); On 2014/08/06 16:31:31, aandrey wrote: > redundant? Removed. https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:131: void PromiseTracker::didRemovePromise(WeakPtr<PromiseData>& promiseData) On 2014/08/06 16:31:31, aandrey wrote: > don't use WeakPtr here Done. https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:133: ASSERT(isEnabled()); On 2014/08/06 16:31:31, aandrey wrote: > remove Done. https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:142: found = true; On 2014/08/06 16:31:31, aandrey wrote: > too much copy-pasted code duplication, this is a no-go. > this var is unused. Refactored the methods. https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.h (right): https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:53: struct PromiseDataWrapper; On 2014/08/06 16:31:31, aandrey wrote: > remove Done. https://codereview.chromium.org/433653003/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:56: friend struct PromiseDataWrapper; On 2014/08/06 16:31:32, aandrey wrote: > remove Done.
https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:16: WTF_MAKE_NONCOPYABLE(PromiseDataWrapper); remove https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:17: public: remove https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:28: PromiseTracker::PromiseDataMap* map = wrapper->m_map; nit: move below the if- check https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:44: PromiseTracker::PromiseDataMap* m_map; m_promiseDataMap https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:62: PromiseDataWrapper* wrapper = new PromiseDataWrapper(m_weakPtrFactory.createWeakPtr(), &tracker->m_promiseDataMap); maybe move this code into the tracker to remove PromiseTracker* ref from constructor? https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:82: PromiseTracker::PromiseDataVector* PromiseTracker::promiseVector(int promiseHash) remove this method https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:94: PromiseDataVector* vector = promiseVector(promise->GetIdentityHash()); GetIdentityHash should be called once https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:96: RefPtr<PromiseData> data = vector->at(index); still code dup. maybe create a static helper method: static int indexOf(PromiseDataVector* vector, v8::Handle<v8::Object> promise) { ... } https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:107: vector->append(adoptRef(new PromiseData(scriptState->isolate(), promise, parentPromise->IsObject() ? parentPromise->ToObject() : v8::Handle<v8::Object>(), 0, this, status == 0 && !parentPromise->IsObject()))); 1) extract bool captureStack 2) status may not be 0
https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:107: vector->append(adoptRef(new PromiseData(scriptState->isolate(), promise, parentPromise->IsObject() ? parentPromise->ToObject() : v8::Handle<v8::Object>(), 0, this, status == 0 && !parentPromise->IsObject()))); check parentPromise.IsEmpty()
https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:32: PromiseTracker::PromiseDataVector* vector = &map->find(promiseHash)->value; can find nothing, add safety check
https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:16: WTF_MAKE_NONCOPYABLE(PromiseDataWrapper); On 2014/08/07 17:12:47, aandrey wrote: > remove Done. https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:17: public: On 2014/08/07 17:12:48, aandrey wrote: > remove Done. https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:28: PromiseTracker::PromiseDataMap* map = wrapper->m_map; On 2014/08/07 17:12:48, aandrey wrote: > nit: move below the if- check Done. https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:32: PromiseTracker::PromiseDataVector* vector = &map->find(promiseHash)->value; On 2014/08/07 17:29:29, aandrey wrote: > can find nothing, add safety check It should find the correct vector, added an ASSERT below. https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:44: PromiseTracker::PromiseDataMap* m_map; On 2014/08/07 17:12:47, aandrey wrote: > m_promiseDataMap Done. https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:62: PromiseDataWrapper* wrapper = new PromiseDataWrapper(m_weakPtrFactory.createWeakPtr(), &tracker->m_promiseDataMap); On 2014/08/07 17:12:47, aandrey wrote: > maybe move this code into the tracker to remove PromiseTracker* ref from > constructor? Done. https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:82: PromiseTracker::PromiseDataVector* PromiseTracker::promiseVector(int promiseHash) On 2014/08/07 17:12:47, aandrey wrote: > remove this method Done. https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:94: PromiseDataVector* vector = promiseVector(promise->GetIdentityHash()); On 2014/08/07 17:12:47, aandrey wrote: > GetIdentityHash should be called once Done. https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:96: RefPtr<PromiseData> data = vector->at(index); On 2014/08/07 17:12:47, aandrey wrote: > still code dup. maybe create a static helper method: > > static int indexOf(PromiseDataVector* vector, v8::Handle<v8::Object> promise) > { > ... > } Introduced a static helper method, but it can't be used right here. I'll use it in future CLs. https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:107: vector->append(adoptRef(new PromiseData(scriptState->isolate(), promise, parentPromise->IsObject() ? parentPromise->ToObject() : v8::Handle<v8::Object>(), 0, this, status == 0 && !parentPromise->IsObject()))); On 2014/08/07 17:12:47, aandrey wrote: > 1) extract bool captureStack > 2) status may not be 0 1) Done. 2) Is status != 0, then it is not a new promise event, so we shouldn't capture the stack. https://codereview.chromium.org/433653003/diff/100001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:107: vector->append(adoptRef(new PromiseData(scriptState->isolate(), promise, parentPromise->IsObject() ? parentPromise->ToObject() : v8::Handle<v8::Object>(), 0, this, status == 0 && !parentPromise->IsObject()))); On 2014/08/07 17:19:02, aandrey wrote: > check parentPromise.IsEmpty() Done.
almost there https://codereview.chromium.org/433653003/diff/140001/Source/core/inspector/P... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/140001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:17: if (data->promise() == promise) if (vector->at(index)->promise() == promise) https://codereview.chromium.org/433653003/diff/140001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:98: if (data->m_promise == promise) { RefPtr<PromiseData> data; size_t index = indexOf(vector, promise); if (index == -1) { data = adoptRef(new PromiseData()); // append_into_vector & set_weak } else { data = vector->at(index); } if (parentPromise.IsEmpty()) { // %DebugPromiseEvent({ promise: promise, status: status, value: value }); // ... } else { // %DebugPromiseEvent({ promise: deferred.promise, parentPromise: this }); ASSERT(parentPromise->IsPromise()); // ... } https://codereview.chromium.org/433653003/diff/140001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:109: PromiseData* data = new PromiseData(scriptState->isolate(), promiseHash, promise, parentPromise->IsObject() ? parentPromise->ToObject() : v8::Handle<v8::Object>(), 0, captureStack); parentPromise->IsObject() will crash if parentPromise IsEmpty
https://codereview.chromium.org/433653003/diff/140001/Source/core/inspector/P... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/140001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:17: if (data->promise() == promise) On 2014/08/25 09:48:37, aandrey wrote: > if (vector->at(index)->promise() == promise) Done. https://codereview.chromium.org/433653003/diff/140001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:98: if (data->m_promise == promise) { On 2014/08/25 09:48:37, aandrey wrote: > RefPtr<PromiseData> data; > size_t index = indexOf(vector, promise); > if (index == -1) { > data = adoptRef(new PromiseData()); > // append_into_vector & set_weak > } else { > data = vector->at(index); > } > > if (parentPromise.IsEmpty()) { > // %DebugPromiseEvent({ promise: promise, status: status, value: value }); > // ... > } else { > // %DebugPromiseEvent({ promise: deferred.promise, parentPromise: this }); > ASSERT(parentPromise->IsPromise()); > // ... > } Done. https://codereview.chromium.org/433653003/diff/140001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:109: PromiseData* data = new PromiseData(scriptState->isolate(), promiseHash, promise, parentPromise->IsObject() ? parentPromise->ToObject() : v8::Handle<v8::Object>(), 0, captureStack); On 2014/08/25 09:48:37, aandrey wrote: > parentPromise->IsObject() will crash if parentPromise IsEmpty Fixed this method as described above.
https://codereview.chromium.org/433653003/diff/160001/Source/core/inspector/P... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/160001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:89: v8::Isolate* isolate = scriptState->isolate(); nit: put a blank line before this line https://codereview.chromium.org/433653003/diff/160001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:101: if (status != 0) { why not just: if (!parentPromise.IsEmpty()) { data->m_parentPromise.set(isolate, parentPromise->ToObject()); } else { data->m_status = status; if (!status) { // capture stack... } } ?
https://codereview.chromium.org/433653003/diff/160001/Source/core/inspector/P... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/160001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:56: , m_parentPromise(isolate, v8::Handle<v8::Object>()) just remove the line
https://codereview.chromium.org/433653003/diff/160001/Source/core/inspector/P... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/160001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:56: , m_parentPromise(isolate, v8::Handle<v8::Object>()) On 2014/08/25 14:38:52, aandrey wrote: > just remove the line Done. https://codereview.chromium.org/433653003/diff/160001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:89: v8::Isolate* isolate = scriptState->isolate(); On 2014/08/25 14:36:54, aandrey wrote: > nit: put a blank line before this line Done. https://codereview.chromium.org/433653003/diff/160001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:101: if (status != 0) { On 2014/08/25 14:36:54, aandrey wrote: > why not just: > > if (!parentPromise.IsEmpty()) { > data->m_parentPromise.set(isolate, parentPromise->ToObject()); > } else { > data->m_status = status; > if (!status) { > // capture stack... > } > } > > ? Thanks, done.
LGTM. @yurys, please do an owner review
https://codereview.chromium.org/433653003/diff/180001/Source/core/inspector/P... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/180001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:24: struct PromiseDataWrapper { Let's make it a class. https://codereview.chromium.org/433653003/diff/180001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:46: These fields should be private. https://codereview.chromium.org/433653003/diff/180001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:94: PromiseDataWrapper* wrapper = new PromiseDataWrapper(data->m_weakPtrFactory.createWeakPtr(), &m_promiseDataMap); adoptPtr(...).leakPtr() or better assign to an OwnPtr and release it below. https://codereview.chromium.org/433653003/diff/180001/Source/core/inspector/P... File Source/core/inspector/PromiseTracker.h (right): https://codereview.chromium.org/433653003/diff/180001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.h:34: class PromiseData : public RefCounted<PromiseData> { Why is this public? https://codereview.chromium.org/433653003/diff/180001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.h:55: typedef HashMap<int, PromiseDataVector> PromiseDataMap; Why not use HashMap<Handle<Promise>, PromiseData> ?
https://codereview.chromium.org/433653003/diff/180001/Source/core/inspector/P... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/433653003/diff/180001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:24: struct PromiseDataWrapper { On 2014/08/26 14:06:36, yurys wrote: > Let's make it a class. Done. https://codereview.chromium.org/433653003/diff/180001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:46: On 2014/08/26 14:06:36, yurys wrote: > These fields should be private. Done. https://codereview.chromium.org/433653003/diff/180001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.cpp:94: PromiseDataWrapper* wrapper = new PromiseDataWrapper(data->m_weakPtrFactory.createWeakPtr(), &m_promiseDataMap); On 2014/08/26 14:06:36, yurys wrote: > adoptPtr(...).leakPtr() or better assign to an OwnPtr and release it below. Assigned to an OwnPtr and used leakPtr() below since setWeak needs a raw pointer. https://codereview.chromium.org/433653003/diff/180001/Source/core/inspector/P... File Source/core/inspector/PromiseTracker.h (right): https://codereview.chromium.org/433653003/diff/180001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.h:34: class PromiseData : public RefCounted<PromiseData> { On 2014/08/26 14:06:36, yurys wrote: > Why is this public? It's used in PromiseDataWrapper that's wrapped in an unnamed namespace, left only declaration here. https://codereview.chromium.org/433653003/diff/180001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.h:55: typedef HashMap<int, PromiseDataVector> PromiseDataMap; On 2014/08/26 14:06:36, yurys wrote: > Why not use HashMap<Handle<Promise>, PromiseData> ? We're going to migrate to WeakMap in future, using this ad hoc implementation of hashmap for now.
https://codereview.chromium.org/433653003/diff/200001/Source/core/inspector/P... File Source/core/inspector/PromiseTracker.h (right): https://codereview.chromium.org/433653003/diff/200001/Source/core/inspector/P... Source/core/inspector/PromiseTracker.h:32: class PromiseData; make this and below private? (if needed remove anonymous namespace around PromiseDataWrapper)
lgtm provided aandrey's comment is addressed.
On 2014/08/28 14:05:53, aandrey wrote: > https://codereview.chromium.org/433653003/diff/200001/Source/core/inspector/P... > File Source/core/inspector/PromiseTracker.h (right): > > https://codereview.chromium.org/433653003/diff/200001/Source/core/inspector/P... > Source/core/inspector/PromiseTracker.h:32: class PromiseData; > make this and below private? (if needed remove anonymous namespace around > PromiseDataWrapper)
lgtm
The CQ bit was checked by amikhaylova@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amikhaylova@google.com/433653003/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/24682)
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as 181047 |
