Remove TraceController

Remove TraceController

TraceController is obsoleted by TracingController.
Changed all remaining clients to use TracingController.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237280

[Xianzhu, 2013-11-15 23:02:49]

Hi, could you review the following files in this CL:

dsinclair@: base/debug/trace*, content/browser/tracing/trac*
nduca@: components/tracing/*
pfeldman@: content/browser/devtools
piman@: Other content/ files
rkc@: chrome/browser/feedbacks/*
jam@: Other chrome/ files
cdn@: chrome/common/automation_messages_internal.h,
components/tracing/tracing_messages.h

This CL is now depending on the following pending CLs:
https://codereview.chromium.org/66193005/,
https://codereview.chromium.org/66893003/,
https://codereview.chromium.org/65343006/ and
https://codereview.chromium.org/24355002/.

[rkc, 2013-11-15 23:13:49]

Rubberstamp LGTM for Feedback/*
Still need zork@'s review since all the changes are tracing related.

[Zachary Kuznia, 2013-11-15 23:21:36]

https://codereview.chromium.org/67683003/diff/30001/chrome/browser/feedback/f...
File chrome/browser/feedback/feedback_util.h (right):

https://codereview.chromium.org/67683003/diff/30001/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_util.h:41: bool ZipFile(const base::FilePath&
filename, std::string* compressed_data);
You should put a comment noting that the file will be deleted after the data is
compressed.

https://codereview.chromium.org/67683003/diff/30001/chrome/browser/feedback/t...
File chrome/browser/feedback/tracing_manager.cc (right):

https://codereview.chromium.org/67683003/diff/30001/chrome/browser/feedback/t...
chrome/browser/feedback/tracing_manager.cc:104: base::DeleteFile(path, false);
Is there a reason you delete the file both here and in ZipFile?

[nduca, 2013-11-15 23:23:51]

lgtm

[Xianzhu, 2013-11-15 23:25:32]

https://codereview.chromium.org/67683003/diff/30001/chrome/browser/feedback/f...
File chrome/browser/feedback/feedback_util.h (right):

https://codereview.chromium.org/67683003/diff/30001/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_util.h:41: bool ZipFile(const base::FilePath&
filename, std::string* compressed_data);
On 2013/11/15 23:21:36, Zachary Kuznia wrote:
> You should put a comment noting that the file will be deleted after the data
is
> compressed.

ZipFile deletes only the temporary zip file which is created by the function and
not exposed by the function, not the input file.

https://codereview.chromium.org/67683003/diff/30001/chrome/browser/feedback/t...
File chrome/browser/feedback/tracing_manager.cc (right):

https://codereview.chromium.org/67683003/diff/30001/chrome/browser/feedback/t...
chrome/browser/feedback/tracing_manager.cc:104: base::DeleteFile(path, false);
On 2013/11/15 23:21:36, Zachary Kuznia wrote:
> Is there a reason you delete the file both here and in ZipFile?

ZipFile deletes the temporary zip file, and here is deleted the temporary trace
file.

[Zachary Kuznia, 2013-11-15 23:27:26]

Ah, I see now.  LGTM

[piman, 2013-11-15 23:42:51]

https://codereview.chromium.org/67683003/diff/30001/content/browser/devtools/...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/67683003/diff/30001/content/browser/devtools/...
content/browser/devtools/devtools_tracing_handler.cc:57: base::Unretained(this),
path));
What makes Unretained here safe?

https://codereview.chromium.org/67683003/diff/30001/content/browser/devtools/...
content/browser/devtools/devtools_tracing_handler.cc:92:
base::Unretained(this)));
Same here, what makes Unretained safe?

https://codereview.chromium.org/67683003/diff/30001/content/browser/devtools/...
content/browser/devtools/devtools_tracing_handler.cc:102:
base::Unretained(this)));
And here.

https://codereview.chromium.org/67683003/diff/30001/content/browser/tracing/t...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/67683003/diff/30001/content/browser/tracing/t...
content/browser/tracing/tracing_controller_impl.cc:615:
base::Unretained(this)));
What makes Unretained here safe?

[Xianzhu, 2013-11-16 01:16:13]

https://codereview.chromium.org/67683003/diff/30001/content/browser/devtools/...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/67683003/diff/30001/content/browser/devtools/...
content/browser/devtools/devtools_tracing_handler.cc:57: base::Unretained(this),
path));
On 2013/11/15 23:42:52, piman wrote:
> What makes Unretained here safe?

Used this because I saw existing Unretained(this) and thought it were safe.
Actually it's not. The existing Unretaineds in the constructor are safe because
the callbacks won't outlive this.

Changed to use weak_ptr.

https://codereview.chromium.org/67683003/diff/30001/content/browser/devtools/...
content/browser/devtools/devtools_tracing_handler.cc:92:
base::Unretained(this)));
On 2013/11/15 23:42:52, piman wrote:
> Same here, what makes Unretained safe?

Done.

https://codereview.chromium.org/67683003/diff/30001/content/browser/devtools/...
content/browser/devtools/devtools_tracing_handler.cc:102:
base::Unretained(this)));
On 2013/11/15 23:42:52, piman wrote:
> And here.

Done.

https://codereview.chromium.org/67683003/diff/30001/content/browser/tracing/t...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/67683003/diff/30001/content/browser/tracing/t...
content/browser/tracing/tracing_controller_impl.cc:615:
base::Unretained(this)));
On 2013/11/15 23:42:52, piman wrote:
> What makes Unretained here safe?

TracingControllerImpl instance is a leaky singleton which outlives the callback.

[pfeldman, 2013-11-16 13:11:20]

devtools not lgtm. You are breaking semantics of the Tracing.* domain of the
remote debugging protocol. You should still issue Tracing.tracingComplete and
support old signature of Tracing.start. Adding new events in fine.

https://codereview.chromium.org/67683003/diff/160001/content/browser/devtools...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/67683003/diff/160001/content/browser/devtools...
content/browser/devtools/devtools_tracing_handler.cc:74: if
(!TracingControllerImpl::ParseTracingParams(command->params(),
You should declare complete scheme of this command in
devtools_protocol_constants. Parsing of all of its parameters must be here. Note
that this same handler serves no the renderer target, so you need to update its
scheme upstream in protocol.json.

[nduca, 2013-11-16 22:19:20]

Oh good catch pavel. @wangxianzhu, lets make the devtools protocol and any other
parts of the system completely unchanged in behavior in this patch. If we want
to make individual systems slightly better (more efficient) we can do it as a
followup perhaps?

[jam, 2013-11-18 16:57:20]

On 2013/11/15 23:02:49, Xianzhu wrote:
> Hi, could you review the following files in this CL:
> 
> rkc@: chrome/browser/feedbacks/*
> jam@: Other chrome/ files

lgtm

https://codereview.chromium.org/67683003/diff/160001/chrome/test/base/tracing.cc
File chrome/test/base/tracing.cc (right):

https://codereview.chromium.org/67683003/diff/160001/chrome/test/base/tracing...
chrome/test/base/tracing.cc:101:
base::ThreadRestrictions::SetIOAllowed(old_io_allowed);
nit: use base::ThreadRestrictions::ScopedAllowIO

[Xianzhu, 2013-11-19 18:28:13]

pfeldman@ PTAL. Now keep the original devtools protocol.

[piman, 2013-11-19 20:28:39]

LGTM for content/

[pfeldman, 2013-11-20 00:10:09]

https://codereview.chromium.org/67683003/diff/230001/content/browser/devtools...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/67683003/diff/230001/content/browser/devtools...
content/browser/devtools/devtools_tracing_handler.cc:45: void
DevToolsTracingHandler::ReadRecordingResult(const base::FilePath& path) {
Please make sure order of methods in cc matches the one in the header.

https://codereview.chromium.org/67683003/diff/230001/content/browser/devtools...
content/browser/devtools/devtools_tracing_handler.cc:50: if
(!base::ReadFileToString(path, &trace_data)) {
Why do we need to deal with files in the handler?

https://codereview.chromium.org/67683003/diff/230001/content/browser/devtools...
content/browser/devtools/devtools_tracing_handler.cc:80: is_running_ = false;
Do we still need is_running_?

https://codereview.chromium.org/67683003/diff/230001/content/browser/devtools...
content/browser/devtools/devtools_tracing_handler.cc:113: ret &=
~TracingController::RECORD_CONTINUOUSLY;
Is this change intentional?

[Xianzhu, 2013-11-20 00:37:21]

https://codereview.chromium.org/67683003/diff/230001/content/browser/devtools...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/67683003/diff/230001/content/browser/devtools...
content/browser/devtools/devtools_tracing_handler.cc:45: void
DevToolsTracingHandler::ReadRecordingResult(const base::FilePath& path) {
On 2013/11/20 00:10:10, pfeldman wrote:
> Please make sure order of methods in cc matches the one in the header.

Done.

https://codereview.chromium.org/67683003/diff/230001/content/browser/devtools...
content/browser/devtools/devtools_tracing_handler.cc:50: if
(!base::ReadFileToString(path, &trace_data)) {
On 2013/11/20 00:10:10, pfeldman wrote:
> Why do we need to deal with files in the handler?

This is how the new TracingController interface is defined. I didn't contribute
discussion when defining the interface, but I think the purpose is to simplify
most of the clients (single callback, avoid passing big data through IPC, etc.)

Here the code becomes more complex because we need to conform to the original
devtools protocol. My next step work for
https://code.google.com/p/trace-viewer/issues/detail?id=414) will use and change
the protocol and simplify code here.

BTW, is there already existing client of the protocol about tracing?

https://codereview.chromium.org/67683003/diff/230001/content/browser/devtools...
content/browser/devtools/devtools_tracing_handler.cc:80: is_running_ = false;
On 2013/11/20 00:10:10, pfeldman wrote:
> Do we still need is_running_?

Seems useless (even in the original code). Removed.

https://codereview.chromium.org/67683003/diff/230001/content/browser/devtools...
content/browser/devtools/devtools_tracing_handler.cc:113: ret &=
~TracingController::RECORD_CONTINUOUSLY;
On 2013/11/20 00:10:10, pfeldman wrote:
> Is this change intentional?

Yes. TracingController::Options doesn't have RECORD_UNTIL_FULL, but imply it by
default. Here just ensures the RECORD_CONTINUOUSLY bit is cleared if
kRecordUntilFull is specified (in case kRecordContinuously is specified
eariler). Can also be a no-op.

Will change the code here when changing the devtools tracing protocol.

[pfeldman, 2013-11-20 00:50:34]

lgtm

[commit-bot: I haz the power, 2013-11-21 19:09:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/67683003/720001

[Xianzhu, 2013-11-21 19:20:02]

+cdn@ for messages.

[Xianzhu, 2013-11-22 17:01:26]

+jln (for components/tracing/tracing_messages.h and
chrome/common/automation_messages_internal.h)

[Xianzhu, 2013-11-22 19:30:16]

+tsepez for *messages*.h

[Tom Sepez, 2013-11-22 19:47:48]

On 2013/11/22 19:30:16, Xianzhu wrote:
> +tsepez for *messages*.h

Where are you generating the file paths that you are passing around? Which
processes get to specify these?

[Xianzhu, 2013-11-22 21:19:55]

On 2013/11/22 19:47:48, Tom Sepez wrote:
> On 2013/11/22 19:30:16, Xianzhu wrote:
> > +tsepez for *messages*.h
> 
> Where are you generating the file paths that you are passing around? Which
> processes get to specify these?

This is because of the new TracingController interface. The automation client
sends the affected IPC message (AutomationMsg_EndTracing) to the browser process
and the file path is part of the result. This is for testing only.

[Tom Sepez, 2013-11-22 21:23:16]

messages LGTM. Thanks.

[commit-bot: I haz the power, 2013-11-22 21:33:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/67683003/720001

[commit-bot: I haz the power, 2013-11-22 22:08:30]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-11-22 23:22:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/67683003/1090001

[commit-bot: I haz the power, 2013-11-23 01:03:24]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-23 01:13:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/67683003/1090001

[commit-bot: I haz the power, 2013-11-23 04:11:04]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-25 20:04:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/67683003/1660001

[commit-bot: I haz the power, 2013-11-25 20:57:09]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-11-25 21:55:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/67683003/1640002

[commit-bot: I haz the power, 2013-11-25 22:13:49]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-25 22:30:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/67683003/1690001

[commit-bot: I haz the power, 2013-11-26 00:27:47]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-26 01:23:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/67683003/1710001

[commit-bot: I haz the power, 2013-11-26 02:12:27]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-26 04:59:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/67683003/1710001

[commit-bot: I haz the power, 2013-11-26 06:13:14]

Change committed as 237280

[no sievers, 2013-12-05 02:25:57]

https://codereview.chromium.org/67683003/diff/1710001/content/browser/tracing...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/67683003/diff/1710001/content/browser/tracing...
content/browser/tracing/tracing_controller_impl.cc:637:
base::Unretained(this)));
missing return, i'll upload a patch