Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2368393003:
[inspector] added inspector test runner [part 2] (Closed)
Description[inspector] added inspector test runner [part 2]
- added the channel implementation,
- added inspector implementation,
- added v8::Extension for communication between backend and frontend.
BUG=chromium:635948
R=dgozman@chromium.org,alph@chromium.org
Committed: https://crrev.com/cceaa1225c6a96a28d2c7410d1db520423fb8c31
Committed: https://crrev.com/751814a1282396aef80b48f7f41ea38eab5e1456
Cr-Original-Commit-Position: refs/heads/master@{#39888}
Cr-Commit-Position: refs/heads/master@{#39928}
Patch Set 1 #Patch Set 2 : convert utf16 into utf8 in SendMessageToFrontend #Patch Set 3 : added missing handle scope #Patch Set 4 : added missing microtask scope #
Total comments: 12
Patch Set 5 : addressed comments #
Total comments: 4
Patch Set 6 : addressed comments #Patch Set 7 : move more InspectorClientImpl functions to private #
Total comments: 4
Patch Set 8 : std::string > String16 #Patch Set 9 : StringView -> String16 in SendMessageToBackendTask cstor #Patch Set 10 : rebased #Patch Set 11 : rebased #Patch Set 12 : fixed linter errors #
Total comments: 6
Patch Set 13 : addressed comments #
Messages
Total messages: 37 (20 generated)
Dmitry, please take a look.
https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... File test/inspector-protocol/inspector-impl.cc (right): https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... test/inspector-protocol/inspector-impl.cc:114: void BackendExtension::EvaluateInFrontend( Let's remove this. https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... test/inspector-protocol/inspector-impl.cc:143: v8::MicrotasksScope microtasks_scope(isolate, Let's remove this. Why is it needed? https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... File test/inspector-protocol/inspector-impl.h (right): https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... test/inspector-protocol/inspector-impl.h:15: class Semaphore; Include it. https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... test/inspector-protocol/inspector-impl.h:19: class TaskRunner; Remove. https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... test/inspector-protocol/inspector-impl.h:23: ChannelImpl(TaskRunner* frontend_task_runner) explicit https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... test/inspector-protocol/inspector-impl.h:77: bool is_protocol_task() { return true; } is_inspector_task
Patchset #5 (id:80001) has been deleted
All done. Please take a look! https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... File test/inspector-protocol/inspector-impl.cc (right): https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... test/inspector-protocol/inspector-impl.cc:114: void BackendExtension::EvaluateInFrontend( On 2016/09/27 16:30:36, dgozman wrote: > Let's remove this. Done. https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... test/inspector-protocol/inspector-impl.cc:143: v8::MicrotasksScope microtasks_scope(isolate, On 2016/09/27 16:30:36, dgozman wrote: > Let's remove this. Why is it needed? Done. https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... File test/inspector-protocol/inspector-impl.h (right): https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... test/inspector-protocol/inspector-impl.h:15: class Semaphore; On 2016/09/27 16:30:36, dgozman wrote: > Include it. Done. https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... test/inspector-protocol/inspector-impl.h:19: class TaskRunner; On 2016/09/27 16:30:36, dgozman wrote: > Remove. I need this for ChannelImpl constructor. https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... test/inspector-protocol/inspector-impl.h:23: ChannelImpl(TaskRunner* frontend_task_runner) On 2016/09/27 16:30:36, dgozman wrote: > explicit Done. https://codereview.chromium.org/2368393003/diff/60001/test/inspector-protocol... test/inspector-protocol/inspector-impl.h:77: bool is_protocol_task() { return true; } On 2016/09/27 16:30:36, dgozman wrote: > is_inspector_task Changed in https://codereview.chromium.org/2361623006/
https://codereview.chromium.org/2368393003/diff/100001/test/inspector-protoco... File test/inspector-protocol/DEPS (right): https://codereview.chromium.org/2368393003/diff/100001/test/inspector-protoco... test/inspector-protocol/DEPS:7: "+src/vector.h", Unused? https://codereview.chromium.org/2368393003/diff/100001/test/inspector-protoco... File test/inspector-protocol/inspector-impl.h (right): https://codereview.chromium.org/2368393003/diff/100001/test/inspector-protoco... test/inspector-protocol/inspector-impl.h:66: class ConnectTask : public TaskRunner::Task { We can remove this and ChannelImpl from header and do InspectorClientImpl(task_runner, frontend_task_runner, semaphore)
All done. Please take another look! https://codereview.chromium.org/2368393003/diff/100001/test/inspector-protoco... File test/inspector-protocol/DEPS (right): https://codereview.chromium.org/2368393003/diff/100001/test/inspector-protoco... test/inspector-protocol/DEPS:7: "+src/vector.h", On 2016/09/27 18:41:20, dgozman wrote: > Unused? Done. https://codereview.chromium.org/2368393003/diff/100001/test/inspector-protoco... File test/inspector-protocol/inspector-impl.h (right): https://codereview.chromium.org/2368393003/diff/100001/test/inspector-protoco... test/inspector-protocol/inspector-impl.h:66: class ConnectTask : public TaskRunner::Task { On 2016/09/27 18:41:20, dgozman wrote: > We can remove this and ChannelImpl from header and do > > InspectorClientImpl(task_runner, frontend_task_runner, semaphore) Done.
Description was changed from ========== [inspector] added inspector protocol test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:645640 R=dgozman@chromium.org,alph@chromium.org ========== to ========== [inspector] added inspector protocol test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:645640 R=dgozman@chromium.org,alph@chromium.org ==========
kozyatinskiy@chromium.org changed reviewers: + machenbach@chromium.org
lgtm https://codereview.chromium.org/2368393003/diff/140001/test/inspector-protoco... File test/inspector-protocol/inspector-impl.cc (right): https://codereview.chromium.org/2368393003/diff/140001/test/inspector-protoco... test/inspector-protocol/inspector-impl.cc:128: std::string message_; String16? https://codereview.chromium.org/2368393003/diff/140001/test/inspector-protoco... File test/inspector-protocol/inspector-impl.h (right): https://codereview.chromium.org/2368393003/diff/140001/test/inspector-protoco... test/inspector-protocol/inspector-impl.h:56: class FrontendExtension : public v8::Extension { SendMessageToBackendExtension
All done. https://codereview.chromium.org/2368393003/diff/140001/test/inspector-protoco... File test/inspector-protocol/inspector-impl.cc (right): https://codereview.chromium.org/2368393003/diff/140001/test/inspector-protoco... test/inspector-protocol/inspector-impl.cc:128: std::string message_; On 2016/09/28 16:10:57, dgozman wrote: > String16? Done. https://codereview.chromium.org/2368393003/diff/140001/test/inspector-protoco... File test/inspector-protocol/inspector-impl.h (right): https://codereview.chromium.org/2368393003/diff/140001/test/inspector-protoco... test/inspector-protocol/inspector-impl.h:56: class FrontendExtension : public v8::Extension { On 2016/09/28 16:10:58, dgozman wrote: > SendMessageToBackendExtension Done.
Description was changed from ========== [inspector] added inspector protocol test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:645640 R=dgozman@chromium.org,alph@chromium.org ========== to ========== [inspector] added inspector protocol test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org ==========
Description was changed from ========== [inspector] added inspector protocol test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org ========== to ========== [inspector] added inspector test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org ==========
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...
Alexey, please take a look!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ nits https://codereview.chromium.org/2368393003/diff/240001/test/inspector/inspect... File test/inspector/inspector-impl.h (right): https://codereview.chromium.org/2368393003/diff/240001/test/inspector/inspect... test/inspector/inspector-impl.h:18: virtual ~FrontendChannel() {} = default; https://codereview.chromium.org/2368393003/diff/240001/test/inspector/inspect... test/inspector/inspector-impl.h:30: v8::Local<v8::Context> ensureDefaultContextInGroup(int) override; arg name https://codereview.chromium.org/2368393003/diff/240001/test/inspector/inspect... test/inspector/inspector-impl.h:32: void runMessageLoopOnPause(int) override; ditto
Patchset #13 (id:260001) has been deleted
All done. thanks! https://codereview.chromium.org/2368393003/diff/240001/test/inspector/inspect... File test/inspector/inspector-impl.h (right): https://codereview.chromium.org/2368393003/diff/240001/test/inspector/inspect... test/inspector/inspector-impl.h:18: virtual ~FrontendChannel() {} On 2016/09/29 20:05:52, alph wrote: > = default; Done. https://codereview.chromium.org/2368393003/diff/240001/test/inspector/inspect... test/inspector/inspector-impl.h:30: v8::Local<v8::Context> ensureDefaultContextInGroup(int) override; On 2016/09/29 20:05:52, alph wrote: > arg name Done. https://codereview.chromium.org/2368393003/diff/240001/test/inspector/inspect... test/inspector/inspector-impl.h:32: void runMessageLoopOnPause(int) override; On 2016/09/29 20:05:52, alph wrote: > ditto Done.
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, alph@chromium.org Link to the patchset: https://codereview.chromium.org/2368393003/#ps280001 (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 ========== [inspector] added inspector test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org ========== to ========== [inspector] added inspector test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #13 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [inspector] added inspector test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org ========== to ========== [inspector] added inspector test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/cceaa1225c6a96a28d2c7410d1db520423fb8c31 Cr-Commit-Position: refs/heads/master@{#39888} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/cceaa1225c6a96a28d2c7410d1db520423fb8c31 Cr-Commit-Position: refs/heads/master@{#39888}
Message was sent while issue was closed.
Description was changed from ========== [inspector] added inspector test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/cceaa1225c6a96a28d2c7410d1db520423fb8c31 Cr-Commit-Position: refs/heads/master@{#39888} ========== to ========== [inspector] added inspector test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/cceaa1225c6a96a28d2c7410d1db520423fb8c31 Cr-Commit-Position: refs/heads/master@{#39888} ==========
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...
The CQ bit was unchecked by kozyatinskiy@chromium.org
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 ========== [inspector] added inspector test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/cceaa1225c6a96a28d2c7410d1db520423fb8c31 Cr-Commit-Position: refs/heads/master@{#39888} ========== to ========== [inspector] added inspector test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/cceaa1225c6a96a28d2c7410d1db520423fb8c31 Cr-Commit-Position: refs/heads/master@{#39888} ==========
Message was sent while issue was closed.
Committed patchset #13 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [inspector] added inspector test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/cceaa1225c6a96a28d2c7410d1db520423fb8c31 Cr-Commit-Position: refs/heads/master@{#39888} ========== to ========== [inspector] added inspector test runner [part 2] - added the channel implementation, - added inspector implementation, - added v8::Extension for communication between backend and frontend. BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/cceaa1225c6a96a28d2c7410d1db520423fb8c31 Committed: https://crrev.com/751814a1282396aef80b48f7f41ea38eab5e1456 Cr-Original-Commit-Position: refs/heads/master@{#39888} Cr-Commit-Position: refs/heads/master@{#39928} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/751814a1282396aef80b48f7f41ea38eab5e1456 Cr-Commit-Position: refs/heads/master@{#39928} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
