Pepper: Move JsonManifestMap to json_manifest.h

Pepper: Move JsonManifestMap to json_manifest.h

I'd like to break out OpenManifestEntry and some other functions out of
ppb_nacl_private_impl.cc so they're easier to reuse and less tightly-bound to
the PPB_NaCl_Private interface.

BUG=239656
R=dmichael@chromium.org

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

[dmichael (off chromium), 2014-06-03 17:46:17]

https://codereview.chromium.org/310113002/diff/1/components/nacl/renderer/jso...
File components/nacl/renderer/json_manifest.h (right):

https://codereview.chromium.org/310113002/diff/1/components/nacl/renderer/jso...
components/nacl/renderer/json_manifest.h:28: void DeleteJsonManifest(PP_Instance
instance);
Arguably these could be static members, and you could drop JsonManifest from the
names. I feel like that's a more common style in Chromium.

I take it the lifetime of NexeLoadManager isn't quite right for just making each
NexeLoadManager own a JsonManifest?

[teravest, 2014-06-03 17:49:57]

On Tue, Jun 3, 2014 at 11:46 AM,  <dmichael@chromium.org> wrote:
>
>
https://codereview.chromium.org/310113002/diff/1/components/nacl/renderer/jso...
> File components/nacl/renderer/json_manifest.h (right):
>
>
https://codereview.chromium.org/310113002/diff/1/components/nacl/renderer/jso...
> components/nacl/renderer/json_manifest.h:28: void
> DeleteJsonManifest(PP_Instance instance);
> Arguably these could be static members, and you could drop JsonManifest
> from the names. I feel like that's a more common style in Chromium.

I understand that for Get(), but isn't that weird for Add() and Delete()?

>
> I take it the lifetime of NexeLoadManager isn't quite right for just
> making each NexeLoadManager own a JsonManifest?

I might be able to tie the lifetimes to NexeLoadManager; I just didn't
want to make that any larger than it already is.

>
> https://codereview.chromium.org/310113002/

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

[dmichael (off chromium), 2014-06-03 18:00:51]

On 2014/06/03 17:49:57, teravest wrote:
> On Tue, Jun 3, 2014 at 11:46 AM,  <mailto:dmichael@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/310113002/diff/1/components/nacl/renderer/jso...
> > File components/nacl/renderer/json_manifest.h (right):
> >
> >
>
https://codereview.chromium.org/310113002/diff/1/components/nacl/renderer/jso...
> > components/nacl/renderer/json_manifest.h:28: void
> > DeleteJsonManifest(PP_Instance instance);
> > Arguably these could be static members, and you could drop JsonManifest
> > from the names. I feel like that's a more common style in Chromium.
> 
> I understand that for Get(), but isn't that weird for Add() and Delete()?
I'm not sure. Trying to think of examples. To me personally,
JsonManifest::Create(instance);
JsonManifest::Get(instance);
JsonManifest::Delete(instance);
would all make sense and read well. But it might be my personal preference. The
fact that there's an implied global map is kind of icky in either case, though.
I'd prefer having a place to hang it.

If you stick with the global map-ish way, I guess while I slightly prefer static
methods, I don't care enough to fight for it.

> 
> >
> > I take it the lifetime of NexeLoadManager isn't quite right for just
> > making each NexeLoadManager own a JsonManifest?
> 
> I might be able to tie the lifetimes to NexeLoadManager; I just didn't
> want to make that any larger than it already is.
I think just having a member that's a JsonManifest (possibly by pointer) with a
getter doesn't seem to add too much, though.

I could see it gradually becoming a grab bag for all the things we need to load
1 single nexe, but where most of the guts live in other classes. So it wouldn't
be too big itself, but would delegate to other classes.

Your way seems fine for now, too; lgtm and use your judgement.

[teravest, 2014-06-04 17:46:51]

Committed patchset #4 manually as r274860 (presubmit successful).