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

Issue 11548032: Telemetry / Devtools TraceHandler: exposes tracing via dev tools. (Closed)

Created:
8 years ago by bulach
Modified:
7 years, 10 months ago
CC:
chromium-reviews, chrome-speed-team+watch_google.com, vsevik, jam, yurys, joi+watch-content_chromium.org, darin-cc_chromium.org, pam+watch_chromium.org, telemetry+watch_chromium.org
Visibility:
Public.

Description

Telemetry / Devtools TraceHandler: exposes tracing via dev tools. - exposes start / stop / get request methods via devtools. This allows Telemetry to integrate trace collection across all platforms in a consistent way. BUG= TEST=./tools/telemetry/run_tests --browser=android-content-shell testGotTrace Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173595

Patch Set 1 #

Total comments: 1

Patch Set 2 : WIP! DONT SUBMIT. #

Total comments: 24

Patch Set 3 : Pavel comment #

Total comments: 39

Patch Set 4 : More comments #

Patch Set 5 : More comments #

Total comments: 19

Patch Set 6 : Error handling and comments #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+534 lines, -57 lines) Patch
A content/browser/debugger/devtools_browser_target.h View 1 2 3 4 5 1 chunk +68 lines, -0 lines 0 comments Download
A content/browser/debugger/devtools_browser_target.cc View 1 2 3 4 5 1 chunk +118 lines, -0 lines 0 comments Download
M content/browser/debugger/devtools_http_handler_impl.h View 1 2 3 chunks +3 lines, -1 line 0 comments Download
M content/browser/debugger/devtools_http_handler_impl.cc View 1 2 3 4 5 4 chunks +33 lines, -3 lines 0 comments Download
A content/browser/debugger/devtools_tracing_handler.h View 1 2 3 4 5 1 chunk +50 lines, -0 lines 0 comments Download
A content/browser/debugger/devtools_tracing_handler.cc View 1 2 3 4 5 1 chunk +103 lines, -0 lines 0 comments Download
M content/content_browser.gypi View 1 2 3 2 chunks +4 lines, -0 lines 0 comments Download
M tools/telemetry/telemetry/browser.py View 1 2 3 1 chunk +13 lines, -0 lines 0 comments Download
M tools/telemetry/telemetry/browser_backend.py View 1 2 3 4 chunks +20 lines, -0 lines 0 comments Download
M tools/telemetry/telemetry/page_runner.py View 1 2 3 3 chunks +26 lines, -53 lines 0 comments Download
A tools/telemetry/telemetry/tracing_backend.py View 1 2 3 1 chunk +63 lines, -0 lines 0 comments Download
A tools/telemetry/telemetry/tracing_backend_unittest.py View 1 2 1 chunk +33 lines, -0 lines 1 comment Download

Messages

Total messages: 21 (0 generated)
bulach
following email discussions, ptal
8 years ago (2012-12-12 17:37:12 UTC) #1
pfeldman
https://codereview.chromium.org/11548032/diff/1/content/browser/debugger/devtools_http_handler_impl.cc File content/browser/debugger/devtools_http_handler_impl.cc (right): https://codereview.chromium.org/11548032/diff/1/content/browser/debugger/devtools_http_handler_impl.cc#newcode597 content/browser/debugger/devtools_http_handler_impl.cc:597: size_t pos = info.path.find(prefix); This discovery handler is supposed ...
8 years ago (2012-12-12 17:45:22 UTC) #2
pfeldman
I know you want this fast and you don't want to implement alternate transport for ...
8 years ago (2012-12-12 18:16:41 UTC) #3
pfeldman
Here is the way I see it: - Change ::OnWebSocketRequestUI to do something like the ...
8 years ago (2012-12-12 18:34:29 UTC) #4
bulach
thanks pavel, insightful comments. the latest patchset is nowhere near finished, however would like to ...
8 years ago (2012-12-12 19:35:32 UTC) #5
pfeldman
Overall looks good. A couple of nits inline. https://codereview.chromium.org/11548032/diff/7001/content/browser/debugger/devtools_browser_target.cc File content/browser/debugger/devtools_browser_target.cc (right): https://codereview.chromium.org/11548032/diff/7001/content/browser/debugger/devtools_browser_target.cc#newcode16 content/browser/debugger/devtools_browser_target.cc:16: int ...
8 years ago (2012-12-12 20:17:11 UTC) #6
bulach
thanks pavel! I think I got my head around "target", "domain" and "method", thanks for ...
8 years ago (2012-12-13 17:36:18 UTC) #7
pfeldman
Overall, looks good. Review comments below. https://codereview.chromium.org/11548032/diff/18001/content/browser/debugger/devtools_browser_target.cc File content/browser/debugger/devtools_browser_target.cc (right): https://codereview.chromium.org/11548032/diff/18001/content/browser/debugger/devtools_browser_target.cc#newcode34 content/browser/debugger/devtools_browser_target.cc:34: if (command && ...
8 years ago (2012-12-14 10:39:33 UTC) #8
bulach
thanks pavel! all comments addressed, another look please? https://codereview.chromium.org/11548032/diff/18001/content/browser/debugger/devtools_browser_target.cc File content/browser/debugger/devtools_browser_target.cc (right): https://codereview.chromium.org/11548032/diff/18001/content/browser/debugger/devtools_browser_target.cc#newcode34 content/browser/debugger/devtools_browser_target.cc:34: if ...
8 years ago (2012-12-14 16:03:51 UTC) #9
pfeldman
devtools lgtm just to not slow you down. Please fix as many nits as possible, ...
8 years ago (2012-12-14 18:35:05 UTC) #10
bulach
thanks pavel! all comments addressed. I'm really sorry but won't be able to follow up ...
8 years ago (2012-12-14 19:53:39 UTC) #11
pfeldman
devtools lgtm https://codereview.chromium.org/11548032/diff/20014/content/browser/debugger/devtools_http_handler_impl.cc File content/browser/debugger/devtools_http_handler_impl.cc (right): https://codereview.chromium.org/11548032/diff/20014/content/browser/debugger/devtools_http_handler_impl.cc#newcode601 content/browser/debugger/devtools_http_handler_impl.cc:601: browser_target_->RegisterHandler("Tracing", new DevToolsTracingHandler()); Yes, domain(), sorry
8 years ago (2012-12-14 20:15:14 UTC) #12
nduca
lgtm.
8 years ago (2012-12-14 20:38:33 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/11548032/25003
8 years ago (2012-12-17 11:41:59 UTC) #14
commit-bot: I haz the power
Presubmit check for 11548032-25003 failed and returned exit status 1. Running presubmit commit checks ...
8 years ago (2012-12-17 11:42:10 UTC) #15
Tom Hudson
Antoine, we need content OWNERS signoff for the change to content_browser.gypi; can you provide or ...
8 years ago (2012-12-17 15:21:51 UTC) #16
piman
lgtm
8 years ago (2012-12-17 17:14:58 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/11548032/25003
8 years ago (2012-12-17 17:16:41 UTC) #18
commit-bot: I haz the power
Change committed as 173595
8 years ago (2012-12-18 00:32:12 UTC) #19
marja
A post-commit question about this CL: https://chromiumcodereview.appspot.com/11548032/diff/25003/tools/telemetry/telemetry/tracing_backend_unittest.py File tools/telemetry/telemetry/tracing_backend_unittest.py (right): https://chromiumcodereview.appspot.com/11548032/diff/25003/tools/telemetry/telemetry/tracing_backend_unittest.py#newcode27 tools/telemetry/telemetry/tracing_backend_unittest.py:27: self._browser.http_server.UrlOf('image.png') Btw, why ...
7 years, 10 months ago (2013-01-31 16:13:39 UTC) #20
Sami
7 years, 10 months ago (2013-01-31 16:55:46 UTC) #21
Message was sent while issue was closed.
> Btw, why this UrlOf line? (Asks nduca in another code review.)

Looks like a copy & paste error to me. It might've come from
InspectorTimelineTabTest.testGotTimeline where we used to decode an image and
checked that it showed up on the timeline.

I'll clean it up.

Powered by Google App Engine
This is Rietveld 408576698