Add C++ wrappers for output parameters.

Add C++ wrappers for output parameters.

Define helper routines for doing array output using the new PP_OutputArray
struct. Define routines in the completion callback factory for doing output
parameters as parameters to the callback function.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126781

[brettw, 2012-03-08 20:01:01]

Okay, this is ready for review. I extracted just the C++ wrapper part from the
previous demo patch.

[dmichael (off chromium), 2012-03-09 18:23:27]

Is there some dev interface you could port to this so that it's getting
exercised a little bit? (I know you can't really test it in isolation)

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/array_output.cc
File ppapi/cpp/array_output.cc (right):

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/array_output.cc#newco...
ppapi/cpp/array_output.cc:29: // callback factory uses this. So ensure that
we're empty.
Ah. A comment of explanation in the header or a reference to this comment would
be good. Do they all need to support copy? I think most of them are just
providing the compiler defaults, except the WithStorage types. That makes sense,
but a clear explanation somewhere might prevent somebody from coming along and
breaking it by "fixing" it.

Or... we can see if there's a way to make CompletionCallbackFactory not require
the copy (haven't looked at that yet).

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/array_output.h
File ppapi/cpp/array_output.h (right):

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/array_output.h#newcode90
ppapi/cpp/array_output.h:90: PP_DCHECK(element_count > 0);
can't you be called with 0 elements? (here and elsewhere)

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/array_output.h#newcod...
ppapi/cpp/array_output.h:123: ResourceArrayOutputAdapter(std::vector<T>* output)
: output_(output) {
explicit (here and other 1-arg constructors)

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/array_output.h#newcod...
ppapi/cpp/array_output.h:126: ~ResourceArrayOutputAdapter() {
Considering you are using virtual methods here, I think you want virtual
destructors as well?

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/array_output.h#newcod...
ppapi/cpp/array_output.h:166: //    std::vector<int> result = adapter.output().
Looks like an unfinished line?

I still think something like:
std::vector<int> result;
result.swap(adapter.output());
would be nicer to put in the example, to keep people from needlessly copying.

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/array_output.h#newcod...
ppapi/cpp/array_output.h:173: ArrayOutputAdapterWithStorage(const
ArrayOutputAdapterWithStorage<T>& other) {
Do you really need a copy constructor (and assignment)? If so, shouldn't it copy
other.output_storage_? If the code here is really what you intend, it's
deserving of a comment.

I would have guessed we'd want to disallow copy & assign in all these.

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/array_output.h#newcod...
ppapi/cpp/array_output.h:198: //    std::vector<pp::Var> result =
adapter.output().
ditto on the example

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/array_output.h#newcod...
ppapi/cpp/array_output.h:200: // Thie one is non-inline since it's not
templatized.
nit: Thie->This

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/completion_callback.h
File ppapi/cpp/completion_callback.h (right):

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/completion_callback.h...
ppapi/cpp/completion_callback.h:198:
CompletionCallbackWithOutput(OutputStorageType* output)
explicit

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/completion_callback.h...
ppapi/cpp/completion_callback.h:209: /// @param[in] user_data The user data to
be passed to the callback function.
nit: I'm used to seeing the params listed in the same order as in the parameter
list.

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/completion_callback.h...
ppapi/cpp/completion_callback.h:229: /// @param[in] flags Bit field combination
of
nit: order again

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/output_traits.h
File ppapi/cpp/output_traits.h (right):

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/output_traits.h#newco...
ppapi/cpp/output_traits.h:26: namespace internal {
I've seen (and used) "subtle" for this kind of thing in the past. I actually
like "internal" better, but is subtle any kind of de-facto convention we should
follow?

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/output_traits.h#newco...
ppapi/cpp/output_traits.h:35: };
You're not using EnableIf anymore.

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/output_traits.h#newco...
ppapi/cpp/output_traits.h:106: return T(::pp::PASS_REF, t);
I don't think ::pp:: is necessary here.

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/output_traits.h#newco...
ppapi/cpp/output_traits.h:138: // be one reference that the browser has assigned
to us for the out param!
(This comment also applies to resources.)

http://codereview.chromium.org/9651002/diff/1/ppapi/utility/completion_callba...
File ppapi/utility/completion_callback_factory.h (right):

http://codereview.chromium.org/9651002/diff/1/ppapi/utility/completion_callba...
ppapi/utility/completion_callback_factory.h:21: typedef const T& ForwardType;
You don't appear to use ForwardType

http://codereview.chromium.org/9651002/diff/1/ppapi/utility/completion_callba...
ppapi/utility/completion_callback_factory.h:23: typedef const T& ParamType;
And ParamType is unnecessary; you can just use "const T&" in all cases (even if
the type comes in as const Foo&, it's guaranteed to do the right thing).

http://codereview.chromium.org/9651002/diff/1/ppapi/utility/completion_callba...
ppapi/utility/completion_callback_factory.h:528: BackPointer(FactoryType*
factory)
nit: explicit

http://codereview.chromium.org/9651002/diff/1/ppapi/utility/completion_callba...
ppapi/utility/completion_callback_factory.h:600: DispatcherWithOutput0(Method
method) : method_(method) {
nit: explicit

[brettw, 2012-03-11 05:15:42]

I tried a different approach to avoid the weird copy constructors on the array
output objects.

Instead, I never copy and instead swap in the contents of the dispatchers in the
completion callback factory. This means I have to provide swap() implementation
for all of my array adapters.

This is nice because it's also more efficient for creating the closure, even for
the non-output case, since it avoids copying the bound arguments.

The other option is to kill the NewCallbackHelper functions and inline that code
in all of the NewCallback implementations, or to allocate the dispatcher on the
heap.

Allocating it on the heap is tempting, let me know what you think about that.

The swap function on the arrays are a bit weird since I need to implement it for
each one and I didn't really want a virtual function. This works out fine for us
since we never reference the base class. I'm not sure if there's a best practice
for this.

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/array_output.h
File ppapi/cpp/array_output.h (right):

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/array_output.h#newcod...
ppapi/cpp/array_output.h:198: //    std::vector<pp::Var> result =
adapter.output().
I just changed these to const refs.

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/output_traits.h
File ppapi/cpp/output_traits.h (right):

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/output_traits.h#newco...
ppapi/cpp/output_traits.h:26: namespace internal {
There's a difference between "subtle" and "internal". "subtle" means "here be
dragons" for stuff like atomic operations. "internal" is for stuff you're not
supposed to be using but has to be in a public header file for some reason.

[brettw, 2012-03-11 05:40:43]

I think I'd prefer to heap allocate the dispatcher. Lemme know what you think
before I actually bother writing that.

I didn't really want to implement any interface with this yet to avoid breaking
people and making this more difficult to land. That's why I did the patch this
is based on that has a new interface for testing. I was planning on converting
some stuff once this is checked in.

[dmichael (off chromium), 2012-03-13 05:04:04]

I agree, heap allocating the dispatcher will be cleaner.

I think it would make sense to land the removal of "NewRequiredCallback" in a
separate CL before this one.

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/array_output.h
File ppapi/cpp/array_output.h (right):

http://codereview.chromium.org/9651002/diff/1/ppapi/cpp/array_output.h#newcod...
ppapi/cpp/array_output.h:198: //    std::vector<pp::Var> result =
adapter.output().
On 2012/03/11 05:15:42, brettw wrote:
> I just changed these to const refs.
That's fine. I think non-const ref is also fine here (and allows swap).

http://codereview.chromium.org/9651002/diff/4002/ppapi/cpp/array_output.h
File ppapi/cpp/array_output.h (right):

http://codereview.chromium.org/9651002/diff/4002/ppapi/cpp/array_output.h#new...
ppapi/cpp/array_output.h:130: ~ResourceArrayOutputAdapter() {
style nit: virtual

http://codereview.chromium.org/9651002/diff/4002/ppapi/utility/completion_cal...
File ppapi/utility/completion_callback_factory.h (right):

http://codereview.chromium.org/9651002/diff/4002/ppapi/utility/completion_cal...
ppapi/utility/completion_callback_factory.h:526: BackPointer(FactoryType*
factory)
nit: explicit

http://codereview.chromium.org/9651002/diff/4002/ppapi/utility/completion_cal...
ppapi/utility/completion_callback_factory.h:586: Dispatcher0(Method method) :
method_(method) {
nit: explicit

http://codereview.chromium.org/9651002/diff/4002/ppapi/utility/completion_cal...
ppapi/utility/completion_callback_factory.h:605: DispatcherWithOutput0(Method
method) : method_(method) {
nit: explicit

[brettw, 2012-03-13 05:16:21]

Oh, sorry. The NewRequiredCallback stuff has already landed but was in my git
history. It's not part of this patch.

[brettw, 2012-03-13 23:26:03]

PTAL

[dmichael (off chromium), 2012-03-14 18:47:08]

lgtm

http://codereview.chromium.org/9651002/diff/15007/ppapi/cpp/array_output.h
File ppapi/cpp/array_output.h (right):

http://codereview.chromium.org/9651002/diff/15007/ppapi/cpp/array_output.h#ne...
ppapi/cpp/array_output.h:98: std::vector<T>* output_;
you should probably disallow copy & assign (at least at the base) since the
default will do the wrong thing for some of these classes.

http://codereview.chromium.org/9651002/diff/15007/ppapi/utility/completion_ca...
File ppapi/utility/completion_callback_factory.h (right):

http://codereview.chromium.org/9651002/diff/15007/ppapi/utility/completion_ca...
ppapi/utility/completion_callback_factory.h:605: DispatcherWithOutput0(Method
method) : method_(method) {
nit:explicit

[commit-bot: I haz the power, 2012-03-14 19:34:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brettw@chromium.org/9651002/14003

[commit-bot: I haz the power, 2012-03-14 23:17:15]

Change committed as 126781