[Startup Tracing] Enable startup tracing from config file

[Startup Tracing] Enable startup tracing from config file

This CL adds a new way to enable startup tracing via Telemetry. Telemetry will put the config file to a known location. The browser can enable startup tracing according to the config file.

The old way of using --trace-startup is still kept.

Startup tracing design doc:
https://docs.google.com/document/d/1yRCXhrQ-0rsfUgNHt9T4YdnmJYrXKN6aK56Ozk3kPVc/edit?usp=sharing

BUG=317481, 482098
Committed: https://crrev.com/062a945213d8a029d3cc1ba239aa567f64971adf
Cr-Commit-Position: refs/heads/master@{#335555}

[Zhen Wang, 2015-06-11 05:50:19]

zhenw@chromium.org changed reviewers:
+ dsinclair@chromium.org

[Zhen Wang, 2015-06-11 05:50:21]

ptal

[dsinclair, 2015-06-11 14:37:09]

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
File base/trace_event/startup_tracing.cc (right):

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:18: // - Android:
/data/local/chrome-trace-config.json
Why not /data/local/chrome-telemetry/chrome-tracing-config.json seems a bit more
consistent?

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:29: #if defined(OS_POSIX) || defined(OS_WIN)
Is this ifdef needed? This is always the name of the json file.

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:44: FilePath path;
Should this log a warning that the config file path doesn't exist?

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:51: void
EnableStartupTracingFromConfigFile() {
This seems slightly misnamed?

EnableStartupTracingIfConfigFileExists()? or something? Just, this doesn't
always enable, it depends on the config file being there.

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:53: if (!trace_config_file_path.empty() &&
PathExists(trace_config_file_path)) {
if (trace_config_file_path.empty() || !PathExists(trace_config_file_path))
 return;

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:55:
base::ReadFileToString(trace_config_file_path, &trace_config_str);
Can this fail?

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:56: TraceConfig
trace_config(trace_config_str);
Was TraceConfig converted over to SafeJSONReader?

[Zhen Wang, 2015-06-12 01:26:04]

zhenw@chromium.org changed reviewers:
+ nednguyen@google.com

[Zhen Wang, 2015-06-12 01:26:06]

Do you know how to fix the build failure on linux_chromium_gn_dbg? I suspect
that nacl does not include the source files of file_util and path_service. But
nacl depends on tracing.

See
https://code.google.com/p/chromium/codesearch#chromium/src/base/BUILD.gn&q=pa...

+nednguyen@ because it is related to Telemetry

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
File base/trace_event/startup_tracing.cc (right):

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:18: // - Android:
/data/local/chrome-trace-config.json
On 2015/06/11 14:37:08, dsinclair wrote:
> Why not /data/local/chrome-telemetry/chrome-tracing-config.json seems a bit
more
> consistent?

Done.

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:29: #if defined(OS_POSIX) || defined(OS_WIN)
On 2015/06/11 14:37:09, dsinclair wrote:
> Is this ifdef needed? This is always the name of the json file.

This is for the cases where it is neither POSIX nor WIN. For example, IOS.

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:44: FilePath path;
On 2015/06/11 14:37:09, dsinclair wrote:
> Should this log a warning that the config file path doesn't exist? 

This is for the cases where it is neither POSIX nor WIN. For example, IOS. And
we currently do not support it.

For POSIX and WIN, the config file path always exists. The file itself may not
exist.

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:51: void
EnableStartupTracingFromConfigFile() {
On 2015/06/11 14:37:09, dsinclair wrote:
> This seems slightly misnamed?
> 
> EnableStartupTracingIfConfigFileExists()? or something? Just, this doesn't
> always enable, it depends on the config file being there.

Makes sense. Renamed.

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:53: if (!trace_config_file_path.empty() &&
PathExists(trace_config_file_path)) {
On 2015/06/11 14:37:09, dsinclair wrote:
> if (trace_config_file_path.empty() || !PathExists(trace_config_file_path))
>  return;

Done.

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:55:
base::ReadFileToString(trace_config_file_path, &trace_config_str);
On 2015/06/11 14:37:09, dsinclair wrote:
> Can this fail?

Thanks for catching this. Now checking the return value.

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:56: TraceConfig
trace_config(trace_config_str);
On 2015/06/11 14:37:09, dsinclair wrote:
> Was TraceConfig converted over to SafeJSONReader?

No. It still uses normal json reader. So the decision is to put the config file
to /data/local/ on Android and rooted device is required. See
https://docs.google.com/document/d/1yRCXhrQ-0rsfUgNHt9T4YdnmJYrXKN6aK56Ozk3kP...

[dsinclair, 2015-06-12 14:04:15]

>Do you know how to fix the build failure on linux_chromium_gn_dbg? I suspect
that nacl does not include the source files of file_util and path_service. But
nacl depends on tracing.


I think you probably want to not have this code in for tracing, so either #if
!defined(OS_NACL) or do the build rule logic to not include the startup tracing
files for a nacl build (or both probably, ifdef out the few calling points,
remove the startup_tracing cc/h files from the nacl build)

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
File base/trace_event/startup_tracing.cc (right):

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:29: #if defined(OS_POSIX) || defined(OS_WIN)
On 2015/06/12 01:26:05, Zhen Wang wrote:
> On 2015/06/11 14:37:09, dsinclair wrote:
> > Is this ifdef needed? This is always the name of the json file.
> 
> This is for the cases where it is neither POSIX nor WIN. For example, IOS.


I think we can still have those constant defined on IOS, and I'd assume if we
get startup tracing on IOS then we could use them same directory names? Is this
just to save space in the final binary?

[Zhen Wang, 2015-06-12 15:41:34]

> >Do you know how to fix the build failure on linux_chromium_gn_dbg? I suspect
> that nacl does not include the source files of file_util and path_service. But
> nacl depends on tracing.
> 
> I think you probably want to not have this code in for tracing, so either #if
> !defined(OS_NACL) or do the build rule logic to not include the startup
tracing
> files for a nacl build (or both probably, ifdef out the few calling points,
> remove the startup_tracing cc/h files from the nacl build)

Maybe I should move startup_tracing files to content/browser/tracing/, which
seems more appropriate?

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
File base/trace_event/startup_tracing.cc (right):

https://codereview.chromium.org/1177853003/diff/60001/base/trace_event/startu...
base/trace_event/startup_tracing.cc:29: #if defined(OS_POSIX) || defined(OS_WIN)
On 2015/06/12 14:04:14, dsinclair wrote:
> On 2015/06/12 01:26:05, Zhen Wang wrote:
> > On 2015/06/11 14:37:09, dsinclair wrote:
> > > Is this ifdef needed? This is always the name of the json file.
> > 
> > This is for the cases where it is neither POSIX nor WIN. For example, IOS.
> 
> 
> I think we can still have those constant defined on IOS, and I'd assume if we
> get startup tracing on IOS then we could use them same directory names? Is
this
> just to save space in the final binary?

OK. defined is now removed.

[dsinclair, 2015-06-12 15:44:55]

On 2015/06/12 15:41:34, Zhen Wang wrote:
> > >Do you know how to fix the build failure on linux_chromium_gn_dbg? I
suspect
> > that nacl does not include the source files of file_util and path_service.
But
> > nacl depends on tracing.
> > 
> > I think you probably want to not have this code in for tracing, so either
#if
> > !defined(OS_NACL) or do the build rule logic to not include the startup
> tracing
> > files for a nacl build (or both probably, ifdef out the few calling points,
> > remove the startup_tracing cc/h files from the nacl build)
> 
> Maybe I should move startup_tracing files to content/browser/tracing/, which
> seems more appropriate?
> 

That does seem like it would make sense. startup is a function of the browser
and not the trace framework.

[Zhen Wang, 2015-06-12 16:04:16]

On 2015/06/12 15:44:55, dsinclair wrote:
> On 2015/06/12 15:41:34, Zhen Wang wrote:
> > > >Do you know how to fix the build failure on linux_chromium_gn_dbg? I
> suspect
> > > that nacl does not include the source files of file_util and path_service.
> But
> > > nacl depends on tracing.
> > > 
> > > I think you probably want to not have this code in for tracing, so either
> #if
> > > !defined(OS_NACL) or do the build rule logic to not include the startup
> > tracing
> > > files for a nacl build (or both probably, ifdef out the few calling
points,
> > > remove the startup_tracing cc/h files from the nacl build)
> > 
> > Maybe I should move startup_tracing files to content/browser/tracing/, which
> > seems more appropriate?
> > 
> 
> That does seem like it would make sense. startup is a function of the browser
> and not the trace framework.


Shall I actually move startup_tracing to src/components/tracing/? mandoline and
mojo also have startup tracing code. These places cannot use code in content/.

[dsinclair, 2015-06-12 17:06:03]

On 2015/06/12 16:04:16, Zhen Wang wrote:
> On 2015/06/12 15:44:55, dsinclair wrote:
> > On 2015/06/12 15:41:34, Zhen Wang wrote:
> > > > >Do you know how to fix the build failure on linux_chromium_gn_dbg? I
> > suspect
> > > > that nacl does not include the source files of file_util and
path_service.
> > But
> > > > nacl depends on tracing.
> > > > 
> > > > I think you probably want to not have this code in for tracing, so
either
> > #if
> > > > !defined(OS_NACL) or do the build rule logic to not include the startup
> > > tracing
> > > > files for a nacl build (or both probably, ifdef out the few calling
> points,
> > > > remove the startup_tracing cc/h files from the nacl build)
> > > 
> > > Maybe I should move startup_tracing files to content/browser/tracing/,
which
> > > seems more appropriate?
> > > 
> > 
> > That does seem like it would make sense. startup is a function of the
browser
> > and not the trace framework.
> 
> 
> Shall I actually move startup_tracing to src/components/tracing/? mandoline
and
> mojo also have startup tracing code. These places cannot use code in content/.


Sure, wherever is the most appropriate place given what you're trying to do.

[nednguyen, 2015-06-12 20:12:18]

https://codereview.chromium.org/1177853003/diff/140001/base/trace_event/start...
File base/trace_event/startup_tracing.cc (right):

https://codereview.chromium.org/1177853003/diff/140001/base/trace_event/start...
base/trace_event/startup_tracing.cc:20: // - Win:
%USERPROFILE%/.chrome-telemetry/chrome-trace-config.json
I am not sure whether putting the name "telemetry" here is a good idea.  

I would rather have the chrome browser system to not make any assumption about
the naming of some test automation system that runs it. 

How about just "../chrome-config/chrome-trace-config.json" ?

[Zhen Wang, 2015-06-13 03:48:03]

I have moved the files to components/. ptal

https://codereview.chromium.org/1177853003/diff/140001/base/trace_event/start...
File base/trace_event/startup_tracing.cc (right):

https://codereview.chromium.org/1177853003/diff/140001/base/trace_event/start...
base/trace_event/startup_tracing.cc:20: // - Win:
%USERPROFILE%/.chrome-telemetry/chrome-trace-config.json
On 2015/06/12 20:12:18, nednguyen wrote:
> I am not sure whether putting the name "telemetry" here is a good idea.  
> 
> I would rather have the chrome browser system to not make any assumption about
> the naming of some test automation system that runs it. 
> 
> How about just "../chrome-config/chrome-trace-config.json" ?

Now changed to .config, which is the convention.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro...

We do not use GetDefaultUserDataDirectory() because Telemetry does not know if
the browser is official build or not.

[dsinclair, 2015-06-15 13:49:45]

https://codereview.chromium.org/1177853003/diff/200001/components/tracing/sta...
File components/tracing/startup_tracing.cc (right):

https://codereview.chromium.org/1177853003/diff/200001/components/tracing/sta...
components/tracing/startup_tracing.cc:48: if (trace_config_file_path.empty()
This can never be empty given the above always appends kChromeConfigDir and
kTraceConfigFileName.

https://codereview.chromium.org/1177853003/diff/200001/components/tracing/sta...
components/tracing/startup_tracing.cc:53: if
(!base::ReadFileToString(trace_config_file_path, &trace_config_str))
Should we verify the file size before reading? Could it be possible to put a
huge file here and the browser runs out of memory trying to read it?

[nednguyen, 2015-06-15 15:03:39]

How do we usually add test for this kind of functionality?

https://codereview.chromium.org/1177853003/diff/200001/components/tracing/sta...
File components/tracing/startup_tracing.cc (right):

https://codereview.chromium.org/1177853003/diff/200001/components/tracing/sta...
components/tracing/startup_tracing.cc:53: if
(!base::ReadFileToString(trace_config_file_path, &trace_config_str))
On 2015/06/15 13:49:45, dsinclair wrote:
> Should we verify the file size before reading? Could it be possible to put a
> huge file here and the browser runs out of memory trying to read it?

Looks like there is a ReadFileToString with a max size limit
(https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_ut...)

[Zhen Wang, 2015-06-15 16:58:30]

https://codereview.chromium.org/1177853003/diff/200001/components/tracing/sta...
File components/tracing/startup_tracing.cc (right):

https://codereview.chromium.org/1177853003/diff/200001/components/tracing/sta...
components/tracing/startup_tracing.cc:48: if (trace_config_file_path.empty()
On 2015/06/15 13:49:45, dsinclair wrote:
> This can never be empty given the above always appends kChromeConfigDir and
> kTraceConfigFileName.

Done.

https://codereview.chromium.org/1177853003/diff/200001/components/tracing/sta...
components/tracing/startup_tracing.cc:53: if
(!base::ReadFileToString(trace_config_file_path, &trace_config_str))
On 2015/06/15 15:03:39, nednguyen wrote:
> On 2015/06/15 13:49:45, dsinclair wrote:
> > Should we verify the file size before reading? Could it be possible to put a
> > huge file here and the browser runs out of memory trying to read it?
> 
> Looks like there is a ReadFileToString with a max size limit
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_ut...)

Done.

[dsinclair, 2015-06-15 19:27:12]

lgtm

[Zhen Wang, 2015-06-15 20:08:37]

+erikwright for components/
+sievers for content/
+sky for mojo/ and mandoline/

ptal :-)

[Zhen Wang, 2015-06-15 20:12:13]

zhenw@chromium.org changed reviewers:
+ erikwright@chromium.org, sievers@chromium.org, sky@chromium.org

[Zhen Wang, 2015-06-15 20:12:14]

+erikwright for components/
+sievers for content/
+sky for mojo/ and mandoline/

ptal :-)

[no sievers, 2015-06-15 21:41:48]

sievers@chromium.org changed reviewers:
+ jam@chromium.org

[no sievers, 2015-06-15 21:44:01]

+jam to double-check the content->component dependency.

Looks like content/child and common also depend on tracing already.

https://codereview.chromium.org/1177853003/diff/220001/content/app/DEPS
File content/app/DEPS (right):

https://codereview.chromium.org/1177853003/diff/220001/content/app/DEPS#newcode3
content/app/DEPS:3: "+components/tracing/startup_tracing.h",
nit: order

[Zhen Wang, 2015-06-15 22:09:11]

https://codereview.chromium.org/1177853003/diff/220001/content/app/DEPS
File content/app/DEPS (right):

https://codereview.chromium.org/1177853003/diff/220001/content/app/DEPS#newcode3
content/app/DEPS:3: "+components/tracing/startup_tracing.h",
On 2015/06/15 21:44:01, sievers wrote:
> nit: order

Done.

[sky, 2015-06-15 22:24:55]

sky@chromium.org changed reviewers:
+ yzshen@chromium.org

[sky, 2015-06-15 22:24:55]

+yuzhu as he's been looking at telemetry
How does this relate to the tracking mojo service?

[Zhen Wang, 2015-06-15 22:29:17]

On 2015/06/15 22:24:55, sky wrote:
> +yuzhu as he's been looking at telemetry
> How does this relate to the tracking mojo service?

What is the tracking mojo service?

I actually want to know if mojo/mandoline needs this. I thought the
mojo/mandoline startup tracing code was adapted from
content/app/content_main_runner.cc and content/browser/browser_main_loop.cc.

[sky, 2015-06-16 00:04:30]

On Mon, Jun 15, 2015 at 3:29 PM,  <zhenw@chromium.org> wrote:
> On 2015/06/15 22:24:55, sky wrote:
>>
>> +yuzhu as he's been looking at telemetry
>> How does this relate to the tracking mojo service?
>
>
> What is the tracking mojo service?

Sorry. I meant tracing service. It's here:
https://chromium.googlesource.com/chromium/src/+/master/mojo/services/tracing/
.

>
> I actually want to know if mojo/mandoline needs this. I thought the
> mojo/mandoline startup tracing code was adapted from
> content/app/content_main_runner.cc and content/browser/browser_main_loop.cc.

There are likely some parts of overlap. jam is also on this review, he
may have thoughts here.

  -Scott

>
> https://codereview.chromium.org/1177853003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Zhen Wang, 2015-06-16 15:13:25]

On 2015/06/16 00:04:30, sky wrote:
> On Mon, Jun 15, 2015 at 3:29 PM,  <mailto:zhenw@chromium.org> wrote:
> > On 2015/06/15 22:24:55, sky wrote:
> >>
> >> +yuzhu as he's been looking at telemetry
> >> How does this relate to the tracking mojo service?
> >
> >
> > What is the tracking mojo service?
> 
> Sorry. I meant tracing service. It's here:
> https://chromium.googlesource.com/chromium/src/+/master/mojo/services/tracing/

I took a look at mojo tracing service and cannot see how it uses TraceLog etc in
base. Maybe someone from mojo team can advise? If startup tracing via Telemetry
is not what mojo/mandoline needs right now, I can remove the changes in
mojo/mandoline. And We can always adds the support latter if needed.

[erikwright (departed), 2015-06-16 15:24:26]

erikwright@chromium.org changed reviewers:
- erikwright@chromium.org

[erikwright (departed), 2015-06-16 15:24:28]

Please use a more specific owner for components/tracing and the gyp/gn files in
components.

Specifically jbauman or nduca.

[erikwright (departed), 2015-06-16 15:25:56]

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

[erikwright (departed), 2015-06-16 15:25:57]

components/BUILD.gn LGTM

[Zhen Wang, 2015-06-16 16:15:49]

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

[Zhen Wang, 2015-06-16 16:15:50]

+nduca for /components/tracing/

[yzshen1, 2015-06-16 23:44:58]

Thanks for pointing me to this CL, Scott.

Yes. Telemetry needs tracing support. It is probably the next thing that I will
look at after I successfully run some simple telemetry tests for Mandoline on
the buildbots. But currently I haven't taken a close look.

https://codereview.chromium.org/1177853003/diff/230001/mojo/runner/desktop/la...
File mojo/runner/desktop/launcher_process.cc (right):

https://codereview.chromium.org/1177853003/diff/230001/mojo/runner/desktop/la...
mojo/runner/desktop/launcher_process.cc:92:
tracing::EnableStartupTracingIfConfigFileExists();
I haven't looked at the code very closely. In this case, g_tracing is not
touched, is that expected? For example, StopTracingAndFlushToDisk() is not
called for sure.

[Zhen Wang, 2015-06-16 23:49:00]

https://codereview.chromium.org/1177853003/diff/230001/mojo/runner/desktop/la...
File mojo/runner/desktop/launcher_process.cc (right):

https://codereview.chromium.org/1177853003/diff/230001/mojo/runner/desktop/la...
mojo/runner/desktop/launcher_process.cc:92:
tracing::EnableStartupTracingIfConfigFileExists();
On 2015/06/16 23:44:58, yzshen1 wrote:
> I haven't looked at the code very closely. In this case, g_tracing is not
> touched, is that expected? For example, StopTracingAndFlushToDisk() is not
> called for sure.

Yes. This is expected. So the existing way is to use --trace-startup flag. You
need to specify the time period when trace is running. So it will schedule a
function (StopTracingAndFlushToDisk) to stop tracing automatically after the
specified period.

If using startup tracing via Telemetry, Telemetry will control the config file.
Telemetry puts the config file to the well-known location before the browser
starts. Later, Telemetry will stop tracing via DevTools. Therefore, there is no
need to call StopTracingAndFlushToDisk().

[yzshen1, 2015-06-16 23:50:44]

On 2015/06/16 23:49:00, Zhen Wang wrote:
>
https://codereview.chromium.org/1177853003/diff/230001/mojo/runner/desktop/la...
> File mojo/runner/desktop/launcher_process.cc (right):
> 
>
https://codereview.chromium.org/1177853003/diff/230001/mojo/runner/desktop/la...
> mojo/runner/desktop/launcher_process.cc:92:
> tracing::EnableStartupTracingIfConfigFileExists();
> On 2015/06/16 23:44:58, yzshen1 wrote:
> > I haven't looked at the code very closely. In this case, g_tracing is not
> > touched, is that expected? For example, StopTracingAndFlushToDisk() is not
> > called for sure.
> 
> Yes. This is expected. So the existing way is to use --trace-startup flag. You
> need to specify the time period when trace is running. So it will schedule a
> function (StopTracingAndFlushToDisk) to stop tracing automatically after the
> specified period.
> 
> If using startup tracing via Telemetry, Telemetry will control the config
file.
> Telemetry puts the config file to the well-known location before the browser
> starts. Later, Telemetry will stop tracing via DevTools. Therefore, there is
no
> need to call StopTracingAndFlushToDisk().

Okay, it seems nice to add a comment why g_tracing is not touched.

(LGTM)

[jam, 2015-06-18 21:36:45]

lgtm

[Zhen Wang, 2015-06-18 22:06:19]

- sievers@, can you double check content/ since jam@ has lg2m it?
- sky@, can you double check mojo/ and mandoline/ since yzshen1@ has lg2m it?
- nduca@, can you stamp components/tracing since dsinclair@ has lg2m it?

Thanks!

[sky, 2015-06-18 22:18:16]

https://codereview.chromium.org/1177853003/diff/250001/mandoline/app/desktop/...
File mandoline/app/desktop/launcher_process.cc (right):

https://codereview.chromium.org/1177853003/diff/250001/mandoline/app/desktop/...
mandoline/app/desktop/launcher_process.cc:95:
tracing::EnableStartupTracingIfConfigFileExists();
Don't you need to update BUILD.gn for the new dep?

https://codereview.chromium.org/1177853003/diff/250001/mojo/common/BUILD.gn
File mojo/common/BUILD.gn (right):

https://codereview.chromium.org/1177853003/diff/250001/mojo/common/BUILD.gn#n...
mojo/common/BUILD.gn:42: "//components/tracing:startup_tracing",
This isn't actually using statup_tracing. It's the other places that are and
should have this dep, right?

[Zhen Wang, 2015-06-18 22:37:59]

https://codereview.chromium.org/1177853003/diff/250001/mojo/common/BUILD.gn
File mojo/common/BUILD.gn (right):

https://codereview.chromium.org/1177853003/diff/250001/mojo/common/BUILD.gn#n...
mojo/common/BUILD.gn:42: "//components/tracing:startup_tracing",
On 2015/06/18 22:18:16, sky wrote:
> This isn't actually using statup_tracing. It's the other places that are and
> should have this dep, right?

I was following the pattern in content/child/BUILD.gn to add the dependency to
common. Should I add the dependency directly to the followings?
- mojo_runner_lib in src/mojo/runner/BUILD.gn
- mandoline in mandoline/app/desktop/BUILD.gn

[no sievers, 2015-06-18 23:21:46]

On 2015/06/18 22:06:19, Zhen Wang wrote:
> - sievers@, can you double check content/ since jam@ has lg2m it?

lgtm

[sky, 2015-06-18 23:43:08]

https://codereview.chromium.org/1177853003/diff/250001/mojo/common/BUILD.gn
File mojo/common/BUILD.gn (right):

https://codereview.chromium.org/1177853003/diff/250001/mojo/common/BUILD.gn#n...
mojo/common/BUILD.gn:42: "//components/tracing:startup_tracing",
On 2015/06/18 22:37:58, Zhen Wang wrote:
> On 2015/06/18 22:18:16, sky wrote:
> > This isn't actually using statup_tracing. It's the other places that are and
> > should have this dep, right?
> 
> I was following the pattern in content/child/BUILD.gn to add the dependency to
> common. Should I add the dependency directly to the followings?
> - mojo_runner_lib in src/mojo/runner/BUILD.gn
> - mandoline in mandoline/app/desktop/BUILD.gn

Each step of the build should list targets it depends upon. Otherwise dependency
checking won't work correctly. So yes, put the deps where the files are
included.

[Zhen Wang, 2015-06-19 03:26:11]

https://codereview.chromium.org/1177853003/diff/250001/mojo/common/BUILD.gn
File mojo/common/BUILD.gn (right):

https://codereview.chromium.org/1177853003/diff/250001/mojo/common/BUILD.gn#n...
mojo/common/BUILD.gn:42: "//components/tracing:startup_tracing",
On 2015/06/18 23:43:08, sky wrote:
> On 2015/06/18 22:37:58, Zhen Wang wrote:
> > On 2015/06/18 22:18:16, sky wrote:
> > > This isn't actually using statup_tracing. It's the other places that are
and
> > > should have this dep, right?
> > 
> > I was following the pattern in content/child/BUILD.gn to add the dependency
to
> > common. Should I add the dependency directly to the followings?
> > - mojo_runner_lib in src/mojo/runner/BUILD.gn
> > - mandoline in mandoline/app/desktop/BUILD.gn
> 
> Each step of the build should list targets it depends upon. Otherwise
dependency
> checking won't work correctly. So yes, put the deps where the files are
> included.

Done.

[sky, 2015-06-19 03:55:52]

LGTM

[nduca, 2015-06-22 16:46:10]

rubberstamplgtm

[nduca, 2015-06-22 16:46:25]

@#$)(@)* lgtm

[Zhen Wang, 2015-06-22 16:47:58]

The CQ bit was checked by zhenw@chromium.org

[Zhen Wang, 2015-06-22 16:47:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org, jam@chromium.org, yzshen@chromium.org,
erikwright@chromium.org, sievers@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1177853003/#ps290001
(title: "rebase")

[commit-bot: I haz the power, 2015-06-22 16:48:49]

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

[commit-bot: I haz the power, 2015-06-22 17:04:48]

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

[commit-bot: I haz the power, 2015-06-22 17:04:51]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)

[Zhen Wang, 2015-06-22 20:48:13]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2015-06-22 20:48:49]

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

[commit-bot: I haz the power, 2015-06-22 20:55:40]

Committed patchset #17 (id:290001)

[commit-bot: I haz the power, 2015-06-22 20:56:36]

Patchset 17 (id:??) landed as
https://crrev.com/062a945213d8a029d3cc1ba239aa567f64971adf
Cr-Commit-Position: refs/heads/master@{#335555}