App APIs in Pepper: C++ APIs

ppapi/cpp/dev/struct_wrapper_base_dev.h

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef PPAPI_CPP_DEV_STRUCT_WRAPPER_BASE_DEV_
+#define PPAPI_CPP_DEV_STRUCT_WRAPPER_BASE_DEV_
+
+#include "ppapi/cpp/dev/may_own_ptr_dev.h"
+
+namespace pp {
+namespace internal {
+
+// The only purpose of this class is for CallbackOutputTraits to determined

[dmichael (off chromium), 2013/12/17 18:03:25]

determined->determine

[yzshen1, 2013/12/17 23:52:21]

Done.

+// whether a class is a struct wrapper.
+class StructWrapperIdentifier {

[dmichael (off chromium), 2013/12/17 18:03:25]

I mentioned this elsewhere, but I don't think you need this. You can do an
explicit full specialization for each "struct wrapper" kind of class.

[yzshen1, 2013/12/17 23:52:21]

I think one good thing about this approach is that it doesn't need to have
specialized output traits defined in those "user-facing" C++ API headers (e.g.,
alarms_dev.h). Having less and simpler code (especially internal impl details) 
in the C++ API headers may be easier for developers. Although they could read
our documents, I think many of them will still want to look at the headers. I
would like to keep things that they don't need to care out of the API headers,
if possible.

And this approach is the same as what we did for pp::Resource subclasses. I
think using the same approach is nice.

Is it your concern that this approach causes output_traits.h to depend on a file
in the /dev folder? I think once we support the new dev api design, this problem
will be gone. And before that, if we want to avoid the issue, we could move this

StructWrapperIdentifier class to the output_traits.h file.

What do you think? Thanks!
On 2013/12/17 18:03:25, dmichael wrote:

[dmichael (off chromium), 2013/12/18 18:24:04]

Similarly, it also leaves part of the public API out of the header. I think a
little bit of inscrutable code is probably fine, if it means (a) our maintenance
is easier and (b) the header is _in_total_ easier to understand, because it
won't hide things in a template base that the developer has to look at and
understand (filling in template parameters in their head).
I agree with that. I think we might actually be making things worse by removing
public API from the header.
If that's important enough to us, we can separate some things out (like
specializations) in to a separate header that the developer-facing one includes.
E.g.:
ppb_foo.h
ppb_foo_internal.h

I expect that wouldn't be worth the trouble, though.
In my mind, this is different from pp::Resource in a couple of important ways:
1) pp::Resource meets the "is-a" test for those classes. They inherit
pp::Resource because they *are* Resources. "StructWrapperBase" feels more like
implementation inheritence to me; we don't seem to actually share an API surface
there. Certainly not in StructWrapperIdentifier. The only virtual functions
don't really need to be virtual if we remove that inheritance.
2) The existing pp::Resource classes already existed when the
CompletionCallbackWithOutput stuff was written, and they're not automatically
generated. The little "trick" was applied to a base class that already existed
to solve this problem for existing code (and future) hand-written code. In this
CL, you introduce a base class for the sole purpose of doing the inheritance
trick.
I don't really mind that; as you say, it's temporary. I just disagree with the
tradeoff we're making here. The template goop is complicated and difficult to
maintain. We shouldn't add more complexity there lightly. I like using templates
for simplifying hand-written code. In this case, the code we're concerned with
is generated, so I think we don't get enough benefit from adding the complexity
of introducing StructWrapperBase et al.

[yzshen1, 2013/12/18 19:18:56]

Thanks David!

I think you persuaded me. I will remove
StructWrapperBase/StructWrapperIdentifier and update the CL.

On 2013/12/18 18:24:04, dmichael wrote:

[dmichael (off chromium), 2013/12/19 15:57:44]

Thanks, Yuzhu! I hope it turns out clean. If it causes problems, let me know and
we can re-evaluate.

[yzshen1, 2013/12/19 17:57:58]

I think it looks nice now. You have a good point that putting part of the public
methods in the base class doesn't make it more readable/cleaner. Thanks!

+};
+
+// The base class for struct wrappers.
+template <typename Type, typename ArrayType = void>
+class StructWrapperBase : public StructWrapperIdentifier {

[dmichael (off chromium), 2013/12/17 18:03:25]

It seems slightly messy to me to have this as a base class, when I think it's
not really necessary. This seems to have two separate responsibilities:
- A traits style class that does typedefs for you. That could be a separate
traits class instead of a base class, I think.
- A data holder (using MayOwnPtr). This has some weird consequences; since this
class isn't copyable, you have to use operator= in the derived classes.

Would it be possible to separate this in to two things:
1) A "traits" class that only has typedefs and possibly static functions?
2) A "holder" class that the generated class can hold as a member? (Possibly
just MayOwnPtr, I don't know).

It's very possible I'm missing something, but if there's a cleaner way that
doesn't rely on implementation inheritance, I think I might like that better.

WDYT?

[yzshen1, 2013/12/17 23:52:21]

I made this StructWrapperBase class based on the following reasons:
- As I mentioned above, I wish those "user-facing" headers (such as
alarms_dev.h) to only contain code that developers need to care. Therefore, I
tried to move things needed only by our internal impl (typedef / ToStruct /
StartRawUpdate / EndRawUpdate) into this base class. I know it is somewhat
cheating, they are still public in the subclasses. :)  But the headers do seem
cleaner to me.

- Having the typedef in the struct wrapper doesn't seem too bad to me. It is
reasonable that a struct wrapper has type information about the C struct that it
wraps. If I make separate traits classes, the specialization has to live in
those "user-facing" headers, which I would like to avoid. But probably I could
remove CInputType and COutputType entirely.

- Having |storage_| in the base class is nice because it has to be created
before (and destroyed after) all the accessors that attach to it. (By accessors,
I mean things like Alarms_Dev::name_wrapper_.)

What do you think? Thanks!

[dmichael (off chromium), 2013/12/18 18:24:04]

Alternately, if you need the typedefs, maybe you can generate them directly in
the ppb_alarms header. That would be the most straightforward to read and
understand.
The generator can make sure that happens if it's a member, though, right? That
might even make the ordering dependency more explicit.

+ public:
+  // The C struct type that this class acts as a wrapper for.
+  typedef Type CType;
+  // The corresponding C array type, used by pp::Array.
+  typedef ArrayType CArrayType;
+
+  // The corresponding C output parameter type.
+  typedef CType* COutputType;
+  // The corresponding C input parameter type.
+  typedef const CType* CInputType;
+
+  StructWrapperBase() {}
+
+  // It doesn't take ownership of |storage|, therefore |storage| must live
+  // longer than this object. |storage| must be zero-initialized.

[dmichael (off chromium), 2013/12/17 18:03:25]

I think you mean that the caller must zero-initialize |*storage|? It would be
good to clarify. (note you mean the contents of *storage, right, not the pointer
itself?)

[yzshen1, 2013/12/17 23:52:21]

Right, the contents pointed to by |storage| must be zero-initialized by the
caller.
Done. I revised the comments.

On 2013/12/17 18:03:25, dmichael wrote:

+  StructWrapperBase(CType* storage, NotOwned) : storage_(storage, NOT_OWNED) {}

[dmichael (off chromium), 2013/12/17 18:03:25]

If there's not a constructor that takes ownership, why is there all this
complexity around specifying "NOT_OWNED" and having a MayOwnPtr?

[yzshen1, 2013/12/17 23:52:21]

I was thinking NOT_OWNED makes it more obvious that this constructor doesn't
take ownership. You are right, no constructor will take ownership of passed-in
storage. Do you think I should remove NOT_OWNED and only rely on the comments to
make the ownership clear?

MayOwnPtr is necessary: Other constructors will create the underlying storage
which is owned by the MayOwnPtr.

The reason of this design is that a struct wrapper (and many other things such
as Optional<>, Array<>, StringWrapper) can act as a standalone object (owns the
underlying C struct), or an accessor (doesn't own).

For example:
struct PP_Bar {};
struct PP_Foo {
  struct PP_Bar bar;
};

class Foo {
...
  private:
    MayOwnPtr<PP_Foo> storage_;
    Bar bar_wrapper_;  // this accessor is attached to |storage_->bar|, it
doesn't own the memory.
};

[dmichael (off chromium), 2013/12/18 18:24:04]

Sorry, I realized later that a default-constructed StructWrapperBase would own
the memory, and forgot to revisit this comment.

I think this is one of the things where a motivating example or test might help.
I wish we didn't need these dual ways of owning the data. But I think you're
looking ahead to dealing with parameters to the browser (plugin-owned) and
parameters to the plugin (browser-owned), and I understand it might be
necessary.

[yzshen1, 2013/12/18 19:18:56]

I did the MayOwnPtr<> design mostly for accessing structure members and array
elements.
For example,

struct PP_Bar {};
struct PP_Foo {
  struct PP_Bar bar;
};

The memory of |bar| is owned by the containing struct PP_Foo. When creating an
accessor to wrap |bar|, it cannot manage the memory.

Take Alarm_Dev as an example, |period_in_minutes_wrapper_| is directly accessing
the member |period_in_minutes| in PP_Alarms_AlarmCreateInfo_Dev. It cannot own
that memory.
I considered to support accessor for const pointers (so that we can wrap those
input parameters passing from the browser to the plugin, e.g., for event
listeners). But I finally decided that the performance gain didn't outweigh the
complexity. (I would be happy to talk more about it if you are interested.) For
those browser->plugin parameter, currently I am using the Foo(const PP_Foo&)
constructor which does a deep copy.

[dmichael (off chromium), 2013/12/19 15:57:44]

Do you mean in PP_Alarms_Alarm_Dev?
Oops, I missed that detail before. I think I'm more confused :). I might have
guessed that the straightforward way to deal with it would be to make the
accessor/setter work directly on the PP_Alarms_Alarm_Dev's field? If I was
hand-writing it, that's how I think I would do it.

Especially if we remove the base class and make the struct a member (either
directly or with some helper/wrapper around it), if you can make the code
generator just reach directly in to the struct, that seems easier to understand.

WDYT?

[yzshen1, 2013/12/19 17:57:58]

Yes. Typo. :)
Currently I only directly access struct members of primitive types, such as
Alarm_Dev::scheduled_time(). All other things are accessed with the help of
accessor objects, including Optional<double>. I think it is okay to expand the
logic of simple things like Optional<double> into their owner classes. Other
things, for example, StringWrapper, OptionalStringWrapper, Array<>,
Optional<Array<> > are more complex accessors, it is not suitable to expand the
logic into their owner classes, IMO. And also consider struct members (still use
the Foo/Bar example mentioned above):

class Foo {
  // We will need to create an accessor for PP_Foo.bar struct member anyway.
  Bar& bar();
  const Bar& bar() const;
};

What do you think?

+
+  virtual ~StructWrapperBase() {}
+
+  // For input parameters to call PPB_* functions.
+  const CType* ToStruct() const { return storage_.get(); }
+
+  // For output parameters to call PPB_* functions. The returned pointer is
+  // still owned by this object. And after it is used, EndRawUpdate() must be
+  // called.
+  CType* StartRawUpdate() {
+    NotifyStartRawUpdate();
+    return storage_.get();
+  }
+
+  void EndRawUpdate() { NotifyEndRawUpdate(); }
+
+ protected:

[dmichael (off chromium), 2013/12/17 18:03:25]

If subclasses aren't supposed to _invoke_ these, maybe they should be private.

[yzshen1, 2013/12/17 23:52:21]

You are right. Thanks! Done.

+  // Subclasses should implement these methods to notify members that raw update
+  // to the C struct has started / ended.
+  virtual void NotifyStartRawUpdate() = 0;
+  virtual void NotifyEndRawUpdate() = 0;

[dmichael (off chromium), 2013/12/17 18:03:25]

I think these wouldn't be necessary if the struct wrapper thingy was a member
instead of a base class. The generated code would just provide its own
StartRawUpdate that calls StartRawUpdate on all members, including the struct
wrapper one.

[yzshen1, 2013/12/17 23:52:21]

I used them so that the public section of the subclasses doesn't have to
explicitly declare  StartRawUpdate()/EndRawUpdate(). I thought that makes the
user-facing headers look simpler, because developers usually don't need to care
about them. (But I agree that they are still public in the subclasses.)

On the other hand, removing them has the upside that this class is not virtual
and is more efficient..

I am fine with either way.  What do you think? 

On 2013/12/17 18:03:25, dmichael wrote:

+
+  MayOwnPtr<CType> storage_;
+
+ private:
+  // Although subclasses usually support copying and assignment, they shouldn't
+  // try to copy or assign the contents of this class.
+  StructWrapperBase(const StructWrapperBase<CType, CArrayType>& other);
+  StructWrapperBase<CType, CArrayType>& operator=(
+      const StructWrapperBase<CType, CArrayType>& other);
+};
+
+}  // namespace internal
+}  // namespace pp
+
+#endif  // PPAPI_CPP_DEV_STRUCT_WRAPPER_BASE_DEV_