|
|
Created:
6 years, 3 months ago by Alexandra Mikhaylova Modified:
6 years, 3 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
Description[WIP] Protocol for sending information about Promises to frontend.
BUG=None
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181646
Patch Set 1 #
Total comments: 32
Patch Set 2 : Address review comments + add a simple test #
Total comments: 32
Patch Set 3 : Address comments #
Total comments: 2
Patch Set 4 : Address comments #
Total comments: 4
Patch Set 5 : Address comments + REBASE #
Messages
Total messages: 16 (4 generated)
amikhaylova@google.com changed reviewers: + aandrey@chromium.org
https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Inspec... File Source/core/inspector/InspectorDebuggerAgent.cpp (right): https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorDebuggerAgent.cpp:167: m_state->setBoolean(DebuggerAgentState::promiseTrackerEnabled, false); move 1 line above https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorDebuggerAgent.cpp:232: m_promiseTracker.enable(); setEnabled()? https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:39: extra line https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:116: m_lastPromiseId = 0; remove https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:119: PassRefPtr<PromiseTracker::PromiseData> PromiseTracker::findOrAddPromiseData(v8::Isolate* isolate, v8::Handle<v8::Object> promise) findOrAddPromiseData -> createPromiseDataIfNeeded https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:132: data = adoptRef(new PromiseData(isolate, promiseHash, ++m_lastPromiseId, promise)); use this pattern: int AsyncCallStackTracker::ExecutionContextData::circularSequentialID() { ++m_circularSequentialID; if (m_circularSequentialID <= 0) m_circularSequentialID = 1; return m_circularSequentialID; } https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:181: .release(); use ScriptCallFrame::buildInspectorObject() instead https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:186: .setCallFrame(callFrame) this should be optional, added only if we captured the stack. instead of ScriptCallFrame m_callFrame let's use RefPtr<ScriptCallStack> m_callStack; https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:187: .release(); remove release() call https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:188: result->addItem(promiseDetails); doesn't this crash? result is nullptr here https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... File Source/core/inspector/PromiseTracker.h (right): https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.h:33: PassRefPtr<TypeBuilder::Array<TypeBuilder::Debugger::PromiseDetails> > getPromises(); getPromises -> promises https://codereview.chromium.org/529723002/diff/1/Source/devtools/protocol.json File Source/devtools/protocol.json (right): https://codereview.chromium.org/529723002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:3134: "description": "Information about a tracked promise.", "a tracked" -> "the" https://codereview.chromium.org/529723002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:3137: { "name": "parentId", "type": "number", "description": "Id of the parent promise." }, optional https://codereview.chromium.org/529723002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:3138: { "name": "status", "type": "number", "description": "Status of the promise." }, say more in description about possible values https://codereview.chromium.org/529723002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:3139: { "name": "callFrame", "$ref": "Console.CallFrame", "description": "Top call frame on promise creation."} optional https://codereview.chromium.org/529723002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:3440: "description": "Pulls information about stored promises." "Returns detailed information about all <code>Promise</code>s that were created or updated after the <code>enablePromiseTracker</code> command, and were not yet garbage collected."
https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Inspec... File Source/core/inspector/InspectorDebuggerAgent.cpp (right): https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorDebuggerAgent.cpp:167: m_state->setBoolean(DebuggerAgentState::promiseTrackerEnabled, false); On 2014/09/01 13:39:27, aandrey wrote: > move 1 line above Done. https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorDebuggerAgent.cpp:232: m_promiseTracker.enable(); On 2014/09/01 13:39:27, aandrey wrote: > setEnabled()? Done, should we also change InspectorDebuggerAgent api? https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:39: On 2014/09/01 13:39:27, aandrey wrote: > extra line Fixed. https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:116: m_lastPromiseId = 0; On 2014/09/01 13:39:27, aandrey wrote: > remove I think we shouldn't remove it as we're using integer and its "default" value is not 0 in fact. https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:119: PassRefPtr<PromiseTracker::PromiseData> PromiseTracker::findOrAddPromiseData(v8::Isolate* isolate, v8::Handle<v8::Object> promise) On 2014/09/01 13:39:27, aandrey wrote: > findOrAddPromiseData -> createPromiseDataIfNeeded Done. https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:132: data = adoptRef(new PromiseData(isolate, promiseHash, ++m_lastPromiseId, promise)); On 2014/09/01 13:39:27, aandrey wrote: > use this pattern: > > int AsyncCallStackTracker::ExecutionContextData::circularSequentialID() > { > ++m_circularSequentialID; > if (m_circularSequentialID <= 0) > m_circularSequentialID = 1; > return m_circularSequentialID; > } Done, also renamed m_lastPromiseId to m_circularSequentialId to ease search. https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:181: .release(); On 2014/09/01 13:39:27, aandrey wrote: > use ScriptCallFrame::buildInspectorObject() instead Thanks! Done. https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:186: .setCallFrame(callFrame) On 2014/09/01 13:39:27, aandrey wrote: > this should be optional, added only if we captured the stack. > instead of ScriptCallFrame m_callFrame let's use RefPtr<ScriptCallStack> > m_callStack; Done. https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:187: .release(); On 2014/09/01 13:39:27, aandrey wrote: > remove release() call Done. https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.cpp:188: result->addItem(promiseDetails); On 2014/09/01 13:39:27, aandrey wrote: > doesn't this crash? result is nullptr here Thanks, fixed it. https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... File Source/core/inspector/PromiseTracker.h (right): https://codereview.chromium.org/529723002/diff/1/Source/core/inspector/Promis... Source/core/inspector/PromiseTracker.h:33: PassRefPtr<TypeBuilder::Array<TypeBuilder::Debugger::PromiseDetails> > getPromises(); On 2014/09/01 13:39:27, aandrey wrote: > getPromises -> promises Done. https://codereview.chromium.org/529723002/diff/1/Source/devtools/protocol.json File Source/devtools/protocol.json (right): https://codereview.chromium.org/529723002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:3134: "description": "Information about a tracked promise.", On 2014/09/01 13:39:27, aandrey wrote: > "a tracked" -> "the" Done. https://codereview.chromium.org/529723002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:3137: { "name": "parentId", "type": "number", "description": "Id of the parent promise." }, On 2014/09/01 13:39:27, aandrey wrote: > optional Done. https://codereview.chromium.org/529723002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:3138: { "name": "status", "type": "number", "description": "Status of the promise." }, On 2014/09/01 13:39:27, aandrey wrote: > say more in description about possible values Done. https://codereview.chromium.org/529723002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:3139: { "name": "callFrame", "$ref": "Console.CallFrame", "description": "Top call frame on promise creation."} On 2014/09/01 13:39:27, aandrey wrote: > optional Done. https://codereview.chromium.org/529723002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:3440: "description": "Pulls information about stored promises." On 2014/09/01 13:39:27, aandrey wrote: > "Returns detailed information about all <code>Promise</code>s that were created > or updated after the <code>enablePromiseTracker</code> command, and were not yet > garbage collected." Done.
https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... File LayoutTests/inspector/sources/debugger/promise-tracker-expected.txt (right): https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/promise-tracker-expected.txt:9: testFunction :9 why no URL? should be: testFunction (promise-tracker.html:9) https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... File LayoutTests/inspector/sources/debugger/promise-tracker.html (right): https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/promise-tracker.html:43: function didGetPromises(error, response) { new line before { https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/promise-tracker.html:58: "\n id: " + promise.id + should be (promise.id - minPromiseId) to avoid test flakiness https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/promise-tracker.html:60: "\n parent id: " + promise.parentId; ditto https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/promise-tracker.html:63: promiseInfo = promiseInfo + promiseInfo += ... https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/In... File Source/core/inspector/InspectorDebuggerAgent.cpp (right): https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDebuggerAgent.cpp:1173: m_promiseTracker.setEnabled(); setEnabled(true) https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:41: RefPtr<ScriptCallStack> m_callStack; move after m_weakPtrFactory https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:43: ScopedPersistent<v8::Object> m_parentPromise; ditto https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:45: extra line https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:111: m_circularSequentialId = 0; remove. let's not reset the ID counter upon disable command. https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:159: if (!status) { if (!status && !data->m_callStack) https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:182: if (data->m_callStack.get()) drop ".get()" https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.h (right): https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:26: void setEnabled(bool = true); drop "= true" https://codereview.chromium.org/529723002/diff/20001/Source/devtools/protocol... File Source/devtools/protocol.json (right): https://codereview.chromium.org/529723002/diff/20001/Source/devtools/protocol... Source/devtools/protocol.json:3437: { "name": "promises", "type": "array", "items": { "$ref": "PromiseDetails" }, "description": "Returns detailed information about all <code>Promise</code>s that were created or updated after the <code>enablePromiseTracker</code> command, and were not yet garbage collected." } this should be a description for "getPromises" command itself, not the "promises" result
sergeyv@chromium.org changed reviewers: + sergeyv@chromium.org
https://codereview.chromium.org/529723002/diff/20001/Source/devtools/protocol... File Source/devtools/protocol.json (right): https://codereview.chromium.org/529723002/diff/20001/Source/devtools/protocol... Source/devtools/protocol.json:3137: { "name": "status", "type": "integer", "description": "Status of the promise. Equal to <code>1</code> for resolved, <code>-1</code> for rejected and <code>0</code> for pending ones." }, Since you have very limited range of possible values, don't you want use enum type for this property? See Scope type for example
https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... File LayoutTests/inspector/sources/debugger/promise-tracker-expected.txt (right): https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/promise-tracker-expected.txt:9: testFunction :9 On 2014/09/02 14:34:48, aandrey wrote: > why no URL? > should be: testFunction (promise-tracker.html:9) It's empty on server side for some reason. https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... File LayoutTests/inspector/sources/debugger/promise-tracker.html (right): https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/promise-tracker.html:43: function didGetPromises(error, response) { On 2014/09/02 14:34:49, aandrey wrote: > new line before { Done. https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/promise-tracker.html:58: "\n id: " + promise.id + On 2014/09/02 14:34:49, aandrey wrote: > should be (promise.id - minPromiseId) to avoid test flakiness Done. https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/promise-tracker.html:60: "\n parent id: " + promise.parentId; On 2014/09/02 14:34:49, aandrey wrote: > ditto Done. https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/promise-tracker.html:63: promiseInfo = promiseInfo + On 2014/09/02 14:34:49, aandrey wrote: > promiseInfo += ... Done. https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/In... File Source/core/inspector/InspectorDebuggerAgent.cpp (right): https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/In... Source/core/inspector/InspectorDebuggerAgent.cpp:1173: m_promiseTracker.setEnabled(); On 2014/09/02 14:34:49, aandrey wrote: > setEnabled(true) Done. https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:41: RefPtr<ScriptCallStack> m_callStack; On 2014/09/02 14:34:49, aandrey wrote: > move after m_weakPtrFactory Done. https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:43: ScopedPersistent<v8::Object> m_parentPromise; On 2014/09/02 14:34:49, aandrey wrote: > ditto Done. https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:45: On 2014/09/02 14:34:49, aandrey wrote: > extra line Done. https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:111: m_circularSequentialId = 0; On 2014/09/02 14:34:49, aandrey wrote: > remove. let's not reset the ID counter upon disable command. Done. https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:159: if (!status) { On 2014/09/02 14:34:49, aandrey wrote: > if (!status && !data->m_callStack) Done. https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.cpp:182: if (data->m_callStack.get()) On 2014/09/02 14:34:49, aandrey wrote: > drop ".get()" Done. https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... File Source/core/inspector/PromiseTracker.h (right): https://codereview.chromium.org/529723002/diff/20001/Source/core/inspector/Pr... Source/core/inspector/PromiseTracker.h:26: void setEnabled(bool = true); On 2014/09/02 14:34:49, aandrey wrote: > drop "= true" Done. https://codereview.chromium.org/529723002/diff/20001/Source/devtools/protocol... File Source/devtools/protocol.json (right): https://codereview.chromium.org/529723002/diff/20001/Source/devtools/protocol... Source/devtools/protocol.json:3137: { "name": "status", "type": "integer", "description": "Status of the promise. Equal to <code>1</code> for resolved, <code>-1</code> for rejected and <code>0</code> for pending ones." }, On 2014/09/02 15:25:36, sergeyv wrote: > Since you have very limited range of possible values, don't you want use enum > type for this property? See Scope type for example Thanks, done so. https://codereview.chromium.org/529723002/diff/20001/Source/devtools/protocol... Source/devtools/protocol.json:3437: { "name": "promises", "type": "array", "items": { "$ref": "PromiseDetails" }, "description": "Returns detailed information about all <code>Promise</code>s that were created or updated after the <code>enablePromiseTracker</code> command, and were not yet garbage collected." } On 2014/09/02 14:34:49, aandrey wrote: > this should be a description for "getPromises" command itself, not the > "promises" result Oops. Thanks, fixed this.
aandrey@chromium.org changed reviewers: + yurys@chromium.org
lgtm https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... File LayoutTests/inspector/sources/debugger/promise-tracker.html (right): https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/promise-tracker.html:43: function didGetPromises(error, response) { On 2014/09/03 14:28:54, Alexandra Mikhaylova wrote: > On 2014/09/02 14:34:49, aandrey wrote: > > new line before { > > Done. *before* {, not after https://codereview.chromium.org/529723002/diff/40001/LayoutTests/inspector/so... File LayoutTests/inspector/sources/debugger/promise-tracker.html (right): https://codereview.chromium.org/529723002/diff/40001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/promise-tracker.html:58: for (var i = 0; i < response.length; i++) { drop { and }
https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... File LayoutTests/inspector/sources/debugger/promise-tracker.html (right): https://codereview.chromium.org/529723002/diff/20001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/promise-tracker.html:43: function didGetPromises(error, response) { On 2014/09/03 14:57:57, aandrey wrote: > On 2014/09/03 14:28:54, Alexandra Mikhaylova wrote: > > On 2014/09/02 14:34:49, aandrey wrote: > > > new line before { > > > > Done. > > *before* {, not after Done. https://codereview.chromium.org/529723002/diff/40001/LayoutTests/inspector/so... File LayoutTests/inspector/sources/debugger/promise-tracker.html (right): https://codereview.chromium.org/529723002/diff/40001/LayoutTests/inspector/so... LayoutTests/inspector/sources/debugger/promise-tracker.html:58: for (var i = 0; i < response.length; i++) { On 2014/09/03 14:57:57, aandrey wrote: > drop { and } Done.
lgtm https://codereview.chromium.org/529723002/diff/60001/Source/devtools/protocol... File Source/devtools/protocol.json (right): https://codereview.chromium.org/529723002/diff/60001/Source/devtools/protocol... Source/devtools/protocol.json:3427: "description": "Enables promise tracking." Please add a description of what will change when this command is handled. https://codereview.chromium.org/529723002/diff/60001/Source/devtools/protocol... Source/devtools/protocol.json:3440: "description": "Returns detailed information about all <code>Promise</code>s that were created or updated after the <code>enablePromiseTracker</code> command, and were not yet garbage collected." ...and have not been garbage collected yet.
https://codereview.chromium.org/529723002/diff/60001/Source/devtools/protocol... File Source/devtools/protocol.json (right): https://codereview.chromium.org/529723002/diff/60001/Source/devtools/protocol... Source/devtools/protocol.json:3427: "description": "Enables promise tracking." On 2014/09/08 11:52:52, yurys wrote: > Please add a description of what will change when this command is handled. Done. https://codereview.chromium.org/529723002/diff/60001/Source/devtools/protocol... Source/devtools/protocol.json:3440: "description": "Returns detailed information about all <code>Promise</code>s that were created or updated after the <code>enablePromiseTracker</code> command, and were not yet garbage collected." On 2014/09/08 11:52:52, yurys wrote: > ...and have not been garbage collected yet. Done.
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/529723002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 181646 |