[Mac] Maximise timer slack for background tabs

[Mac] Maximise timer slack for background tabs

When a tab not playing audio is sent to the background, set timer slack to its maximum value.

Support for setting timer slack is added at the MessageLoop level, the concrete implementation of this CL only affects CFMessagePump backed MessageLoops (which means just the main thread for backgrounded renderer processes at present).

The MessageLoop implementation is designed to support its use on Windows and Linux (the Windows API sets slack per-timer like the Mac one, while on Linux slack, is set per-thread via a call to prctl() using PR_SET_TIMERSLACK).

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

[jeremy, 2014-05-20 12:59:15]

Avi/Kari/Tony: Could you please take a quick look and let me know if you think
the approach taken by this CL looks right?  After an initial OK I'll add polish
and unit tests.

[tonyg, 2014-05-20 14:14:52]

I'm really excited to see this moving forward! Please treat my comments more as
questions than anything else. I'm not sure I understand all the rationale for
the current approach yet.

https://codereview.chromium.org/289863005/diff/20001/base/message_loop/timer_...
File base/message_loop/timer_slack.h (right):

https://codereview.chromium.org/289863005/diff/20001/base/message_loop/timer_...
base/message_loop/timer_slack.h:17: // Sets the global timer slack number for
this process.
Why is this process wide? I thought that we could get per-thread everywhere and
per-timer some places. So a good starter implementation seems to be per-thread
only (since we can do that everywhere).

Would it be possible to do that on the MessageLoop instance itself instead of as
a global static setting like this? It guarantees a max of one MessageLoop
instance per thread.

https://codereview.chromium.org/289863005/diff/20001/chrome/browser/resource_...
File chrome/browser/resource_throttle.h (right):

https://codereview.chromium.org/289863005/diff/20001/chrome/browser/resource_...
chrome/browser/resource_throttle.h:14: class ResourceThrottle
Kari and I have been talking a lot about how to solve the general problem of
resource management. She's collected a handful of signals like this which we
think'd be useful for coalescing and prioritizing the network, cpu and other
system resources. We've been wondering whether we should have a single manager
or multiple.

Currently content/browser/loader/resource_scheduler.h does all of the network
scheduling and she's working on adding these signals to it as well. We could
either generalize one system_resource_manager which listens to all of the
signals we care about and sets all resource priorities or else we could do a
manager-per-resource: rename resource_scheduler.h to network_resource_manager.h
and name this cpu_resource_manager.h or something like that. Thoughts?

In any case, this functionality probably belongs in content/ not chrome/.

https://codereview.chromium.org/289863005/diff/20001/chrome/common/render_mes...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/289863005/diff/20001/chrome/common/render_mes...
chrome/common/render_messages.h:238: bool /* enabled */)
I might be missing something, but the renderer already knows whether it is
visible or audible. Then it notifies the browser which now turns around and
notifies the renderer to coalesce timers. I'm wondering now whether the logic to
apply coalescing should live in the renderer process to avoid the extra ipc.

Also, I think everywhere we talk about coalescing, we should pass a timer_slack
enum instead of a bool. That way if we add a small coalescing (in between NONE
and MAXIMUM), we don't have to rework the API. For instance, we may discover
with experimentation that even the foreground tab could handle 5 or 10% slack.

[Avi (use Gerrit), 2014-05-20 14:41:38]

I like the ideas but wonder about the implementation.

I have the same question: is this really a global? I get that you're trying to
minimize changes to the API, but what you have here are differing objects that
have different tolerance requirements, but who try to influence their timers by
stomping on each others' use of a global value. That looks like a nightmare.

I would much rather bite the bullet and make a new API.

https://codereview.chromium.org/289863005/diff/20001/chrome/browser/resource_...
File chrome/browser/resource_throttle.h (right):

https://codereview.chromium.org/289863005/diff/20001/chrome/browser/resource_...
chrome/browser/resource_throttle.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
No (c)

https://codereview.chromium.org/289863005/diff/20001/chrome/browser/resource_...
chrome/browser/resource_throttle.h:21: // From WebContentsObserver .
Why the space before the .? (here and below too)

https://codereview.chromium.org/289863005/diff/20001/chrome/browser/ui/tab_he...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/289863005/diff/20001/chrome/browser/ui/tab_he...
chrome/browser/ui/tab_helpers.cc:117:
ResourceThrottle::CreateForWebContents(web_contents);
Alphabetically, please!

https://codereview.chromium.org/289863005/diff/20001/content/public/browser/w...
File content/public/browser/web_contents_user_data.h (right):

https://codereview.chromium.org/289863005/diff/20001/content/public/browser/w...
content/public/browser/web_contents_user_data.h:26: // };
This is correct but doesn't belong in this CL.

[aiolos (Not reviewing), 2014-05-20 17:45:22]

I'd agree with this not needing to be global as well. It's a good start though.

https://codereview.chromium.org/289863005/diff/20001/chrome/browser/resource_...
File chrome/browser/resource_throttle.cc (right):

https://codereview.chromium.org/289863005/diff/20001/chrome/browser/resource_...
chrome/browser/resource_throttle.cc:38: void
ResourceThrottle::OnCreatingAudioStream(int render_process_id,
Were you planning on using OnAudioStreamPlaying/OnAudioStreamStopped as signals
as well?

https://codereview.chromium.org/289863005/diff/20001/chrome/browser/resource_...
File chrome/browser/resource_throttle.h (right):

https://codereview.chromium.org/289863005/diff/20001/chrome/browser/resource_...
chrome/browser/resource_throttle.h:14: class ResourceThrottle
I agree with Tony that you'll want to move this to content instead of chrome if
possible. Either way I'd use content::MediaObserver here instead of
MediaCaptureDevicesDispatcher::Observer.

The debate over whether we should have one scheduler to rule them all is an
interesting one and I definitely think it's worth more discussion. There are
many signals that are important to every scheduler, like whether a tab/app/etc
is frontmost or playing audio, but there are also a few more specialized ones,
like whether a tab is pinned, that only seem important in specific cases.
Perhaps it would be cleaner to have a resource_manager with all the generally
important signals that can then be extended for each kind of resource. I don't
have a good enough feel for what would be the best option yet, so it might be
better to just rename this for the moment and start a broader conversation on
what to do moving forward. Thoughts?

On 2014/05/20 14:14:52, tonyg wrote:
> Kari and I have been talking a lot about how to solve the general problem of
> resource management. She's collected a handful of signals like this which we
> think'd be useful for coalescing and prioritizing the network, cpu and other
> system resources. We've been wondering whether we should have a single manager
> or multiple.
> 
> Currently content/browser/loader/resource_scheduler.h does all of the network
> scheduling and she's working on adding these signals to it as well. We could
> either generalize one system_resource_manager which listens to all of the
> signals we care about and sets all resource priorities or else we could do a
> manager-per-resource: rename resource_scheduler.h to
network_resource_manager.h
> and name this cpu_resource_manager.h or something like that. Thoughts?
> 
> In any case, this functionality probably belongs in content/ not chrome/.

https://codereview.chromium.org/289863005/diff/20001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/289863005/diff/20001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:1847: 'browser/resource_throttle.cc',
I assume one of these should be 'browser/resource_throttle.h',

[jeremy, 2014-05-21 10:23:48]

Thanks!!

Summary of changes I plan to make:
* Move code to content/.
* Make throttling per thread rather than global.
* Rename ResourceThrottle so it doesn't sound like it's responsible for
more than just timer coalescing.

Notably I want to leave the controlling code on the browser side, unless
people feel strongly I don't think the single IPC on background/foreground
switch is an issue and the browser side means we can experiment more easily
and it makes for a cleaner change.

Does that sound ok to everyone?


On Tue, May 20, 2014 at 5:41 PM, <avi@chromium.org> wrote:

> I like the ideas but wonder about the implementation.
>
> I have the same question: is this really a global? I get that you're
> trying to
> minimize changes to the API, but what you have here are differing objects
> that
> have different tolerance requirements, but who try to influence their
> timers by
> stomping on each others' use of a global value. That looks like a
> nightmare.
>
> I would much rather bite the bullet and make a new API.
>
>
>
> https://codereview.chromium.org/289863005/diff/20001/
> chrome/browser/resource_throttle.h
> File chrome/browser/resource_throttle.h (right):
>
> https://codereview.chromium.org/289863005/diff/20001/
> chrome/browser/resource_throttle.h#newcode1
>  chrome/browser/resource_throttle.h:1: // Copyright (c) 2014 The Chromium
> Authors. All rights reserved.
> No (c)
>
> https://codereview.chromium.org/289863005/diff/20001/
> chrome/browser/resource_throttle.h#newcode21
> chrome/browser/resource_throttle.h:21: // From WebContentsObserver .
> Why the space before the .? (here and below too)
>
> https://codereview.chromium.org/289863005/diff/20001/
> chrome/browser/ui/tab_helpers.cc
> File chrome/browser/ui/tab_helpers.cc (right):
>
> https://codereview.chromium.org/289863005/diff/20001/
> chrome/browser/ui/tab_helpers.cc#newcode117
> chrome/browser/ui/tab_helpers.cc:117:
> ResourceThrottle::CreateForWebContents(web_contents);
> Alphabetically, please!
>
> https://codereview.chromium.org/289863005/diff/20001/
> content/public/browser/web_contents_user_data.h
> File content/public/browser/web_contents_user_data.h (right):
>
> https://codereview.chromium.org/289863005/diff/20001/
> content/public/browser/web_contents_user_data.h#newcode26
> content/public/browser/web_contents_user_data.h:26: // };
> This is correct but doesn't belong in this CL.
>
> https://codereview.chromium.org/289863005/
>

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

[tonyg, 2014-05-21 11:14:21]

SGTM
On May 21, 2014 12:23 PM, "Jeremy Moskovich" <jeremy@chromium.org> wrote:

> Thanks!!
>
> Summary of changes I plan to make:
> * Move code to content/.
> * Make throttling per thread rather than global.
> * Rename ResourceThrottle so it doesn't sound like it's responsible for
> more than just timer coalescing.
>
> Notably I want to leave the controlling code on the browser side, unless
> people feel strongly I don't think the single IPC on background/foreground
> switch is an issue and the browser side means we can experiment more easily
> and it makes for a cleaner change.
>
> Does that sound ok to everyone?
>
>
> On Tue, May 20, 2014 at 5:41 PM, <avi@chromium.org> wrote:
>
>> I like the ideas but wonder about the implementation.
>>
>> I have the same question: is this really a global? I get that you're
>> trying to
>> minimize changes to the API, but what you have here are differing objects
>> that
>> have different tolerance requirements, but who try to influence their
>> timers by
>> stomping on each others' use of a global value. That looks like a
>> nightmare.
>>
>> I would much rather bite the bullet and make a new API.
>>
>>
>>
>> https://codereview.chromium.org/289863005/diff/20001/
>> chrome/browser/resource_throttle.h
>> File chrome/browser/resource_throttle.h (right):
>>
>> https://codereview.chromium.org/289863005/diff/20001/
>> chrome/browser/resource_throttle.h#newcode1
>>  chrome/browser/resource_throttle.h:1: // Copyright (c) 2014 The Chromium
>> Authors. All rights reserved.
>> No (c)
>>
>> https://codereview.chromium.org/289863005/diff/20001/
>> chrome/browser/resource_throttle.h#newcode21
>> chrome/browser/resource_throttle.h:21: // From WebContentsObserver .
>> Why the space before the .? (here and below too)
>>
>> https://codereview.chromium.org/289863005/diff/20001/
>> chrome/browser/ui/tab_helpers.cc
>> File chrome/browser/ui/tab_helpers.cc (right):
>>
>> https://codereview.chromium.org/289863005/diff/20001/
>> chrome/browser/ui/tab_helpers.cc#newcode117
>> chrome/browser/ui/tab_helpers.cc:117:
>> ResourceThrottle::CreateForWebContents(web_contents);
>> Alphabetically, please!
>>
>> https://codereview.chromium.org/289863005/diff/20001/
>> content/public/browser/web_contents_user_data.h
>> File content/public/browser/web_contents_user_data.h (right):
>>
>> https://codereview.chromium.org/289863005/diff/20001/
>> content/public/browser/web_contents_user_data.h#newcode26
>> content/public/browser/web_contents_user_data.h:26: // };
>> This is correct but doesn't belong in this CL.
>>
>> https://codereview.chromium.org/289863005/
>>
>
>

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

[chromium-reviews, 2014-05-21 15:17:41]

SGTM

Cheers,
Kari


On Wed, May 21, 2014 at 4:14 AM, Tony Gentilcore <tonyg@chromium.org> wrote:

> SGTM
> On May 21, 2014 12:23 PM, "Jeremy Moskovich" <jeremy@chromium.org> wrote:
>
>> Thanks!!
>>
>> Summary of changes I plan to make:
>> * Move code to content/.
>> * Make throttling per thread rather than global.
>> * Rename ResourceThrottle so it doesn't sound like it's responsible for
>> more than just timer coalescing.
>>
>> Notably I want to leave the controlling code on the browser side, unless
>> people feel strongly I don't think the single IPC on background/foreground
>> switch is an issue and the browser side means we can experiment more easily
>> and it makes for a cleaner change.
>>
>> Does that sound ok to everyone?
>>
>>
>> On Tue, May 20, 2014 at 5:41 PM, <avi@chromium.org> wrote:
>>
>>> I like the ideas but wonder about the implementation.
>>>
>>> I have the same question: is this really a global? I get that you're
>>> trying to
>>> minimize changes to the API, but what you have here are differing
>>> objects that
>>> have different tolerance requirements, but who try to influence their
>>> timers by
>>> stomping on each others' use of a global value. That looks like a
>>> nightmare.
>>>
>>> I would much rather bite the bullet and make a new API.
>>>
>>>
>>>
>>> https://codereview.chromium.org/289863005/diff/20001/
>>> chrome/browser/resource_throttle.h
>>> File chrome/browser/resource_throttle.h (right):
>>>
>>> https://codereview.chromium.org/289863005/diff/20001/
>>> chrome/browser/resource_throttle.h#newcode1
>>>  chrome/browser/resource_throttle.h:1: // Copyright (c) 2014 The
>>> Chromium
>>> Authors. All rights reserved.
>>> No (c)
>>>
>>> https://codereview.chromium.org/289863005/diff/20001/
>>> chrome/browser/resource_throttle.h#newcode21
>>> chrome/browser/resource_throttle.h:21: // From WebContentsObserver .
>>> Why the space before the .? (here and below too)
>>>
>>> https://codereview.chromium.org/289863005/diff/20001/
>>> chrome/browser/ui/tab_helpers.cc
>>> File chrome/browser/ui/tab_helpers.cc (right):
>>>
>>> https://codereview.chromium.org/289863005/diff/20001/
>>> chrome/browser/ui/tab_helpers.cc#newcode117
>>> chrome/browser/ui/tab_helpers.cc:117:
>>> ResourceThrottle::CreateForWebContents(web_contents);
>>> Alphabetically, please!
>>>
>>> https://codereview.chromium.org/289863005/diff/20001/
>>> content/public/browser/web_contents_user_data.h
>>> File content/public/browser/web_contents_user_data.h (right):
>>>
>>> https://codereview.chromium.org/289863005/diff/20001/
>>> content/public/browser/web_contents_user_data.h#newcode26
>>> content/public/browser/web_contents_user_data.h:26: // };
>>> This is correct but doesn't belong in this CL.
>>>
>>> https://codereview.chromium.org/289863005/
>>>
>>
>>

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

[jeremy, 2014-05-29 13:10:00]

This patch is based on top of Dale's in-progress patches:
https://codereview.chromium.org/305533006/
https://codereview.chromium.org/298253004/

The main change from Dale's work is that the
ChildProcessMsg_SetProcessBackgrounded IPC is sent on all platforms (not just
Windows).

Please provide a high-level review, if there's nothing glaringly wrong I'll add
unit tests and any missing owners for a final review round.

[tonyg, 2014-05-29 14:05:59]

lgtm

Much nicer!

The main unittest I'd like to see is that a renderer process with a tab in the
foreground and a tab in the background does the right thing to both loops.

https://codereview.chromium.org/289863005/diff/80001/content/child/child_thre...
File content/child/child_thread.cc (right):

https://codereview.chromium.org/289863005/diff/80001/content/child/child_thre...
content/child/child_thread.cc:562: // from within the process.
nit: indent

[Avi (use Gerrit), 2014-05-29 14:24:15]

I like this much better.

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/messag...
File base/message_loop/message_pump_mac.mm (right):

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/messag...
base/message_loop/message_pump_mac.mm:47: static bool function_lookup_error =
false;
Is this thread-safe?

We could (theoretically) get calls to SetTimerTolerance from multiple threads,
and race in initializing these statics. dispatch_once, perhaps? (See
src/base/sys_info_ios.mm for an example.)

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/timer_...
File base/message_loop/timer_slack.h (right):

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/timer_...
base/message_loop/timer_slack.h:1: // Copyright (c) 2014 The Chromium Authors.
All rights reserved.
Nit: No (c).

https://codereview.chromium.org/289863005/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/289863005/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1960: // processes, so
control backgrounding from the process host.
Move the comment about Vista+ requiring doing it from the process here.

https://codereview.chromium.org/289863005/diff/80001/content/child/child_thre...
File content/child/child_thread.cc (right):

https://codereview.chromium.org/289863005/diff/80001/content/child/child_thre...
content/child/child_thread.cc:559:
base::MessageLoop::current()->SetTimerSlack(timer_slack);
Only the current thread?

[DaleCurtis, 2014-05-29 18:03:08]

lgtm. Another area for improvement might be implementing
SetProcessBackgrounded() for OSX (whatever that looks like).

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/messag...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/messag...
base/message_loop/message_loop.h:268: pump_->SetTimerSlack(timer_slack);
nit: indent is off.

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/timer_...
File base/message_loop/timer_slack.h (right):

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/timer_...
base/message_loop/timer_slack.h:14: };
We only have two volumes, off and 11. :)

[tonyg, 2014-05-29 19:34:13]

> The main unittest I'd like to see is that a renderer process with a tab in the
> foreground and a tab in the background does the right thing to both loops.

Ignore my babbling. There'd only be one main thread in that case :)

[jeremy, 2014-06-02 10:21:27]

Mark: Base owner review please :)

This should be final except I still need to add a unit test, looking at doing
this as a ContentBrowserTest - does that sound like the right way to go?

Ultimately I want to set timer slack on all threads in the renderer process - do
we have a mechanism for iterating over all message loops/pumps in a process?

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/messag...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/messag...
base/message_loop/message_loop.h:268: pump_->SetTimerSlack(timer_slack);
On 2014/05/29 18:03:08, DaleCurtis wrote:
> nit: indent is off.

Done.

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/messag...
File base/message_loop/message_pump_mac.mm (right):

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/messag...
base/message_loop/message_pump_mac.mm:47: static bool function_lookup_error =
false;
On 2014/05/29 14:24:16, Avi wrote:
> Is this thread-safe?
> 
> We could (theoretically) get calls to SetTimerTolerance from multiple threads,
> and race in initializing these statics. dispatch_once, perhaps? (See
> src/base/sys_info_ios.mm for an example.)

Done.

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/timer_...
File base/message_loop/timer_slack.h (right):

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/timer_...
base/message_loop/timer_slack.h:1: // Copyright (c) 2014 The Chromium Authors.
All rights reserved.
On 2014/05/29 14:24:16, Avi wrote:
> Nit: No (c).

Done.

https://codereview.chromium.org/289863005/diff/80001/base/message_loop/timer_...
base/message_loop/timer_slack.h:14: };
:))

https://codereview.chromium.org/289863005/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/289863005/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1960: // processes, so
control backgrounding from the process host.
On 2014/05/29 14:24:16, Avi wrote:
> Move the comment about Vista+ requiring doing it from the process here.

Done.

https://codereview.chromium.org/289863005/diff/80001/content/child/child_thre...
File content/child/child_thread.cc (right):

https://codereview.chromium.org/289863005/diff/80001/content/child/child_thre...
content/child/child_thread.cc:559:
base::MessageLoop::current()->SetTimerSlack(timer_slack);
In the current CL  only the main renderer thread coalesces timers (it's the only
thread in the renderer process which is backed by a CFMessagePump).

In the general case we'd like to set this for all threads, is there a supported
way to iterate over all message loop over various threads in the process?

https://codereview.chromium.org/289863005/diff/80001/content/child/child_thre...
content/child/child_thread.cc:562: // from within the process.
On 2014/05/29 14:06:00, tonyg wrote:
> nit: indent

Done.

[Mark Mentovai, 2014-06-03 13:15:22]

https://codereview.chromium.org/289863005/diff/120001/base/message_loop/messa...
File base/message_loop/message_pump_mac.mm (right):

https://codereview.chromium.org/289863005/diff/120001/base/message_loop/messa...
base/message_loop/message_pump_mac.mm:9: #include <dlfcn.h>
This belongs in the C system header section (with Foundation.h), not the C++
section.

https://codereview.chromium.org/289863005/diff/120001/base/message_loop/messa...
base/message_loop/message_pump_mac.mm:17: #include
"base/message_loop/timer_slack.h"
Sort: message < metrics.

https://codereview.chromium.org/289863005/diff/120001/base/message_loop/messa...
base/message_loop/message_pump_mac.mm:52: dlsym(RTLD_DEFAULT,
"CFRunLoopTimerSetTolerance"));
Don’t use RTLD_DEFAULT, use a handle to CoreFoundation.framework.

https://codereview.chromium.org/289863005/diff/120001/base/message_loop/timer...
File base/message_loop/timer_slack.h (right):

https://codereview.chromium.org/289863005/diff/120001/base/message_loop/timer...
base/message_loop/timer_slack.h:12: TIMER_SLACK_NONE,
You should have a comment on each of these explaining how they’ll be
interpreted.

[Avi (use Gerrit), 2014-06-03 14:51:54]

LGTM with Mark's issues addressed.

[jeremy, 2014-06-05 12:09:21]

Mark: All fixed, thanks!
Jschuh: Owner review for content/common/child_process_messages.h please,
rubberstamp OK.

[jschuh, 2014-06-05 13:11:41]

ipc security rubberstamp lgtm (notes: bool that was previously windows-only)

[Mark Mentovai, 2014-06-05 13:26:00]

LGTM

[jeremy, 2014-06-05 14:11:09]

The CQ bit was checked by jeremy@chromium.org

[commit-bot: I haz the power, 2014-06-05 14:13:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jeremy@chromium.org/289863005/160001

[commit-bot: I haz the power, 2014-06-05 14:32:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/13365)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/16454)

[commit-bot: I haz the power, 2014-06-05 14:35:09]

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

[commit-bot: I haz the power, 2014-06-05 14:35:09]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[jeremy, 2014-06-05 15:54:02]

The CQ bit was checked by jeremy@chromium.org

[commit-bot: I haz the power, 2014-06-05 15:55:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jeremy@chromium.org/289863005/180001

[commit-bot: I haz the power, 2014-06-05 19:36:25]

Change committed as 275218