Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
177773002:
Support Promises instrumentation on backend. (Closed)
|
Created:
6 years, 9 months ago by Alexandra Mikhaylova Modified:
6 years, 9 months ago Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionSupport Promises instrumentation on backend.
BUG=348919
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169247
Patch Set 1 #Patch Set 2 : Implement updatePromiseParent + update instrumentation #Patch Set 3 : Fix in printing NULL parent promises #
Total comments: 26
Patch Set 4 : Address review comments + track Promise result #
Total comments: 64
Patch Set 5 : Address review comments + track call stacks and time #Patch Set 6 : Fix style #
Total comments: 37
Patch Set 7 : Small refactoring to avoid unused variables #Patch Set 8 : Address review comments #Patch Set 9 : Revert blank line #
Total comments: 12
Patch Set 10 : Address comments #
Total comments: 9
Patch Set 11 : PromiseDataMap refactoring #
Total comments: 11
Patch Set 12 : Address review comments #Patch Set 13 : Rename PromiseOfficer -> PromiseTracker #
Total comments: 3
Patch Set 14 : V8PromiseCustom::setState -> (anonymous namespace)::setStateForPromise + REBASE #Messages
Total messages: 37 (0 generated)
Work is in progress, and this is not for review yet.
More instrumentation + implement updating promise parent
https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:393: InspectorInstrumentation::updatePromiseState(currentExecutionContext(isolate), promise, V8PromiseCustom::Fulfilled); move inside V8PromiseCustom::setState https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:402: InspectorInstrumentation::updatePromiseState(currentExecutionContext(isolate), promise, V8PromiseCustom::Rejected); ditto https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:644: InspectorInstrumentation::addPromise(currentExecutionContext(isolate), one line https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:646: v8::Handle<v8::Object>::Cast(v8::Null(isolate)), v8::Handle<v8::Object>() https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:647: Pending); remove Pending, move up before setState() https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:718: setState(internal, Following, valuePromise, isolate); // TODO: how can we track the Following state? remove todo https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:746: InspectorInstrumentation::updatePromiseParent(currentExecutionContext(isolate), derivedPromise, promise); move inside updateDerivedFromPromise() https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorDebuggerAgent.cpp (right): https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorDebuggerAgent.cpp:809: m_promiseOfficer.addPromise(context, promise, parentPromise, state); if enabled() check https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorInstrumentation.idl (right): https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorInstrumentation.idl:65: #include <v8.h> introduce new interface InspectorPromiseInstrumentation https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorInstrumentation.idl:220: void updatePromiseParent([Keep] ExecutionContext*, v8::Handle<v8::Object> promise, v8::Handle<v8::Object> newParentPromise); newParentPromise -> parent https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/Pr... File Source/core/inspector/PromiseOfficer.cpp (right): https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:3: * Copyright (C) 2013 Google Inc. All rights reserved. 2014 https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:106: void PromiseOfficer::updatePromiseState(ExecutionContext* context, v8::Handle<v8::Object> promise, V8PromiseCustom::PromiseState newState) { remove code dups
https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:393: InspectorInstrumentation::updatePromiseState(currentExecutionContext(isolate), promise, V8PromiseCustom::Fulfilled); On 2014/02/25 12:36:07, aandrey wrote: > move inside V8PromiseCustom::setState It seems that we can't move the instrumentation inside V8PromiseCustom as there's no our promise (but only internal of this promise) there. I've instrumented all the places where V8PromiseCustom::setState is called, instead. https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:402: InspectorInstrumentation::updatePromiseState(currentExecutionContext(isolate), promise, V8PromiseCustom::Rejected); On 2014/02/25 12:36:07, aandrey wrote: > ditto see above https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:644: InspectorInstrumentation::addPromise(currentExecutionContext(isolate), On 2014/02/25 12:36:07, aandrey wrote: > one line Done. https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:646: v8::Handle<v8::Object>::Cast(v8::Null(isolate)), On 2014/02/25 12:36:07, aandrey wrote: > v8::Handle<v8::Object>() Done. https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:647: Pending); On 2014/02/25 12:36:07, aandrey wrote: > remove Pending, move up before setState() Done. https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:718: setState(internal, Following, valuePromise, isolate); // TODO: how can we track the Following state? On 2014/02/25 12:36:07, aandrey wrote: > remove todo Done. https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:746: InspectorInstrumentation::updatePromiseParent(currentExecutionContext(isolate), derivedPromise, promise); On 2014/02/25 12:36:07, aandrey wrote: > move inside updateDerivedFromPromise() Done. https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorDebuggerAgent.cpp (right): https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorDebuggerAgent.cpp:809: m_promiseOfficer.addPromise(context, promise, parentPromise, state); On 2014/02/25 12:36:07, aandrey wrote: > if enabled() check Done. https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorInstrumentation.idl (right): https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorInstrumentation.idl:65: #include <v8.h> On 2014/02/25 12:36:07, aandrey wrote: > introduce new interface InspectorPromiseInstrumentation Done. https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorInstrumentation.idl:220: void updatePromiseParent([Keep] ExecutionContext*, v8::Handle<v8::Object> promise, v8::Handle<v8::Object> newParentPromise); On 2014/02/25 12:36:07, aandrey wrote: > newParentPromise -> parent I've done some renaming. https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/Pr... File Source/core/inspector/PromiseOfficer.cpp (right): https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:3: * Copyright (C) 2013 Google Inc. All rights reserved. On 2014/02/25 12:36:07, aandrey wrote: > 2014 Done. https://codereview.chromium.org/177773002/diff/60001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:106: void PromiseOfficer::updatePromiseState(ExecutionContext* context, v8::Handle<v8::Object> promise, V8PromiseCustom::PromiseState newState) { On 2014/02/25 12:36:07, aandrey wrote: > remove code dups Done, but updatePromiseParent and updatePromiseStateAndResult should be two different methods as we must check whether arguments (future PromiseData attribute values) are valid or not before updating information.
https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... File Source/core/inspector/PromiseOfficer.h (right): https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:99: m_promiseDataMap.clear(); Maybe disable() and clear() should be two different methods
https://codereview.chromium.org/177773002/diff/80001/LayoutTests/inspector/re... File LayoutTests/inspector/resources/example-fileset-for-test.js (right): https://codereview.chromium.org/177773002/diff/80001/LayoutTests/inspector/re... LayoutTests/inspector/resources/example-fileset-for-test.js:105: "./Source/core/inspector/InspectorPromiseInstrumentation.z", remove https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:47: #include "core/inspector/InspectorInstrumentation.h" remove https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:394: InspectorInstrumentation::setPromiseStateAndResult(currentExecutionContext(isolate), promise, value, V8PromiseCustom::Fulfilled); move inside setState https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:403: InspectorInstrumentation::setPromiseStateAndResult(currentExecutionContext(isolate), promise, reason, V8PromiseCustom::Rejected); ditto https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:646: InspectorInstrumentation::setPromiseStateAndResult(currentExecutionContext(isolate), promise, v8::Undefined(isolate), Pending); remove https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:649: revert https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:712: InspectorInstrumentation::setPromiseStateAndResult(currentExecutionContext(isolate), promise, valuePromiseFollowing, Following); move inside setState https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:721: InspectorInstrumentation::setPromiseStateAndResult(currentExecutionContext(isolate), promise, valuePromise, Following); move inside setState https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... File Source/core/inspector/InspectorDebuggerAgent.cpp (right): https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorDebuggerAgent.cpp:801: if (m_promiseOfficer.isEnabled()) { drop curly braces. also below https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorDebuggerAgent.cpp:1243: m_asyncCallStackTracker.clear(); clear promise officer data here https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorDebuggerAgent.cpp:1282: m_asyncCallStackTracker.clear(); clear promise officer data here https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... File Source/core/inspector/InspectorInstrumentation.idl (right): https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorInstrumentation.idl:547: void addPromise([Keep] ExecutionContext*, v8::Handle<v8::Object> promise, v8::Handle<v8::Object> parentPromise, V8PromiseCustom::PromiseState state = V8PromiseCustom::Pending); addPromise -> didCreatePromise https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorInstrumentation.idl:550: void setPromiseParent([Keep] ExecutionContext*, v8::Handle<v8::Object> promise, v8::Handle<v8::Object> parentPromise); didUpdatePromiseParent https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorInstrumentation.idl:553: void setPromiseStateAndResult([Keep] ExecutionContext*, v8::Handle<v8::Object> promise, v8::Handle<v8::Value> result, V8PromiseCustom::PromiseState state); didUpdatePromiseState https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... File Source/core/inspector/PromiseOfficer.cpp (right): https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:36: void PromiseOfficer::addPromise(ExecutionContext* context, v8::Handle<v8::Object> promise, v8::Handle<v8::Object> parentPromise, V8PromiseCustom::PromiseState state) { style: "{" on new line https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:41: for (size_t index = 0; index < vector.size(); index++) { remove or guard with "#ifndef NDEBUG" https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:48: vector.append(adoptRef(new PromiseData(promise, parentPromise, v8::Undefined(toIsolate(context)), state, context))); we can remove code dup: PromiseDataVector* vector; PromiseDataMap::iterator it = m_promiseDataMap.find(promiseHash); if (it != m_promiseDataMap.end()) vector = &m_promiseDataMap.set(promiseHash, PromiseDataVector()).storedValue->value; else vector = &it->value; vector->append(...); https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:56: ASSERT(it != m_promiseDataMap.end()); // This must be a promise that's already been tracked. we may be activated after a web page was loaded https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:60: if (data->getPromise() == promise && data->getParentPromise() != parentPromise) { remove: data->getParentPromise() != parentPromise https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:62: vector.append(adoptRef(new PromiseData(promise, parentPromise, data->getResult(), data->getState(), context))); just update the parent https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:71: ASSERT(it != m_promiseDataMap.end()); // This must be a promise that's already been tracked. ditto https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:75: if (data->getPromise() == promise && data->getState() != state) { remove: data->getState() != state https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:77: vector.append(adoptRef(new PromiseData(promise, data->getParentPromise(), result, state, context))); just update https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... File Source/core/inspector/PromiseOfficer.h (right): https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:49: class PromiseData : public RefCounted<PromiseData> { 1) make it private 2) why RefCounted? remove it? https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:57: { } style: { } https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:59: v8::Handle<v8::Object> getPromise() { style: "{" on the next line. also fix below https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:81: // TODO: maybe add m_scriptLocation or other label? remove https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:88: bool isEnabled() { bool isEnabled() const { return m_isEnabled; } https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:92: void enable() { move to cpp https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:96: void disable() { move to cpp https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:109: typedef Vector<RefPtr<PromiseData> > PromiseDataVector; Vector<PromiseData> + drop RefCounted
https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:393: InspectorInstrumentation::updatePromiseState(currentExecutionContext(isolate), promise, V8PromiseCustom::Fulfilled); On 2014/02/26 14:08:41, Alexandra Mikhaylova wrote: > On 2014/02/25 12:36:07, aandrey wrote: > > move inside V8PromiseCustom::setState > > It seems that we can't move the instrumentation inside V8PromiseCustom as > there's no our promise (but only internal of this promise) there. > > I've instrumented all the places where V8PromiseCustom::setState is called, > instead. make V8PromiseCustom::setState accept a promise
https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/60001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:393: InspectorInstrumentation::updatePromiseState(currentExecutionContext(isolate), promise, V8PromiseCustom::Fulfilled); On 2014/02/26 16:17:42, aandrey wrote: > On 2014/02/26 14:08:41, Alexandra Mikhaylova wrote: > > On 2014/02/25 12:36:07, aandrey wrote: > > > move inside V8PromiseCustom::setState > > > > It seems that we can't move the instrumentation inside V8PromiseCustom as > > there's no our promise (but only internal of this promise) there. > > > > I've instrumented all the places where V8PromiseCustom::setState is called, > > instead. > > make V8PromiseCustom::setState accept a promise Done. https://codereview.chromium.org/177773002/diff/80001/LayoutTests/inspector/re... File LayoutTests/inspector/resources/example-fileset-for-test.js (right): https://codereview.chromium.org/177773002/diff/80001/LayoutTests/inspector/re... LayoutTests/inspector/resources/example-fileset-for-test.js:105: "./Source/core/inspector/InspectorPromiseInstrumentation.z", On 2014/02/26 16:11:30, aandrey wrote: > remove Done. https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:47: #include "core/inspector/InspectorInstrumentation.h" On 2014/02/26 16:11:30, aandrey wrote: > remove Done. https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:394: InspectorInstrumentation::setPromiseStateAndResult(currentExecutionContext(isolate), promise, value, V8PromiseCustom::Fulfilled); On 2014/02/26 16:11:30, aandrey wrote: > move inside setState Done. https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:403: InspectorInstrumentation::setPromiseStateAndResult(currentExecutionContext(isolate), promise, reason, V8PromiseCustom::Rejected); On 2014/02/26 16:11:30, aandrey wrote: > ditto Done. https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:646: InspectorInstrumentation::setPromiseStateAndResult(currentExecutionContext(isolate), promise, v8::Undefined(isolate), Pending); On 2014/02/26 16:11:30, aandrey wrote: > remove Done. https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:649: On 2014/02/26 16:11:30, aandrey wrote: > revert Done. https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:712: InspectorInstrumentation::setPromiseStateAndResult(currentExecutionContext(isolate), promise, valuePromiseFollowing, Following); On 2014/02/26 16:11:30, aandrey wrote: > move inside setState Done. https://codereview.chromium.org/177773002/diff/80001/Source/bindings/v8/custo... Source/bindings/v8/custom/V8PromiseCustom.cpp:721: InspectorInstrumentation::setPromiseStateAndResult(currentExecutionContext(isolate), promise, valuePromise, Following); On 2014/02/26 16:11:30, aandrey wrote: > move inside setState Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... File Source/core/inspector/InspectorDebuggerAgent.cpp (right): https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorDebuggerAgent.cpp:801: if (m_promiseOfficer.isEnabled()) { On 2014/02/26 16:11:30, aandrey wrote: > drop curly braces. also below Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorDebuggerAgent.cpp:1243: m_asyncCallStackTracker.clear(); On 2014/02/26 16:11:30, aandrey wrote: > clear promise officer data here Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorDebuggerAgent.cpp:1282: m_asyncCallStackTracker.clear(); On 2014/02/26 16:11:30, aandrey wrote: > clear promise officer data here Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... File Source/core/inspector/InspectorInstrumentation.idl (right): https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorInstrumentation.idl:547: void addPromise([Keep] ExecutionContext*, v8::Handle<v8::Object> promise, v8::Handle<v8::Object> parentPromise, V8PromiseCustom::PromiseState state = V8PromiseCustom::Pending); On 2014/02/26 16:11:30, aandrey wrote: > addPromise -> didCreatePromise Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorInstrumentation.idl:550: void setPromiseParent([Keep] ExecutionContext*, v8::Handle<v8::Object> promise, v8::Handle<v8::Object> parentPromise); On 2014/02/26 16:11:30, aandrey wrote: > didUpdatePromiseParent Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/In... Source/core/inspector/InspectorInstrumentation.idl:553: void setPromiseStateAndResult([Keep] ExecutionContext*, v8::Handle<v8::Object> promise, v8::Handle<v8::Value> result, V8PromiseCustom::PromiseState state); On 2014/02/26 16:11:30, aandrey wrote: > didUpdatePromiseState Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... File Source/core/inspector/PromiseOfficer.cpp (right): https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:36: void PromiseOfficer::addPromise(ExecutionContext* context, v8::Handle<v8::Object> promise, v8::Handle<v8::Object> parentPromise, V8PromiseCustom::PromiseState state) { On 2014/02/26 16:11:30, aandrey wrote: > style: "{" on new line Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:41: for (size_t index = 0; index < vector.size(); index++) { On 2014/02/26 16:11:30, aandrey wrote: > remove or guard with "#ifndef NDEBUG" Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:48: vector.append(adoptRef(new PromiseData(promise, parentPromise, v8::Undefined(toIsolate(context)), state, context))); On 2014/02/26 16:11:30, aandrey wrote: > we can remove code dup: > > PromiseDataVector* vector; > PromiseDataMap::iterator it = m_promiseDataMap.find(promiseHash); > if (it != m_promiseDataMap.end()) > vector = &m_promiseDataMap.set(promiseHash, > PromiseDataVector()).storedValue->value; > else > vector = &it->value; > vector->append(...); Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:56: ASSERT(it != m_promiseDataMap.end()); // This must be a promise that's already been tracked. On 2014/02/26 16:11:30, aandrey wrote: > we may be activated after a web page was loaded Done, removed ASSERT. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:60: if (data->getPromise() == promise && data->getParentPromise() != parentPromise) { On 2014/02/26 16:11:30, aandrey wrote: > remove: data->getParentPromise() != parentPromise Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:62: vector.append(adoptRef(new PromiseData(promise, parentPromise, data->getResult(), data->getState(), context))); On 2014/02/26 16:11:30, aandrey wrote: > just update the parent Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:71: ASSERT(it != m_promiseDataMap.end()); // This must be a promise that's already been tracked. On 2014/02/26 16:11:30, aandrey wrote: > ditto Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:75: if (data->getPromise() == promise && data->getState() != state) { On 2014/02/26 16:11:30, aandrey wrote: > remove: data->getState() != state Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.cpp:77: vector.append(adoptRef(new PromiseData(promise, data->getParentPromise(), result, state, context))); On 2014/02/26 16:11:30, aandrey wrote: > just update Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... File Source/core/inspector/PromiseOfficer.h (right): https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:49: class PromiseData : public RefCounted<PromiseData> { On 2014/02/26 16:11:30, aandrey wrote: > 1) make it private > 2) why RefCounted? remove it? Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:57: { } On 2014/02/26 16:11:30, aandrey wrote: > style: > { > } Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:59: v8::Handle<v8::Object> getPromise() { On 2014/02/26 16:11:30, aandrey wrote: > style: "{" on the next line. > also fix below Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:81: // TODO: maybe add m_scriptLocation or other label? On 2014/02/26 16:11:30, aandrey wrote: > remove Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:88: bool isEnabled() { On 2014/02/26 16:11:30, aandrey wrote: > bool isEnabled() const { return m_isEnabled; } Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:92: void enable() { On 2014/02/26 16:11:30, aandrey wrote: > move to cpp Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:96: void disable() { On 2014/02/26 16:11:30, aandrey wrote: > move to cpp Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:99: m_promiseDataMap.clear(); On 2014/02/26 14:11:51, Alexandra Mikhaylova wrote: > Maybe disable() and clear() should be two different methods Done. https://codereview.chromium.org/177773002/diff/80001/Source/core/inspector/Pr... Source/core/inspector/PromiseOfficer.h:109: typedef Vector<RefPtr<PromiseData> > PromiseDataVector; On 2014/02/26 16:11:30, aandrey wrote: > Vector<PromiseData> + drop RefCounted Done.
https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.cpp (right): https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:59: double timestamp = currentTimeMS(); Maybe we should move this inside V8PromiseCustom::createPromise and just pass timestamp as an argument here. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:89: double timestamp = currentTimeMS(); Maybe we should do it inside V8PromiseCustom::setState.
https://codereview.chromium.org/177773002/diff/120001/Source/bindings/v8/cust... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/120001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:388: v8::Local<v8::Object> internal = V8PromiseCustom::getInternal(promise); move inside ASSERT: ASSERT(V8PromiseCustom::getState(V8PromiseCustom::getInternal(promise)) != V8PromiseCustom::Fulfilled && V8PromiseCustom::getState(V8PromiseCustom::getInternal(promise)) != V8PromiseCustom::Rejected); https://codereview.chromium.org/177773002/diff/120001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:396: v8::Local<v8::Object> internal = V8PromiseCustom::getInternal(promise); ditto https://codereview.chromium.org/177773002/diff/120001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:472: InspectorInstrumentation::didUpdatePromiseParent(currentExecutionContext(isolate), derivedPromise, promise); move to the end of this func https://codereview.chromium.org/177773002/diff/120001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:647: InspectorInstrumentation::didCreatePromise(currentExecutionContext(isolate), promise, v8::Handle<v8::Object>()); remove dummy v8::Handle<v8::Object>() argument https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/I... File Source/core/inspector/InspectorDebuggerAgent.h (right): https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/I... Source/core/inspector/InspectorDebuggerAgent.h:165: void didCreatePromise(ExecutionContext*, v8::Handle<v8::Object> promise, v8::Handle<v8::Object> parentPromise, V8PromiseCustom::PromiseState state = V8PromiseCustom::Pending); we can remove the "state" argument - a newly created promise is always Pending https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.cpp (right): https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:46: if (m_isEnabled) { remove the check https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:59: double timestamp = currentTimeMS(); On 2014/02/28 14:07:34, Alexandra Mikhaylova wrote: > Maybe we should move this inside V8PromiseCustom::createPromise and just pass > timestamp as an argument here. no, here is best https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:64: if (it != m_promiseDataMap.end()) { remove "{" and "}" https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:70: vector->append(new PromiseData(promise, parentPromise, v8::Undefined(isolate), state, timestamp, isolate)); will vector->append(PromiseData(...)) work? otherwise, where is the corresponding "delete"? https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:79: PromiseData* data = vector.at(index); nit: vector[index] https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:99: if (state == V8PromiseCustom::Fulfilled || state == V8PromiseCustom::Rejected) { remove "{" and "}" https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.h (right): https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:52: : m_isEnabled(false) make all in one line: PromiseOfficer() : m_isEnabled(false) { } https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:62: void didCreatePromise(ExecutionContext* context, v8::Handle<v8::Object> promise, v8::Handle<v8::Object> parentPromise, V8PromiseCustom::PromiseState state); nit: remove "state", leave just V8PromiseCustom::PromiseState in general: don't put arg's names in header if they are obvious https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:64: void didUpdatePromiseState(v8::Handle<v8::Object> promise, V8PromiseCustom::PromiseState state, v8::Handle<v8::Value> result); remove "state" https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:69: PromiseData(v8::Handle<v8::Object> promise, v8::Handle<v8::Object> parentPromise, v8::Handle<v8::Value> result, V8PromiseCustom::PromiseState state, double timestamp, v8::Isolate* isolate) move to cpp https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:82: v8::Handle<v8::Object> getPromise() can you remove all these methods from the header file? just make the PromiseData a struct (all members public) https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:140: typedef Vector<PromiseData*> PromiseDataVector; why Vector<PromiseData*>? I think Vector<PromiseData> is better, and will reduce number of memory allocations
https://codereview.chromium.org/177773002/diff/120001/Source/bindings/v8/cust... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/120001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:388: v8::Local<v8::Object> internal = V8PromiseCustom::getInternal(promise); On 2014/02/28 16:08:13, aandrey wrote: > move inside ASSERT: > > ASSERT(V8PromiseCustom::getState(V8PromiseCustom::getInternal(promise)) != > V8PromiseCustom::Fulfilled && > V8PromiseCustom::getState(V8PromiseCustom::getInternal(promise)) != > V8PromiseCustom::Rejected); Done. https://codereview.chromium.org/177773002/diff/120001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:396: v8::Local<v8::Object> internal = V8PromiseCustom::getInternal(promise); On 2014/02/28 16:08:13, aandrey wrote: > ditto Done. https://codereview.chromium.org/177773002/diff/120001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:472: InspectorInstrumentation::didUpdatePromiseParent(currentExecutionContext(isolate), derivedPromise, promise); On 2014/02/28 16:08:13, aandrey wrote: > move to the end of this func Done. https://codereview.chromium.org/177773002/diff/120001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:647: InspectorInstrumentation::didCreatePromise(currentExecutionContext(isolate), promise, v8::Handle<v8::Object>()); On 2014/02/28 16:08:13, aandrey wrote: > remove dummy v8::Handle<v8::Object>() argument Removed promise parent argument in didCreatePromise as parent is always empty on promise creation and is adjusted later. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/I... File Source/core/inspector/InspectorDebuggerAgent.h (right): https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/I... Source/core/inspector/InspectorDebuggerAgent.h:165: void didCreatePromise(ExecutionContext*, v8::Handle<v8::Object> promise, v8::Handle<v8::Object> parentPromise, V8PromiseCustom::PromiseState state = V8PromiseCustom::Pending); On 2014/02/28 16:08:13, aandrey wrote: > we can remove the "state" argument - a newly created promise is always Pending Removed it also from PromiseOfficer. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.cpp (right): https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:46: if (m_isEnabled) { On 2014/02/28 16:08:13, aandrey wrote: > remove the check Done. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:59: double timestamp = currentTimeMS(); On 2014/02/28 16:08:13, aandrey wrote: > On 2014/02/28 14:07:34, Alexandra Mikhaylova wrote: > > Maybe we should move this inside V8PromiseCustom::createPromise and just pass > > timestamp as an argument here. > > no, here is best Ok, thanks. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:64: if (it != m_promiseDataMap.end()) { On 2014/02/28 16:08:13, aandrey wrote: > remove "{" and "}" Done. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:70: vector->append(new PromiseData(promise, parentPromise, v8::Undefined(isolate), state, timestamp, isolate)); On 2014/02/28 16:08:13, aandrey wrote: > will vector->append(PromiseData(...)) work? > > otherwise, where is the corresponding "delete"? I've returned to RefPtr<PromiseData> to avoid copying PromiseData instances inside Vector::append. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:79: PromiseData* data = vector.at(index); On 2014/02/28 16:08:13, aandrey wrote: > nit: vector[index] Done. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:89: double timestamp = currentTimeMS(); On 2014/02/28 14:07:34, Alexandra Mikhaylova wrote: > Maybe we should do it inside V8PromiseCustom::setState. No, it' ok here. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:99: if (state == V8PromiseCustom::Fulfilled || state == V8PromiseCustom::Rejected) { On 2014/02/28 16:08:13, aandrey wrote: > remove "{" and "}" Done. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.h (right): https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:52: : m_isEnabled(false) On 2014/02/28 16:08:13, aandrey wrote: > make all in one line: > PromiseOfficer() : m_isEnabled(false) { } Done. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:62: void didCreatePromise(ExecutionContext* context, v8::Handle<v8::Object> promise, v8::Handle<v8::Object> parentPromise, V8PromiseCustom::PromiseState state); On 2014/02/28 16:08:13, aandrey wrote: > nit: remove "state", leave just V8PromiseCustom::PromiseState > in general: don't put arg's names in header if they are obvious Done. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:64: void didUpdatePromiseState(v8::Handle<v8::Object> promise, V8PromiseCustom::PromiseState state, v8::Handle<v8::Value> result); On 2014/02/28 16:08:13, aandrey wrote: > remove "state" Done. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:69: PromiseData(v8::Handle<v8::Object> promise, v8::Handle<v8::Object> parentPromise, v8::Handle<v8::Value> result, V8PromiseCustom::PromiseState state, double timestamp, v8::Isolate* isolate) On 2014/02/28 16:08:13, aandrey wrote: > move to cpp Done. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:82: v8::Handle<v8::Object> getPromise() On 2014/02/28 16:08:13, aandrey wrote: > can you remove all these methods from the header file? > just make the PromiseData a struct (all members public) Done. https://codereview.chromium.org/177773002/diff/120001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:140: typedef Vector<PromiseData*> PromiseDataVector; On 2014/02/28 16:08:13, aandrey wrote: > why Vector<PromiseData*>? > I think Vector<PromiseData> is better, and will reduce number of memory > allocations Vector::append copies its argument, so I think it's more applicable to put RefPtr<PromiseData> back.
lgtm, also need an owner's stamp https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.cpp (right): https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:77: double timestamp = currentTimeMS(); nit: add ASSERT(isEnabled()); https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:92: int promiseHash = promise->GetIdentityHash(); nit: add ASSERT(isEnabled()); https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:106: double timestamp = currentTimeMS(); nit: add ASSERT(isEnabled()); https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.h (right): https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:63: struct PromiseData : public RefCounted<PromiseData> { now struct -> class, but leave all members public https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:68: v8::Isolate* isolate; m_isolate https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:70: ScopedPersistent<v8::Object> promise; m_promise same below
https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.cpp (right): https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:77: double timestamp = currentTimeMS(); On 2014/03/04 09:26:28, aandrey wrote: > nit: add ASSERT(isEnabled()); Done. https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:92: int promiseHash = promise->GetIdentityHash(); On 2014/03/04 09:26:28, aandrey wrote: > nit: add ASSERT(isEnabled()); Done. https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:106: double timestamp = currentTimeMS(); On 2014/03/04 09:26:28, aandrey wrote: > nit: add ASSERT(isEnabled()); Done. https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.h (right): https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:63: struct PromiseData : public RefCounted<PromiseData> { On 2014/03/04 09:26:28, aandrey wrote: > now struct -> class, but leave all members public Done. https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:68: v8::Isolate* isolate; On 2014/03/04 09:26:28, aandrey wrote: > m_isolate Done. https://codereview.chromium.org/177773002/diff/170001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:70: ScopedPersistent<v8::Object> promise; On 2014/03/04 09:26:28, aandrey wrote: > m_promise > > same below Done.
https://codereview.chromium.org/177773002/diff/190001/Source/bindings/v8/cust... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/190001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:388: ASSERT(V8PromiseCustom::getState(V8PromiseCustom::getInternal(promise)) != V8PromiseCustom::Fulfilled && V8PromiseCustom::getState(V8PromiseCustom::getInternal(promise)) != V8PromiseCustom::Rejected); Internal is still used twice so it would make sense to leave the variable. https://codereview.chromium.org/177773002/diff/190001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:395: ASSERT(V8PromiseCustom::getState(V8PromiseCustom::getInternal(promise)) != V8PromiseCustom::Fulfilled && V8PromiseCustom::getState(V8PromiseCustom::getInternal(promise)) != V8PromiseCustom::Rejected); ditto https://codereview.chromium.org/177773002/diff/190001/Source/core/inspector/I... File Source/core/inspector/InspectorInstrumentation.idl (right): https://codereview.chromium.org/177773002/diff/190001/Source/core/inspector/I... Source/core/inspector/InspectorInstrumentation.idl:533: void didCreatePromise([Keep] ExecutionContext*, v8::Handle<v8::Object> promise); We don't usually expose v8::* types outside of bindings layer, they can be wrapped into Script* objects instead if you need them here. https://codereview.chromium.org/177773002/diff/190001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.cpp (right): https://codereview.chromium.org/177773002/diff/190001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:49: , m_callStackOnSettle(RefPtr<ScriptCallStack>()) compiler will generate this automatically https://codereview.chromium.org/177773002/diff/190001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:83: PromiseDataMap::iterator it = m_promiseDataMap.find(promiseHash); Why not use a HashMap (with appropriate HashTraits) from promise to PromiseData instead of this ad hoc implementation of a hash map?
https://codereview.chromium.org/177773002/diff/190001/Source/bindings/v8/cust... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/190001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:388: ASSERT(V8PromiseCustom::getState(V8PromiseCustom::getInternal(promise)) != V8PromiseCustom::Fulfilled && V8PromiseCustom::getState(V8PromiseCustom::getInternal(promise)) != V8PromiseCustom::Rejected); On 2014/03/05 15:23:35, yurys wrote: > Internal is still used twice so it would make sense to leave the variable. Then there would be problems with the release build as internal would be considered an unused variable. https://codereview.chromium.org/177773002/diff/190001/Source/core/inspector/I... File Source/core/inspector/InspectorInstrumentation.idl (right): https://codereview.chromium.org/177773002/diff/190001/Source/core/inspector/I... Source/core/inspector/InspectorInstrumentation.idl:533: void didCreatePromise([Keep] ExecutionContext*, v8::Handle<v8::Object> promise); On 2014/03/05 15:23:35, yurys wrote: > We don't usually expose v8::* types outside of bindings layer, they can be > wrapped into Script* objects instead if you need them here. Done. https://codereview.chromium.org/177773002/diff/190001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.cpp (right): https://codereview.chromium.org/177773002/diff/190001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:49: , m_callStackOnSettle(RefPtr<ScriptCallStack>()) On 2014/03/05 15:23:35, yurys wrote: > compiler will generate this automatically Done. https://codereview.chromium.org/177773002/diff/190001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:83: PromiseDataMap::iterator it = m_promiseDataMap.find(promiseHash); On 2014/03/05 15:23:35, yurys wrote: > Why not use a HashMap (with appropriate HashTraits) from promise to PromiseData > instead of this ad hoc implementation of a hash map? Thanks, fixed it.
looks good https://codereview.chromium.org/177773002/diff/210001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.h (right): https://codereview.chromium.org/177773002/diff/210001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:81: struct ScriptObjectHash { plz move these to a separate file bindings/v8/ScriptObjectTraits.h https://codereview.chromium.org/177773002/diff/210001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:91: static bool isDeletedValue(ScriptObject value) { return value == ScriptObject(); } { return value.hasNoValue(); }
yhirano, please take a look at V8PromiseCustom
https://codereview.chromium.org/177773002/diff/210001/Source/bindings/v8/cust... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/210001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:478: if (InspectorInstrumentation::isPromiseOfficerEnabled(context)) { isPromiseInstrumentationEnabled https://codereview.chromium.org/177773002/diff/210001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.cpp (right): https://codereview.chromium.org/177773002/diff/210001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:53: m_callStackOnSettle = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true); Can we merge this with call stack tracking in AsyncCallStackTracker ? https://codereview.chromium.org/177773002/diff/210001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.h (right): https://codereview.chromium.org/177773002/diff/210001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:62: class PromiseData : public RefCounted<PromiseData> { Forward declaration of PromiseData should be enough in the header.
https://codereview.chromium.org/177773002/diff/210001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.cpp (right): https://codereview.chromium.org/177773002/diff/210001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.cpp:53: m_callStackOnSettle = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true); On 2014/03/12 10:56:53, yurys wrote: > Can we merge this with call stack tracking in AsyncCallStackTracker ? I don't think we can. The purposes are very different: different call stacks (debugger vs console) as well as different points where we capture the call stacks (triggering an async event vs updating the state of an object/promise). https://codereview.chromium.org/177773002/diff/210001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.h (right): https://codereview.chromium.org/177773002/diff/210001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:62: class PromiseData : public RefCounted<PromiseData> { On 2014/03/12 10:56:53, yurys wrote: > Forward declaration of PromiseData should be enough in the header. I doubt.
https://codereview.chromium.org/177773002/diff/210001/Source/bindings/v8/cust... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/210001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:478: if (InspectorInstrumentation::isPromiseOfficerEnabled(context)) { On 2014/03/12 10:56:53, yurys wrote: > isPromiseInstrumentationEnabled Done. https://codereview.chromium.org/177773002/diff/210001/Source/core/inspector/P... File Source/core/inspector/PromiseOfficer.h (right): https://codereview.chromium.org/177773002/diff/210001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:81: struct ScriptObjectHash { On 2014/03/11 10:43:38, aandrey wrote: > plz move these to a separate file bindings/v8/ScriptObjectTraits.h Done. https://codereview.chromium.org/177773002/diff/210001/Source/core/inspector/P... Source/core/inspector/PromiseOfficer.h:91: static bool isDeletedValue(ScriptObject value) { return value == ScriptObject(); } On 2014/03/11 10:43:38, aandrey wrote: > { return value.hasNoValue(); } Done.
lgtm
https://codereview.chromium.org/177773002/diff/210001/Source/bindings/v8/cust... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/210001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:478: if (InspectorInstrumentation::isPromiseOfficerEnabled(context)) { On 2014/03/12 10:56:53, yurys wrote: > isPromiseInstrumentationEnabled The async stacks instrumentation should always be called. It will be completely wrong to wrap those calls into this if- check. And with the general name like "isPromiseInstrumentationEnabled" it will be easy to break.
On 2014/03/12 12:16:32, aandrey wrote: > https://codereview.chromium.org/177773002/diff/210001/Source/bindings/v8/cust... > File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): > > https://codereview.chromium.org/177773002/diff/210001/Source/bindings/v8/cust... > Source/bindings/v8/custom/V8PromiseCustom.cpp:478: if > (InspectorInstrumentation::isPromiseOfficerEnabled(context)) { > On 2014/03/12 10:56:53, yurys wrote: > > isPromiseInstrumentationEnabled > > The async stacks instrumentation should always be called. It will be completely > wrong to wrap those calls into this if- check. And with the general name like > "isPromiseInstrumentationEnabled" it will be easy to break. not lgtm
On 2014/03/12 12:16:47, aandrey wrote: > On 2014/03/12 12:16:32, aandrey wrote: > > > https://codereview.chromium.org/177773002/diff/210001/Source/bindings/v8/cust... > > File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): > > > > > https://codereview.chromium.org/177773002/diff/210001/Source/bindings/v8/cust... > > Source/bindings/v8/custom/V8PromiseCustom.cpp:478: if > > (InspectorInstrumentation::isPromiseOfficerEnabled(context)) { > > On 2014/03/12 10:56:53, yurys wrote: > > > isPromiseInstrumentationEnabled > > > > The async stacks instrumentation should always be called. It will be > completely > > wrong to wrap those calls into this if- check. And with the general name like > > "isPromiseInstrumentationEnabled" it will be easy to break. > > not lgtm Ok, would it be better to rename it back to isPromiseOfficerEnabled?
On 2014/03/12 12:18:12, Alexandra Mikhaylova wrote: > On 2014/03/12 12:16:47, aandrey wrote: > > On 2014/03/12 12:16:32, aandrey wrote: > > > > > > https://codereview.chromium.org/177773002/diff/210001/Source/bindings/v8/cust... > > > File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): > > > > > > > > > https://codereview.chromium.org/177773002/diff/210001/Source/bindings/v8/cust... > > > Source/bindings/v8/custom/V8PromiseCustom.cpp:478: if > > > (InspectorInstrumentation::isPromiseOfficerEnabled(context)) { > > > On 2014/03/12 10:56:53, yurys wrote: > > > > isPromiseInstrumentationEnabled > > > > > > The async stacks instrumentation should always be called. It will be > > completely > > > wrong to wrap those calls into this if- check. And with the general name > like > > > "isPromiseInstrumentationEnabled" it will be easy to break. > > > > not lgtm > > Ok, would it be better to rename it back to isPromiseOfficerEnabled? Maybe PromiseOfficer -> PromiseTracker and isPromiseOfficerEnabled -> isPromiseTrackerEnabled ?
On 2014/03/12 12:21:40, aandrey wrote: > On 2014/03/12 12:18:12, Alexandra Mikhaylova wrote: > > On 2014/03/12 12:16:47, aandrey wrote: > > > On 2014/03/12 12:16:32, aandrey wrote: > > > > > > > > > > https://codereview.chromium.org/177773002/diff/210001/Source/bindings/v8/cust... > > > > File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/177773002/diff/210001/Source/bindings/v8/cust... > > > > Source/bindings/v8/custom/V8PromiseCustom.cpp:478: if > > > > (InspectorInstrumentation::isPromiseOfficerEnabled(context)) { > > > > On 2014/03/12 10:56:53, yurys wrote: > > > > > isPromiseInstrumentationEnabled > > > > > > > > The async stacks instrumentation should always be called. It will be > > > completely > > > > wrong to wrap those calls into this if- check. And with the general name > > like > > > > "isPromiseInstrumentationEnabled" it will be easy to break. > > > > > > not lgtm > > > > Ok, would it be better to rename it back to isPromiseOfficerEnabled? > > Maybe PromiseOfficer -> PromiseTracker > and isPromiseOfficerEnabled -> isPromiseTrackerEnabled > ? Ok, done.
lgtm
https://codereview.chromium.org/177773002/diff/250001/Source/bindings/v8/cust... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/250001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:671: void V8PromiseCustom::setState(v8::Handle<v8::Object> promise, PromiseState state, v8::Handle<v8::Value> value, v8::Isolate* isolate) I don't like the subtle difference of the parameter between setState and getState. Moreover, if you change the "public" API, please change the description in V8PromiseCustom.h as well. https://codereview.chromium.org/177773002/diff/250001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:671: void V8PromiseCustom::setState(v8::Handle<v8::Object> promise, PromiseState state, v8::Handle<v8::Value> value, v8::Isolate* isolate) Is it possible to delete V8PromiseCustom::setState and add (anon namespace)::setStateForPromise instead? It is helpful to comment that it takes a Promise contrary to getState.
https://codereview.chromium.org/177773002/diff/250001/Source/bindings/v8/cust... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/177773002/diff/250001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8PromiseCustom.cpp:671: void V8PromiseCustom::setState(v8::Handle<v8::Object> promise, PromiseState state, v8::Handle<v8::Value> value, v8::Isolate* isolate) On 2014/03/13 01:43:00, yhirano wrote: > Is it possible to delete V8PromiseCustom::setState and add (anon > namespace)::setStateForPromise instead? > It is helpful to comment that it takes a Promise contrary to getState. Thanks, fixed it.
On 2014/03/13 01:42:59, yhirano wrote: > https://codereview.chromium.org/177773002/diff/250001/Source/bindings/v8/cust... > File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): > > https://codereview.chromium.org/177773002/diff/250001/Source/bindings/v8/cust... > Source/bindings/v8/custom/V8PromiseCustom.cpp:671: void > V8PromiseCustom::setState(v8::Handle<v8::Object> promise, PromiseState state, > v8::Handle<v8::Value> value, v8::Isolate* isolate) > I don't like the subtle difference of the parameter between setState and > getState. > Moreover, if you change the "public" API, please change the description in > V8PromiseCustom.h as well. > > https://codereview.chromium.org/177773002/diff/250001/Source/bindings/v8/cust... > Source/bindings/v8/custom/V8PromiseCustom.cpp:671: void > V8PromiseCustom::setState(v8::Handle<v8::Object> promise, PromiseState state, > v8::Handle<v8::Value> value, v8::Isolate* isolate) > Is it possible to delete V8PromiseCustom::setState and add (anon > namespace)::setStateForPromise instead? > It is helpful to comment that it takes a Promise contrary to getState. yhirano, can you please take a look if it's ok now?
lgtm
FYI: we are switching the Promise implementation from this to the v8 Promises.
On 2014/03/14 12:10:41, yhirano wrote: > FYI: we are switching the Promise implementation from this to the v8 Promises. is there a bug tracker for this?
On 2014/03/14 12:17:04, aandrey wrote: > On 2014/03/14 12:10:41, yhirano wrote: > > FYI: we are switching the Promise implementation from this to the v8 Promises. > > is there a bug tracker for this? Oh, there is no master bug: I will create it and cc you.
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/177773002/270001
Message was sent while issue was closed.
Change committed as 169247 |
