Create IPC channel to communicate with the renderer in NaClListener::OnStart().

Create IPC channel to communicate with the renderer in NaClListener::OnStart().

This is the preparation of the irt_ppapi implementation for non-SFI mode.
Currently (on SFI mode), the IPC channel between the plugin and the renderer
is created in the CreateNaClChannel message handled by NaClIPCAdapter.
However, we won't need nor use NaClIPCAdapter, and it causes the circular
dependency of plugin initialization.
To split the dependency, create the IPC channels for the browser process and
the renderer process at once (in NaClListener::OnStart()).
Note that we cannot remove CreateNaClChannel message, because it is only way
to pass the initialization argument from the browser to plugin. So, it is just
kept with renaming.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3734
TEST=Ran trybot.

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

[hidehiko, 2014-02-04 14:47:55]

This CL is based on Mark's CL (crrev.com/153453002), so it is necessary to land
this after the NaCl roll with the CL.
I ran unit_tests with manually patching it to my local env.
I'll run try-bot after the NaCl roll is done.

Mark, could you take a look?
Also, could you kindly advice me whom should I ask the review for ppapi/...
files (or add to the reviewer list)?

Thanks!
- hidehiko

[Mark Seaborn, 2014-02-04 19:28:11]

+bbudge as a PPAPI reviewer.  Bill, this is the simplification change that we
talked about last week.  It removes some of the (unused) logic for handling
multiple PPAPI plugins per NaCl process, which was causing problems for
implementing PPAPI startup for Bare Metal Mode.

+dmichael and teravest too.

The change looks good to me (with the changes below), but I will hold off from
giving a formal LG until sel_main_chrome.h on the NaCl side has settled.

https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
File components/nacl/loader/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_ipc_adapter.cc:528: case
ppapi::proxy::SerializedHandle::CHANNEL_HANDLE:
Should we also remove the definition of CHANNEL_HANDLE from the enum in
ppapi/proxy/serialized_handle.h?

https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl.cc
File ppapi/proxy/plugin_main_nacl.cc (right):

https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl...
ppapi/proxy/plugin_main_nacl.cc:50: #define NACL_IPC_BROWSER_FD 6
Can you use NACL_CHROME_DESC_BASE instead, please?

https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl...
ppapi/proxy/plugin_main_nacl.cc:204: void
PpapiDispatcher::OnMsgInitializeNaClDispatcher(
Can you add a check that would abort or ignore the message if a second message
is received?

https://codereview.chromium.org/150713003/diff/1/ppapi/shared_impl/ppapi_init...
File ppapi/shared_impl/ppapi_initialize_nacl_dispatcher_args.h (right):

https://codereview.chromium.org/150713003/diff/1/ppapi/shared_impl/ppapi_init...
ppapi/shared_impl/ppapi_initialize_nacl_dispatcher_args.h:15: struct
PPAPI_SHARED_EXPORT PpapiInitializeNaClDispatcherArgs {
FWIW, I don't think you need to rename this struct.  But if you do, maybe it
should be named after what it does broadly, rather than after the specific
internal interface it is passed to?  "PpapiNaClPluginArgs" perhaps?

[teravest, 2014-02-04 21:43:49]

FYI- Bill's out on vacation until 2/21.

On Tue, Feb 4, 2014 at 12:28 PM,  <mseaborn@chromium.org> wrote:
> +bbudge as a PPAPI reviewer.  Bill, this is the simplification change that
> we
> talked about last week.  It removes some of the (unused) logic for handling
> multiple PPAPI plugins per NaCl process, which was causing problems for
> implementing PPAPI startup for Bare Metal Mode.
>
> +dmichael and teravest too.
>
> The change looks good to me (with the changes below), but I will hold off
> from
> giving a formal LG until sel_main_chrome.h on the NaCl side has settled.
>
>
>
https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
> File components/nacl/loader/nacl_ipc_adapter.cc (right):
>
>
https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
> components/nacl/loader/nacl_ipc_adapter.cc:528: case
> ppapi::proxy::SerializedHandle::CHANNEL_HANDLE:
> Should we also remove the definition of CHANNEL_HANDLE from the enum in
> ppapi/proxy/serialized_handle.h?
>
>
https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl.cc
> File ppapi/proxy/plugin_main_nacl.cc (right):
>
>
https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl...
> ppapi/proxy/plugin_main_nacl.cc:50: #define NACL_IPC_BROWSER_FD 6
> Can you use NACL_CHROME_DESC_BASE instead, please?
>
>
https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl...
> ppapi/proxy/plugin_main_nacl.cc:204: void
> PpapiDispatcher::OnMsgInitializeNaClDispatcher(
> Can you add a check that would abort or ignore the message if a second
> message is received?
>
>
https://codereview.chromium.org/150713003/diff/1/ppapi/shared_impl/ppapi_init...
> File ppapi/shared_impl/ppapi_initialize_nacl_dispatcher_args.h (right):
>
>
https://codereview.chromium.org/150713003/diff/1/ppapi/shared_impl/ppapi_init...
> ppapi/shared_impl/ppapi_initialize_nacl_dispatcher_args.h:15: struct
> PPAPI_SHARED_EXPORT PpapiInitializeNaClDispatcherArgs {
> FWIW, I don't think you need to rename this struct.  But if you do,
> maybe it should be named after what it does broadly, rather than after
> the specific internal interface it is passed to?  "PpapiNaClPluginArgs"
> perhaps?
>
> https://codereview.chromium.org/150713003/

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

[Mark Seaborn, 2014-02-04 21:56:55]

On 2014/02/04 21:43:49, teravest wrote:
> FYI- Bill's out on vacation until 2/21.

OK.  David, can you review this, please?

[teravest, 2014-02-04 22:21:05]

lgtm

I don't have much to add to Mark's comments.

Sadly, this changes a bunch of code I was touching to change load_module to do
Chromium IPC. I suppose I'll wait for this to land and rebase what I've got, but
it's frustrating to learn this now.

https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_listener.cc:27: #include
"native_client/src/public/nacl_app.h"
Presumably this is gated on some change to native_client?

https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_listener.cc:266: DCHECK(nap != NULL);
I think you want to LOG(ERROR) and return if nap is NULL here, instead of
DCHECK().

[hidehiko, 2014-02-05 06:30:16]

[+jln for nacl_messages.h and ppapi_messages.h]
Thank you for your review. PTAL?

Justin, sorry about the conflicting with your coding, and thank you for your
support.

Julien, could you review nacl_messages.h and ppapi_messages.h as an OWNER?

Thanks,
- hidehiko

https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
File components/nacl/loader/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_ipc_adapter.cc:528: case
ppapi::proxy::SerializedHandle::CHANNEL_HANDLE:
On 2014/02/04 19:28:12, Mark Seaborn wrote:
> Should we also remove the definition of CHANNEL_HANDLE from the enum in
> ppapi/proxy/serialized_handle.h?

Done.

https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_listener.cc:27: #include
"native_client/src/public/nacl_app.h"
On 2014/02/04 22:21:06, teravest wrote:
> Presumably this is gated on some change to native_client?

Yes, this is based on crrev.com/153453002, and I'll wait for the NaCl roll with
it.

https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_listener.cc:266: DCHECK(nap != NULL);
On 2014/02/04 22:21:06, teravest wrote:
> I think you want to LOG(ERROR) and return if nap is NULL here, instead of
> DCHECK().

Done, but the method won't seem to return NULL (i.e., if failed it would crash)?

https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl.cc
File ppapi/proxy/plugin_main_nacl.cc (right):

https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl...
ppapi/proxy/plugin_main_nacl.cc:50: #define NACL_IPC_BROWSER_FD 6
On 2014/02/04 19:28:12, Mark Seaborn wrote:
> Can you use NACL_CHROME_DESC_BASE instead, please?

Done. Note that we cannot include sel_main_chrome.h directly, as it is under
trusted/.

https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl...
ppapi/proxy/plugin_main_nacl.cc:204: void
PpapiDispatcher::OnMsgInitializeNaClDispatcher(
On 2014/02/04 19:28:12, Mark Seaborn wrote:
> Can you add a check that would abort or ignore the message if a second message
> is received?

Done.

https://codereview.chromium.org/150713003/diff/1/ppapi/shared_impl/ppapi_init...
File ppapi/shared_impl/ppapi_initialize_nacl_dispatcher_args.h (right):

https://codereview.chromium.org/150713003/diff/1/ppapi/shared_impl/ppapi_init...
ppapi/shared_impl/ppapi_initialize_nacl_dispatcher_args.h:15: struct
PPAPI_SHARED_EXPORT PpapiInitializeNaClDispatcherArgs {
On 2014/02/04 19:28:12, Mark Seaborn wrote:
> FWIW, I don't think you need to rename this struct.  But if you do, maybe it
> should be named after what it does broadly, rather than after the specific
> internal interface it is passed to?  "PpapiNaClPluginArgs" perhaps?

Good point. Actually I just followed the code around here, but I didn't think it
was good name for this case. Renamed.

[teravest, 2014-02-05 17:12:44]

On Tue, Feb 4, 2014 at 11:30 PM,  <hidehiko@chromium.org> wrote:
> [+jln for nacl_messages.h and ppapi_messages.h]
> Thank you for your review. PTAL?
>
> Justin, sorry about the conflicting with your coding, and thank you for your
> support.
>
> Julien, could you review nacl_messages.h and ppapi_messages.h as an OWNER?
>
> Thanks,
> - hidehiko
>
>
>
>
>
https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
> File components/nacl/loader/nacl_ipc_adapter.cc (right):
>
>
https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
> components/nacl/loader/nacl_ipc_adapter.cc:528: case
> ppapi::proxy::SerializedHandle::CHANNEL_HANDLE:
> On 2014/02/04 19:28:12, Mark Seaborn wrote:
>>
>> Should we also remove the definition of CHANNEL_HANDLE from the enum
>
> in
>>
>> ppapi/proxy/serialized_handle.h?
>
>
> Done.
>
>
>
https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
> File components/nacl/loader/nacl_listener.cc (right):
>
>
https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
> components/nacl/loader/nacl_listener.cc:27: #include
> "native_client/src/public/nacl_app.h"
> On 2014/02/04 22:21:06, teravest wrote:
>>
>> Presumably this is gated on some change to native_client?
>
>
> Yes, this is based on crrev.com/153453002, and I'll wait for the NaCl
> roll with it.
>
>
>
https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
> components/nacl/loader/nacl_listener.cc:266: DCHECK(nap != NULL);
> On 2014/02/04 22:21:06, teravest wrote:
>>
>> I think you want to LOG(ERROR) and return if nap is NULL here, instead
>
> of
>>
>> DCHECK().
>
>
> Done, but the method won't seem to return NULL (i.e., if failed it would
> crash)?

I just want the code to do something reasonable here when nap is NULL
on Release builds.
I'll leave it up to you for what you think it should do.

>
>
>
https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl.cc
> File ppapi/proxy/plugin_main_nacl.cc (right):
>
>
https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl...
> ppapi/proxy/plugin_main_nacl.cc:50: #define NACL_IPC_BROWSER_FD 6
> On 2014/02/04 19:28:12, Mark Seaborn wrote:
>>
>> Can you use NACL_CHROME_DESC_BASE instead, please?
>
>
> Done. Note that we cannot include sel_main_chrome.h directly, as it is
> under trusted/.
>
>
>
https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl...
> ppapi/proxy/plugin_main_nacl.cc:204: void
> PpapiDispatcher::OnMsgInitializeNaClDispatcher(
> On 2014/02/04 19:28:12, Mark Seaborn wrote:
>>
>> Can you add a check that would abort or ignore the message if a second
>
> message
>>
>> is received?
>
>
> Done.
>
>
>
https://codereview.chromium.org/150713003/diff/1/ppapi/shared_impl/ppapi_init...
> File ppapi/shared_impl/ppapi_initialize_nacl_dispatcher_args.h (right):
>
>
https://codereview.chromium.org/150713003/diff/1/ppapi/shared_impl/ppapi_init...
> ppapi/shared_impl/ppapi_initialize_nacl_dispatcher_args.h:15: struct
> PPAPI_SHARED_EXPORT PpapiInitializeNaClDispatcherArgs {
> On 2014/02/04 19:28:12, Mark Seaborn wrote:
>>
>> FWIW, I don't think you need to rename this struct.  But if you do,
>
> maybe it
>>
>> should be named after what it does broadly, rather than after the
>
> specific
>>
>> internal interface it is passed to?  "PpapiNaClPluginArgs" perhaps?
>
>
> Good point. Actually I just followed the code around here, but I didn't
> think it was good name for this case. Renamed.
>
> https://codereview.chromium.org/150713003/

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

[Mark Seaborn, 2014-02-05 17:25:14]

https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl.cc
File ppapi/proxy/plugin_main_nacl.cc (right):

https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl...
ppapi/proxy/plugin_main_nacl.cc:50: #define NACL_IPC_BROWSER_FD 6
On 2014/02/05 06:30:16, hidehiko wrote:
> On 2014/02/04 19:28:12, Mark Seaborn wrote:
> > Can you use NACL_CHROME_DESC_BASE instead, please?
> 
> Done. Note that we cannot include sel_main_chrome.h directly, as it is under
> trusted/.

If checkdeps complained about using sel_main_chrome.h we could have changed the
DEPS file to make an exception -- that would be better than duplicating the
number here.

However, I've moved sel_main_chrome.h to native_client/src/public/chrome_main.h
(https://src.chromium.org/viewvc/native_client?revision=12705&view=revision), so
you can just #include the latter now.

[hidehiko, 2014-02-05 17:30:43]

On 2014/02/05 17:12:44, teravest wrote:
> On Tue, Feb 4, 2014 at 11:30 PM,  <mailto:hidehiko@chromium.org> wrote:
> > [+jln for nacl_messages.h and ppapi_messages.h]
> > Thank you for your review. PTAL?
> >
> > Justin, sorry about the conflicting with your coding, and thank you for your
> > support.
> >
> > Julien, could you review nacl_messages.h and ppapi_messages.h as an OWNER?
> >
> > Thanks,
> > - hidehiko
> >
> >
> >
> >
> >
>
https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
> > File components/nacl/loader/nacl_ipc_adapter.cc (right):
> >
> >
>
https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
> > components/nacl/loader/nacl_ipc_adapter.cc:528: case
> > ppapi::proxy::SerializedHandle::CHANNEL_HANDLE:
> > On 2014/02/04 19:28:12, Mark Seaborn wrote:
> >>
> >> Should we also remove the definition of CHANNEL_HANDLE from the enum
> >
> > in
> >>
> >> ppapi/proxy/serialized_handle.h?
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
> > File components/nacl/loader/nacl_listener.cc (right):
> >
> >
>
https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
> > components/nacl/loader/nacl_listener.cc:27: #include
> > "native_client/src/public/nacl_app.h"
> > On 2014/02/04 22:21:06, teravest wrote:
> >>
> >> Presumably this is gated on some change to native_client?
> >
> >
> > Yes, this is based on crrev.com/153453002, and I'll wait for the NaCl
> > roll with it.
> >
> >
> >
>
https://codereview.chromium.org/150713003/diff/1/components/nacl/loader/nacl_...
> > components/nacl/loader/nacl_listener.cc:266: DCHECK(nap != NULL);
> > On 2014/02/04 22:21:06, teravest wrote:
> >>
> >> I think you want to LOG(ERROR) and return if nap is NULL here, instead
> >
> > of
> >>
> >> DCHECK().
> >
> >
> > Done, but the method won't seem to return NULL (i.e., if failed it would
> > crash)?
> 
> I just want the code to do something reasonable here when nap is NULL
> on Release builds.
> I'll leave it up to you for what you think it should do.

Thank you for comment.
Then, IMHO, the current code is ok, because;
1) now, even on release build, the NaClAppMake() won't return NULL (aborted if
failed).
2) If something gets changed and NaClAppMake() starts to return NULL, the error
handling
  code is in the same manner around here. So, it should handled appropriately.

Mark, could you double check?

Thanks,
- hidehiko


> 
> >
> >
> >
>
https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl.cc
> > File ppapi/proxy/plugin_main_nacl.cc (right):
> >
> >
>
https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl...
> > ppapi/proxy/plugin_main_nacl.cc:50: #define NACL_IPC_BROWSER_FD 6
> > On 2014/02/04 19:28:12, Mark Seaborn wrote:
> >>
> >> Can you use NACL_CHROME_DESC_BASE instead, please?
> >
> >
> > Done. Note that we cannot include sel_main_chrome.h directly, as it is
> > under trusted/.
> >
> >
> >
>
https://codereview.chromium.org/150713003/diff/1/ppapi/proxy/plugin_main_nacl...
> > ppapi/proxy/plugin_main_nacl.cc:204: void
> > PpapiDispatcher::OnMsgInitializeNaClDispatcher(
> > On 2014/02/04 19:28:12, Mark Seaborn wrote:
> >>
> >> Can you add a check that would abort or ignore the message if a second
> >
> > message
> >>
> >> is received?
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/150713003/diff/1/ppapi/shared_impl/ppapi_init...
> > File ppapi/shared_impl/ppapi_initialize_nacl_dispatcher_args.h (right):
> >
> >
>
https://codereview.chromium.org/150713003/diff/1/ppapi/shared_impl/ppapi_init...
> > ppapi/shared_impl/ppapi_initialize_nacl_dispatcher_args.h:15: struct
> > PPAPI_SHARED_EXPORT PpapiInitializeNaClDispatcherArgs {
> > On 2014/02/04 19:28:12, Mark Seaborn wrote:
> >>
> >> FWIW, I don't think you need to rename this struct.  But if you do,
> >
> > maybe it
> >>
> >> should be named after what it does broadly, rather than after the
> >
> > specific
> >>
> >> internal interface it is passed to?  "PpapiNaClPluginArgs" perhaps?
> >
> >
> > Good point. Actually I just followed the code around here, but I didn't
> > think it was good name for this case. Renamed.
> >
> > https://codereview.chromium.org/150713003/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Mark Seaborn, 2014-02-06 01:31:53]

LGTM with the NACL_CHROME_DESC_BASE change I mentioned earlier.  Also, TEST=
should say "browser_tests" -- "unit_tests" won't cover NaCl.

Can you let these two changes land first:
https://codereview.chromium.org/155513005/
https://codereview.chromium.org/137623018/
- and also rebase your change on top of the second of those?

Feel free to tick the Commit Queue box on those two changes.

[hidehiko, 2014-02-06 19:27:30]

Thank you for your review.
Rebased and addressed Mark's comment. Also, I ran trybot.
PTAL just in case?

Julien, friendly ping? Could you review {nacl,ppapi}_messages.h files as an
OWNER?

Thanks,
- hidehiko

https://chromiumcodereview.appspot.com/150713003/diff/1/ppapi/proxy/plugin_ma...
File ppapi/proxy/plugin_main_nacl.cc (right):

https://chromiumcodereview.appspot.com/150713003/diff/1/ppapi/proxy/plugin_ma...
ppapi/proxy/plugin_main_nacl.cc:50: #define NACL_IPC_BROWSER_FD 6
On 2014/02/05 17:25:14, Mark Seaborn wrote:
> On 2014/02/05 06:30:16, hidehiko wrote:
> > On 2014/02/04 19:28:12, Mark Seaborn wrote:
> > > Can you use NACL_CHROME_DESC_BASE instead, please?
> > 
> > Done. Note that we cannot include sel_main_chrome.h directly, as it is under
> > trusted/.
> 
> If checkdeps complained about using sel_main_chrome.h we could have changed
the
> DEPS file to make an exception -- that would be better than duplicating the
> number here.
> 
> However, I've moved sel_main_chrome.h to
native_client/src/public/chrome_main.h
> (https://src.chromium.org/viewvc/native_client?revision=12705&view=revision),
so
> you can just #include the latter now.

Done. Note that, native_client/src/public was not in the include_rules of
ppapi/proxy/DEPS, so I added it.

[jln (very slow on Chromium), 2014-02-06 19:32:50]

On 2014/02/06 19:27:30, hidehiko wrote:
> Thank you for your review.
> Rebased and addressed Mark's comment. Also, I ran trybot.
> PTAL just in case?
> 
> Julien, friendly ping? Could you review {nacl,ppapi}_messages.h files as an
> OWNER?

*_messages.h lgtm

Sorry for the delay.

[Mark Seaborn, 2014-02-06 19:38:05]

LGTM still

[hidehiko, 2014-02-06 19:43:26]

On 2014/02/06 19:38:05, Mark Seaborn wrote:
> LGTM still

Thank you for your review!
Sending to CQ.

- hidehiko

[hidehiko, 2014-02-06 19:43:39]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-02-06 19:43:46]

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

[commit-bot: I haz the power, 2014-02-06 23:28:27]

Change committed as 249531