Convert NaCl renderer-loader messages to mojo.

Convert NaCl renderer-loader messages to mojo.

BUG=577685
Committed: https://crrev.com/eb52daa79263d35ea1c62b717427bf68467be04f
Cr-Commit-Position: refs/heads/master@{#439770}

[Sam McNally, 2016-11-22 00:26:13]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 00:26:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-11-22 00:27:28]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-11-22 00:31:31]

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

[commit-bot: I haz the power, 2016-11-22 00:31:32]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Sam McNally, 2016-11-22 00:45:26]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 00:46:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 01:14:13]

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

[commit-bot: I haz the power, 2016-11-22 01:14:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sam McNally, 2016-11-22 01:36:18]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 01:37:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 02:52:43]

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

[commit-bot: I haz the power, 2016-11-22 02:52:43]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-11-22 07:07:22]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2016-11-22 07:23:10]

Description was changed from

==========
Convert NaCl renderer messages to mojo.

BUG=577685
==========

to

==========
Convert NaCl renderer-plugin messages to mojo.

BUG=577685
==========

[Sam McNally, 2016-11-22 23:24:37]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 23:25:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-11-23 00:19:46]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2016-11-23 00:33:33]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 00:34:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-11-23 00:47:42]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2016-11-23 00:49:09]

sammc@chromium.org changed reviewers:
+ mseaborn@chromium.org

[commit-bot: I haz the power, 2016-11-23 02:33:24]

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

[commit-bot: I haz the power, 2016-11-23 02:33:25]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2016-11-30 10:03:07]

ping

[Mark Seaborn, 2016-12-06 23:16:23]

https://codereview.chromium.org/2525483002/diff/80001/components/nacl/loader/...
File components/nacl/loader/nacl_trusted_listener.cc (right):

https://codereview.chromium.org/2525483002/diff/80001/components/nacl/loader/...
components/nacl/loader/nacl_trusted_listener.cc:39: io_task_runner->PostTask(
What happens if the channel's connection gets dropped before this posted task
runs?  Will the call to NaClExit() not happen?

[Sam McNally, 2016-12-07 03:20:35]

https://codereview.chromium.org/2525483002/diff/80001/components/nacl/loader/...
File components/nacl/loader/nacl_trusted_listener.cc (right):

https://codereview.chromium.org/2525483002/diff/80001/components/nacl/loader/...
components/nacl/loader/nacl_trusted_listener.cc:39: io_task_runner->PostTask(
On 2016/12/06 23:16:23, Mark Seaborn wrote:
> What happens if the channel's connection gets dropped before this posted task
> runs?  Will the call to NaClExit() not happen?

No. The close of the other end of the pipe will be detected after the binding is
created.

[Sam McNally, 2016-12-19 09:44:45]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 09:44:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-12-19 09:51:48]

sammc@chromium.org changed reviewers:
+ bradnelson@chromium.org

[Sam McNally, 2016-12-19 09:51:49]

+bradnelson

[commit-bot: I haz the power, 2016-12-19 10:50:49]

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

[commit-bot: I haz the power, 2016-12-19 10:50:50]

Dry run: This issue passed the CQ dry run.

[Mark Seaborn, 2016-12-19 19:00:37]

LGTM

https://codereview.chromium.org/2525483002/diff/100001/components/nacl/common...
File components/nacl/common/nacl.mojom (right):

https://codereview.chromium.org/2525483002/diff/100001/components/nacl/common...
components/nacl/common/nacl.mojom:24: // An empty interface whose closure
indicates that the NaCl plugin should exit.
Nit: "plugin" -> "loader process"

In the NaCl/Chromium code, we need to distinguish between the part of the plugin
that runs in the renderer process and the part that runs in the NaCl loader
process.  (The latter is not a great term, but it's the term that's used in the
codebase.)

Also, rather than saying "closure", how about "When this is closed, it indicates
that...".  The term "closure" could be misinterpreted.

https://codereview.chromium.org/2525483002/diff/100001/components/nacl/loader...
File components/nacl/loader/nacl_trusted_listener.h (right):

https://codereview.chromium.org/2525483002/diff/100001/components/nacl/loader...
components/nacl/loader/nacl_trusted_listener.h:21:
nacl::mojom::NaClRendererHost& renderer_host() { return *renderer_host_; }
Is there a reason why this is a reference?  I thought non-const references are
against the Chromium style.  Can you make it a pointer instead, please?

[Sam McNally, 2016-12-19 23:05:58]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 23:06:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-12-19 23:12:11]

sammc@chromium.org changed reviewers:
+ dcheng@chromium.org, jam@chromium.org

[Sam McNally, 2016-12-19 23:12:12]

+jam for //content, //components/typemaps.gni and the DEPS change

+dcheng for the mojom, messages and traits

https://codereview.chromium.org/2525483002/diff/100001/components/nacl/common...
File components/nacl/common/nacl.mojom (right):

https://codereview.chromium.org/2525483002/diff/100001/components/nacl/common...
components/nacl/common/nacl.mojom:24: // An empty interface whose closure
indicates that the NaCl plugin should exit.
On 2016/12/19 19:00:36, Mark Seaborn wrote:
> Nit: "plugin" -> "loader process"
> 
> In the NaCl/Chromium code, we need to distinguish between the part of the
plugin
> that runs in the renderer process and the part that runs in the NaCl loader
> process.  (The latter is not a great term, but it's the term that's used in
the
> codebase.)
> 
> Also, rather than saying "closure", how about "When this is closed, it
indicates
> that...".  The term "closure" could be misinterpreted.

Done.

https://codereview.chromium.org/2525483002/diff/100001/components/nacl/loader...
File components/nacl/loader/nacl_trusted_listener.h (right):

https://codereview.chromium.org/2525483002/diff/100001/components/nacl/loader...
components/nacl/loader/nacl_trusted_listener.h:21:
nacl::mojom::NaClRendererHost& renderer_host() { return *renderer_host_; }
On 2016/12/19 19:00:36, Mark Seaborn wrote:
> Is there a reason why this is a reference?  I thought non-const references are
> against the Chromium style.  Can you make it a pointer instead, please?

They're only banned as function parameters. Changed it anyway.

[jam, 2016-12-19 23:53:29]

On 2016/12/19 23:12:12, Sam McNally wrote:
> +jam for //content, //components/typemaps.gni and the DEPS change
lgtm

[commit-bot: I haz the power, 2016-12-20 00:17:55]

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

[commit-bot: I haz the power, 2016-12-20 00:17:58]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-12-20 06:49:55]

https://codereview.chromium.org/2525483002/diff/120001/components/nacl/common...
File components/nacl/common/nacl.mojom (right):

https://codereview.chromium.org/2525483002/diff/120001/components/nacl/common...
components/nacl/common/nacl.mojom:25: // should exit.
Possibly dumb quetion: what's the difference between the NaCl process and the
NaCl loader process?

https://codereview.chromium.org/2525483002/diff/120001/components/nacl/loader...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/2525483002/diff/120001/components/nacl/loader...
components/nacl/loader/nacl_listener.cc:355:
nacl::mojom::NaClRendererHost::Version_)),
Nit: can we document what "version" means somewhere? I looked at
InterfacePtrInfo, and don't really understand what it means for this to be 0 vs
something else. Perhaps it's related to mojom versioning?

https://codereview.chromium.org/2525483002/diff/120001/components/nacl/loader...
File components/nacl/loader/nacl_trusted_listener.cc (right):

https://codereview.chromium.org/2525483002/diff/120001/components/nacl/loader...
components/nacl/loader/nacl_trusted_listener.cc:26:
->set_connection_error_handler(base::Bind(&NaClExit, 0));
This feels like an abuse of mojo::MakeStrongBinding. At some point, I still hope
that we can land some variant of https://codereview.chromium.org/2515873003/.

https://codereview.chromium.org/2525483002/diff/120001/components/nacl/loader...
File components/nacl/loader/nonsfi/nonsfi_listener.cc (right):

https://codereview.chromium.org/2525483002/diff/120001/components/nacl/loader...
components/nacl/loader/nonsfi/nonsfi_listener.cc:128:
nacl::mojom::NaClRendererHost::Version_)),
I do kind of wonder if it's worth writing a simple template helper for this, but
I guess MakeProxy is not used a lot.

[Sam McNally, 2016-12-20 09:43:40]

The CQ bit was checked by sammc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 09:43:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-12-20 10:02:19]

https://codereview.chromium.org/2525483002/diff/120001/components/nacl/loader...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/2525483002/diff/120001/components/nacl/loader...
components/nacl/loader/nacl_listener.cc:355:
nacl::mojom::NaClRendererHost::Version_)),
On 2016/12/20 06:49:54, dcheng wrote:
> Nit: can we document what "version" means somewhere? I looked at
> InterfacePtrInfo, and don't really understand what it means for this to be 0
vs
> something else. Perhaps it's related to mojom versioning?

Yes, it's for mojom versioning.
https://www.chromium.org/developers/design-documents/mojo/versioning doesn't
seem to cover this part. Since this interface isn't versioned it doesn't
actually matter here.

In this case, since this is sending the pipe handle, it can start with the nice
type and extract the pipe handle instead of building the nice type from the pipe
handle. Once the other NaCl IPCs are converted, the pipe won't need to be
visible here.

https://codereview.chromium.org/2525483002/diff/120001/components/nacl/loader...
File components/nacl/loader/nacl_trusted_listener.cc (right):

https://codereview.chromium.org/2525483002/diff/120001/components/nacl/loader...
components/nacl/loader/nacl_trusted_listener.cc:26:
->set_connection_error_handler(base::Bind(&NaClExit, 0));
On 2016/12/20 06:49:54, dcheng wrote:
> This feels like an abuse of mojo::MakeStrongBinding. At some point, I still
hope
> that we can land some variant of https://codereview.chromium.org/2515873003/.

Yes, but it was so simple. Added a class for the binding to own.

https://codereview.chromium.org/2525483002/diff/120001/components/nacl/loader...
File components/nacl/loader/nonsfi/nonsfi_listener.cc (right):

https://codereview.chromium.org/2525483002/diff/120001/components/nacl/loader...
components/nacl/loader/nonsfi/nonsfi_listener.cc:128:
nacl::mojom::NaClRendererHost::Version_)),
On 2016/12/20 06:49:54, dcheng wrote:
> I do kind of wonder if it's worth writing a simple template helper for this,
but
> I guess MakeProxy is not used a lot.

I'm not entirely sure which part you're referring to. Using MakeProxy to go from
InterfacePtrInfo to InterfacePtr seems fairly straightforward. Making an
InterfacePtrInfo from a message pipe handle should be rare outside things like
InterfaceProvider/InterfaceRegistry and process/service startup. This one is
only here until the browser-nacl-loader IPCs are converted.

[dcheng, 2016-12-20 10:03:47]

mojo lgtm

[Sam McNally, 2016-12-20 11:09:36]

Description was changed from

==========
Convert NaCl renderer-plugin messages to mojo.

BUG=577685
==========

to

==========
Convert NaCl renderer-loader messages to mojo.

BUG=577685
==========

[Sam McNally, 2016-12-20 11:10:21]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-12-20 11:10:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2525483002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-12-20 11:10:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 11:42:58]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1482232221122650,
"parent_rev": "1f35ce81ccecf24689af31538b61215ef7021933", "commit_rev":
"792bf8919dd9d23e2812f14ba49f9858bf7ecc82"}

[commit-bot: I haz the power, 2016-12-20 11:43:24]

Description was changed from

==========
Convert NaCl renderer-loader messages to mojo.

BUG=577685
==========

to

==========
Convert NaCl renderer-loader messages to mojo.

BUG=577685

Review-Url: https://codereview.chromium.org/2525483002
==========

[commit-bot: I haz the power, 2016-12-20 11:43:25]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2016-12-20 11:46:06]

Description was changed from

==========
Convert NaCl renderer-loader messages to mojo.

BUG=577685

Review-Url: https://codereview.chromium.org/2525483002
==========

to

==========
Convert NaCl renderer-loader messages to mojo.

BUG=577685

Committed: https://crrev.com/eb52daa79263d35ea1c62b717427bf68467be04f
Cr-Commit-Position: refs/heads/master@{#439770}
==========

[commit-bot: I haz the power, 2016-12-20 11:46:07]

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

[findit-for-me, 2016-12-20 12:04:42]

FYI: Findit identified this CL at revision 439770 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[markusheintz_, 2016-12-20 12:07:43]

A revert of this CL (patchset #4 id:140001) has been created in
https://codereview.chromium.org/2590853002/ by markusheintz@chromium.org.

The reason for reverting is: CL does not compile on build bots

e.g.
https://build.chromium.org/p/chromium.linux/builders/Linux%20Builder%20%28dbg....

[markusheintz_, 2016-12-20 12:18:36]

On 2016/12/20 12:07:43, markusheintz_ wrote:
> A revert of this CL (patchset #4 id:140001) has been created in
> https://codereview.chromium.org/2590853002/ by
mailto:markusheintz@chromium.org.
> 
> The reason for reverting is: CL does not compile on build bots
> 
> e.g.
>
https://build.chromium.org/p/chromium.linux/builders/Linux%20Builder%20%28dbg....

For more context:

Colin (blundell@) pointed out that there was a race between his CL
https://codereview.chromium.org/2589663003 (which landed right before) and this
one. 

He actually already had a fix prepared, but I saw his message too late when I
already had the revert in. Sorry about that.

[blundell, 2016-12-20 12:22:05]

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

[blundell, 2016-12-20 12:22:05]

I'm landing a fixed version in https://codereview.chromium.org/2590173002/.