Add PPB_VarDictionary_Dev support - part 1.

Add PPB_VarDictionary_Dev support - part 1.

It includes:
- C/C++ interface implementation.
- Conversions between PP_Var and base::Value.

It dones't include:
- Serialization code for IPC.

BUG=None
TEST=a new unittest

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

[yzshen1, 2013-03-03 23:33:49]

Hi, David.

Would you please take a look? Thanks!

[dmichael (off chromium), 2013-03-05 22:01:03]

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
File ppapi/shared_impl/dictionary_var.cc (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
ppapi/shared_impl/dictionary_var.cc:47:
PpapiGlobals::Get()->GetVarTracker()->AddRefVar(iter->second.get());
If the plugin mis-manages its ref-count and decrements one more than it should,
the actual backing Var may already be deleted. It will cause you to hit a
NOTREACHED in AddRefVar (and returns false.)

So in a release build iter->second.get() is an invalid PP_Var in that case.

Maybe it would be enough to just check the result of AddRefVar? If it fails you
can (optionally) erase the iter and return PP_MakeUndefined()? I know it's a
corner case, but that seems nicer than returning PP_MakeUndefined.

Alternately, you could keep an internal ref using scoped_refptr. Then even if
they screw up their ref-counting, they get an actual object back. You could
replace the ScopedPPVar stuff with just that. (But that goes to your point about
documenting the ref-counting in the header. Maybe it's enough to just say that
the children will not be destroyed as long as they are in a container or
something).

I'm torn on which I like better :-). WDYT?

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
File ppapi/shared_impl/dictionary_var.h (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
ppapi/shared_impl/dictionary_var.h:35: // The returned PP_Var has been added ref
on behalf of the caller.
nit: wording is a little akward (maybe just make it match what you do for Array
as Raymes suggested)

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
ppapi/shared_impl/dictionary_var.h:40: // The returned PP_Var has been added ref
on behalf of the caller.
ditto, and please make it plural (returned PP_Vars, etc)

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
ppapi/shared_impl/dictionary_var.h:56: KeyValueMap key_value_map_;
So I take it you're not worrying about making it work in v8 yet? If you were, I
would expect a separate host implementation. Alternately, we can do the
conversion for the purposes of Messaging at the point a message comes in or goes
out...  but that will sacrifice a bit of performance.

As we discussed over IM before, I think it's fine to leave out messaging support
for now. But I think we need to support messaging before taking Array/Dictionary
out of Dev.

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
File ppapi/shared_impl/var_value_conversions.cc (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions.cc:56: parent_ids->end()) {
So the conversion is O(n^2). You could drop it to O(n) expected time by also
having a hash set. Not sure if it's worth it in practice, though.

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions.cc:87: parent_ids);
If a plugin creates a really deep object, they can induce stack overflow here.
It might be better to have a work-queue kind of approach rather than recursion.

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions.cc:158: case base::Value::TYPE_BINARY: {
nit: It might be nice to order these cases the same as the order in
CreateValueFromVarImpl

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions.cc:175: for
(base::DictionaryValue::Iterator iter(dict_value); !iter.IsAtEnd();
nit: I prefer each statement in the for to be on its own line if they don't fit
on 1.

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
File ppapi/shared_impl/var_value_conversions.h (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions.h:26: PPAPI_SHARED_EXPORT base::Value*
CreateValueFromVar(const PP_Var& var);
did you consider using scoped_ptr for the return to make the ownership
self-documenting?

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
File ppapi/shared_impl/var_value_conversions_unittest.cc (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions_unittest.cc:78: !value_iter.IsAtEnd();
value_iter.Advance()) {
nit: I prefer 1 statement per line if they don't fit

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions_unittest.cc:122: // Undefined var is not
allowed even if it is stored in a dictionary var.
FWIW, I would consider this overly restrictive for Messaging. I would want
messaging to happily provide JS with an Object with an undefined field (and vice
versa). But I guess they don't need to be consistent; you're expecting the dev
to provide you with an object of a certain form.

How do you deal with optional fields? Are you expecting that they have to be
absent from the Dictionary?

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions_unittest.cc:146:
dict_var_1->DeleteWithStringKey("key_1");
Maybe assert at the end of all of these that the var tracker has no live vars?

[yzshen1, 2013-03-14 05:38:21]

Thanks David! Please take another look.

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
File ppapi/shared_impl/dictionary_var.cc (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
ppapi/shared_impl/dictionary_var.cc:47:
PpapiGlobals::Get()->GetVarTracker()->AddRefVar(iter->second.get());
On 2013/03/05 22:01:03, dmichael wrote:
> If the plugin mis-manages its ref-count and decrements one more than it
should,
> the actual backing Var may already be deleted. It will cause you to hit a
> NOTREACHED in AddRefVar (and returns false.)
> 
> So in a release build iter->second.get() is an invalid PP_Var in that case.
> 
> Maybe it would be enough to just check the result of AddRefVar?
AddRefVar also returns false for those non-ref-counted var types.

> If it fails you
> can (optionally) erase the iter and return PP_MakeUndefined()? I know it's a
> corner case, but that seems nicer than returning PP_MakeUndefined.
(I think the last PP_MakeUndefined you meant the invalid PP_Var, right?)

My understanding is that there won't be any negative consequence on our side if
we return the invalid PP_Var (such as breaking the integrity of our internal
state / crashing in unnecessary cases / causing security problem). Right?
If that is true I slightly lean towards returning what the plugin has given us.
The plugin will find out when it tries to run PPB functions with that invalid
PP_Var.

(But I don't really have a strong preference. I could change it if you prefer
that way.)

> Alternately, you could keep an internal ref using scoped_refptr. Then even if
> they screw up their ref-counting, they get an actual object back.

It is easier to leak if we keep internal refs. In some cases, we will forcefully
cleanup PP_Var refs (only for object PP_Vars when an instance dies, I think). If
we keep internal refs instead, those objects may not go away. I personally think
that it is safer to keep PP_Var refs.
WDYT? :)

> You could
> replace the ScopedPPVar stuff with just that. (But that goes to your point
about
> documenting the ref-counting in the header. Maybe it's enough to just say that
> the children will not be destroyed as long as they are in a container or
> something).
> 
> I'm torn on which I like better :-). WDYT?

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
File ppapi/shared_impl/dictionary_var.h (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
ppapi/shared_impl/dictionary_var.h:35: // The returned PP_Var has been added ref
on behalf of the caller.
Done. Yeah, it sounds better that way.
On 2013/03/05 22:01:03, dmichael wrote:
> nit: wording is a little akward (maybe just make it match what you do for
Array
> as Raymes suggested)

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
ppapi/shared_impl/dictionary_var.h:40: // The returned PP_Var has been added ref
on behalf of the caller.
Done changing it to "had a ref added on behalf of the caller."

(I may be wrong since I am not a native speaker, but better to ask.)
I thought usually we use the singular form in this case. For example, this is a
comment from another file:
  // Creates a PP_Var corresponding to this object. The return value will have
  // one reference addrefed on behalf of the caller.
  PP_Var GetPPVar();

On 2013/03/05 22:01:03, dmichael wrote:
> ditto, and please make it plural (returned PP_Vars, etc)

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
ppapi/shared_impl/dictionary_var.h:56: KeyValueMap key_value_map_;
On 2013/03/05 22:01:03, dmichael wrote:
> So I take it you're not worrying about making it work in v8 yet? If you were,
I
> would expect a separate host implementation.
Correct. I haven't done that part.
I think at least this implementation is needed for the plugin side. So I decided
to implement it first.

> Alternately, we can do the
> conversion for the purposes of Messaging at the point a message comes in or
goes
> out...  but that will sacrifice a bit of performance.
Right.
I don't think we need to access the same struct from the both sides, so a
copy-and-convert approach should be okay (but may be inefficient, as you have
pointed out).
> 
> As we discussed over IM before, I think it's fine to leave out messaging
support
> for now. But I think we need to support messaging before taking
Array/Dictionary
> out of Dev.
Agreed!

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
File ppapi/shared_impl/var_value_conversions.cc (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions.cc:56: parent_ids->end()) {
On 2013/03/05 22:01:03, dmichael wrote:
> So the conversion is O(n^2). You could drop it to O(n) expected time by also
> having a hash set. Not sure if it's worth it in practice, though.
Done. I changed it to a set.

(I think it should be better than O(n^2) though. Because |parent_ids| only
contains the ancestors of the current node, so the size of |parent_ids| should
be something like O(lg(n)) in average cases, and conversion will be O(nlg(n)).)

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions.cc:87: parent_ids);
Yeah, this is a good idea.
Thanks!
On 2013/03/05 22:01:03, dmichael wrote:
> If a plugin creates a really deep object, they can induce stack overflow here.
> It might be better to have a work-queue kind of approach rather than
recursion.

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions.cc:158: case base::Value::TYPE_BINARY: {
I ordered them according to the enum type definition. 
On 2013/03/05 22:01:03, dmichael wrote:
> nit: It might be nice to order these cases the same as the order in
> CreateValueFromVarImpl

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions.cc:175: for
(base::DictionaryValue::Iterator iter(dict_value); !iter.IsAtEnd();
On 2013/03/05 22:01:03, dmichael wrote:
> nit: I prefer each statement in the for to be on its own line if they don't
fit
> on 1.

Done.

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
File ppapi/shared_impl/var_value_conversions.h (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions.h:26: PPAPI_SHARED_EXPORT base::Value*
CreateValueFromVar(const PP_Var& var);
I think that it is consistent with things such as
Value* CreateNullValue();
StringValue* CreateFilePathValue();
WDYT? :)

On 2013/03/05 22:01:03, dmichael wrote:
> did you consider using scoped_ptr for the return to make the ownership
> self-documenting?

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
File ppapi/shared_impl/var_value_conversions_unittest.cc (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions_unittest.cc:78: !value_iter.IsAtEnd();
value_iter.Advance()) {
On 2013/03/05 22:01:03, dmichael wrote:
> nit: I prefer 1 statement per line if they don't fit

Done.

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions_unittest.cc:122: // Undefined var is not
allowed even if it is stored in a dictionary var.
On 2013/03/05 22:01:03, dmichael wrote:
> FWIW, I would consider this overly restrictive for Messaging. I would want
> messaging to happily provide JS with an Object with an undefined field (and
vice
> versa). But I guess they don't need to be consistent; you're expecting the dev
> to provide you with an object of a certain form.

> How do you deal with optional fields? Are you expecting that they have to be
> absent from the Dictionary?

Thanks! I looked at the relevant code more closely and changed  the behavior to:
- PP_VARTYPE_UNDEFINED -> base::Value::TYPE_NULL (This is mostly for array
conversion.)
- If a dictionary var has a field whose value is PP_VARTYPE_UNDEFINED, the field
is skipped when converted to base::DictionaryValue.

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions_unittest.cc:146:
dict_var_1->DeleteWithStringKey("key_1");
Done. I added a check in TearDown().
On 2013/03/05 22:01:03, dmichael wrote:
> Maybe assert at the end of all of these that the var tracker has no live vars?

[dmichael (off chromium), 2013-03-15 17:35:48]

I think we already talked about the biggest stuff offline, but I thought I
should go ahead and publish all my remaining comments.

Overall, looks good. I'm just hoping you're able to simplify a little bit of it.

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
File ppapi/shared_impl/dictionary_var.cc (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
ppapi/shared_impl/dictionary_var.cc:47:
PpapiGlobals::Get()->GetVarTracker()->AddRefVar(iter->second.get());
On 2013/03/14 05:38:21, yzshen1 wrote:
> On 2013/03/05 22:01:03, dmichael wrote:
> > If the plugin mis-manages its ref-count and decrements one more than it
> should,
> > the actual backing Var may already be deleted. It will cause you to hit a
> > NOTREACHED in AddRefVar (and returns false.)
> > 
> > So in a release build iter->second.get() is an invalid PP_Var in that case.
> > 
> > Maybe it would be enough to just check the result of AddRefVar?
> AddRefVar also returns false for those non-ref-counted var types.
> 
> > If it fails you
> > can (optionally) erase the iter and return PP_MakeUndefined()? I know it's a
> > corner case, but that seems nicer than returning PP_MakeUndefined.
> (I think the last PP_MakeUndefined you meant the invalid PP_Var, right?)
Right, that's what I meant.

> 
> My understanding is that there won't be any negative consequence on our side
if
> we return the invalid PP_Var (such as breaking the integrity of our internal
> state / crashing in unnecessary cases / causing security problem). Right?
> If that is true I slightly lean towards returning what the plugin has given
us.
> The plugin will find out when it tries to run PPB functions with that invalid
> PP_Var.
Right, I'm more thinking of the plugin. If we do it the way you have written,
then you're right: the plugin will see something wrong later when they try to
use the PP_Var for something else. It may be pretty hard in that case for them
to track down what they're doing wrong.

If we check the result of AddRefVar and return PP_MakeUndefined, I think that
will make it more obvious to a plugin author what went wrong.

The alternative of using scoped_refptr would mean that it would actually still
work, even though they screwed up their ref-counting.

Those latter two both sound a bit nicer to me.

> 
> (But I don't really have a strong preference. I could change it if you prefer
> that way.)
> 
> > Alternately, you could keep an internal ref using scoped_refptr. Then even
if
> > they screw up their ref-counting, they get an actual object back.
> 
> It is easier to leak if we keep internal refs. In some cases, we will
forcefully
> cleanup PP_Var refs (only for object PP_Vars when an instance dies, I think).
If
> we keep internal refs instead, those objects may not go away. I personally
think
> that it is safer to keep PP_Var refs.
> WDYT? :)
I don't actually want us to support putting Object vars in Dictionary and Array.
We already don't allow sending them through Messaging. If we allow Object and
Dictionary, then we have an ambiguity if JS tries to send a JS object via
postMessage (plus, it's slightly complicated to implement IIRC).

So I wouldn't worry about that when you decide.

> 
> > You could
> > replace the ScopedPPVar stuff with just that. (But that goes to your point
> about
> > documenting the ref-counting in the header. Maybe it's enough to just say
that
> > the children will not be destroyed as long as they are in a container or
> > something).
> > 
> > I'm torn on which I like better :-). WDYT?

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
File ppapi/shared_impl/dictionary_var.h (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
ppapi/shared_impl/dictionary_var.h:40: // The returned PP_Var has been added ref
on behalf of the caller.
On 2013/03/14 05:38:21, yzshen1 wrote:
> Done changing it to "had a ref added on behalf of the caller."
> 
> (I may be wrong since I am not a native speaker, but better to ask.)
> I thought usually we use the singular form in this case. For example, this is
a
> comment from another file:
>   // Creates a PP_Var corresponding to this object. The return value will have
>   // one reference addrefed on behalf of the caller.
>   PP_Var GetPPVar();
> 
> On 2013/03/05 22:01:03, dmichael wrote:
> > ditto, and please make it plural (returned PP_Vars, etc)
Sorry, I had the array ref-counts on the brain. You're right.

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
File ppapi/shared_impl/var_value_conversions.cc (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions.cc:56: parent_ids->end()) {
On 2013/03/14 05:38:21, yzshen1 wrote:
> On 2013/03/05 22:01:03, dmichael wrote:
> > So the conversion is O(n^2). You could drop it to O(n) expected time by also
> > having a hash set. Not sure if it's worth it in practice, though.
> Done. I changed it to a set.
> 
> (I think it should be better than O(n^2) though.
O(n^2) worst-case for a line of Object->Child->ChildOfChild->ChildOfChildOfChild
etc.

> Because |parent_ids| only
> contains the ancestors of the current node, so the size of |parent_ids| should
> be something like O(lg(n)) in average cases, and conversion will be
O(nlg(n)).)
Fair enough.

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
File ppapi/shared_impl/var_value_conversions.h (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions.h:26: PPAPI_SHARED_EXPORT base::Value*
CreateValueFromVar(const PP_Var& var);
On 2013/03/14 05:38:21, yzshen1 wrote:
> I think that it is consistent with things such as
> Value* CreateNullValue();
> StringValue* CreateFilePathValue();
> WDYT? :)
> 
> On 2013/03/05 22:01:03, dmichael wrote:
> > did you consider using scoped_ptr for the return to make the ownership
> > self-documenting?
> 
Sure, consistency is worthwhile. sgtm

https://codereview.chromium.org/12387073/diff/11001/ppapi/shared_impl/var_val...
File ppapi/shared_impl/var_value_conversions.cc (right):

https://codereview.chromium.org/12387073/diff/11001/ppapi/shared_impl/var_val...
ppapi/shared_impl/var_value_conversions.cc:106: std::set<int64_t>* parent_ids,
As I mentioned on IM, I think we probably should deal with more than just
cycles, since Value doesn't support a node having >1 parent.

E.g., stuff like:
  A
 / \
B-->C

  A
 / \
B   C
|   ^
v   |
D----

(I.e., C is a child of both A and D).

Put another way, I think we only want to support tree structures, not DAGs.

This might be a different story for Messaging when we convert these things to
WebKit/v8 types. But for Value it kind of looks like we should detect these
cases and spit out an error.

https://codereview.chromium.org/12387073/diff/11001/ppapi/shared_impl/var_val...
ppapi/shared_impl/var_value_conversions.cc:142: if
(parent_ids->find(var.value.as_id) != parent_ids->end()) {
optional suggestion: you can also ContainsKey from stl_util.h for this

https://codereview.chromium.org/12387073/diff/11001/ppapi/shared_impl/var_val...
ppapi/shared_impl/var_value_conversions.cc:272: current_value.release());
As we discussed offline, I had a little bit of trouble following the algorithm
initially. There are a lot of moving pieces, and they aren't really explained. I
think the part that was most confusing to me was the state that's held in the
VarNode things. There's sort of an inner loop here, where state.top() will
remain unchanged for a while as you iterate through its children via the if/else
below. It's an unusual construct. I'd prefer if it was avoided somehow and you
had a clear nested loop. Failing that, some explanatory comments about it would
be nice.

Also as mentioned in IM...  I think it might be cleaner as a BFS, work-queue
approach. I'm not tied to either way, but I would prefer if this logic was made
a little easier to follow.

[yzshen1, 2013-03-15 22:56:49]

Thanks David! The conversion code looks a lot simpler.
Please take another look!

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
File ppapi/shared_impl/dictionary_var.cc (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/dictiona...
ppapi/shared_impl/dictionary_var.cc:47:
PpapiGlobals::Get()->GetVarTracker()->AddRefVar(iter->second.get());

> Right, I'm more thinking of the plugin. If we do it the way you have written,
> then you're right: the plugin will see something wrong later when they try to
> use the PP_Var for something else. It may be pretty hard in that case for them
> to track down what they're doing wrong.
> 
> If we check the result of AddRefVar and return PP_MakeUndefined, I think that
> will make it more obvious to a plugin author what went wrong.
> 
> The alternative of using scoped_refptr would mean that it would actually still
> work, even though they screwed up their ref-counting.

I changed to return an undefined var in that case. (Also fixed AddRefVar()).
I lean towards not giving them a usable result if they screwed up their
ref-counting. If we are too tolerant they never learn about it. :)

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
File ppapi/shared_impl/var_value_conversions.cc (right):

https://codereview.chromium.org/12387073/diff/2001/ppapi/shared_impl/var_valu...
ppapi/shared_impl/var_value_conversions.cc:56: parent_ids->end()) {
> O(n^2) worst-case for a line of
Object->Child->ChildOfChild->ChildOfChildOfChild
> etc.
Right. That is true.

https://codereview.chromium.org/12387073/diff/11001/ppapi/shared_impl/var_val...
File ppapi/shared_impl/var_value_conversions.cc (right):

https://codereview.chromium.org/12387073/diff/11001/ppapi/shared_impl/var_val...
ppapi/shared_impl/var_value_conversions.cc:106: std::set<int64_t>* parent_ids,
Done. (Discussed offline.)
On 2013/03/15 17:35:48, dmichael wrote:
> As I mentioned on IM, I think we probably should deal with more than just
> cycles, since Value doesn't support a node having >1 parent.
> 
> E.g., stuff like:
>   A
>  / \
> B-->C
> 
>   A
>  / \
> B   C
> |   ^
> v   |
> D----
> 
> (I.e., C is a child of both A and D).
> 
> Put another way, I think we only want to support tree structures, not DAGs.
> 
> This might be a different story for Messaging when we convert these things to
> WebKit/v8 types. But for Value it kind of looks like we should detect these
> cases and spit out an error.

https://codereview.chromium.org/12387073/diff/11001/ppapi/shared_impl/var_val...
ppapi/shared_impl/var_value_conversions.cc:142: if
(parent_ids->find(var.value.as_id) != parent_ids->end()) {
On 2013/03/15 17:35:48, dmichael wrote:
> optional suggestion: you can also ContainsKey from stl_util.h for this

Done.

https://codereview.chromium.org/12387073/diff/11001/ppapi/shared_impl/var_val...
ppapi/shared_impl/var_value_conversions.cc:272: current_value.release());
Done. Thanks!

On 2013/03/15 17:35:48, dmichael wrote:
> As we discussed offline, I had a little bit of trouble following the algorithm
> initially. There are a lot of moving pieces, and they aren't really explained.
I
> think the part that was most confusing to me was the state that's held in the
> VarNode things. There's sort of an inner loop here, where state.top() will
> remain unchanged for a while as you iterate through its children via the
if/else
> below. It's an unusual construct. I'd prefer if it was avoided somehow and you
> had a clear nested loop. Failing that, some explanatory comments about it
would
> be nice.
> 
> Also as mentioned in IM...  I think it might be cleaner as a BFS, work-queue
> approach. I'm not tied to either way, but I would prefer if this logic was
made
> a little easier to follow.

[dmichael (off chromium), 2013-03-18 16:21:05]

Thanks, it looks pretty clear now. lgtm

https://codereview.chromium.org/12387073/diff/47001/ppapi/shared_impl/var_val...
File ppapi/shared_impl/var_value_conversions.cc (right):

https://codereview.chromium.org/12387073/diff/47001/ppapi/shared_impl/var_val...
ppapi/shared_impl/var_value_conversions.cc:110: value->reset(new
base::DictionaryValue());
I think it's worth noting somewhere (probably the .h file) that if a node is
repeated in the Var that you're actually deep copying it.

[yzshen1, 2013-03-18 17:24:09]

Thanks!

https://codereview.chromium.org/12387073/diff/47001/ppapi/shared_impl/var_val...
File ppapi/shared_impl/var_value_conversions.cc (right):

https://codereview.chromium.org/12387073/diff/47001/ppapi/shared_impl/var_val...
ppapi/shared_impl/var_value_conversions.cc:110: value->reset(new
base::DictionaryValue());
On 2013/03/18 16:21:05, dmichael wrote:
> I think it's worth noting somewhere (probably the .h file) that if a node is
> repeated in the Var that you're actually deep copying it.

Done.