Add granular file tracing.

Add granular file tracing.

R=thestig@chromium.org,nduca@chromium.org
BUG=480665
Committed: https://crrev.com/492dc31b433656444c9c74213a133c19c8ebfa21
Cr-Commit-Position: refs/heads/master@{#329114}

[Dan Beam, 2015-04-23 17:18:24]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2015-04-23 17:35:27]

Patchset #1 (id:20001) has been deleted

[Dan Beam, 2015-04-23 18:21:37]

Patchset #2 (id:60001) has been deleted

[Dan Beam, 2015-04-23 18:21:42]

Patchset #1 (id:40001) has been deleted

[Dan Beam, 2015-04-23 21:34:38]

Patchset #2 (id:100001) has been deleted

[Dan Beam, 2015-04-23 22:04:41]

Patchset #3 (id:140001) has been deleted

[Dan Beam, 2015-04-23 22:16:12]

Patchset #2 (id:120001) has been deleted

[Dan Beam, 2015-04-23 22:16:37]

Patchset #3 (id:170001) has been deleted

[Dan Beam, 2015-04-23 22:32:19]

screenshots: http://imgur.com/a/dM3U1

shared traces internally with folks via drive (they were too big to attachment
to the bug).

https://codereview.chromium.org/1072133006/diff/160001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1072133006/diff/160001/base/files/file.cc#new...
base/files/file.cc:98: "path", path_.AsUTF8Unsafe());
this seemed like the sanest place to start this, but open to suggestions...

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
File base/trace_event/scoped_file_trace.h (right):

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
base/trace_event/scoped_file_trace.h:34: static const char kGroup[];
don't really care where this lives; it's arguable it should be reversed
(base::File::kTraceGroup and it's used here).

[Dan Beam, 2015-04-24 03:45:19]

https://codereview.chromium.org/1072133006/diff/160001/base/files/file_win.cc
File base/files/file_win.cc (right):

https://codereview.chromium.org/1072133006/diff/160001/base/files/file_win.cc...
base/files/file_win.cc:135: ScopedFileTrace trace(path_, "WriteAtCurrentPos",
size);
It's also arguable that we'd just want these all to be "Write" (same with
"ReadAtCurrentPos").

https://codereview.chromium.org/1072133006/diff/160001/base/files/file_win.cc...
base/files/file_win.cc:165: ScopedFileTrace trace(path_, "SetLength", length);
Also, should I be passing length here?

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
File base/trace_event/scoped_file_trace.h (right):

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
base/trace_event/scoped_file_trace.h:38: std::string path_;
Note to self: const

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
base/trace_event/scoped_file_trace.h:44: size_t bytes_;
Also const if it's too hard to update with real byte amount later

[Sami, 2015-04-24 10:42:54]

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

[Sami, 2015-04-24 10:42:55]

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
File base/trace_event/scoped_file_trace.cc (right):

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
base/trace_event/scoped_file_trace.cc:17: : path_(path.AsUTF8Unsafe()),
event_(event), bytes_(bytes) {
This causes some string copying and memory allocation churn even if tracing is
disabled. Could we avoid that somehow?

[Dan Beam, 2015-04-24 16:37:19]

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
File base/trace_event/scoped_file_trace.cc (right):

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
base/trace_event/scoped_file_trace.cc:17: : path_(path.AsUTF8Unsafe()),
event_(event), bytes_(bytes) {
On 2015/04/24 10:42:55, Sami wrote:
> This causes some string copying and memory allocation churn even if tracing is
> disabled. Could we avoid that somehow?

How is tracing disabled? At compile time or runtime?

There's a couple things we could do. Compile-time:

#if !defined(ENABLE_TRACING)

ScopedFileTrace::ScopedFileTrace(
    const FilePath& path, const char* event, size_t bytes) {}
ScopedFileTrace::~ScopedFileTrace() {}

#else
... functional code ...
#endif

would probably be the easiest if this gets stripped out by the compiler.

Runtime:

ScopedFileTrace::ScopedFileTrace(
    const FilePath& path, const char* event, size_t bytes) {
  if (IsTracingOn()) {
    event_ = event;
    ...
  } else {
    event_ = nullptr;
  }
}
ScopedFileTrace::~ScopedFileTrace() {
  if (event_) {  // instead of IsTracingOn()
    ...
  }
}

Seems like the best way as it only stack allocates 1 size_t, 1 pointer (only set
once), and 1 empty string (maybe 1 pointer? not sure).

Is this sufficient?  We could try to employ some hacks with heap allocation or
use const refs because these are all used in the same scope but these seem like
premature optimizations (and more prone to use-after-free).

I agree we shouldn't copy and/or convert the path from UTF-8 (which will be from
wide on windows, I believe).

[Dan Beam, 2015-04-24 16:44:49]

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
File base/trace_event/scoped_file_trace.cc (right):

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
base/trace_event/scoped_file_trace.cc:17: : path_(path.AsUTF8Unsafe()),
event_(event), bytes_(bytes) {
On 2015/04/24 16:37:19, Dan Beam wrote:
> On 2015/04/24 10:42:55, Sami wrote:
> > This causes some string copying and memory allocation churn even if tracing
is
> > disabled. Could we avoid that somehow?
> 
> How is tracing disabled? At compile time or runtime?
> 
> There's a couple things we could do. Compile-time:
> 
> #if !defined(ENABLE_TRACING)
> 
> ScopedFileTrace::ScopedFileTrace(
>     const FilePath& path, const char* event, size_t bytes) {}
> ScopedFileTrace::~ScopedFileTrace() {}
> 
> #else
> ... functional code ...
> #endif
> 
> would probably be the easiest if this gets stripped out by the compiler.
> 
> Runtime:
> 
> ScopedFileTrace::ScopedFileTrace(
>     const FilePath& path, const char* event, size_t bytes) {
>   if (IsTracingOn()) {
>     event_ = event;
>     ...
>   } else {
>     event_ = nullptr;
>   }
> }
> ScopedFileTrace::~ScopedFileTrace() {
>   if (event_) {  // instead of IsTracingOn()
>     ...
>   }
> }
> 
> Seems like the best way as it only stack allocates 1 size_t, 1 pointer (only
set
> once), and 1 empty string (maybe 1 pointer? not sure).
> 
> Is this sufficient?  We could try to employ some hacks with heap allocation or
> use const refs because these are all used in the same scope but these seem
like
> premature optimizations (and more prone to use-after-free).
> 
> I agree we shouldn't copy and/or convert the path from UTF-8 (which will be
from
> wide on windows, I believe).

Or we could just add a bool tracing_enabled_.

[Sami, 2015-04-24 16:45:42]

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
File base/trace_event/scoped_file_trace.cc (right):

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
base/trace_event/scoped_file_trace.cc:17: : path_(path.AsUTF8Unsafe()),
event_(event), bytes_(bytes) {
On 2015/04/24 16:37:19, Dan Beam wrote:
> On 2015/04/24 10:42:55, Sami wrote:
> > This causes some string copying and memory allocation churn even if tracing
is
> > disabled. Could we avoid that somehow?
> 
> How is tracing disabled? At compile time or runtime?
> 
> There's a couple things we could do. Compile-time:
> 
> #if !defined(ENABLE_TRACING)
> 
> ScopedFileTrace::ScopedFileTrace(
>     const FilePath& path, const char* event, size_t bytes) {}
> ScopedFileTrace::~ScopedFileTrace() {}
> 
> #else
> ... functional code ...
> #endif
> 
> would probably be the easiest if this gets stripped out by the compiler.
> 
> Runtime:
> 
> ScopedFileTrace::ScopedFileTrace(
>     const FilePath& path, const char* event, size_t bytes) {
>   if (IsTracingOn()) {
>     event_ = event;
>     ...
>   } else {
>     event_ = nullptr;
>   }
> }
> ScopedFileTrace::~ScopedFileTrace() {
>   if (event_) {  // instead of IsTracingOn()
>     ...
>   }
> }
> 
> Seems like the best way as it only stack allocates 1 size_t, 1 pointer (only
set
> once), and 1 empty string (maybe 1 pointer? not sure).
> 
> Is this sufficient?  We could try to employ some hacks with heap allocation or
> use const refs because these are all used in the same scope but these seem
like
> premature optimizations (and more prone to use-after-free).
> 
> I agree we shouldn't copy and/or convert the path from UTF-8 (which will be
from
> wide on windows, I believe).

Tracing is enabled at runtime and included in all builds. You can check if
tracing is enabled via TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED.

However, note that TRACE_EVENT() is already scoped and won't evaluate its
arguments unless tracing is enabled, so it seems like we could just replace
ScopedFileTrace with a corresponding TRACE_EVENT macro invocation like most
other subsystems do. WDYT?

[Dan Beam, 2015-04-24 17:29:50]

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
File base/trace_event/scoped_file_trace.cc (right):

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
base/trace_event/scoped_file_trace.cc:17: : path_(path.AsUTF8Unsafe()),
event_(event), bytes_(bytes) {
On 2015/04/24 16:45:41, Sami wrote:
> On 2015/04/24 16:37:19, Dan Beam wrote:
> > On 2015/04/24 10:42:55, Sami wrote:
> > > This causes some string copying and memory allocation churn even if
tracing
> is
> > > disabled. Could we avoid that somehow?
> > 
> > How is tracing disabled? At compile time or runtime?
> > 
> > There's a couple things we could do. Compile-time:
> > 
> > #if !defined(ENABLE_TRACING)
> > 
> > ScopedFileTrace::ScopedFileTrace(
> >     const FilePath& path, const char* event, size_t bytes) {}
> > ScopedFileTrace::~ScopedFileTrace() {}
> > 
> > #else
> > ... functional code ...
> > #endif
> > 
> > would probably be the easiest if this gets stripped out by the compiler.
> > 
> > Runtime:
> > 
> > ScopedFileTrace::ScopedFileTrace(
> >     const FilePath& path, const char* event, size_t bytes) {
> >   if (IsTracingOn()) {
> >     event_ = event;
> >     ...
> >   } else {
> >     event_ = nullptr;
> >   }
> > }
> > ScopedFileTrace::~ScopedFileTrace() {
> >   if (event_) {  // instead of IsTracingOn()
> >     ...
> >   }
> > }
> > 
> > Seems like the best way as it only stack allocates 1 size_t, 1 pointer (only
> set
> > once), and 1 empty string (maybe 1 pointer? not sure).
> > 
> > Is this sufficient?  We could try to employ some hacks with heap allocation
or
> > use const refs because these are all used in the same scope but these seem
> like
> > premature optimizations (and more prone to use-after-free).
> > 
> > I agree we shouldn't copy and/or convert the path from UTF-8 (which will be
> from
> > wide on windows, I believe).
> 
> Tracing is enabled at runtime and included in all builds. You can check if
> tracing is enabled via TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED.
> 
> However, note that TRACE_EVENT() is already scoped and won't evaluate its
> arguments unless tracing is enabled, so it seems like we could just replace
> ScopedFileTrace with a corresponding TRACE_EVENT macro invocation like most
> other subsystems do. WDYT?

We could if we want to duplicate some macros or rewrite base::File to have less
if (...) { return ...; } code paths.  there's about 16 of these currently.

[Sami, 2015-04-24 17:34:47]

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
File base/trace_event/scoped_file_trace.cc (right):

https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
base/trace_event/scoped_file_trace.cc:17: : path_(path.AsUTF8Unsafe()),
event_(event), bytes_(bytes) {
On 2015/04/24 17:29:50, Dan Beam wrote:
> On 2015/04/24 16:45:41, Sami wrote:
> > On 2015/04/24 16:37:19, Dan Beam wrote:
> > > On 2015/04/24 10:42:55, Sami wrote:
> > > > This causes some string copying and memory allocation churn even if
> tracing
> > is
> > > > disabled. Could we avoid that somehow?
> > > 
> > > How is tracing disabled? At compile time or runtime?
> > > 
> > > There's a couple things we could do. Compile-time:
> > > 
> > > #if !defined(ENABLE_TRACING)
> > > 
> > > ScopedFileTrace::ScopedFileTrace(
> > >     const FilePath& path, const char* event, size_t bytes) {}
> > > ScopedFileTrace::~ScopedFileTrace() {}
> > > 
> > > #else
> > > ... functional code ...
> > > #endif
> > > 
> > > would probably be the easiest if this gets stripped out by the compiler.
> > > 
> > > Runtime:
> > > 
> > > ScopedFileTrace::ScopedFileTrace(
> > >     const FilePath& path, const char* event, size_t bytes) {
> > >   if (IsTracingOn()) {
> > >     event_ = event;
> > >     ...
> > >   } else {
> > >     event_ = nullptr;
> > >   }
> > > }
> > > ScopedFileTrace::~ScopedFileTrace() {
> > >   if (event_) {  // instead of IsTracingOn()
> > >     ...
> > >   }
> > > }
> > > 
> > > Seems like the best way as it only stack allocates 1 size_t, 1 pointer
(only
> > set
> > > once), and 1 empty string (maybe 1 pointer? not sure).
> > > 
> > > Is this sufficient?  We could try to employ some hacks with heap
allocation
> or
> > > use const refs because these are all used in the same scope but these seem
> > like
> > > premature optimizations (and more prone to use-after-free).
> > > 
> > > I agree we shouldn't copy and/or convert the path from UTF-8 (which will
be
> > from
> > > wide on windows, I believe).
> > 
> > Tracing is enabled at runtime and included in all builds. You can check if
> > tracing is enabled via TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED.
> > 
> > However, note that TRACE_EVENT() is already scoped and won't evaluate its
> > arguments unless tracing is enabled, so it seems like we could just replace
> > ScopedFileTrace with a corresponding TRACE_EVENT macro invocation like most
> > other subsystems do. WDYT?
> 
> We could if we want to duplicate some macros or rewrite base::File to have
less
> if (...) { return ...; } code paths.  there's about 16 of these currently.

I'm not sure I follow? TRACE_EVENT<N> works fine with early returns, e.g:

  {
    TRACE_EVENT0("category", "event");
    if (x > y) {
      return;
    }
    SomeOperation();
  }

The event will either be ended at the return statement or after SomeOperation()
depending on which branch is taken, similar to ScopedFiledTrace.

[Dan Beam, 2015-04-24 18:06:21]

On 2015/04/24 17:34:47, Sami wrote:
>
https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
> File base/trace_event/scoped_file_trace.cc (right):
> 
>
https://codereview.chromium.org/1072133006/diff/160001/base/trace_event/scope...
> base/trace_event/scoped_file_trace.cc:17: : path_(path.AsUTF8Unsafe()),
> event_(event), bytes_(bytes) {
> On 2015/04/24 17:29:50, Dan Beam wrote:
> > On 2015/04/24 16:45:41, Sami wrote:
> > > On 2015/04/24 16:37:19, Dan Beam wrote:
> > > > On 2015/04/24 10:42:55, Sami wrote:
> > > > > This causes some string copying and memory allocation churn even if
> > tracing
> > > is
> > > > > disabled. Could we avoid that somehow?
> > > > 
> > > > How is tracing disabled? At compile time or runtime?
> > > > 
> > > > There's a couple things we could do. Compile-time:
> > > > 
> > > > #if !defined(ENABLE_TRACING)
> > > > 
> > > > ScopedFileTrace::ScopedFileTrace(
> > > >     const FilePath& path, const char* event, size_t bytes) {}
> > > > ScopedFileTrace::~ScopedFileTrace() {}
> > > > 
> > > > #else
> > > > ... functional code ...
> > > > #endif
> > > > 
> > > > would probably be the easiest if this gets stripped out by the compiler.
> > > > 
> > > > Runtime:
> > > > 
> > > > ScopedFileTrace::ScopedFileTrace(
> > > >     const FilePath& path, const char* event, size_t bytes) {
> > > >   if (IsTracingOn()) {
> > > >     event_ = event;
> > > >     ...
> > > >   } else {
> > > >     event_ = nullptr;
> > > >   }
> > > > }
> > > > ScopedFileTrace::~ScopedFileTrace() {
> > > >   if (event_) {  // instead of IsTracingOn()
> > > >     ...
> > > >   }
> > > > }
> > > > 
> > > > Seems like the best way as it only stack allocates 1 size_t, 1 pointer
> (only
> > > set
> > > > once), and 1 empty string (maybe 1 pointer? not sure).
> > > > 
> > > > Is this sufficient?  We could try to employ some hacks with heap
> allocation
> > or
> > > > use const refs because these are all used in the same scope but these
seem
> > > like
> > > > premature optimizations (and more prone to use-after-free).
> > > > 
> > > > I agree we shouldn't copy and/or convert the path from UTF-8 (which will
> be
> > > from
> > > > wide on windows, I believe).
> > > 
> > > Tracing is enabled at runtime and included in all builds. You can check if
> > > tracing is enabled via TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED.
> > > 
> > > However, note that TRACE_EVENT() is already scoped and won't evaluate its
> > > arguments unless tracing is enabled, so it seems like we could just
replace
> > > ScopedFileTrace with a corresponding TRACE_EVENT macro invocation like
most
> > > other subsystems do. WDYT?
> > 
> > We could if we want to duplicate some macros or rewrite base::File to have
> less
> > if (...) { return ...; } code paths.  there's about 16 of these currently.
> 
> I'm not sure I follow? TRACE_EVENT<N> works fine with early returns, e.g:
> 
>   {
>     TRACE_EVENT0("category", "event");
>     if (x > y) {
>       return;
>     }
>     SomeOperation();
>   }
> 
> The event will either be ended at the return statement or after
SomeOperation()
> depending on which branch is taken, similar to ScopedFiledTrace.

oh, if we don't need BEGIN/END, sure that'll work.

[Dan Beam, 2015-04-24 22:27:02]

updated to just use TRACE_EVENT*() macros

https://codereview.chromium.org/1072133006/diff/190001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1072133006/diff/190001/base/files/file.cc#new...
base/files/file.cc:75: "path", path_.AsUTF8Unsafe());
should I be passing along the path here?

[Dan Beam, 2015-04-25 05:21:47]

dbeam@chromium.org changed reviewers:
- skyostil@chromium.org, thestig@chromium.org

[Dan Beam, 2015-04-25 05:22:04]

dbeam@chromium.org changed reviewers:
+ thestig@chromium.org
- nduca@chromium.org

[Dan Beam, 2015-04-25 05:24:45]

Moving nduca@ and skyostil@ to CC

Ping thestig@

[Lei Zhang, 2015-04-27 22:55:55]

https://codereview.chromium.org/1072133006/diff/190001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1072133006/diff/190001/base/files/file.cc#new...
base/files/file.cc:74: TRACE_EVENT_ASYNC_END1(kTraceGroup, "File", this,
We may not always call TRACE_EVENT_ASYNC_BEGIN1() in Initialize(), would that
mismatch cause trouble here?

https://codereview.chromium.org/1072133006/diff/190001/base/files/file.cc#new...
base/files/file.cc:83: created_ = other.object->created();
Set |path_| here too?

https://codereview.chromium.org/1072133006/diff/190001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/1072133006/diff/190001/base/files/file_posix....
base/files/file_posix.cc:585: #else
how about:

{
#if defined(OS_NACL)
...
return;
#elif defined(OS_LINUX) || defined(OS_ANDROID)
#define FLUSHFUNC fdatasync
#else
#define FLUSHFUNC fsync
#endif
TRACE_EVENT()
return !HANDLE_EINTR(FLUSHFUNC(file_.get()));
}

[Dan Beam, 2015-04-28 03:17:11]

https://codereview.chromium.org/1072133006/diff/190001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1072133006/diff/190001/base/files/file.cc#new...
base/files/file.cc:74: TRACE_EVENT_ASYNC_END1(kTraceGroup, "File", this,
On 2015/04/27 22:55:55, Lei Zhang wrote:
> We may not always call TRACE_EVENT_ASYNC_BEGIN1() in Initialize(), would that
> mismatch cause trouble here?

not sure but just made this TRACE_EVENT_ASYNC_{BEGIN/END}0 instead and just
passing |this| (omitting path_)

https://codereview.chromium.org/1072133006/diff/190001/base/files/file.cc#new...
base/files/file.cc:83: created_ = other.object->created();
On 2015/04/27 22:55:55, Lei Zhang wrote:
> Set |path_| here too?

Done.

https://codereview.chromium.org/1072133006/diff/190001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/1072133006/diff/190001/base/files/file_posix....
base/files/file_posix.cc:585: #else
On 2015/04/27 22:55:55, Lei Zhang wrote:
> how about:
> 
> {
> #if defined(OS_NACL)
> ...
> return;
> #elif defined(OS_LINUX) || defined(OS_ANDROID)
> #define FLUSHFUNC fdatasync
> #else
> #define FLUSHFUNC fsync
> #endif
> TRACE_EVENT()
> return !HANDLE_EINTR(FLUSHFUNC(file_.get()));
> }

Done.

[Lei Zhang, 2015-04-28 03:53:27]

It'll probably be good to get the blessing from some tracing folks, but
otherwise looks good once you undo my bad advice.

https://codereview.chromium.org/1072133006/diff/190001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/1072133006/diff/190001/base/files/file_posix....
base/files/file_posix.cc:585: #else
On 2015/04/28 03:17:11, Dan Beam wrote:
> On 2015/04/27 22:55:55, Lei Zhang wrote:
> > how about:
> > 
> > {
> > #if defined(OS_NACL)
> > ...
> > return;
> > #elif defined(OS_LINUX) || defined(OS_ANDROID)
> > #define FLUSHFUNC fdatasync
> > #else
> > #define FLUSHFUNC fsync
> > #endif
> > TRACE_EVENT()
> > return !HANDLE_EINTR(FLUSHFUNC(file_.get()));
> > }
> 
> Done.

Oh, sorry for the bad advice. OS_NACL will still try to compile the code even
though it's not reachable.

[Dan Beam, 2015-04-28 04:04:10]

Patchset #4 (id:210001) has been deleted

[Dan Beam, 2015-04-28 04:04:15]

Patchset #4 (id:230001) has been deleted

[Dan Beam, 2015-04-28 04:06:10]

https://codereview.chromium.org/1072133006/diff/190001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/1072133006/diff/190001/base/files/file_posix....
base/files/file_posix.cc:585: #else
On 2015/04/28 03:53:26, Lei Zhang wrote:
> On 2015/04/28 03:17:11, Dan Beam wrote:
> > On 2015/04/27 22:55:55, Lei Zhang wrote:
> > > how about:
> > > 
> > > {
> > > #if defined(OS_NACL)
> > > ...
> > > return;
> > > #elif defined(OS_LINUX) || defined(OS_ANDROID)
> > > #define FLUSHFUNC fdatasync
> > > #else
> > > #define FLUSHFUNC fsync
> > > #endif
> > > TRACE_EVENT()
> > > return !HANDLE_EINTR(FLUSHFUNC(file_.get()));
> > > }
> > 
> > Done.
> 
> Oh, sorry for the bad advice. OS_NACL will still try to compile the code even
> though it's not reachable.

Reverted back to this code.

[Lei Zhang, 2015-04-28 04:22:29]

lgtm

[Sami, 2015-04-28 11:10:38]

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

[Sami, 2015-04-28 11:10:39]

Tracing part lgtm -- assuming this produces useful looking traces :)

https://codereview.chromium.org/1072133006/diff/250001/base/files/file.h
File base/files/file.h (right):

https://codereview.chromium.org/1072133006/diff/250001/base/files/file.h#newc...
base/files/file.h:308: static const char kTraceGroup[];
nit: Usually we just write the trace category explicitly in each trace event. It
makes it easier to grep where trace events are coming from, and the compiler
will fold the identical strings into one anyway.

[Dan Beam, 2015-04-28 15:47:58]

https://codereview.chromium.org/1072133006/diff/250001/base/files/file.h
File base/files/file.h (right):

https://codereview.chromium.org/1072133006/diff/250001/base/files/file.h#newc...
base/files/file.h:308: static const char kTraceGroup[];
On 2015/04/28 11:10:39, Sami wrote:
> nit: Usually we just write the trace category explicitly in each trace event.
It
> makes it easier to grep where trace events are coming from, and the compiler
> will fold the identical strings into one anyway.

I guess I can do this, it's just... TRACE_DISABLED_BY_DEFAULT("file") is pretty
long.

[Sami, 2015-04-28 15:51:05]

https://codereview.chromium.org/1072133006/diff/250001/base/files/file.h
File base/files/file.h (right):

https://codereview.chromium.org/1072133006/diff/250001/base/files/file.h#newc...
base/files/file.h:308: static const char kTraceGroup[];
On 2015/04/28 15:47:58, Dan Beam wrote:
> On 2015/04/28 11:10:39, Sami wrote:
> > nit: Usually we just write the trace category explicitly in each trace
event.
> It
> > makes it easier to grep where trace events are coming from, and the compiler
> > will fold the identical strings into one anyway.
> 
> I guess I can do this, it's just... TRACE_DISABLED_BY_DEFAULT("file") is
pretty
> long.

I don't think it's a hard rule, so I'll leave it up to you to decide.

[groby-ooo-7-16, 2015-04-28 20:26:55]

Drive-by, sorry :)

https://codereview.chromium.org/1072133006/diff/250001/base/files/file.h
File base/files/file.h (right):

https://codereview.chromium.org/1072133006/diff/250001/base/files/file.h#newc...
base/files/file.h:308: static const char kTraceGroup[];
On 2015/04/28 15:51:05, Sami wrote:
> On 2015/04/28 15:47:58, Dan Beam wrote:
> > On 2015/04/28 11:10:39, Sami wrote:
> > > nit: Usually we just write the trace category explicitly in each trace
> event.
> > It
> > > makes it easier to grep where trace events are coming from, and the
compiler
> > > will fold the identical strings into one anyway.
> > 
> > I guess I can do this, it's just... TRACE_DISABLED_BY_DEFAULT("file") is
> pretty
> > long.
> 
> I don't think it's a hard rule, so I'll leave it up to you to decide.

I don't have a specific preference, but consider naming it kFileTraceGroup -
this way we can still grep somewhat reliably.

https://codereview.chromium.org/1072133006/diff/250001/base/files/file.h#newc...
base/files/file.h:370: 
|unsafe_path_|, then?

https://codereview.chromium.org/1072133006/diff/250001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/1072133006/diff/250001/base/files/file_posix....
base/files/file_posix.cc:204: TRACE_EVENT2(kTraceGroup, "Read", "path",
path_.AsUTF8Unsafe(), "size", size);
Given that you call this every read/seek/etc, might be worth storing the UTF8
path name.

[Dan Beam, 2015-04-29 02:48:02]

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

[Dan Beam, 2015-04-29 02:48:03]

+nduca for base/trace_event/OWNERS

skyostil/nduca: what do you think about TRACE_EVENT_ID*()?

https://codereview.chromium.org/1072133006/diff/250001/base/files/file.h
File base/files/file.h (right):

https://codereview.chromium.org/1072133006/diff/250001/base/files/file.h#newc...
base/files/file.h:308: static const char kTraceGroup[];
On 2015/04/28 20:26:54, groby wrote:
> On 2015/04/28 15:51:05, Sami wrote:
> > On 2015/04/28 15:47:58, Dan Beam wrote:
> > > On 2015/04/28 11:10:39, Sami wrote:
> > > > nit: Usually we just write the trace category explicitly in each trace
> > event.
> > > It
> > > > makes it easier to grep where trace events are coming from, and the
> compiler
> > > > will fold the identical strings into one anyway.
> > > 
> > > I guess I can do this, it's just... TRACE_DISABLED_BY_DEFAULT("file") is
> > pretty
> > > long.
> > 
> > I don't think it's a hard rule, so I'll leave it up to you to decide.
> 
> I don't have a specific preference, but consider naming it kFileTraceGroup -
> this way we can still grep somewhat reliably.

  $ git grep \\bkTraceGroup\\b

yields only these results reliably

https://codereview.chromium.org/1072133006/diff/250001/base/files/file.h#newc...
base/files/file.h:370: 
On 2015/04/28 20:26:54, groby wrote:
> |unsafe_path_|, then?

Done.

https://codereview.chromium.org/1072133006/diff/250001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/1072133006/diff/250001/base/files/file_posix....
base/files/file_posix.cc:204: TRACE_EVENT2(kTraceGroup, "Read", "path",
path_.AsUTF8Unsafe(), "size", size);
On 2015/04/28 20:26:55, groby wrote:
> Given that you call this every read/seek/etc, might be worth storing the UTF8
> path name.

this is only called while tracing (in which case all of chrome is equally
slower)

[Sami, 2015-04-29 15:01:51]

https://codereview.chromium.org/1072133006/diff/290001/base/trace_event/trace...
File base/trace_event/trace_event.h (right):

https://codereview.chromium.org/1072133006/diff/290001/base/trace_event/trace...
base/trace_event/trace_event.h:244: category_group, name, id, arg1_name,
arg1_val)
It's not clear what |id| refers to here. Does it need to match the ASYNC id?

Could you upload a sample trace with this data in it just to get an idea of how
this looks? I'm wondering if the NESTABLE_ASYNC event would be more appropriate.

[nduca, 2015-04-29 15:59:34]

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

[nduca, 2015-04-29 15:59:35]

i'm pretty confused what the ids are being used for. i still recommend using
async events. oysteine might be able to advise since i'm a bit swamped this
week.

[Dan Beam, 2015-04-29 18:12:10]

On 2015/04/29 15:59:35, nduca wrote:
> i'm pretty confused what the ids are being used for. i still recommend using
> async events. oysteine might be able to advise since i'm a bit swamped this
> week.

This CL /does/ use async events (see: base/files/file.cc).

Excuse the pseudo-code (written to save you time):

  File::File() { ASYNC_BEGIN(this); }
  File::~File() { ASYNC_END(this); }

Now for every:

  File::Read() { ... }

I'd like to associate with the same "id" (i.e. the same file instance), but
doing this:

  File::Read() {
    ASYNC_BEGIN(this, "Read");
    if (!cool_) {
      ASYNC_END(this, "Read");
      return;
    }
    ...
    if (!nice_) {
      ASYNC_END(this, "Read");
      return;
    }
    ...
    ASYNC_END(this, "Read");
  }

doesn't seem nearly as cool as relying on a scoped trace that can associate to
an ID.

[oystein (OOO til 10th of July), 2015-04-29 18:12:18]

On 2015/04/29 15:59:35, nduca wrote:
> i'm pretty confused what the ids are being used for. i still recommend using
> async events. oysteine might be able to advise since i'm a bit swamped this
> week.

Looks to me like this is actually, in practice, doing what the NESTABLE_ASYNC_*
APIs are supposed to be used for.

The main new thing which (I think) hasn't been done before is that you're doing
scoped events nested within the top level async one.

dbeam: In practice this just means doing what you're already doing, but with a
slightly different API. I think you want
TRACE_EVENT_NESTABLE_ASYNC_BEGIN0/TRACE_EVENT_NESTABLE_ASYNC_END0 pairs in the
File constructors/destructors, and then add TRACE_EVENT_NESTABLE1/2 instead of
TRACE_EVENT_ID1/2.

[Dan Beam, 2015-04-29 18:15:55]

On 2015/04/29 18:12:18, Oystein wrote:
> On 2015/04/29 15:59:35, nduca wrote:
> > i'm pretty confused what the ids are being used for. i still recommend using
> > async events. oysteine might be able to advise since i'm a bit swamped this
> > week.
> 
> Looks to me like this is actually, in practice, doing what the
NESTABLE_ASYNC_*
> APIs are supposed to be used for.
> 
> The main new thing which (I think) hasn't been done before is that you're
doing
> scoped events nested within the top level async one.

exactly

> 
> dbeam: In practice this just means doing what you're already doing, but with a
> slightly different API. I think you want
> TRACE_EVENT_NESTABLE_ASYNC_BEGIN0/TRACE_EVENT_NESTABLE_ASYNC_END0 pairs in the
> File constructors/destructors, and then add TRACE_EVENT_NESTABLE1/2 instead of
> TRACE_EVENT_ID1/2.

alright, I'll change from TRACE_EVENT_ID* -> TRACE_EVENT_NESTABLE* (probably
just a rename and slightly tweaking to my new macros).

[Dan Beam, 2015-04-30 04:08:50]

dbeam@chromium.org changed reviewers:
- oysteine@chromium.org

[Dan Beam, 2015-04-30 04:31:44]

Patchset #7 (id:310001) has been deleted

[Dan Beam, 2015-04-30 04:35:48]

Patchset #7 (id:330001) has been deleted

[Dan Beam, 2015-04-30 04:40:13]

ok, let's try again by adding a scoped NESTABLE macro this time.

this requires a viewer change[1], so sharing a trace isn't as useful as you'd
still have to rebuild :(.

[1] https://codereview.appspot.com/234940043/

https://codereview.chromium.org/1072133006/diff/290001/base/trace_event/trace...
File base/trace_event/trace_event.h (right):

https://codereview.chromium.org/1072133006/diff/290001/base/trace_event/trace...
base/trace_event/trace_event.h:244: category_group, name, id, arg1_name,
arg1_val)
On 2015/04/29 15:01:51, Sami wrote:
> It's not clear what |id| refers to here. Does it need to match the ASYNC id?

yes

> 
> Could you upload a sample trace with this data in it just to get an idea of
how
> this looks? I'm wondering if the NESTABLE_ASYNC event would be more
appropriate.

it probably is.  shared a TRACE_EVENT_ID* version with everybody (before I try
TRACE_EVENT_NESTABLE*).

https://codereview.chromium.org/1072133006/diff/350001/base/trace_event/trace...
File base/trace_event/trace_event.h (right):

https://codereview.chromium.org/1072133006/diff/350001/base/trace_event/trace...
base/trace_event/trace_event.h:1578: ScopedTracer() : initialized_(false) {}
forgot i did this. can revert if you want...

[nduca, 2015-04-30 15:28:21]

i don't think we actually need the complete async event. we can make a scoped
object that issues async begin and end events inside its constructor/destructor
instead. it means making a separate class than what does the regular sync events
but it's simpler.

the only reason for the regular complete event is because there are so many
begin-end events in a trace, adding the complete event saves bytes in the trace.
i would prefer we only add more event phases when we absolutely need them.

[Dan Beam, 2015-04-30 16:55:20]

dbeam@chromium.org changed reviewers:
- skyostil@chromium.org

[Dan Beam, 2015-04-30 21:05:02]

Patchset #8 (id:370001) has been deleted

[Dan Beam, 2015-04-30 21:10:05]

Patchset #8 (id:390001) has been deleted

[Dan Beam, 2015-04-30 21:14:19]

dbeam@chromium.org changed reviewers:
+ oysteine@chromium.org

[Dan Beam, 2015-04-30 21:14:20]

+oysteine@ to review for nduca@

On 2015/04/30 15:28:21, nduca wrote:
> i don't think we actually need the complete async event. we can make a scoped
> object that issues async begin and end events inside its
constructor/destructor
> instead. it means making a separate class than what does the regular sync
events
> but it's simpler.

Done.

> 
> the only reason for the regular complete event is because there are so many
> begin-end events in a trace, adding the complete event saves bytes in the
trace.
> i would prefer we only add more event phases when we absolutely need them.

Removed and closed viewer CL.

[oystein (OOO til 10th of July), 2015-05-01 18:15:31]

I think this is looking pretty good now; minor points below.

https://codereview.chromium.org/1072133006/diff/410001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1072133006/diff/410001/base/files/file.cc#new...
base/files/file.cc:83: unsafe_path_ = path;
What's the reason for setting this here, instead of setting it DoInitialize or
at least after the ReferencesParent() check? I'm worried about codepaths popping
up where the path gets used despite of Initialize() failing, not setting the
member at all seems like a good layered defense.

https://codereview.chromium.org/1072133006/diff/410001/base/files/file_tracing.h
File base/files/file_tracing.h (right):

https://codereview.chromium.org/1072133006/diff/410001/base/files/file_tracin...
base/files/file_tracing.h:12: #define FILE_TRACING_PREFIX "base::File"
I'm not sure if "base" is really adding any useful information to these events,
but maybe I'm missing something. What was the reason for adding this?

https://codereview.chromium.org/1072133006/diff/410001/base/files/file_tracin...
base/files/file_tracing.h:24: bool category_enabled; \
nit: call the scope something other than "trace" (prefix with an underscore
maybe?) and wrap the rest in {} so the macro doesn't pollute the function
namespace unnecessarily.

[oystein (OOO til 10th of July), 2015-05-01 18:21:05]

https://codereview.chromium.org/1072133006/diff/410001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1072133006/diff/410001/base/files/file.cc#new...
base/files/file.cc:25: FILE_TRACING_BEGIN();
It could be cleaner to do these BEGIN/END calls in a class member instead of
duplicating them in all the constructors.

[Dan Beam, 2015-05-01 22:52:09]

Patchset #9 (id:430001) has been deleted

[Dan Beam, 2015-05-01 23:29:04]

https://codereview.chromium.org/1072133006/diff/410001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/1072133006/diff/410001/base/files/file.cc#new...
base/files/file.cc:25: FILE_TRACING_BEGIN();
On 2015/05/01 18:21:05, Oystein wrote:
> It could be cleaner to do these BEGIN/END calls in a class member instead of
> duplicating them in all the constructors.

Done.

https://codereview.chromium.org/1072133006/diff/410001/base/files/file.cc#new...
base/files/file.cc:83: unsafe_path_ = path;
On 2015/05/01 18:15:31, Oystein wrote:
> What's the reason for setting this here, instead of setting it DoInitialize or
> at least after the ReferencesParent() check? I'm worried about codepaths
popping
> up where the path gets used despite of Initialize() failing, not setting the
> member at all seems like a good layered defense.

Done. (originally my rationale was: it'd be weird to do thing.Initialize(path)
-> thing.GetPath() be different, but path_ is never exposed)

https://codereview.chromium.org/1072133006/diff/410001/base/files/file_tracing.h
File base/files/file_tracing.h (right):

https://codereview.chromium.org/1072133006/diff/410001/base/files/file_tracin...
base/files/file_tracing.h:12: #define FILE_TRACING_PREFIX "base::File"
On 2015/05/01 18:15:31, Oystein wrote:
> I'm not sure if "base" is really adding any useful information to these
events,
> but maybe I'm missing something. What was the reason for adding this?

just to match code, removed (noticed many places omit namespace)

https://codereview.chromium.org/1072133006/diff/410001/base/files/file_tracin...
base/files/file_tracing.h:24: bool category_enabled; \
On 2015/05/01 18:15:31, Oystein wrote:
> nit: call the scope something other than "trace" (prefix with an underscore
> maybe?) and wrap the rest in {} so the macro doesn't pollute the function
> namespace unnecessarily.

Done.

[Dan Beam, 2015-05-01 23:29:46]

Patchset #10 (id:470001) has been deleted

[oystein (OOO til 10th of July), 2015-05-02 01:21:28]

On 2015/05/01 23:29:04, Dan Beam wrote:
> https://codereview.chromium.org/1072133006/diff/410001/base/files/file.cc
> File base/files/file.cc (right):
> 
>
https://codereview.chromium.org/1072133006/diff/410001/base/files/file.cc#new...
> base/files/file.cc:25: FILE_TRACING_BEGIN();
> On 2015/05/01 18:21:05, Oystein wrote:
> > It could be cleaner to do these BEGIN/END calls in a class member instead of
> > duplicating them in all the constructors.
> 
> Done.
> 
>
https://codereview.chromium.org/1072133006/diff/410001/base/files/file.cc#new...
> base/files/file.cc:83: unsafe_path_ = path;
> On 2015/05/01 18:15:31, Oystein wrote:
> > What's the reason for setting this here, instead of setting it DoInitialize
or
> > at least after the ReferencesParent() check? I'm worried about codepaths
> popping
> > up where the path gets used despite of Initialize() failing, not setting the
> > member at all seems like a good layered defense.
> 
> Done. (originally my rationale was: it'd be weird to do thing.Initialize(path)
> -> thing.GetPath() be different, but path_ is never exposed)
> 
>
https://codereview.chromium.org/1072133006/diff/410001/base/files/file_tracing.h
> File base/files/file_tracing.h (right):
> 
>
https://codereview.chromium.org/1072133006/diff/410001/base/files/file_tracin...
> base/files/file_tracing.h:12: #define FILE_TRACING_PREFIX "base::File"
> On 2015/05/01 18:15:31, Oystein wrote:
> > I'm not sure if "base" is really adding any useful information to these
> events,
> > but maybe I'm missing something. What was the reason for adding this?
> 
> just to match code, removed (noticed many places omit namespace)
> 
>
https://codereview.chromium.org/1072133006/diff/410001/base/files/file_tracin...
> base/files/file_tracing.h:24: bool category_enabled; \
> On 2015/05/01 18:15:31, Oystein wrote:
> > nit: call the scope something other than "trace" (prefix with an underscore
> > maybe?) and wrap the rest in {} so the macro doesn't pollute the function
> > namespace unnecessarily.
> 
> Done.

This looks pretty solid to me :). non-owner lgtm!

[nduca, 2015-05-04 16:49:16]

lgtm but you'll need some base/ owner

[Dan Beam, 2015-05-04 16:49:45]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2015-05-04 16:49:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1072133006/#ps490001
(title: "remove some ; from end of macros")

[commit-bot: I haz the power, 2015-05-04 16:50:06]

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

[commit-bot: I haz the power, 2015-05-04 18:16:41]

Committed patchset #10 (id:490001)

[commit-bot: I haz the power, 2015-05-04 18:18:34]

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

[Dan Beam, 2015-05-05 00:49:50]

A revert of this CL (patchset #10 id:490001) has been created in
https://codereview.chromium.org/1127713004/ by dbeam@chromium.org.

The reason for reverting is: See if this broke sizes by increasing setup.exe by
0.43%..

[Dan Beam, 2015-05-05 22:25:04]

Patchset #12 (id:530001) has been deleted

[Dan Beam, 2015-05-06 21:57:05]

Patchset #15 (id:610001) has been deleted

[Dan Beam, 2015-05-06 21:57:20]

Patchset #14 (id:590001) has been deleted

[Dan Beam, 2015-05-06 21:57:27]

Patchset #13 (id:570001) has been deleted

[Dan Beam, 2015-05-06 21:57:37]

Patchset #12 (id:550001) has been deleted

[Dan Beam, 2015-05-06 21:57:44]

Patchset #11 (id:510001) has been deleted

[Dan Beam, 2015-05-07 00:12:09]

Patchset #11 (id:630001) has been deleted

[Dan Beam, 2015-05-07 05:01:01]

Patchset #11 (id:650001) has been deleted

[Dan Beam, 2015-05-07 05:19:39]

Patchset #12 (id:690001) has been deleted

[Dan Beam, 2015-05-07 05:19:50]

Patchset #11 (id:670001) has been deleted

[Dan Beam, 2015-05-07 05:26:37]

Patchset #11 (id:710001) has been deleted

[Dan Beam, 2015-05-07 05:29:29]

oysteine@: how 'bout this?

[Dan Beam, 2015-05-07 05:40:36]

Patchset #11 (id:730001) has been deleted

[Dan Beam, 2015-05-07 05:41:22]

Patchset #12 (id:770001) has been deleted

[oystein (OOO til 10th of July), 2015-05-07 23:31:54]

On 2015/05/07 05:29:29, Dan Beam wrote:
> oysteine@: how 'bout this?

Tracing part still lgtm from me, but I think the base/ part has changed enough
that skyostil@/thestig@ may want to glance over it again.

[Lei Zhang, 2015-05-08 17:40:19]

https://codereview.chromium.org/1072133006/diff/750001/base/files/file_tracing.h
File base/files/file_tracing.h (right):

https://codereview.chromium.org/1072133006/diff/750001/base/files/file_tracin...
base/files/file_tracing.h:52: static Provider* g_provider;
Can you have a static SetProvider() method instead of exposing this?

https://codereview.chromium.org/1072133006/diff/750001/base/files/file_tracin...
base/files/file_tracing.h:67: bool ShouldInitialize() const;
Any reason to have this separated from Initialize()?

[Dan Beam, 2015-05-08 18:54:03]

https://codereview.chromium.org/1072133006/diff/750001/base/files/file_tracing.h
File base/files/file_tracing.h (right):

https://codereview.chromium.org/1072133006/diff/750001/base/files/file_tracin...
base/files/file_tracing.h:52: static Provider* g_provider;
On 2015/05/08 17:40:19, Lei Zhang wrote:
> Can you have a static SetProvider() method instead of exposing this?

Done.

https://codereview.chromium.org/1072133006/diff/750001/base/files/file_tracin...
base/files/file_tracing.h:67: bool ShouldInitialize() const;
On 2015/05/08 17:40:19, Lei Zhang wrote:
> Any reason to have this separated from Initialize()?

less work is done in the common case that the category isn't enabled (only the
category is checked and 1 boolean is initialized).

also, it'd be odd to call Initialize() and have initialized_ be false later.  i
could change things around, but I don't see why it's better...

[Lei Zhang, 2015-05-08 19:30:30]

lgtm

https://codereview.chromium.org/1072133006/diff/790001/content/browser/tracin...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/1072133006/diff/790001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl.cc:61:
base::FileTracing::SetProvider(new FileTracingProviderImpl);
Can you mention you are deliberately leaking this?

[Dan Beam, 2015-05-08 20:52:31]

https://codereview.chromium.org/1072133006/diff/790001/content/browser/tracin...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/1072133006/diff/790001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl.cc:61:
base::FileTracing::SetProvider(new FileTracingProviderImpl);
On 2015/05/08 19:30:30, Lei Zhang wrote:
> Can you mention you are deliberately leaking this?

Done.

[Dan Beam, 2015-05-08 20:52:46]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2015-05-08 20:52:46]

The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org,
skyostil@chromium.org, oysteine@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1072133006/#ps810001
(title: "comment")

[commit-bot: I haz the power, 2015-05-08 20:54:12]

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

[commit-bot: I haz the power, 2015-05-09 05:05:14]

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

[commit-bot: I haz the power, 2015-05-09 05:05:16]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Lei Zhang, 2015-05-09 09:32:21]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-05-09 09:33:59]

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

[commit-bot: I haz the power, 2015-05-09 13:39:37]

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

[commit-bot: I haz the power, 2015-05-09 13:39:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)

[Dan Beam, 2015-05-11 05:45:03]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2015-05-11 05:46:32]

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

[commit-bot: I haz the power, 2015-05-11 07:54:00]

Committed patchset #13 (id:810001)

[commit-bot: I haz the power, 2015-05-11 07:54:52]

Patchset 13 (id:??) landed as
https://crrev.com/492dc31b433656444c9c74213a133c19c8ebfa21
Cr-Commit-Position: refs/heads/master@{#329114}