Add PPB_VarDictionary_Dev support - part 1.

ppapi/shared_impl/dictionary_var.cc

+// 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.
+
+#include "ppapi/shared_impl/dictionary_var.h"
+
+#include "base/memory/ref_counted.h"
+#include "base/string_util.h"
+#include "ppapi/shared_impl/ppapi_globals.h"
+#include "ppapi/shared_impl/var_tracker.h"
+
+namespace ppapi {
+
+DictionaryVar::DictionaryVar() {
+}
+
+DictionaryVar::~DictionaryVar() {
+}
+
+// static
+DictionaryVar* DictionaryVar::FromPPVar(const PP_Var& var) {
+  if (var.type != PP_VARTYPE_DICTIONARY)
+    return NULL;
+
+  scoped_refptr<Var> var_object(
+      PpapiGlobals::Get()->GetVarTracker()->GetVar(var));
+  if (!var_object.get())
+    return NULL;
+  return var_object->AsDictionaryVar();
+}
+
+DictionaryVar* DictionaryVar::AsDictionaryVar() {
+  return this;
+}
+
+PP_VarType DictionaryVar::GetType() const {
+  return PP_VARTYPE_DICTIONARY;
+}
+
+PP_Var DictionaryVar::Get(const PP_Var& key) const {
+  StringVar* string_var = StringVar::FromPPVar(key);
+  if (!string_var)
+    return PP_MakeUndefined();
+
+  KeyValueMap::const_iterator iter = key_value_map_.find(string_var->value());
+  if (iter != key_value_map_.end()) {
+    PpapiGlobals::Get()->GetVarTracker()->AddRefVar(iter->second.get());

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

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?

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

AddRefVar also returns false for those non-ref-counted var types.
(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.)
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? :)

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

Right, that's what I meant.
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.
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.

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

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. :)

+    return iter->second.get();
+  } else {
+    return PP_MakeUndefined();
+  }
+}
+
+PP_Bool DictionaryVar::Set(const PP_Var& key, const PP_Var& value) {
+  StringVar* string_var = StringVar::FromPPVar(key);
+  if (!string_var)
+    return PP_FALSE;
+
+  key_value_map_[string_var->value()] = value;
+  return PP_TRUE;
+}
+
+void DictionaryVar::Delete(const PP_Var& key) {
+  StringVar* string_var = StringVar::FromPPVar(key);
+  if (!string_var)
+    return;
+
+  key_value_map_.erase(string_var->value());
+}
+
+PP_Bool DictionaryVar::HasKey(const PP_Var& key) const {
+  StringVar* string_var = StringVar::FromPPVar(key);
+  if (!string_var)
+    return PP_FALSE;
+
+  bool result =
+      key_value_map_.find(string_var->value()) != key_value_map_.end();
+  return PP_FromBool(result);
+}
+
+PP_Var DictionaryVar::GetKeys() const {
+  // TODO(yzshen): Implement once array PP_Var is supported.
+  return PP_MakeNull();
+}
+
+bool DictionaryVar::SetWithStringKey(const std::string& utf8_key,
+                                     const PP_Var& value) {
+  if (!IsStringUTF8(utf8_key))
+    return false;
+
+  key_value_map_[utf8_key] = value;
+  return true;
+}
+
+void DictionaryVar::DeleteWithStringKey(const std::string& utf8_key) {
+  key_value_map_.erase(utf8_key);
+}
+
+}  // namespace ppapi