PPAPI/NaCl: Make NaClIPCAdapter transfer handles more generally

PPAPI/NaCl: Make NaClIPCAdapter transfer handles more generally

This does a couple of things:
- It defines a new wrapper for passing any kind of handle through the PPAPI proxy (SerializedHandle).
- It updates nacl_ipc_adapter to have a more general way to pick apart messages based on their static types (which include the types of all the params).
- It adds support for PPB_Graphics2D and PPB_Graphics3D to the NaCl IPC proxy (e.g., NaCl SDK examples pi_generator and tumbler work in the new proxy with this patch).

The downside is it requires pulling parts of ppapi/shared_impl and ppapi/proxy in to the NaCl Win64 build. 

BUG=116317
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153531

[dmichael (off chromium), 2012-07-28 17:58:11]

This is probably ready for a first look. Audio and ImageData work. Graphics3D
doesn't yet for some reason.

[dmichael (off chromium), 2012-07-30 14:44:01]

I realized I probably ought to test this on Windows before you review, and
there's some more cleanup I could do, so hold off on reviewing (unless you're
curious; it probably won't change that much).

[bbudge, 2012-07-30 17:07:34]

On 2012/07/30 14:44:01, dmichael wrote:
> I realized I probably ought to test this on Windows before you review, and
> there's some more cleanup I could do, so hold off on reviewing (unless you're
> curious; it probably won't change that much).

This looks great David. My only question is how it will play with Brett's nested
resource message scheme.

[dmichael (off chromium), 2012-07-30 17:12:14]

On Mon, Jul 30, 2012 at 11:07 AM, <bbudge@chromium.org> wrote:

> On 2012/07/30 14:44:01, dmichael wrote:
>
>> I realized I probably ought to test this on Windows before you review, and
>> there's some more cleanup I could do, so hold off on reviewing (unless
>> you're
>> curious; it probably won't change that much).
>>
>
> This looks great David. My only question is how it will play with Brett's
> nested
> resource message scheme.
>
Hopefully SerializedHandle will help out with making that change happen.
And when we get there, I'll have to add an overload of ExtractHandle for
dealing with a vector of SerializedHandles, but otherwise it should still
work (assuming we put the handles in the outer-most message). It might be
more general than necessary then, but maybe not overkill, since it will
give us more freedom to tinker with those outer messages without having to
remember to change nacl_ipc_adapter.

I was afraid this might end up a waste of time, but I'm fairly happy with
it now. It was simpler than I expected to pick apart the messages.


>
http://codereview.chromium.**org/10828023/<http://codereview.chromium.org/108...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[dmichael (off chromium), 2012-07-30 17:12:16]

On Mon, Jul 30, 2012 at 11:07 AM, <bbudge@chromium.org> wrote:

> On 2012/07/30 14:44:01, dmichael wrote:
>
>> I realized I probably ought to test this on Windows before you review, and
>> there's some more cleanup I could do, so hold off on reviewing (unless
>> you're
>> curious; it probably won't change that much).
>>
>
> This looks great David. My only question is how it will play with Brett's
> nested
> resource message scheme.
>
Hopefully SerializedHandle will help out with making that change happen.
And when we get there, I'll have to add an overload of ExtractHandle for
dealing with a vector of SerializedHandles, but otherwise it should still
work (assuming we put the handles in the outer-most message). It might be
more general than necessary then, but maybe not overkill, since it will
give us more freedom to tinker with those outer messages without having to
remember to change nacl_ipc_adapter.

I was afraid this might end up a waste of time, but I'm fairly happy with
it now. It was simpler than I expected to pick apart the messages.


>
http://codereview.chromium.**org/10828023/<http://codereview.chromium.org/108...
>

[dmichael (off chromium), 2012-08-16 21:23:17]

bradnelson: General gyp review & advice (I'm sure there are ways to improve it)
brettw: ppapi stuff and nacl_ipc_adapter
piman: gpu
bbudge: General review, if you have time.

http://codereview.chromium.org/10828023/diff/20001/chrome/nacl/nacl_main_plat...
File chrome/nacl/nacl_main_platform_delegate_win.cc (right):

http://codereview.chromium.org/10828023/diff/20001/chrome/nacl/nacl_main_plat...
chrome/nacl/nacl_main_platform_delegate_win.cc:29: NaClSrpcModuleInit();
This fixes (or just works around?) a problem we're having right now; I'm going
to pull it back out of this CL in the next upload, since it doesn't belong here.

http://codereview.chromium.org/10828023/diff/20001/ppapi/shared_impl/host_res...
File ppapi/shared_impl/host_resource.h (right):

http://codereview.chromium.org/10828023/diff/20001/ppapi/shared_impl/host_res...
ppapi/shared_impl/host_resource.h:23: HostResource();
I moved stuff to a body file when I was trying to sort out the windows build. I
don't think it's actually necessary, but it seems cleaner to do it this way.

[brettw, 2012-08-17 23:02:14]

The template stuff seems nice but I'm worried about the gym file changes.

I know you were trying to avoid having to do anything special for each message
containing a handle, but it seems instead we have to do something special when
you add a ppapi proxy or related things). This seems worse since it's less
related to what you're doing.

What would it look like if we have a separate messages file of ones that nacl
needs to decode?

[bradn, 2012-08-17 23:19:44]

One suggestion, but you don't need to take it.
LGTM on the gyp

http://codereview.chromium.org/10828023/diff/8006/gpu/gpu_untrusted.gyp
File gpu/gpu_untrusted.gyp (right):

http://codereview.chromium.org/10828023/diff/8006/gpu/gpu_untrusted.gyp#newcode9
gpu/gpu_untrusted.gyp:9: 'nacl_target': 0,
I realize this pattern is elsewhere, maybe nacl_win64_target might be more
descriptive?

[dmichael (off chromium), 2012-08-17 23:24:59]

On Fri, Aug 17, 2012 at 5:02 PM, <brettw@chromium.org> wrote:

> The template stuff seems nice but I'm worried about the gym file changes.
>
> I know you were trying to avoid having to do anything special for each
> message
> containing a handle, but it seems instead we have to do something special
> when
> you add a ppapi proxy or related things). This seems worse since it's less
> related to what you're doing.
>
Yeah, I was much happier with the patch *before *I realized what it needed
for Windows.


>
> What would it look like if we have a separate messages file of ones that
> nacl
> needs to decode?
>
I was considering breaking all messages & params in to a ppapi_ipc target.
But I wanted to publish this CL before going through the trouble :-). It
would be something like this:
ppapi_proxy (all sender/listener/dispatcher stuff)
     |
     V
 ppapi_ipc (all messages and params)
     |
     V
ppapi_shared (same as it is now; ppapi_ipc really only needs HostResource)

That would be a nice way to factor it, I think. (We could alternatively
flip ppapi_ipc and ppapi_shared and put HostResource in ppapi_ipc).
Admittedly, splitting out the NaCl-specific ones as you suggest would
probably be easier. On the bright side, we probably want to do one or the
other anyway for cracking open the Resource messages you're working on. So
I think it wouldn't be a wasted effort.

WDYT?


>
http://codereview.chromium.**org/10828023/<http://codereview.chromium.org/108...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[dmichael (off chromium), 2012-08-17 23:25:00]

On Fri, Aug 17, 2012 at 5:02 PM, <brettw@chromium.org> wrote:

> The template stuff seems nice but I'm worried about the gym file changes.
>
> I know you were trying to avoid having to do anything special for each
> message
> containing a handle, but it seems instead we have to do something special
> when
> you add a ppapi proxy or related things). This seems worse since it's less
> related to what you're doing.
>
Yeah, I was much happier with the patch *before *I realized what it needed
for Windows.


>
> What would it look like if we have a separate messages file of ones that
> nacl
> needs to decode?
>
I was considering breaking all messages & params in to a ppapi_ipc target.
But I wanted to publish this CL before going through the trouble :-). It
would be something like this:
ppapi_proxy (all sender/listener/dispatcher stuff)
     |
     V
 ppapi_ipc (all messages and params)
     |
     V
ppapi_shared (same as it is now; ppapi_ipc really only needs HostResource)

That would be a nice way to factor it, I think. (We could alternatively
flip ppapi_ipc and ppapi_shared and put HostResource in ppapi_ipc).
Admittedly, splitting out the NaCl-specific ones as you suggest would
probably be easier. On the bright side, we probably want to do one or the
other anyway for cracking open the Resource messages you're working on. So
I think it wouldn't be a wasted effort.

WDYT?


>
http://codereview.chromium.**org/10828023/<http://codereview.chromium.org/108...
>

[brettw, 2012-08-19 22:29:27]

If the definition of ppapi_ipc is "under control" (I'm not quire sure
what kind of dependencies this will entail) then adding this seems
like a good solution.

Brett

On Fri, Aug 17, 2012 at 4:24 PM, David Michael <dmichael@chromium.org> wrote:
> On Fri, Aug 17, 2012 at 5:02 PM, <brettw@chromium.org> wrote:
>>
>> The template stuff seems nice but I'm worried about the gym file changes.
>>
>> I know you were trying to avoid having to do anything special for each
>> message
>> containing a handle, but it seems instead we have to do something special
>> when
>> you add a ppapi proxy or related things). This seems worse since it's less
>> related to what you're doing.
>
> Yeah, I was much happier with the patch before I realized what it needed for
> Windows.
>
>>
>>
>> What would it look like if we have a separate messages file of ones that
>> nacl
>> needs to decode?
>
> I was considering breaking all messages & params in to a ppapi_ipc target.
> But I wanted to publish this CL before going through the trouble :-). It
> would be something like this:
> ppapi_proxy (all sender/listener/dispatcher stuff)
>      |
>      V
>  ppapi_ipc (all messages and params)
>      |
>      V
> ppapi_shared (same as it is now; ppapi_ipc really only needs HostResource)
>
> That would be a nice way to factor it, I think. (We could alternatively flip
> ppapi_ipc and ppapi_shared and put HostResource in ppapi_ipc). Admittedly,
> splitting out the NaCl-specific ones as you suggest would probably be
> easier. On the bright side, we probably want to do one or the other anyway
> for cracking open the Resource messages you're working on. So I think it
> wouldn't be a wasted effort.
>
> WDYT?
>
>>
>> http://codereview.chromium.org/10828023/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[brettw, 2012-08-19 22:29:28]

If the definition of ppapi_ipc is "under control" (I'm not quire sure
what kind of dependencies this will entail) then adding this seems
like a good solution.

Brett

On Fri, Aug 17, 2012 at 4:24 PM, David Michael <dmichael@chromium.org> wrote:
> On Fri, Aug 17, 2012 at 5:02 PM, <brettw@chromium.org> wrote:
>>
>> The template stuff seems nice but I'm worried about the gym file changes.
>>
>> I know you were trying to avoid having to do anything special for each
>> message
>> containing a handle, but it seems instead we have to do something special
>> when
>> you add a ppapi proxy or related things). This seems worse since it's less
>> related to what you're doing.
>
> Yeah, I was much happier with the patch before I realized what it needed for
> Windows.
>
>>
>>
>> What would it look like if we have a separate messages file of ones that
>> nacl
>> needs to decode?
>
> I was considering breaking all messages & params in to a ppapi_ipc target.
> But I wanted to publish this CL before going through the trouble :-). It
> would be something like this:
> ppapi_proxy (all sender/listener/dispatcher stuff)
>      |
>      V
>  ppapi_ipc (all messages and params)
>      |
>      V
> ppapi_shared (same as it is now; ppapi_ipc really only needs HostResource)
>
> That would be a nice way to factor it, I think. (We could alternatively flip
> ppapi_ipc and ppapi_shared and put HostResource in ppapi_ipc). Admittedly,
> splitting out the NaCl-specific ones as you suggest would probably be
> easier. On the bright side, we probably want to do one or the other anyway
> for cracking open the Resource messages you're working on. So I think it
> wouldn't be a wasted effort.
>
> WDYT?
>
>>
>> http://codereview.chromium.org/10828023/
>
>

[dmichael (off chromium), 2012-08-21 21:59:53]

This patch adds a ppapi_ipc target, which actually helped reduce dependencies a
little bit in a few other places. Should be a little simpler now.

PTAL

https://chromiumcodereview.appspot.com/10828023/diff/5064/ppapi/ppapi_proxy_u...
File ppapi/ppapi_proxy_untrusted.gyp (right):

https://chromiumcodereview.appspot.com/10828023/diff/5064/ppapi/ppapi_proxy_u...
ppapi/ppapi_proxy_untrusted.gyp:1: # Copyright (c) 2012 The Chromium Authors.
All rights reserved.
This file is really just a rename of ppapi/ppapi_ipc_proxy_untrusted.gyp, to fit
the naming scheme of the other files and to distinguish it from the new file,
ppapi_ipc_untrusted.gyp. Unfortunately, git+rietveld is not picking it up as a
move :-(

https://chromiumcodereview.appspot.com/10828023/diff/5064/ppapi/ppapi_proxy_u...
ppapi/ppapi_proxy_untrusted.gyp:44:
'../ppapi/ppapi_ipc_untrusted.gyp:ppapi_ipc_untrusted',
^^^^ This line was added. Should otherwise be identical to
ppapi_ipc_proxy_untrusted.gyp

[brettw, 2012-08-22 23:27:06]

This looks a lot better

http://codereview.chromium.org/10828023/diff/5064/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/10828023/diff/5064/chrome/nacl.gypi#newcode112
chrome/nacl.gypi:112: '../ppapi/ppapi_internal.gyp:ppapi_shared_win64',
How much does this bloat the win64 binary by? I'd hope the linker would strip
most of it, but it would be nice to know for sure, since "shared" is nontrivial.

http://codereview.chromium.org/10828023/diff/5064/chrome/nacl/nacl_ipc_adapte...
File chrome/nacl/nacl_ipc_adapter.cc (right):

http://codereview.chromium.org/10828023/diff/5064/chrome/nacl/nacl_ipc_adapte...
chrome/nacl/nacl_ipc_adapter.cc:186: ExtractHandlesImpl(params, handles,
out_msg);
I was a bit thrown by the name "Extract" since I would expect it to give me the
handle in some way, but this actually does the work of conversion for you and
doesn't return it. Maybe it could be called "Convert..." or "Translate..."?

http://codereview.chromium.org/10828023/diff/5064/chrome/nacl/nacl_ipc_adapte...
chrome/nacl/nacl_ipc_adapter.cc:456: // Do I need to skip the ID, or does Params
already include it?
Can you remove this question. It looks like params doesn't include it.

http://codereview.chromium.org/10828023/diff/5064/chrome/nacl/nacl_ipc_adapte...
chrome/nacl/nacl_ipc_adapter.cc:492: reinterpret_cast<const
NaClHandle>(shm_handle),
I'd out-dent these two lines 4 spaces each.

http://codereview.chromium.org/10828023/diff/5064/ppapi/ppapi_internal.gyp
File ppapi/ppapi_internal.gyp (right):

http://codereview.chromium.org/10828023/diff/5064/ppapi/ppapi_internal.gyp#ne...
ppapi/ppapi_internal.gyp:123: # In the component build, we'll just build
ppapi_ipc in to ppapi_proxy.
I'm confused about what's happening here (both here and in the old version).
Presumably since you're tweaking it, you figured it out. How does this pull in
the sources from ppapi_proxy.gypi? I have some idea it involves the
ppapi_proxy_target thingy, but I don't see how that works. Can you add a comment
somewhere?

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/ppapi_param_tra...
File ppapi/proxy/ppapi_param_traits.cc (right):

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/ppapi_param_tra...
ppapi/proxy/ppapi_param_traits.cc:330: // no default so the compiler will warn
on new types.
Style nit: capital N

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/ppb_audio_proxy.cc
File ppapi/proxy/ppb_audio_proxy.cc (right):

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/ppb_audio_proxy...
ppapi/proxy/ppb_audio_proxy.cc:319: handle.size() - sizeof(int32_t),
Can you add a comment why you do "- sizeof(int32_t)"?

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/serialized_stru...
File ppapi/proxy/serialized_structs.cc (right):

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/serialized_stru...
ppapi/proxy/serialized_structs.cc:133: if (!hdr)
I probably wouldn't null check the output param. All callers should supply this
or they will immediately crash and then they will fix their code :)

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/serialized_stru...
File ppapi/proxy/serialized_structs.h (right):

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/serialized_stru...
ppapi/proxy/serialized_structs.h:162: struct Header {
I'd define this at the top with the other types.

[dmichael (off chromium), 2012-08-23 22:55:14]

Comments addressed.

http://codereview.chromium.org/10828023/diff/5064/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/10828023/diff/5064/chrome/nacl.gypi#newcode112
chrome/nacl.gypi:112: '../ppapi/ppapi_internal.gyp:ppapi_shared_win64',
On 2012/08/22 23:27:06, brettw wrote:
> How much does this bloat the win64 binary by? I'd hope the linker would strip
> most of it, but it would be nice to know for sure, since "shared" is
nontrivial.
Good question. It looks like a static, release build locally gives:
old: 1,207,296
new: 1,214,464

So ~7k difference. Seems probably acceptable to me.

http://codereview.chromium.org/10828023/diff/5064/chrome/nacl/nacl_ipc_adapte...
File chrome/nacl/nacl_ipc_adapter.cc (right):

http://codereview.chromium.org/10828023/diff/5064/chrome/nacl/nacl_ipc_adapte...
chrome/nacl/nacl_ipc_adapter.cc:186: ExtractHandlesImpl(params, handles,
out_msg);
On 2012/08/22 23:27:06, brettw wrote:
> I was a bit thrown by the name "Extract" since I would expect it to give me
the
> handle in some way, but this actually does the work of conversion for you and
> doesn't return it. Maybe it could be called "Convert..." or "Translate..."?
Done. Also HandleExtractor->HandleConverter, TranslateMessage->ConvertMessage

http://codereview.chromium.org/10828023/diff/5064/chrome/nacl/nacl_ipc_adapte...
chrome/nacl/nacl_ipc_adapter.cc:456: // Do I need to skip the ID, or does Params
already include it?
On 2012/08/22 23:27:06, brettw wrote:
> Can you remove this question. It looks like params doesn't include it.
Oops, thanks. Figured it out and forgot to remove the comment.

http://codereview.chromium.org/10828023/diff/5064/chrome/nacl/nacl_ipc_adapte...
chrome/nacl/nacl_ipc_adapter.cc:492: reinterpret_cast<const
NaClHandle>(shm_handle),
On 2012/08/22 23:27:06, brettw wrote:
> I'd out-dent these two lines 4 spaces each.

Done.

http://codereview.chromium.org/10828023/diff/5064/ppapi/ppapi_internal.gyp
File ppapi/ppapi_internal.gyp (right):

http://codereview.chromium.org/10828023/diff/5064/ppapi/ppapi_internal.gyp#ne...
ppapi/ppapi_internal.gyp:123: # In the component build, we'll just build
ppapi_ipc in to ppapi_proxy.
On 2012/08/22 23:27:06, brettw wrote:
> I'm confused about what's happening here (both here and in the old version).
> Presumably since you're tweaking it, you figured it out. How does this pull in
> the sources from ppapi_proxy.gypi? I have some idea it involves the
> ppapi_proxy_target thingy, but I don't see how that works. Can you add a
comment
> somewhere?
I sprinkled in a few comments...  one at the first target in the file, and a
couple in this section, since it's a little special here (pulling sources from 2
gypi files in to 1 target). Please let me know if some more explanation is
needed.

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/ppapi_param_tra...
File ppapi/proxy/ppapi_param_traits.cc (right):

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/ppapi_param_tra...
ppapi/proxy/ppapi_param_traits.cc:330: // no default so the compiler will warn
on new types.
On 2012/08/22 23:27:06, brettw wrote:
> Style nit: capital N

Done.

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/ppb_audio_proxy.cc
File ppapi/proxy/ppb_audio_proxy.cc (right):

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/ppb_audio_proxy...
ppapi/proxy/ppb_audio_proxy.cc:319: handle.size() - sizeof(int32_t),
On 2012/08/22 23:27:06, brettw wrote:
> Can you add a comment why you do "- sizeof(int32_t)"?
Oops, I meant to clean that up, thanks for pointing it out.

Instead of commenting here, I exposed a couple of appropriate functions in
PPB_Audio_Shared and used them here. Hopefully it's more self-documenting now.
This can get a little nicer when http://codereview.chromium.org/10826296/ lands.

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/serialized_stru...
File ppapi/proxy/serialized_structs.cc (right):

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/serialized_stru...
ppapi/proxy/serialized_structs.cc:133: if (!hdr)
On 2012/08/22 23:27:06, brettw wrote:
> I probably wouldn't null check the output param. All callers should supply
this
> or they will immediately crash and then they will fix their code :)

Done.

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/serialized_stru...
File ppapi/proxy/serialized_structs.h (right):

http://codereview.chromium.org/10828023/diff/5064/ppapi/proxy/serialized_stru...
ppapi/proxy/serialized_structs.h:162: struct Header {
On 2012/08/22 23:27:06, brettw wrote:
> I'd define this at the top with the other types.

Done.

[bbudge, 2012-08-23 23:26:42]

How did you get around the problem with ppapi/ppapi_proxy_untrusted.gyp, where
the NaCl .gyp file with the same name caused GYP to fail? (We worked around it
by naming it ppapi/ppapi_ipc_proxy_untrusted.gyp).

LGTM

[dmichael (off chromium), 2012-08-24 16:29:19]

On 2012/08/23 23:26:42, bbudge1 wrote:
> How did you get around the problem with ppapi/ppapi_proxy_untrusted.gyp, where
> the NaCl .gyp file with the same name caused GYP to fail? (We worked around it
> by naming it ppapi/ppapi_ipc_proxy_untrusted.gyp).
I wasn't having problems locally or on the bots, but I was able to reproduce the
issue when generating for xcode on Mac. That might be due to a limitation with
xcode. I renamed the file back to what you had and added a comment about it. We
should probably rename it after the proxy switch is complete.

> 
> LGTM

[brettw, 2012-08-24 21:09:51]

lgtm

[commit-bot: I haz the power, 2012-08-27 17:40:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10828023/30005

[commit-bot: I haz the power, 2012-08-27 17:53:19]

Try job failure for 10828023-30005 (retry) on mac_rel for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-27 17:57:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/10828023/31110

[commit-bot: I haz the power, 2012-08-27 20:28:20]

Change committed as 153531