Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(331)

Issue 460018: DevTools: make possible profiling of scripts doing heavy calculations. (Closed)

Created:
11 years ago by mnaganov (inactive)
Modified:
9 years, 7 months ago
Reviewers:
pfeldman
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

DevTools: make possible profiling of scripts doing heavy calculations. Start / stop profiling commands are now sent using debugger protocol, effectively breaking into script execution. Getting profiler log and active modules is executed on IO thread. BUG=28689 TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=34040

Patch Set 1 #

Total comments: 4

Patch Set 2 : Reimplemented reusing RPC #

Total comments: 10

Patch Set 3 : Comments addressed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+519 lines, -276 lines) Patch
M chrome/renderer/devtools_agent.cc View 2 2 chunks +16 lines, -0 lines 0 comments Download
M chrome/renderer/devtools_agent_filter.h View 1 1 chunk +20 lines, -2 lines 0 comments Download
M chrome/renderer/devtools_agent_filter.cc View 1 2 4 chunks +41 lines, -4 lines 0 comments Download
M webkit/glue/devtools/debugger_agent.h View 1 2 chunks +2 lines, -20 lines 0 comments Download
M webkit/glue/devtools/debugger_agent_impl.h View 1 2 chunks +0 lines, -9 lines 0 comments Download
M webkit/glue/devtools/debugger_agent_impl.cc View 1 2 chunks +0 lines, -26 lines 0 comments Download
M webkit/glue/devtools/js/debugger_agent.js View 1 6 chunks +0 lines, -154 lines 0 comments Download
M webkit/glue/devtools/js/devtools.js View 4 chunks +11 lines, -1 line 0 comments Download
M webkit/glue/devtools/js/devtools_host_stub.js View 1 5 chunks +87 lines, -50 lines 0 comments Download
M webkit/glue/devtools/js/inspector_controller_impl.js View 3 chunks +8 lines, -8 lines 0 comments Download
A webkit/glue/devtools/js/profiler_agent.js View 2 1 chunk +191 lines, -0 lines 0 comments Download
M webkit/glue/devtools/js/tests.js View 1 chunk +2 lines, -2 lines 0 comments Download
A webkit/glue/devtools/profiler_agent.h View 1 chunk +31 lines, -0 lines 0 comments Download
A webkit/glue/devtools/profiler_agent_impl.h View 2 1 chunk +35 lines, -0 lines 0 comments Download
A webkit/glue/devtools/profiler_agent_impl.cc View 1 chunk +23 lines, -0 lines 0 comments Download
M webkit/glue/webdevtoolsagent_impl.cc View 2 3 chunks +41 lines, -0 lines 0 comments Download
M webkit/glue/webdevtoolsfrontend_impl.h View 2 chunks +2 lines, -0 lines 0 comments Download
M webkit/glue/webdevtoolsfrontend_impl.cc View 3 chunks +5 lines, -0 lines 0 comments Download
M webkit/webkit.gyp View 2 chunks +4 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
mnaganov (inactive)
11 years ago (2009-12-02 16:12:18 UTC) #1
pfeldman
http://codereview.chromium.org/460018/diff/1/3 File chrome/renderer/devtools_agent_filter.cc (right): http://codereview.chromium.org/460018/diff/1/3#newcode66 chrome/renderer/devtools_agent_filter.cc:66: const int read_size = v8::V8::GetLogLines( no talking to v8 ...
11 years ago (2009-12-02 16:46:52 UTC) #2
mnaganov (inactive)
http://codereview.chromium.org/460018/diff/1/3 File chrome/renderer/devtools_agent_filter.cc (right): http://codereview.chromium.org/460018/diff/1/3#newcode66 chrome/renderer/devtools_agent_filter.cc:66: const int read_size = v8::V8::GetLogLines( On 2009/12/02 16:46:52, pfeldman ...
11 years ago (2009-12-07 10:01:37 UTC) #3
mnaganov (inactive)
This is a corresponding change in WebKit: diff --git a/WebKit/chromium/public/WebDevToolsAgent.h b/WebKit/chromium/public/WebDevToolsAgent.h index 327d445..f9decd4 100644 --- ...
11 years ago (2009-12-07 10:05:13 UTC) #4
pfeldman
LGTM with comments. http://codereview.chromium.org/460018/diff/4001/4003 File chrome/renderer/devtools_agent_filter.cc (right): http://codereview.chromium.org/460018/diff/4001/4003#newcode70 chrome/renderer/devtools_agent_filter.cc:70: message_handled_ = WebDevToolsAgent::lightweightDispatchMessageFromFrontend( dispatchOnIOThread http://codereview.chromium.org/460018/diff/4001/4004 File ...
11 years ago (2009-12-07 10:39:44 UTC) #5
mnaganov (inactive)
11 years ago (2009-12-07 13:11:41 UTC) #6
http://codereview.chromium.org/460018/diff/4001/4003
File chrome/renderer/devtools_agent_filter.cc (right):

http://codereview.chromium.org/460018/diff/4001/4003#newcode70
chrome/renderer/devtools_agent_filter.cc:70: message_handled_ =
WebDevToolsAgent::lightweightDispatchMessageFromFrontend(
On 2009/12/07 10:39:44, pfeldman wrote:
> dispatchOnIOThread

dispatchMessageFromFrontendOnIOThread

http://codereview.chromium.org/460018/diff/4001/4004
File chrome/renderer/devtools_agent_filter.h (right):

http://codereview.chromium.org/460018/diff/4001/4004#newcode53
chrome/renderer/devtools_agent_filter.h:53: static int current_routing_id_;
On 2009/12/07 10:39:44, pfeldman wrote:
> You could do something like RenderThread::current()->devtools_filter() instead
> of making members static.

Unfortunately, current() is implemented using thread local storage, so it's only
accessible inside renderer thread.

http://codereview.chromium.org/460018/diff/4001/4015
File webkit/glue/devtools/profiler_agent_impl.h (right):

http://codereview.chromium.org/460018/diff/4001/4015#newcode14
webkit/glue/devtools/profiler_agent_impl.h:14: class ProfilerAgentImpl : public
ProfilerAgent {
On 2009/12/07 10:39:44, pfeldman wrote:
> Please mention that these methods can be called on IO thread.

Done.

http://codereview.chromium.org/460018/diff/4001/4015#newcode26
webkit/glue/devtools/profiler_agent_impl.h:26: ProfilerAgentDelegate* delegate_;
On 2009/12/07 10:39:44, pfeldman wrote:
> DISALLOW?

Done.

http://codereview.chromium.org/460018/diff/4001/4016
File webkit/glue/webdevtoolsagent_impl.cc (right):

http://codereview.chromium.org/460018/diff/4001/4016#newcode608
webkit/glue/webdevtoolsagent_impl.cc:608: class IORPCDelegate : public
DevToolsRpc::Delegate {
On 2009/12/07 10:39:44, pfeldman wrote:
> Chromium style: 
> 1) Declare on top
> 2) Use anonymous namespace
> 3) Use camel case: IoRpcDelegate
> 4) Disallow

Done.

Powered by Google App Engine
This is Rietveld 408576698