Add synthetic delay testing framework

Add synthetic delay testing framework
    
This patch introduces a synthetic delay framework for testing. It will be used to measure animation smoothness and input latency under varying loads in different parts of the graphics and input pipelines.
    
The basic building block for synthetic delays is the delay point which is inserted to a function scope of interest, e.g.:
    
    TRACE_EVENT_SYNTHETIC_DELAY("cc.DrawAndSwap");
    
Testing code can then configure how long the enclosing scope of the delay point should take to execute. If the function completes faster than the configured duration, a busy loop is used to make up the difference.
    
BUG=307841, 324057
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242947

[Sami, 2013-11-04 18:56:34]

Hi Brian. Here's a rough cut of the synthetic delay framework we were talking
about. It needs tests and some cleanup, but it would still be great to hear what
you think about it.

[brianderson, 2013-11-05 03:12:05]

Neat! Thanks for putting this together, I think it will simplify much of the
plumbing.

Do you think we'll be able to use it to set different delays for the same parts
of the code in the Browser vs Renderer?

Do you think we can extend it so we can have a cycle of delays to simulate jank?

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc
File base/debug/synthetic_delay.cc (right):

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc...
base/debug/synthetic_delay.cc:41: AutoLock lock(lock_);
I don't think this lock is necessary, since it is only called by
SyntheticDelayController::GetOrCreateDelay, which already acquires its own
higher-level lock.

Also, the other methods of SyntheticDelay that acquire the lock will not run
concurrently with this method.

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc...
base/debug/synthetic_delay.cc:63: scoped_ptr<ThreadState> thread_state(new
ThreadState());
Can you avoid allocating a new ThreadState for every Begin? Or is there a
concern that we might end up leaking many ThreadStates (whereas we only leak one
SyntheticDelayController singleton)?

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc...
base/debug/synthetic_delay.cc:77: base::TimeTicks now = base::TimeTicks::Now();
Can you use HighResNow? For the scheduling tests, I think we'll care more about
precision than about overhead, especially since we will be mostly sleeping.

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc...
base/debug/synthetic_delay.cc:78: base::TimeDelta delay = target_duration_ -
(now - thread_state->start_time);
I think you should be able to AutoLock just the line above and remove the
AutoUnlock below.

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc...
base/debug/synthetic_delay.cc:91: LeakySingletonTraits<SyntheticDelayController>
>::get();
What does LeakySingletonTraits do?

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc...
base/debug/synthetic_delay.cc:96:
ANNOTATE_BENIGN_RACE(&delays_[i].target_duration_,
What does ANNOTATE_BENIGN_RACE do?

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc...
base/debug/synthetic_delay.cc:102: AutoLock lock(lock_);
I think you can avoid taking this lock in the common case (after
initialization), by copying the for loop below and pasting it before the lock.

You'll still need to keep the copy of the for loop below that runs with the lock
acquired, to avoid racing the initialization.

You'll also need to load and increment delay_count_ atomically.

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.h
File base/debug/synthetic_delay.h (right):

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.h#...
base/debug/synthetic_delay.h:102: class SyntheticDelayController {
Seems like you could move this class into the implementation file.

[Sami, 2013-11-05 18:53:26]

+nduca@

Thanks for the comments Brian. I'll do a new rev based on them but I wanted to
reply to some things first.

On 2013/11/05 03:12:05, Brian Anderson wrote:
> Do you think we'll be able to use it to set different delays for the same
parts
> of the code in the Browser vs Renderer?

I was thinking that could be handled by naming the delays differently based on
whether there's a parent compositor etc. Can you think of another way to do it
-- maybe based on process names somehow?
 
> Do you think we can extend it so we can have a cycle of delays to simulate
jank?

Totally, my idea was to have one-shot delays that would reset themselves after
getting applied once. Do you think we need more complicated cycles too?

[nduca, 2013-11-05 19:26:47]

I've scheduled a meeting for 9a tomorrow to come up with scheduler test cases.
We need to make this more precise.

[nduca, 2013-11-05 19:27:03]

https://codereview.chromium.org/53923005/diff/1/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/53923005/diff/1/base/base.gypi#newcode151
base/base.gypi:151: 'debug/synthetic_delay.cc',
rename to trace_event_synthetic_delay so i can lg it

[Sami, 2013-11-05 19:36:13]

On 2013/11/05 19:26:47, nduca wrote:
> I've scheduled a meeting for 9a tomorrow to come up with scheduler test cases.
> We need to make this more precise.

sgtm. There's a list of specific things we wanted to insert delays into here:
https://code.google.com/p/chromium/issues/detail?id=307841#c5. If we get each of
those running and make sure the numbers that come out of Yufeng's latency
tracker look like they should, does that give us enough coverage?

Another idea was to turn this around and see how long delays we can tolerate in
each part while having <1 vsync latency -- essentially a latency shit fitter.

[brianderson, 2013-11-06 15:29:42]

On 2013/11/05 18:53:26, Sami wrote:
> +nduca@
> 
> Thanks for the comments Brian. I'll do a new rev based on them but I wanted to
> reply to some things first.
> 
> On 2013/11/05 03:12:05, Brian Anderson wrote:
> > Do you think we'll be able to use it to set different delays for the same
> parts
> > of the code in the Browser vs Renderer?
> 
> I was thinking that could be handled by naming the delays differently based on
> whether there's a parent compositor etc. Can you think of another way to do it
> -- maybe based on process names somehow?

Naming the delays differently should work.

>  
> > Do you think we can extend it so we can have a cycle of delays to simulate
> jank?
> 
> Totally, my idea was to have one-shot delays that would reset themselves after
> getting applied once. Do you think we need more complicated cycles too?

Ok, cool. I think having more complicated cycles will help us test our "jank
recovery"
and long deadlines, for when there is a long delay in one frame followed by a
short delay in the next and we are in a high latency mode that should be able
to buffer and catch up on a jank.

One more concern I have with this method is that it will synchronously block the
delaying thread. If we wanted activation to take a specific amount of time, for
example,
I think we'd end up blocking the impl thread from doing other tasks like
drawing.
Will we need another method where we post a delayed task? Or is there a way
to delay the CanActivate notification for a specific amount of time from one of
the
raster threads?

[Dominik Grewe, 2013-11-07 16:45:53]

Just a thought...

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.h
File base/debug/synthetic_delay.h (right):

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.h#...
base/debug/synthetic_delay.h:11: // sleep until the deadline is met.
Would it be better to busy wait rather than to sleep, so that we're actually
occupying one of the cores?

[brianderson, 2013-11-18 21:04:06]

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.h
File base/debug/synthetic_delay.h (right):

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.h#...
base/debug/synthetic_delay.h:11: // sleep until the deadline is met.
On 2013/11/07 16:45:53, Dominik Grewe wrote:
> Would it be better to busy wait rather than to sleep, so that we're actually
> occupying one of the cores?

A sleep should work well if we are just testing the amount of concurrency the
scheduler can allow.

Too much concurrency can be bad if we just end up fighting for resources though.
Those resources aren't necessarily just CPU time, it could also be CPU bandwidth
or GPU time/bandwidth.

Testing the amount of concurrency the scheduler can allow will already be
difficult enough. I'm not sure if we want to increase the scope of the synthetic
scheduler tests to include resource fighting. I'm hoping we can rely on tests
with real content to give us feedback here. If real content isn't giving us
enough coverage, maybe then we can look into adding resource fighting to the
synthetic tests?

[Sami, 2013-11-22 18:29:13]

Thanks for the comments guys! I've changed this to use busy looping since it
better approximates the kind of delays we usually see in the places we're
testing.

I also changed the BEGIN/END semantics to ACTIVATE/APPLY, where ACTIVATE needs
to be called at least once before APPLY -- but only the first call has an
effect. This is to make it easier to integrate to places where we don't know the
exact workload in advance but want to apply a delay to a burst of work (e.g.,
raster tasks, texture uploads).

I've implemented all the test cases from the doc using this framework
(https://codereview.chromium.org/68203031/) so I think it's now got all we need.

https://codereview.chromium.org/53923005/diff/1/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/53923005/diff/1/base/base.gypi#newcode151
base/base.gypi:151: 'debug/synthetic_delay.cc',
On 2013/11/05 19:27:03, nduca wrote:
> rename to trace_event_synthetic_delay so i can lg it

Done.

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc
File base/debug/synthetic_delay.cc (right):

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc...
base/debug/synthetic_delay.cc:41: AutoLock lock(lock_);
On 2013/11/05 03:12:05, Brian Anderson wrote:
> I don't think this lock is necessary, since it is only called by
> SyntheticDelayController::GetOrCreateDelay, which already acquires its own
> higher-level lock.
> 
> Also, the other methods of SyntheticDelay that acquire the lock will not run
> concurrently with this method.

True, removed.

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc...
base/debug/synthetic_delay.cc:63: scoped_ptr<ThreadState> thread_state(new
ThreadState());
On 2013/11/05 03:12:05, Brian Anderson wrote:
> Can you avoid allocating a new ThreadState for every Begin? Or is there a
> concern that we might end up leaking many ThreadStates (whereas we only leak
one
> SyntheticDelayController singleton)?

Right, the choice is between avoiding the repeated allocations and leaking up to
#threads * kMaxSyntheticDelays structs. Since the allocations could in theory
disturb the code we're trying to test, maybe it's better just to leak the
structs.

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc...
base/debug/synthetic_delay.cc:77: base::TimeTicks now = base::TimeTicks::Now();
On 2013/11/05 03:12:05, Brian Anderson wrote:
> Can you use HighResNow? For the scheduling tests, I think we'll care more
about
> precision than about overhead, especially since we will be mostly sleeping.

Great point, changed.

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc...
base/debug/synthetic_delay.cc:78: base::TimeDelta delay = target_duration_ -
(now - thread_state->start_time);
On 2013/11/05 03:12:05, Brian Anderson wrote:
> I think you should be able to AutoLock just the line above and remove the
> AutoUnlock below.

Done.

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc...
base/debug/synthetic_delay.cc:91: LeakySingletonTraits<SyntheticDelayController>
>::get();
On 2013/11/05 03:12:05, Brian Anderson wrote:
> What does LeakySingletonTraits do?

It's for allocating a singleton class that gets leaked at program exit. The
allocation is annotated so that there's no false positive from leak tracking
tools.

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc...
base/debug/synthetic_delay.cc:96:
ANNOTATE_BENIGN_RACE(&delays_[i].target_duration_,
On 2013/11/05 03:12:05, Brian Anderson wrote:
> What does ANNOTATE_BENIGN_RACE do?

It's another hint for dynamic code analyzers -- mostly TSAN I think. It's
basically saying that there's a known data race here, but we know it is harmless
and don't want to flag it as an error.

Actually I think I should move this closer to the actual race in
TraceEventSyntheticDelay::Begin().

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.cc...
base/debug/synthetic_delay.cc:102: AutoLock lock(lock_);
On 2013/11/05 03:12:05, Brian Anderson wrote:
> I think you can avoid taking this lock in the common case (after
> initialization), by copying the for loop below and pasting it before the lock.
> 
> You'll still need to keep the copy of the for loop below that runs with the
lock
> acquired, to avoid racing the initialization.
> 
> You'll also need to load and increment delay_count_ atomically.

Great idea, done.

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.h
File base/debug/synthetic_delay.h (right):

https://codereview.chromium.org/53923005/diff/1/base/debug/synthetic_delay.h#...
base/debug/synthetic_delay.h:102: class SyntheticDelayController {
On 2013/11/05 03:12:05, Brian Anderson wrote:
> Seems like you could move this class into the implementation file.

I'll need access to GetOrCreateDelay() later configure the delays through IPC. I
think I'll just expose that one function though.

[nduca, 2013-11-24 19:36:40]

minor tweak and then lgtm

https://codereview.chromium.org/53923005/diff/230001/base/debug/trace_event_s...
File base/debug/trace_event_synthetic_delay.h (right):

https://codereview.chromium.org/53923005/diff/230001/base/debug/trace_event_s...
base/debug/trace_event_synthetic_delay.h:16: //  
TRACE_EVENT_SYNTHETIC_DELAY("cc.LayerTreeHost.DrawAndSwap");
i guess it seems possiblein the future that we'll want both busy and yielding
delays. I agree with not adding it now but how would you eventually add it to
this API? You should design that part up front.

for instance
T_E_SYNTHETIC_BUSYLOOP_DELAY
T_E_SYNTHETIC_SLEEPING_DELAY

[brianderson, 2013-11-26 02:48:34]

lgtm

https://codereview.chromium.org/53923005/diff/230001/base/debug/trace_event_s...
File base/debug/trace_event_synthetic_delay_unittest.cc (right):

https://codereview.chromium.org/53923005/diff/230001/base/debug/trace_event_s...
base/debug/trace_event_synthetic_delay_unittest.cc:83: EXPECT_LT(TestFunction(),
kTargetDurationMs);
EXPECT_EQ(TestFunction(), 0)?

[Sami, 2013-11-26 18:59:33]

> i guess it seems possiblein the future that we'll want both busy and yielding
> delays. I agree with not adding it now but how would you eventually add it to
> this API? You should design that part up front.
> 
> for instance
> T_E_SYNTHETIC_BUSYLOOP_DELAY
> T_E_SYNTHETIC_SLEEPING_DELAY

Yup, that's definitely something we should plan for. My idea was to make it
configurable per delay point, e.g.,

  delay->SetDelayMethod({SLEEP, BUSY_LOOP, MEMORY_THRASHING, WHATEVER});

This way you wouldn't need to decide upfront which points use which method.

> base/debug/trace_event_synthetic_delay_unittest.cc:83:
EXPECT_LT(TestFunction(),
> EXPECT_EQ(TestFunction(), 0)?

I wanted to make sure you can run these tests using real time too so the
expected delays are pretty loose. I think I'll tighten them a bit.

[Sami, 2013-11-26 19:09:13]

+jar@ for base/*.gyp* owners' review please.

[jar (doing other things), 2013-11-26 19:21:13]

base/*.gyp LGTM

[commit-bot: I haz the power, 2013-11-26 19:35:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/53923005/310001

[commit-bot: I haz the power, 2013-11-26 19:52:27]

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

[commit-bot: I haz the power, 2013-11-26 19:55:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/53923005/310001

[commit-bot: I haz the power, 2013-11-26 20:24:00]

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

[commit-bot: I haz the power, 2013-11-27 11:10:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/53923005/330001

[commit-bot: I haz the power, 2013-11-27 11:20:50]

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

[commit-bot: I haz the power, 2013-11-27 11:51:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/53923005/350001

[commit-bot: I haz the power, 2013-11-27 12:29:31]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, google_apis_unittests, gpu_unittests,
interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests,
net_unittests, ppapi_unittests, printing_unittests, sandbox_linux_unittests,
sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/53923005/370001

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

Change committed as 237580

[acleung1, 2013-11-27 21:01:56]

A revert of this CL has been created in
https://codereview.chromium.org/92623003/ by acleung@chromium.org.

The reason for reverting is: Breaks an official release bot.

[danakj, 2013-11-27 21:04:47]

Link to the failure please?


On Wed, Nov 27, 2013 at 4:01 PM, <acleung@chromium.org> wrote:

> A revert of this CL has been created in
> https://codereview.chromium.org/92623003/ by acleung@chromium.org.
>
> The reason for reverting is: Breaks an official release bot.
>
> https://codereview.chromium.org/53923005/
>

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

[brianderson, 2013-11-27 21:09:33]

On 2013/11/27 21:04:47, danakj wrote:
> Link to the failure please?
> 
> 
> On Wed, Nov 27, 2013 at 4:01 PM, <mailto:acleung@chromium.org> wrote:
> 
> > A revert of this CL has been created in
> > https://codereview.chromium.org/92623003/ by mailto:acleung@chromium.org.
> >
> > The reason for reverting is: Breaks an official release bot.
> >
> > https://codereview.chromium.org/53923005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Following the bug in the revert patch, the failure is being tracked here:

https://code.google.com/p/chromium/issues/detail?id=324057

https://chromegw.corp.google.com/i/clank/builders/release-official-clankium/b...

[Sami, 2013-11-28 13:09:05]

The problem was that we were running out of TLS slots on Android. The limit is
64 on Jellybean and 128 on KitKat which is why I didn't see the failure locally.

I've now changed the code to only allocate one TLS slot per thread (instead of
per delay point). PTAL.

[brianderson, 2013-12-02 20:37:20]

On 2013/11/28 13:09:05, Sami wrote:
> The problem was that we were running out of TLS slots on Android. The limit is
> 64 on Jellybean and 128 on KitKat which is why I didn't see the failure
locally.
> 
> I've now changed the code to only allocate one TLS slot per thread (instead of
> per delay point). PTAL.

Now that there is one TLS slot per thread instead of per delay point, does that
mean nested delays were possible before but not in the latest patch set? If so,
will we fail early when someone accidentally adds a nested delay?

I'm amazed that there is a hard-coded limit on the number of TLS slots
available. Is the limit a per-thread limit or is it a per-process limit? I
wonder if it would make sense to change base's ThreadLocalPointer so it only
uses one slot per thread.

[jar (doing other things), 2013-12-03 02:14:37]

On 2013/12/02 20:37:20, Brian Anderson wrote:
> On 2013/11/28 13:09:05, Sami wrote:
> > The problem was that we were running out of TLS slots on Android. The limit
is
> > 64 on Jellybean and 128 on KitKat which is why I didn't see the failure
> locally.
> > 
> > I've now changed the code to only allocate one TLS slot per thread (instead
of
> > per delay point). PTAL.
> 
> Now that there is one TLS slot per thread instead of per delay point, does
that
> mean nested delays were possible before but not in the latest patch set? If
so,
> will we fail early when someone accidentally adds a nested delay?
> 
> I'm amazed that there is a hard-coded limit on the number of TLS slots
> available. Is the limit a per-thread limit or is it a per-process limit? I
> wonder if it would make sense to change base's ThreadLocalPointer so it only
> uses one slot per thread.

re: hard coded limit.

The OS on each system I know of has a hard-coded limit on the number of TLS
slots.  The effort in Chrome is to avoid exhausting that OS limit, by only
"asking" the OS for one TLS slot.

..and then...

The Chromium TLS code will effectively (sub?)allocate slots within Chromium, and
can have (a different, and easily larger) static limit.  Yes, a static limit
once again, but at least it will stop being OS dependent.  

If we (on Chromium) are writing code that constantly allocates new TLS slots
(with no compile time determined limit), then there will (currently) be a
problem.  

Memory usage generally grows proportional to the number of slots, times the
number of threads.  We might be able to get away with something that lazily
grows the slot list on each thread (where a slots gets accessed)... but I don't
see a way to bring that size down as we free(?) TLS slots (as there is always
the risk that the slot is still being used on some other thread... that hasn't
had a chance to run and clean up).

I guess it is plausible to have a very inefficient "SlowTLS" slot that can
dynamically be grown/shrunk with effectively no limit... but I'm not that clear
on the use case and utility of it.

[Sami, 2013-12-03 11:41:18]

On 2013/12/02 20:37:20, Brian Anderson wrote:
> Now that there is one TLS slot per thread instead of per delay point, does
that
> mean nested delays were possible before but not in the latest patch set? If
so,
> will we fail early when someone accidentally adds a nested delay?

The switch to a single TLS slot per thread didn't change how the delays
themselves work. I've effectively just added a TLS indirection array like jar
described. Each delay point has an index to the array for its private data, and
each thread has a separate array (pointed to by TLS).

The way nested delays work is that instead Begin()/End() calls that need to be
balanced, there's Activate() that sets the start point for the delay and Apply()
that takes the start and current times and works out how long to block. Only the
first call to Activate() has an effect, and Apply() is a no-op unless the delay
was Activate()'d earlier.

The reason I ended up with this over Begin()/End() was to better match how
workloads like raster tasks and texture uploads are queued. We don't know ahead
of time how many raster tasks there will be per frame, so we don't want to apply
the delay to each one individually but rather to the entire set of tasks that
are needed for activation. So, we just call Activate() for each raster task and
then Apply() when it's time to activate.
 
> I'm amazed that there is a hard-coded limit on the number of TLS slots
> available. Is the limit a per-thread limit or is it a per-process limit? I
> wonder if it would make sense to change base's ThreadLocalPointer so it only
> uses one slot per thread.

Just to add to jar's great explanation, the reason there's a hard limit to TLS
slots (at least on Android) is the entire TLS array is statically allocated for
every thread up front. The larger the array, the more memory wasted per thread,
regardless of whether the slots are actually used. The posix minimum is 128
slots, but Jellybean only went up to 64 and we're already dangerously close to
that limit. Here's the bug for slimming TLS usage in Chrome:
https://code.google.com/p/chromium/issues/detail?id=264406

[Sami, 2013-12-04 16:24:47]

Any objections to relanding this?

[brianderson, 2013-12-10 02:09:17]

On 2013/12/04 16:24:47, Sami wrote:
> Any objections to relanding this?

Nope. lgtm - thanks for answering my questions earlier.

[commit-bot: I haz the power, 2013-12-10 14:36:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/53923005/390001

[commit-bot: I haz the power, 2013-12-10 14:50:29]

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

[Sami, 2013-12-10 15:30:05]

Oops, like like jar@ isn't an owner in base anymore. brettw@, mind having a look
at base/base.gyp{,i}?

[Sami, 2013-12-12 19:26:22]

brettw@: ping?

[Sami, 2013-12-17 16:54:20]

mark@, could you give me a rubber stamp for base/gyp{,i}? I'm not sure if
brettw@ is around.

[Sami, 2014-01-03 13:25:00]

Adding more owners for base/base.gyp and base/base.gypi. PTAL, thanks!

[Mark Mentovai, 2014-01-03 14:19:24]

LGTM. I only reviewed the .gyp and .gypi file. And the copyright boilerplate
junk.

https://codereview.chromium.org/53923005/diff/390001/base/debug/trace_event_s...
File base/debug/trace_event_synthetic_delay.h (right):

https://codereview.chromium.org/53923005/diff/390001/base/debug/trace_event_s...
base/debug/trace_event_synthetic_delay.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
New file, 2014 now. Applies to all of the new files in this change.

[Sami, 2014-01-03 14:40:08]

On 2014/01/03 14:19:24, Mark Mentovai wrote:
> LGTM. I only reviewed the .gyp and .gypi file. And the copyright boilerplate
> junk.
> 
>
https://codereview.chromium.org/53923005/diff/390001/base/debug/trace_event_s...
> File base/debug/trace_event_synthetic_delay.h (right):
> 
>
https://codereview.chromium.org/53923005/diff/390001/base/debug/trace_event_s...
> base/debug/trace_event_synthetic_delay.h:1: // Copyright (c) 2013 The Chromium
> Authors. All rights reserved.
> New file, 2014 now. Applies to all of the new files in this change.

Thanks, all comment blocks updated.

[commit-bot: I haz the power, 2014-01-03 14:40:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/53923005/450001

[commit-bot: I haz the power, 2014-01-03 15:25:13]

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

[commit-bot: I haz the power, 2014-01-03 15:42:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/53923005/450001

[commit-bot: I haz the power, 2014-01-03 16:21:10]

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

[commit-bot: I haz the power, 2014-01-03 21:33:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/53923005/450001

[commit-bot: I haz the power, 2014-01-03 22:37:41]

Change committed as 242947