Move base/debug/trace_event* to base/trace_event/ [part 1]

Move base/debug/trace_event* to base/trace_event/ [part 1]

Trace Event used to be a single file, base/debug/trace_event.h. Then we
added more functionality, so it was base/debug/trace_event*.*
For many years, we have had per-file OWNERS for Trace Event based off
of trace_event*. This let us do trace_event improvements/iteration out
of band with the much slower base/OWNERS process.
But, this leads to a lot of poor filename choices for tracing related
features, specifically to fall under trace event reviewers instead of
base/ reviewers. That's clearly wrong.

BUG=451032
Committed: https://crrev.com/a59d4277a6a82b3f379cf742f7320d9a6f3801ce
Cr-Commit-Position: refs/heads/master@{#313266}

[ssid, 2015-01-22 14:53:48]

ssid@chromium.org changed reviewers:
- sergiyb@chromium.org

[ssid, 2015-01-22 16:33:59]

ssid@chromium.org changed reviewers:
+ picksi@chromium.org, primiano@chromium.org, skyostil@chromium.org

[ssid, 2015-01-22 16:55:15]

ssid@chromium.org changed reviewers:
+ nduca@chromium.org

[ssid, 2015-01-22 16:55:17]

PTAL.
This is the first part of moving files.

https://codereview.chromium.org/837303004/diff/60001/base/trace_event/trace_e...
File base/trace_event/trace_event_impl.h (right):

https://codereview.chromium.org/837303004/diff/60001/base/trace_event/trace_e...
base/trace_event/trace_event_impl.h:814: 
rebase error, will remove

[nduca, 2015-01-22 17:44:17]

your trampoline files should have copyright headers and include guards

https://codereview.chromium.org/837303004/diff/80001/base/debug/trace_event.h
File base/debug/trace_event.h (left):

https://codereview.chromium.org/837303004/diff/80001/base/debug/trace_event.h...
base/debug/trace_event.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
you should have an include guard on the trampolines and keep the copyright

[ssid, 2015-01-22 18:11:44]

Made changes PTAL.

https://codereview.chromium.org/837303004/diff/80001/base/debug/trace_event.h
File base/debug/trace_event.h (left):

https://codereview.chromium.org/837303004/diff/80001/base/debug/trace_event.h...
base/debug/trace_event.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2015/01/22 17:44:17, nduca wrote:
> you should have an include guard on the trampolines and keep the copyright

Done.

[Primiano Tucci (use gerrit), 2015-01-22 19:23:03]

primiano@chromium.org changed reviewers:
- nduca@chromium.org

[Primiano Tucci (use gerrit), 2015-01-22 19:23:06]

Thanks for the patience.
Some comments.

1) You want the format of your CL to be as follows:
Short description < 72 chars
(blank line)
Long description wrapped at 72 chars
(blank line)
BUG=xxx

2) Please file a bug where you describe what you are doing and why and add the
link to the doc there.

3) In the .cc files that you are moving you should point to the new location,
not use the trampoline headers (those should be only for the rest of the code).

4) You seem to have forgotten to rename the include guards (TRACING vs
TRACE_EVENT)

5) Do not change the copyright header for existing files. See 
http://www.chromium.org/developers/coding-style#TOC-File-headers

6) I removed nduca temporarily. In general it's a good practice to get your CL
reviewed from your local reviewers and add the OWNERS only when you get the
local approval. OWNERS tend to receive a lot of codereviews emails and you want
to add them only if you are sure that your code is ready to land.

https://codereview.chromium.org/837303004/diff/140001/base/BUILD.gn
File base/BUILD.gn (left):

https://codereview.chromium.org/837303004/diff/140001/base/BUILD.gn#oldcode179
base/BUILD.gn:179: "debug/trace_event_memory.h",
This doesn't seem to be used by anybody outside of base. You can move it
directly.

https://codereview.chromium.org/837303004/diff/140001/base/BUILD.gn#oldcode183
base/BUILD.gn:183: "debug/trace_event_system_stats_monitor.h",
Same here

https://codereview.chromium.org/837303004/diff/140001/base/base.gyp
File base/base.gyp (left):

https://codereview.chromium.org/837303004/diff/140001/base/base.gyp#oldcode490
base/base.gyp:490: 'debug/trace_event_unittest.h',
Same here

https://codereview.chromium.org/837303004/diff/140001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

https://codereview.chromium.org/837303004/diff/140001/base/debug/trace_event....
base/debug/trace_event.h:5: // In the process of moving the trace event files.
Right now the headers
You want also a TODO(ssid): link_to_crbug.com/XXXX

https://codereview.chromium.org/837303004/diff/140001/base/debug/trace_event_...
File base/debug/trace_event_memory.h (right):

https://codereview.chromium.org/837303004/diff/140001/base/debug/trace_event_...
base/debug/trace_event_memory.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
Don't change this (also we are in 2015 :D)

According to our coding style [1]
"The year should be the current year when a new file is created, and should not
be changed when the file is updated thereafter."

[1] http://www.chromium.org/developers/coding-style#TOC-File-headers

https://codereview.chromium.org/837303004/diff/140001/base/debug/trace_event_...
File base/debug/trace_event_system_stats_monitor.h (right):

https://codereview.chromium.org/837303004/diff/140001/base/debug/trace_event_...
base/debug/trace_event_system_stats_monitor.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
Ditto

https://codereview.chromium.org/837303004/diff/140001/base/debug/trace_event_...
File base/debug/trace_event_unittest.h (right):

https://codereview.chromium.org/837303004/diff/140001/base/debug/trace_event_...
base/debug/trace_event_unittest.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
Ditto

https://codereview.chromium.org/837303004/diff/140001/base/debug/trace_event_...
File base/debug/trace_event_win.h (right):

https://codereview.chromium.org/837303004/diff/140001/base/debug/trace_event_...
base/debug/trace_event_win.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
Ditto

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
File base/trace_event/trace_event_android.cc (right):

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
base/trace_event/trace_event_android.cc:5: #include
"base/debug/trace_event_impl.h"
You want the new .cc files to point to the new headers. No need to trampoline
them.

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
File base/trace_event/trace_event_argument.cc (right):

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
base/trace_event/trace_event_argument.cc:5: #include
"base/debug/trace_event_argument.h"
Ditto

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
File base/trace_event/trace_event_argument_unittest.cc (right):

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
base/trace_event/trace_event_argument_unittest.cc:5: #include
"base/debug/trace_event_argument.h"
ditto

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
base/trace_event/trace_event_impl.cc:5: #include "base/debug/trace_event_impl.h"
Ditto

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
File base/trace_event/trace_event_impl_constants.cc (right):

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
base/trace_event/trace_event_impl_constants.cc:5: #include
"base/debug/trace_event_impl.h"
Ditto

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
File base/trace_event/trace_event_memory.cc (right):

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
base/trace_event/trace_event_memory.cc:5: #include
"base/debug/trace_event_memory.h"
Ditto

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
File base/trace_event/trace_event_memory_unittest.cc (right):

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
base/trace_event/trace_event_memory_unittest.cc:5: #include
"base/debug/trace_event_memory.h"
Ditto

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
File base/trace_event/trace_event_synthetic_delay.cc (right):

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
base/trace_event/trace_event_synthetic_delay.cc:5: #include
"base/debug/trace_event_synthetic_delay.h"
Ditto

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
File base/trace_event/trace_event_synthetic_delay_unittest.cc (right):

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
base/trace_event/trace_event_synthetic_delay_unittest.cc:5: #include
"base/debug/trace_event_synthetic_delay.h"
Ditto

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
File base/trace_event/trace_event_system_stats_monitor.h (right):

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
base/trace_event/trace_event_system_stats_monitor.h:5: #ifndef
BASE_TRACING_TRACE_EVENT_SYSTEM_STATS_MONITOR_H_
All these guards are wrong. The should be TRACE_EVENT not TRACING

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
base/trace_event/trace_event_system_stats_monitor.h:9: #include
"base/debug/trace_event_impl.h"
Ditto

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
File base/trace_event/trace_event_unittest.cc (right):

https://codereview.chromium.org/837303004/diff/140001/base/trace_event/trace_...
base/trace_event/trace_event_unittest.cc:5: #include
"base/debug/trace_event_unittest.h"
ditto

[ssid, 2015-01-22 21:26:47]

Made changes PTAL.

[nduca, 2015-01-22 22:47:01]

nduca@chromium.org changed reviewers:
+ nduca@chromium.org

[nduca, 2015-01-22 22:47:03]

lgtm when you're ready

[Primiano Tucci (use gerrit), 2015-01-23 11:39:59]

LGTM % 1 fix in the build.
Also, please add a space between Part and 1 in the CL title (and message). Even
better if you name it:

Move base/debug/trace_event* to base/trace_event/ [part 1]

Also, no need to change it since it's going to be short lived, but next time,
when you add a reference to a bug in the code, it is preferrable use
crbug.com/XXXX rather than the full url ;-)

https://codereview.chromium.org/837303004/diff/180001/base/BUILD.gn
File base/BUILD.gn (left):

https://codereview.chromium.org/837303004/diff/180001/base/BUILD.gn#oldcode1175
base/BUILD.gn:1175: "debug/trace_event_unittest.h",
Remove this.

[ssid, 2015-01-23 11:47:20]

ssid@chromium.org changed reviewers:
+ danakj@chromium.org

[ssid, 2015-01-23 11:47:21]

danakj@chromium.org: Please review changes. Thanks.

[danakj, 2015-01-23 17:41:44]

https://codereview.chromium.org/837303004/diff/200001/base/BUILD.gn
File base/BUILD.gn (left):

https://codereview.chromium.org/837303004/diff/200001/base/BUILD.gn#oldcode179
base/BUILD.gn:179: "debug/trace_event_memory.h",
don't remove .h files that weren't deleted yet

https://codereview.chromium.org/837303004/diff/200001/base/debug/trace_event_...
File base/debug/trace_event_argument.h (right):

https://codereview.chromium.org/837303004/diff/200001/base/debug/trace_event_...
base/debug/trace_event_argument.h:6: // are being forwarded. IN next CLs the
change will get completed
"In next"

https://codereview.chromium.org/837303004/diff/200001/base/trace_event/trace_...
File base/trace_event/trace_event_win.cc (right):

https://codereview.chromium.org/837303004/diff/200001/base/trace_event/trace_...
base/trace_event/trace_event_win.cc:4: #include
"base/trace_event/trace_event_win.h"
nit: whitespace above this while you're here

https://codereview.chromium.org/837303004/diff/200001/tools/gn/bootstrap/boot...
File tools/gn/bootstrap/bootstrap.py (right):

https://codereview.chromium.org/837303004/diff/200001/tools/gn/bootstrap/boot...
tools/gn/bootstrap/bootstrap.py:228: 'base/debug/trace_event_impl.cc',
i think you mean base/trace_event/

[ssid, 2015-01-23 20:27:38]

PTAL. Thanks for the comments.

https://codereview.chromium.org/837303004/diff/140001/base/base.gyp
File base/base.gyp (left):

https://codereview.chromium.org/837303004/diff/140001/base/base.gyp#oldcode490
base/base.gyp:490: 'debug/trace_event_unittest.h',
On 2015/01/22 19:23:05, Primiano Tucci wrote:
> Same here

This is being used in base/test/trace_event_analyzer_unittest.cc.

https://codereview.chromium.org/837303004/diff/200001/base/BUILD.gn
File base/BUILD.gn (left):

https://codereview.chromium.org/837303004/diff/200001/base/BUILD.gn#oldcode179
base/BUILD.gn:179: "debug/trace_event_memory.h",
On 2015/01/23 17:41:43, danakj wrote:
> don't remove .h files that weren't deleted yet

I meant to remove the files too. Now done.

https://codereview.chromium.org/837303004/diff/200001/base/debug/trace_event_...
File base/debug/trace_event_argument.h (right):

https://codereview.chromium.org/837303004/diff/200001/base/debug/trace_event_...
base/debug/trace_event_argument.h:6: // are being forwarded. IN next CLs the
change will get completed
On 2015/01/23 17:41:43, danakj wrote:
> "In next"

Done.

https://codereview.chromium.org/837303004/diff/200001/base/trace_event/trace_...
File base/trace_event/trace_event_win.cc (right):

https://codereview.chromium.org/837303004/diff/200001/base/trace_event/trace_...
base/trace_event/trace_event_win.cc:4: #include
"base/trace_event/trace_event_win.h"
On 2015/01/23 17:41:43, danakj wrote:
> nit: whitespace above this while you're here

Done.

https://codereview.chromium.org/837303004/diff/200001/tools/gn/bootstrap/boot...
File tools/gn/bootstrap/bootstrap.py (right):

https://codereview.chromium.org/837303004/diff/200001/tools/gn/bootstrap/boot...
tools/gn/bootstrap/bootstrap.py:228: 'base/debug/trace_event_impl.cc',
On 2015/01/23 17:41:43, danakj wrote:
> i think you mean base/trace_event/

Done.

[danakj, 2015-01-23 21:17:50]

https://codereview.chromium.org/837303004/diff/280001/base/BUILD.gn
File base/BUILD.gn (left):

https://codereview.chromium.org/837303004/diff/280001/base/BUILD.gn#oldcode179
base/BUILD.gn:179: "debug/trace_event_memory.h",
base/debug/trace_event_memory.h is still a file in this patch, so it should
still be in the .gn?

https://codereview.chromium.org/837303004/diff/280001/base/BUILD.gn#oldcode183
base/BUILD.gn:183: "debug/trace_event_system_stats_monitor.h",
base/debug/trace_event_system_stats_monitor.h as well

[ssid, 2015-01-23 21:47:05]

https://codereview.chromium.org/837303004/diff/280001/base/BUILD.gn
File base/BUILD.gn (left):

https://codereview.chromium.org/837303004/diff/280001/base/BUILD.gn#oldcode179
base/BUILD.gn:179: "debug/trace_event_memory.h",
On 2015/01/23 21:17:50, danakj wrote:
> base/debug/trace_event_memory.h is still a file in this patch, so it should
> still be in the .gn?

The files have been removed, but the diff here still shows it as modified.

[danakj, 2015-01-23 21:50:33]

https://codereview.chromium.org/837303004/diff/280001/base/BUILD.gn
File base/BUILD.gn (left):

https://codereview.chromium.org/837303004/diff/280001/base/BUILD.gn#oldcode179
base/BUILD.gn:179: "debug/trace_event_memory.h",
On 2015/01/23 21:47:05, ssid wrote:
> On 2015/01/23 21:17:50, danakj wrote:
> > base/debug/trace_event_memory.h is still a file in this patch, so it should
> > still be in the .gn?
> 
> The files have been removed, but the diff here still shows it as modified.

Ah, I see. The inconsistencies between here and the gyp have been hard to
understand. I think I get what's going on now. This file had the headers but the
gyp files don't. We should fix that in the gyp files.

https://codereview.chromium.org/837303004/diff/280001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/837303004/diff/280001/base/base.gypi#newcode683
base/base.gypi:683: 'trace_event/trace_event_system_stats_monitor.cc',
This should have all the files including headers, it looks like they were
missing before, can you add them now?

[ssid, 2015-01-26 12:03:55]

Fixed the build files. PTAL. thanks

https://codereview.chromium.org/837303004/diff/280001/base/BUILD.gn
File base/BUILD.gn (left):

https://codereview.chromium.org/837303004/diff/280001/base/BUILD.gn#oldcode179
base/BUILD.gn:179: "debug/trace_event_memory.h",
On 2015/01/23 21:50:33, danakj wrote:
> On 2015/01/23 21:47:05, ssid wrote:
> > On 2015/01/23 21:17:50, danakj wrote:
> > > base/debug/trace_event_memory.h is still a file in this patch, so it
should
> > > still be in the .gn?
> > 
> > The files have been removed, but the diff here still shows it as modified.
> 
> Ah, I see. The inconsistencies between here and the gyp have been hard to
> understand. I think I get what's going on now. This file had the headers but
the
> gyp files don't. We should fix that in the gyp files.

Done.

https://codereview.chromium.org/837303004/diff/280001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/837303004/diff/280001/base/base.gypi#newcode683
base/base.gypi:683: 'trace_event/trace_event_system_stats_monitor.cc',
On 2015/01/23 21:50:33, danakj wrote:
> This should have all the files including headers, it looks like they were
> missing before, can you add them now?

Done.

[danakj, 2015-01-26 17:49:39]

LGTM

[ssid, 2015-01-26 17:51:11]

The CQ bit was checked by ssid@chromium.org

[commit-bot: I haz the power, 2015-01-26 17:51:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/837303004/310001

[commit-bot: I haz the power, 2015-01-26 17:58:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-26 17:58:15]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Primiano Tucci (use gerrit), 2015-01-27 11:29:56]

The rebase LGTM.
Thanks for the patience. Ship it before America wakes up again :)

[Primiano Tucci (use gerrit), 2015-01-27 11:30:04]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2015-01-27 11:30:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/837303004/330001

[commit-bot: I haz the power, 2015-01-27 13:14:43]

Committed patchset #18 (id:330001)

[commit-bot: I haz the power, 2015-01-27 13:16:52]

Patchset 18 (id:??) landed as
https://crrev.com/a59d4277a6a82b3f379cf742f7320d9a6f3801ce
Cr-Commit-Position: refs/heads/master@{#313266}