Create a VarPrivate interface to contain the scripting helper functions of Var.

Create a VarPrivate interface to contain the scripting helper functions of Var.

Currently, the old functions are left in Var. When people have a chance to move
to this new API, we can delete them from Var.

This also adds new enums for ARRAY and DICTIONARY vars, and makes the var C++
wrapper forward-compatible with them by refcounting any enums that it doesn't
know about.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81068

[dmichael(do not use this one), 2011-04-08 22:16:28]

http://codereview.chromium.org/6823016/diff/2005/ppapi/c/pp_var.h
File ppapi/c/pp_var.h (right):

http://codereview.chromium.org/6823016/diff/2005/ppapi/c/pp_var.h#newcode30
ppapi/c/pp_var.h:30: * A var with an undefined value.
Maybe just say 'An undefined value.' to be consistent with the others.

http://codereview.chromium.org/6823016/diff/2005/ppapi/c/pp_var.h#newcode63
ppapi/c/pp_var.h:63: * Represents a JavaScript object. This vartype is not
currently usable
maybe take out 'currently'?  We should avoid giving hope that we will provide
that, unless we're sure we will.

http://codereview.chromium.org/6823016/diff/2005/ppapi/c/pp_var.h#newcode70
ppapi/c/pp_var.h:70: * in future revisions. These objects are reference counteed
so be sure
s/counteed/counted

http://codereview.chromium.org/6823016/diff/2005/ppapi/c/pp_var.h#newcode75
ppapi/c/pp_var.h:75: PP_VARTYPE_DICTIONARY
After thinking about it a little more, I still have my doubts that we need to
reference-count these.  For that matter, I would rather see string not be
ref-counted;  it seems like premature optimization.  I think ref counting is
harder to understand than just saying "You are responsible for deleting this
when you are done by calling PPB_Core.MemFree."  Provide a deep-copy for
DICTIONARY.  Users can ref-count with their own scheme if they want.

Ref counting is important for PP_VARTYPE_OBJECT, since it's shared between
browser and plugin, but strint, array, and dictionary can just transfer
ownership.

You guys are probably right, but I just don't understand what the big advantage
is with forcing ref counting on developers when the objects aren't shared
between browser and plugin.

Maybe we can chat about this early next week...

http://codereview.chromium.org/6823016/diff/2005/ppapi/c/pp_var.h#newcode119
ppapi/c/pp_var.h:119: * indicate a "NULL handle."
if we _don't_ do reference counting, we can use a bare pointer for
array/dictionary.  If we do ref counting, then I agree with the doc.

http://codereview.chromium.org/6823016/diff/2005/ppapi/cpp/private/var_private.h
File ppapi/cpp/private/var_private.h (right):

http://codereview.chromium.org/6823016/diff/2005/ppapi/cpp/private/var_privat...
ppapi/cpp/private/var_private.h:28: VarPrivate(const VarPrivate& other) :
Var((const Var&)other) {}
Assuming we really do want conversion contructors, should we //NOLINT these?

http://codereview.chromium.org/6823016/diff/2005/ppapi/cpp/var.h
File ppapi/cpp/var.h (right):

http://codereview.chromium.org/6823016/diff/2005/ppapi/cpp/var.h#newcode204
ppapi/cpp/var.h:204: Var(void* non_scriptable_object_pointer);
It seems like this constructor could still be private (VarPrivate shouldn't have
to call it...)

[brettw, 2011-04-09 16:41:34]

http://codereview.chromium.org/6823016/diff/2005/ppapi/c/pp_var.h
File ppapi/c/pp_var.h (right):

http://codereview.chromium.org/6823016/diff/2005/ppapi/c/pp_var.h#newcode75
ppapi/c/pp_var.h:75: PP_VARTYPE_DICTIONARY
If we had designed this API without scripting from the beginning then it's quite
possible we would have come up with a non-refcounting solution.

It seems like it would be a bad idea to have some vars be refcounted and some
require a special free function since it would be impossible to know how to do
any memory management. And given that we have a refcounted API today, I think
it's not really practical to change (and it's worked pretty well, the C++ API is
pretty clean and you don't really have to worry about this stuff).

[brettw, 2011-04-10 05:08:41]

New snap up.

http://codereview.chromium.org/6823016/diff/2005/ppapi/cpp/private/var_private.h
File ppapi/cpp/private/var_private.h (right):

http://codereview.chromium.org/6823016/diff/2005/ppapi/cpp/private/var_privat...
ppapi/cpp/private/var_private.h:28: VarPrivate(const VarPrivate& other) :
Var((const Var&)other) {}
The conversion constructors are important for the Vars being usable (otherwise
the scripting API starts looking like:
  foo.Call(pp::Var("function"), pp::Var(2));
instead of
  foo.Call("function", 2);

I tried sticking NOLINT in and it looked stupid. I'm hoping that this header
will be used externally, so I'd prefer not to ugly up the header with extra
stuff to make our code review tool happy.

BTW The C cast is unnecessary and I removed it (was an experiment I forgot to
remove).

[dmichael (off chromium), 2011-04-11 00:09:15]

LGTM

http://codereview.chromium.org/6823016/diff/2005/ppapi/c/pp_var.h
File ppapi/c/pp_var.h (right):

http://codereview.chromium.org/6823016/diff/2005/ppapi/c/pp_var.h#newcode75
ppapi/c/pp_var.h:75: PP_VARTYPE_DICTIONARY
On 2011/04/09 16:41:34, brettw wrote:
> If we had designed this API without scripting from the beginning then it's
quite
> possible we would have come up with a non-refcounting solution.
> 
> It seems like it would be a bad idea to have some vars be refcounted and some
> require a special free function since it would be impossible to know how to do
> any memory management. And given that we have a refcounted API today, I think
> it's not really practical to change (and it's worked pretty well, the C++ API
is
> pretty clean and you don't really have to worry about this stuff).
Thanks for explaining.  I think we may still need some special 'free' function,
because removing a ref count from a dictionary with a cycle back to the root
shouldn't delete it.  So a nice 'delete this whole graph' function may be
worthwhile.  But otherwise, I can live with ref counting.

http://codereview.chromium.org/6823016/diff/2005/ppapi/cpp/private/var_private.h
File ppapi/cpp/private/var_private.h (right):

http://codereview.chromium.org/6823016/diff/2005/ppapi/cpp/private/var_privat...
ppapi/cpp/private/var_private.h:28: VarPrivate(const VarPrivate& other) :
Var((const Var&)other) {}
On 2011/04/10 05:08:41, brettw wrote:
> The conversion constructors are important for the Vars being usable (otherwise
> the scripting API starts looking like:
>   foo.Call(pp::Var("function"), pp::Var(2));
> instead of
>   foo.Call("function", 2);
> 
> I tried sticking NOLINT in and it looked stupid. I'm hoping that this header
> will be used externally, so I'd prefer not to ugly up the header with extra
> stuff to make our code review tool happy.
> 
> BTW The C cast is unnecessary and I removed it (was an experiment I forgot to
> remove).
Makes sense.