Heads up: src/trusted/plugin/ moved to components/nacl/renderer/plugin/

[Mark Seaborn]

I've moved NaCl's "trusted plugin" code (ppapi/native_client/src/trusted/plugin/) to components/nacl/renderer/plugin/ in this change:

https://codereview.chromium.org/876483002/

So if you're wondering where it went, that's where to find it. :-)

From the commit message:

This moves the "trusted plugin" code so that all the NaCl renderer-side code is in the same place, under components/nacl/renderer/.  I've used the "renderer/plugin/" subdir so that this legacy code is distinguished from the newer code that's directly under "renderer/".

This will make further incremental cleanups easier: It will enable closer integration between renderer/* and renderer/plugin/*.  Rather than indirecting through PPB_NaCl_Private/PPP_PexeStreamHandler, it would be reasonable for the two sides to share pointers to C++ objects.  Doing that will be more acceptable when the two sides live in the same directory.

Cheers,

Mark

[David Michael]

That's great, thank you for working on this!

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

[Mark Seaborn]

I think a good next step would be to move ppb_nacl_private.h out of ppapi/c/private/ and into components/nacl/renderer/, and drop the .idl file that it is generated from.  I've started preparing a change for that (https://codereview.chromium.org/911463003/).

Since PPB_NaCl_Private is both implemented and consumed in components/nacl/renderer/, there's no need for the interface to be defined in ppapi/.  The fact that this is a PPAPI interface at all is just a holdover from when the code for the NaCl trusted plugin lived outside the Chromium repo.

This move would have a few benefits:

 * It reduces the friction for changing ppb_nacl_private.h, because it would no longer be necessary to change ppb_nacl_private.idl and re-run the generator.

 * It removes PPB_NaCl_Private from the PPAPI shims, saving some code size (fixing https://crbug.com/251460).

 * It means ppb_nacl_private.h no longer has to follow the PPAPI C header style.  For example, it would no longer need to define its own "struct PP_NaClFileInfo" type -- it could share a definition that's marshallable over Chrome IPC.  (That would make changes like Yusuke's open_resource() change cleaner -- https://codereview.chromium.org/649603004/.)

Does that sound OK?

After that, a further cleanup would be to remove the PPB_NaCl_Private struct and change the trusted plugin code to directly call the functions in ppb_nacl_private_impl.cc, or have this file define a C++ object.  But that refactoring would be more of a manual change.  We should probably rename "ppb_nacl_private" to something else, too -- any suggestions?

Cheers,

Mark

[David Michael]

SGTM, given that you plan to ultimately make it not a PPAPI struct. It
will be slightly weird in the interim to have a Pepper-looking
interface outside ppapi, but I think we can arguably consider it "not
really part of pepper" even now. Certainly it won't be once it's not
accessed via PPB_GetInterface.

Making it non-Pepper will hopefully also make clear that we no longer
need to worry about having different heaps, so it's OK to do things
like pass std::string across the boundary (correct me if I'm wrong).
Generally, it might lessen a lot of the friction in moving classes and
code from one side of the plugin boundary to the other.

Name suggestions...
ChromiumNaClServices
or something. I'm not too picky.

Thanks again for working on this.

[Bill Budge]

This sounds good to me.

Having IDL for this API is kind of pointless, since it's a single-use API and we have never bothered to version it.

There is enough state being stored in ppb_nacl_private_impl.cc that I think we will probably end up with some types. 'NaClPluginInstance' is defined in this file and it or something similar like NaClPluginInstanceHost seems like a good name to me.

[Bill Budge]

My proposed class name is for the type that corresponds to a single plugin instance. It would need to define some static method or methods to create instances.

-Bill

[Justin TerAvest]

On Mon, Feb 9, 2015 at 1:38 PM, Mark Seaborn <msea...@chromium.org> wrote:
> I think a good next step would be to move ppb_nacl_private.h out of
> ppapi/c/private/ and into components/nacl/renderer/, and drop the .idl file
> that it is generated from. I've started preparing a change for that
> (https://codereview.chromium.org/911463003/).
>
> Since PPB_NaCl_Private is both implemented and consumed in
> components/nacl/renderer/, there's no need for the interface to be defined
> in ppapi/. The fact that this is a PPAPI interface at all is just a
> holdover from when the code for the NaCl trusted plugin lived outside the
> Chromium repo.
>
> This move would have a few benefits:
>
> * It reduces the friction for changing ppb_nacl_private.h, because it would
> no longer be necessary to change ppb_nacl_private.idl and re-run the
> generator.
> * It removes PPB_NaCl_Private from the PPAPI shims, saving some code size
> (fixing https://crbug.com/251460).
> * It means ppb_nacl_private.h no longer has to follow the PPAPI C header
> style. For example, it would no longer need to define its own "struct
> PP_NaClFileInfo" type -- it could share a definition that's marshallable
> over Chrome IPC. (That would make changes like Yusuke's open_resource()
> change cleaner -- https://codereview.chromium.org/649603004/.)
>
> Does that sound OK?

Sounds OK to me, as long as there's a comment in ppb_nacl_private.h
(or whatever its renamed to) with a note about what headers can be
included from there. I know that anything that includes base/logging.h
is problematic, because it causes collisions on some logging macros.

>
> After that, a further cleanup would be to remove the PPB_NaCl_Private struct
> and change the trusted plugin code to directly call the functions in
> ppb_nacl_private_impl.cc, or have this file define a C++ object. But that
> refactoring would be more of a manual change. We should probably rename
> "ppb_nacl_private" to something else, too -- any suggestions?

My only thought is TrustedPluginSupport; though I don't like the term
"trusted plugin" very much.

> --
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Dev" group.

> To unsubscribe from this group and stop receiving emails from it, send an

> email to native-client-...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Mark Seaborn]

Yes, I think there's a conflict between Chromium's base/logging.h and NaCl's nacl_log.h, which both define LOG_FATAL etc.

We can avoid that by changing the Chromium-side code to stop using nacl_log.h.  I think there's only two files using it remaining (components/nacl/renderer/plugin/service_runtime.cc and components/nacl/loader/nonsfi/irt_exception_handling.cc).

With that fixed, I suspect it should be possible for the code in nacl/renderer/plugin/ to start using base/ and Chrome IPC directly.

Cheers,

Mark