Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2318853002:
[DevTools] Handle navigation in console.log (Closed)
|
Created:
4 years, 3 months ago by kozy Modified:
4 years, 2 months ago Reviewers:
dgozman CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Handle navigation in console.log
BUG=642496
R=dgozman@chromium.org
Committed: https://crrev.com/37faa2db833127ac9b6d461d8bc49952e557c743
Committed: https://crrev.com/41b43609f912087b49e6d5c0950a5f3cde5c232f
Cr-Original-Commit-Position: refs/heads/master@{#417958}
Cr-Commit-Position: refs/heads/master@{#422802}
Patch Set 1 #
Total comments: 8
Patch Set 2 : addressed comments #
Total comments: 6
Patch Set 3 : addressed comments #
Messages
Total messages: 25 (13 generated)
Dmitry, please take a look!
https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-console-log-wont-crash.html (right): https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-console-log-wont-crash.html:34: </body> This tests how navigation is handled from inside debugger code (console.log). https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/V8ConsoleAgentImpl.cpp (right): https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/V8ConsoleAgentImpl.cpp:79: if (!m_session->inspector()->hasConsoleMessageStorage(m_session->contextGroupId())) if (!reportMessage(...)) return; https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/V8ConsoleMessage.cpp (right): https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/V8ConsoleMessage.cpp:426: if (session) remove if https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/V8RuntimeAgentImpl.cpp (right): https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/V8RuntimeAgentImpl.cpp:621: reportMessage(message.get(), false); if (!reportMessage(...)) return;
all done, please take another look. https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-console-log-wont-crash.html (right): https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-console-log-wont-crash.html:34: </body> On 2016/09/07 00:21:03, dgozman wrote: > This tests how navigation is handled from inside debugger code (console.log). Done. https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/V8ConsoleAgentImpl.cpp (right): https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/V8ConsoleAgentImpl.cpp:79: if (!m_session->inspector()->hasConsoleMessageStorage(m_session->contextGroupId())) On 2016/09/07 00:21:03, dgozman wrote: > if (!reportMessage(...)) > return; Done. https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/V8ConsoleMessage.cpp (right): https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/V8ConsoleMessage.cpp:426: if (session) On 2016/09/07 00:21:03, dgozman wrote: > remove if Done. https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/v8_inspector/V8RuntimeAgentImpl.cpp (right): https://codereview.chromium.org/2318853002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/v8_inspector/V8RuntimeAgentImpl.cpp:621: reportMessage(message.get(), false); On 2016/09/07 00:21:03, dgozman wrote: > if (!reportMessage(...)) > return; Done.
lgtm https://codereview.chromium.org/2318853002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-console-log-handle-navigate-expected.txt (right): https://codereview.chromium.org/2318853002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-console-log-handle-navigate-expected.txt:11: id : 2 Do not dump ids. https://codereview.chromium.org/2318853002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-console-log-handle-navigate.html (right): https://codereview.chromium.org/2318853002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-console-log-handle-navigate.html:29: .then(() => InspectorTest.completeTest()); Just InspectorTest.completeTest? https://codereview.chromium.org/2318853002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-console-log-handle-navigate.html:38: .then((r) => InspectorTest.logObject(r)); Just InspectorTest.logObject?
Description was changed from ========== [DevTools] Improve console.log formatting BUG=642496 R=dgozman@chromium.org ========== to ========== [DevTools] Handle navigation in console.log BUG=642496 R=dgozman@chromium.org ==========
all done. removed V8 related part. https://codereview.chromium.org/2318853002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-console-log-handle-navigate-expected.txt (right): https://codereview.chromium.org/2318853002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-console-log-handle-navigate-expected.txt:11: id : 2 On 2016/09/07 23:52:14, dgozman wrote: > Do not dump ids. Done. https://codereview.chromium.org/2318853002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-console-log-handle-navigate.html (right): https://codereview.chromium.org/2318853002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-console-log-handle-navigate.html:29: .then(() => InspectorTest.completeTest()); On 2016/09/07 23:52:14, dgozman wrote: > Just InspectorTest.completeTest? I'd like to be pretty sure about this and use the alignment as much as possible. https://codereview.chromium.org/2318853002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-console-log-handle-navigate.html:38: .then((r) => InspectorTest.logObject(r)); On 2016/09/07 23:52:14, dgozman wrote: > Just InspectorTest.logObject? ditto.
The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2318853002/#ps40001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Handle navigation in console.log BUG=642496 R=dgozman@chromium.org ========== to ========== [DevTools] Handle navigation in console.log BUG=642496 R=dgozman@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Handle navigation in console.log BUG=642496 R=dgozman@chromium.org ========== to ========== [DevTools] Handle navigation in console.log BUG=642496 R=dgozman@chromium.org Committed: https://crrev.com/37faa2db833127ac9b6d461d8bc49952e557c743 Cr-Commit-Position: refs/heads/master@{#417958} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/37faa2db833127ac9b6d461d8bc49952e557c743 Cr-Commit-Position: refs/heads/master@{#417958}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2332203002/ by kozyatinskiy@chromium.org. The reason for reverting is: Produce leak on Asan try bot..
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Handle navigation in console.log BUG=642496 R=dgozman@chromium.org Committed: https://crrev.com/37faa2db833127ac9b6d461d8bc49952e557c743 Cr-Commit-Position: refs/heads/master@{#417958} ========== to ========== [DevTools] Handle navigation in console.log BUG=642496 R=dgozman@chromium.org Committed: https://crrev.com/37faa2db833127ac9b6d461d8bc49952e557c743 Cr-Commit-Position: refs/heads/master@{#417958} ==========
The CQ bit was checked by kozyatinskiy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Handle navigation in console.log BUG=642496 R=dgozman@chromium.org Committed: https://crrev.com/37faa2db833127ac9b6d461d8bc49952e557c743 Cr-Commit-Position: refs/heads/master@{#417958} ========== to ========== [DevTools] Handle navigation in console.log BUG=642496 R=dgozman@chromium.org Committed: https://crrev.com/37faa2db833127ac9b6d461d8bc49952e557c743 Cr-Commit-Position: refs/heads/master@{#417958} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Handle navigation in console.log BUG=642496 R=dgozman@chromium.org Committed: https://crrev.com/37faa2db833127ac9b6d461d8bc49952e557c743 Cr-Commit-Position: refs/heads/master@{#417958} ========== to ========== [DevTools] Handle navigation in console.log BUG=642496 R=dgozman@chromium.org Committed: https://crrev.com/37faa2db833127ac9b6d461d8bc49952e557c743 Committed: https://crrev.com/41b43609f912087b49e6d5c0950a5f3cde5c232f Cr-Original-Commit-Position: refs/heads/master@{#417958} Cr-Commit-Position: refs/heads/master@{#422802} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/41b43609f912087b49e6d5c0950a5f3cde5c232f Cr-Commit-Position: refs/heads/master@{#422802} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
