Introduce RawVarData and associated classes for serializing PP_Vars

ppapi/proxy/raw_var_data.cc

Index: ppapi/proxy/raw_var_data.cc
diff --git a/ppapi/proxy/raw_var_data.cc b/ppapi/proxy/raw_var_data.cc
new file mode 100644
index 0000000000000000000000000000000000000000..84dc755b78425a87fa9b21c39b6d5766f098d8f3
--- /dev/null
+++ b/ppapi/proxy/raw_var_data.cc
@@ -0,0 +1,651 @@
+// 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/proxy/raw_var_data.h"
+
+#include "base/stl_util.h"
+#include "ipc/ipc_message.h"
+#include "ppapi/proxy/ppapi_param_traits.h"
+#include "ppapi/shared_impl/array_var.h"
+#include "ppapi/shared_impl/dictionary_var.h"
+#include "ppapi/shared_impl/ppapi_globals.h"
+#include "ppapi/shared_impl/scoped_pp_var.h"
+#include "ppapi/shared_impl/var.h"
+#include "ppapi/shared_impl/var_graph.h"
+#include "ppapi/shared_impl/var_tracker.h"
+
+namespace ppapi {
+namespace proxy {
+
+namespace {
+
+// When sending array buffers, if the size is over 256K, we use shared
+// memory instead of sending the data over IPC. Light testing suggests
+// shared memory is much faster for 256K and larger messages.
+static const uint32 kMinimumArrayBufferSizeForShmem = 256 * 1024;
+
+RawVarData* CreateRawVarData(const PP_Var& var,
+                             const VarGraph* graph,

[dmichael (off chromium), 2013/04/12 22:40:38]

Why not const&?

+                             PP_Instance instance) {
+  switch (var.type) {
+    case PP_VARTYPE_UNDEFINED:
+    case PP_VARTYPE_NULL:
+    case PP_VARTYPE_BOOL:
+    case PP_VARTYPE_INT32:
+    case PP_VARTYPE_DOUBLE:
+      return new BasicRawVarData(var);
+    case PP_VARTYPE_STRING:
+      return new StringRawVarData(var);
+    case PP_VARTYPE_ARRAY_BUFFER:
+      return new ArrayBufferRawVarData(var, instance);
+    case PP_VARTYPE_ARRAY:
+      return new ArrayRawVarData(var, *graph);
+    case PP_VARTYPE_DICTIONARY:
+      return new DictionaryRawVarData(var, *graph);
+    case PP_VARTYPE_OBJECT:
+      NOTREACHED();
+      return NULL;
+  }
+  NOTREACHED();
+  return NULL;
+}
+
+PP_Var ReferenceToVar(const RawVarData::VarReference& ref,

[dmichael (off chromium), 2013/04/12 22:40:38]

"Reference" is a little overloaded, given these are reference-counted, and it
makes a name like ReferenceToVar a little confusing to me. Maybe VarReference
should be something like VarHandle?

[raymes, 2013/04/14 16:32:57]

This has all been removed now.

+                      const std::vector<PP_Var>& graph) {
+  PP_Var var = ref.var;
+  if (VarTracker::IsVarTypeRefcounted(var.type)) {
+    if (!ref.valid_var_ref) {
+      var.value.as_id = 0;
+    } else {
+      if (graph.size() <= ref.ref_id) {
+        NOTREACHED();
+        return PP_MakeUndefined();
+      }
+      var.value.as_id = graph[ref.ref_id].value.as_id;
+    }
+  }
+  return var;
+}
+
+RawVarData::VarReference VarToReference(const PP_Var& var,
+                                        const VarGraph& graph) {
+  RawVarData::VarReference result;

[dmichael (off chromium), 2013/04/12 22:40:38]

Given VarReference has no default constructor, everything inside goes
uninitialized. I think you want to either add a default constructor or use "=
{}" everywhere you make one of these. I think the style guide would recommended
a default constructor.

[raymes, 2013/04/14 16:32:57]

"

+  result.var = var;
+  if (VarTracker::IsVarTypeRefcounted(var.type)) {
+    std::map<int64_t, size_t>::const_iterator iterator =
+        graph.id_map.find(var.value.as_id);
+    if (iterator == graph.id_map.end()) {
+      result.valid_var_ref = false;
+      return result;
+    }
+    result.ref_id = iterator->second;
+  }
+  return result;
+}
+
+PP_Var MakeInvalidRef(PP_VarType type) {

[dmichael (off chromium), 2013/04/12 22:40:38]

I don't think we should even serialize invalid vars, so maybe you can get rid of
this.

[raymes, 2013/04/14 16:32:57]

Done - this is all removed.

+  DCHECK(VarTracker::IsVarTypeRefcounted(type));
+  PP_Var result;
+  result.type = type;
+  result.value.as_id = 0;
+  return result;
+}
+
+void WriteVarReference(IPC::Message* m,
+                       const HandleWriter handle_writer,
+                       const RawVarData::VarReference& var_reference) {
+  if (!VarTracker::IsVarTypeRefcounted(var_reference.var.type)) {
+    BasicRawVarData(var_reference.var).Write(m, handle_writer);
+  } else {
+    m->WriteInt(var_reference.var.type);
+    m->WriteBool(var_reference.valid_var_ref);
+    m->WriteUInt32(var_reference.ref_id);
+  }
+}
+
+bool ReadVarReference(const IPC::Message* m,
+                      PickleIterator* iter,
+                      RawVarData::VarReference* result_out) {
+  RawVarData::VarReference result;
+  int32_t type;
+  if (!m->ReadInt(iter, &type))
+    return false;
+  result.var.type = static_cast<PP_VarType>(type);
+  if (!VarTracker::IsVarTypeRefcounted(result.var.type)) {
+    scoped_ptr<BasicRawVarData> data(
+        BasicRawVarData::Read(result.var.type, m, iter));
+    result.var = data->CreatePPVar(0);
+  } else {
+    if (!m->ReadBool(iter, &result.valid_var_ref))
+      return false;
+    if (!m->ReadUInt32(iter, &result.ref_id))
+      return false;
+  }
+  *result_out = result;
+  return true;
+}
+
+void DefaultHandleWriter(IPC::Message* m, const SerializedHandle& handle) {
+  IPC::ParamTraits<SerializedHandle>::Write(m, handle);
+}
+
+}  // namespace
+
+// RawVarDataGraph ------------------------------------------------------------
+RawVarDataGraph::RawVarDataGraph() {
+}
+
+RawVarDataGraph::~RawVarDataGraph() {
+}
+
+RawVarDataGraph::RawVarDataGraph(const PP_Var& var, PP_Instance instance) {
+  if (PpapiGlobals::Get()->GetVarTracker()->GetVar(var) == NULL) {
+    // If the node is a primitive value, just create a single raw var data.
+    data_.push_back(CreateRawVarData(var, NULL, instance));

[dmichael (off chromium), 2013/04/12 22:40:38]

Why do you need the if at all? (Notes to self: Looks like it's because if the
var is a container, the root will end up in the graph...  so adding the root and
the contents of the graph could give a duplicate? But you could see if the graph
is empty to decide if you need to add the root. Or...  why does VarGraph not
have all the values, too? That would simplify things here. What about
elsewhere?)

[raymes, 2013/04/14 16:32:57]

I changed the way this works, hopefully it is clearer now.

+  } else {
+    VarGraph graph;
+    bool success = VarGraph::Create(var, &graph);
+    if (!success) {
+      NOTREACHED();
+      return;
+    }
+
+    for (size_t i = 0; i < graph.nodes.size(); ++i)
+      data_.push_back(CreateRawVarData(var, &graph, instance));
+  }
+}
+
+PP_Var RawVarDataGraph::CreatePPVar(PP_Instance instance) {
+  // Create and initialize each node in the graph.
+  std::vector<PP_Var> graph;
+  for (size_t i = 0; i < data_.size(); ++i)
+    graph.push_back(data_[i]->CreatePPVar(instance));
+  for (size_t i = 0; i < data_.size(); ++i)
+    data_[i]->InitPPVar(graph[i], graph);

[dmichael (off chromium), 2013/04/12 22:40:38]

I found InitPPVar a bit confusing as a name. Maybe PopulateVarFromRawData, or
something?

Maybe it would be more obvious if it took a Var* out-param instead of a PP_Var?

[raymes, 2013/04/14 16:32:57]

See my reply in the header. I changed the name as you suggested.

+  // The first element is the root.
+  return graph[0];
+}
+
+void RawVarDataGraph::Write(IPC::Message* m,
+                            const HandleWriter& handle_writer) {
+  // Write the size, followed by each node in the graph.
+  m->WriteInt(data_.size());
+  for (size_t i = 0; i < data_.size(); ++i)
+    data_[i]->Write(m, handle_writer);
+}
+
+void RawVarDataGraph::Write(IPC::Message* m) {
+  Write(m, base::Bind(&DefaultHandleWriter));
+}
+
+// static
+scoped_ptr<RawVarDataGraph> RawVarDataGraph::Read(const IPC::Message* m,
+                                                  PickleIterator* iter) {
+  scoped_ptr<RawVarDataGraph> result(new RawVarDataGraph);
+  int32_t size = 0;
+  if (!m->ReadInt(iter, &size))
+    return scoped_ptr<RawVarDataGraph>();
+  for (int32_t i = 0; i < size; ++i) {
+    int32_t type;
+    if (!m->ReadInt(iter, &type))
+      return scoped_ptr<RawVarDataGraph>();
+    PP_VarType var_type = static_cast<PP_VarType>(type);
+    switch (var_type) {
+      case PP_VARTYPE_UNDEFINED:
+      case PP_VARTYPE_NULL:
+      case PP_VARTYPE_BOOL:
+      case PP_VARTYPE_INT32:
+      case PP_VARTYPE_DOUBLE:
+        result->data_.push_back(BasicRawVarData::Read(var_type, m, iter));

[dmichael (off chromium), 2013/04/12 22:40:38]

Random idea, not sure how hard this is...  It would be nice if there was a way
to restructure to have only one factory function that does the switch statement
to var type to a specialization of RawVarData. I.e., make something like
CreateRawVarData that's more general purpose and makes an empty RawVarData. Then
the code here could be more like:
result->data_.push_back(CreateRawVarData(var_type));
result->data_.back().Read(m, iter);
without the whole switch thing.

[raymes, 2013/04/14 16:32:57]

I think that's a good idea. Done.

+        break;
+      case PP_VARTYPE_STRING:
+        result->data_.push_back(StringRawVarData::Read(var_type, m, iter));
+        break;
+      case PP_VARTYPE_ARRAY:
+        result->data_.push_back(ArrayRawVarData::Read(var_type, m, iter));
+        break;
+      case PP_VARTYPE_DICTIONARY:
+        result->data_.push_back(DictionaryRawVarData::Read(var_type, m, iter));
+        break;
+      case PP_VARTYPE_ARRAY_BUFFER:
+        result->data_.push_back(ArrayBufferRawVarData::Read(var_type, m, iter));
+        break;
+      case PP_VARTYPE_OBJECT:
+        NOTREACHED();
+        scoped_ptr<RawVarDataGraph>();
+    }
+  }
+  return result.Pass();
+}
+
+// BasicRawVarData -------------------------------------------------------------
+BasicRawVarData::BasicRawVarData(const PP_Var& var) : var_(var) {
+}
+
+BasicRawVarData::~BasicRawVarData() {
+}
+
+PP_Var BasicRawVarData::CreatePPVar(PP_Instance instance) {
+  return var_;
+}
+
+void BasicRawVarData::InitPPVar(const PP_Var& var,
+                                const std::vector<PP_Var>& graph) {
+}
+
+void BasicRawVarData::Write(
+    IPC::Message* m,
+    const HandleWriter& handle_writer) {
+  m->WriteInt(static_cast<int>(var_.type));
+  switch (var_.type) {
+    case PP_VARTYPE_UNDEFINED:
+    case PP_VARTYPE_NULL:
+      // These don't need any data associated with them other than the type we
+      // just serialized.
+      break;
+    case PP_VARTYPE_BOOL:
+      m->WriteBool(PP_ToBool(var_.value.as_bool));
+      break;
+    case PP_VARTYPE_INT32:
+      m->WriteInt(var_.value.as_int);
+      break;
+    case PP_VARTYPE_DOUBLE:
+      IPC::ParamTraits<double>::Write(m, var_.value.as_double);
+      break;
+    default:
+      NOTREACHED();
+  }
+}
+
+// static
+BasicRawVarData* BasicRawVarData::Read(PP_VarType type,
+                                       const IPC::Message* m,
+                                       PickleIterator* iter) {
+  PP_Var result;
+  result.type = type;
+  switch (type) {
+    case PP_VARTYPE_UNDEFINED:
+    case PP_VARTYPE_NULL:
+      // These don't have any data associated with them other than the type we
+      // just serialized.
+      break;
+    case PP_VARTYPE_BOOL: {
+      bool bool_value;
+      if (!m->ReadBool(iter, &bool_value))
+        return NULL;
+      result.value.as_bool = PP_FromBool(bool_value);
+      break;
+    }
+    case PP_VARTYPE_INT32:
+      if (!m->ReadInt(iter, &result.value.as_int))
+        return NULL;
+      break;
+    case PP_VARTYPE_DOUBLE:
+      if (!IPC::ParamTraits<double>::Read(m, iter, &result.value.as_double))
+        return NULL;
+      break;
+    default:
+      NOTREACHED();
+      return NULL;
+  }
+  return new BasicRawVarData(result);
+}
+
+// StringRawVarData ------------------------------------------------------------
+StringRawVarData::StringRawVarData(const PP_Var& var) : valid_var_ref_(true) {
+  DCHECK(var.type == PP_VARTYPE_STRING);
+  StringVar* string_var = StringVar::FromPPVar(var);
+  if (!string_var)
+    valid_var_ref_ = false;
+  else
+    data_ = *string_var->ptr();

[dmichael (off chromium), 2013/04/12 22:40:38]

It's worth thinking about if there are ways to reduce copies. Our strings are
immutable, so keeping scoped_refptr<StringVar> instead of std::string might be
interesting. I think we'd have to change Var's base class to
RefCountedThreadSafe, though.

I think what you have models the current implementation. So that's probably the
right first approach...

[raymes, 2013/04/14 16:32:57]

I haven't yet addressed any of these comments related to reducing copies yet.
I'm going to wait until the high level design is right. Also it might be worth
getting something comitted first and then optimizing later.

+}
+
+StringRawVarData::~StringRawVarData() {
+}
+
+PP_Var StringRawVarData::CreatePPVar(PP_Instance instance) {
+  if (!valid_var_ref_)
+    return MakeInvalidRef(PP_VARTYPE_STRING);
+  return StringVar::StringToPPVar(data_);

[dmichael (off chromium), 2013/04/12 22:40:38]

I think you should use StringVar::SwapValidatedUTF8StringIntoPPVar instead to
avoid this copy.

[raymes, 2013/04/14 16:32:57]

"

+}
+
+void StringRawVarData::InitPPVar(const PP_Var& var,
+                                 const std::vector<PP_Var>& graph) {
+}
+
+void StringRawVarData::Write(IPC::Message* m,
+                             const HandleWriter& handle_writer) {
+  m->WriteInt(PP_VARTYPE_STRING);

[dmichael (off chromium), 2013/04/12 22:40:38]

It feels a little odd that the Write virtual function writes the type, but Read
does not read it. Maybe each child of RawVarData should _not_ write its own
type? Do it at the caller site instead, like for Read?

[raymes, 2013/04/14 16:32:57]

Done.

+  m->WriteBool(valid_var_ref_);
+  if (valid_var_ref_)
+    m->WriteString(data_);
+}
+
+// static
+StringRawVarData* StringRawVarData::Read(PP_VarType type,
+                                         const IPC::Message* m,
+                                         PickleIterator* iter) {
+  bool valid_var_ref;
+  std::string data;
+  if (!m->ReadBool(iter, &valid_var_ref))
+    return NULL;
+  if (valid_var_ref) {
+    if (!m->ReadString(iter, &data))
+      return NULL;
+  }
+  return new StringRawVarData(valid_var_ref, data);

[dmichael (off chromium), 2013/04/12 22:40:38]

I can't see where this constructor is actually implemented...?

Seems like you could use data as an in-out param and swap() it inside that
constructor to avoid a copy.

[raymes, 2013/04/14 16:32:57]

"

+}
+
+// ArrayBufferRawVarData -------------------------------------------------------
+ArrayBufferRawVarData::ArrayBufferRawVarData(const PP_Var& var,
+                                             PP_Instance instance)
+    : valid_var_ref_(true) {
+  DCHECK(var.type == PP_VARTYPE_ARRAY_BUFFER);
+  ArrayBufferVar* buffer_var = ArrayBufferVar::FromPPVar(var);
+  if (!buffer_var) {
+    valid_var_ref_ = false;
+  } else {
+    bool using_shmem = false;
+    if (buffer_var->ByteLength() >= kMinimumArrayBufferSizeForShmem &&
+        instance != 0) {
+      int host_handle_id;
+      base::SharedMemoryHandle plugin_handle;
+      using_shmem = buffer_var->CopyToNewShmem(instance,
+                                               &host_handle_id,
+                                               &plugin_handle);
+      if (using_shmem) {
+        if (host_shm_handle_id_ != -1) {
+          DCHECK(!base::SharedMemory::IsHandleValid(plugin_handle));
+          DCHECK(PpapiGlobals::Get()->IsPluginGlobals());
+          type_ = ARRAY_BUFFER_SHMEM_HOST;
+          host_shm_handle_id_ = host_handle_id;
+        } else {
+          DCHECK(base::SharedMemory::IsHandleValid(plugin_handle));
+          DCHECK(PpapiGlobals::Get()->IsHostGlobals());
+          type_ = ARRAY_BUFFER_SHMEM_PLUGIN;
+          plugin_shm_handle_ = SerializedHandle(plugin_handle,
+                                                buffer_var->ByteLength());
+        }
+      }
+    }
+    if (!using_shmem) {
+      type_ = ARRAY_BUFFER_NO_SHMEM;
+      data_ = std::string(static_cast<const char*>(buffer_var->Map()),
+                          buffer_var->ByteLength());

[dmichael (off chromium), 2013/04/12 22:40:38]

Same comment as for String...  seems like we could avoid this copy.

[raymes, 2013/04/14 16:32:57]

"

+    }
+  }
+}
+
+ArrayBufferRawVarData::ArrayBufferRawVarData(bool valid_var_ref,
+                                             ShmemType type)
+    : valid_var_ref_(valid_var_ref),
+      type_(type) {
+}
+
+ArrayBufferRawVarData::~ArrayBufferRawVarData() {
+}
+
+PP_Var ArrayBufferRawVarData::CreatePPVar(PP_Instance instance) {
+  if (!valid_var_ref_)
+    return MakeInvalidRef(PP_VARTYPE_ARRAY_BUFFER);
+  PP_Var result = PP_MakeUndefined();
+  switch (type_) {
+    case ARRAY_BUFFER_SHMEM_HOST: {
+      base::SharedMemoryHandle host_handle;
+      uint32 size_in_bytes;
+      bool ok = PpapiGlobals::Get()->GetVarTracker()->
+          StopTrackingSharedMemoryHandle(host_shm_handle_id_,
+                                         instance,
+                                         &host_handle,
+                                         &size_in_bytes);
+      if (ok) {
+        result = PpapiGlobals::Get()->GetVarTracker()->MakeArrayBufferPPVar(
+            size_in_bytes, host_handle);
+      } else {
+        LOG(ERROR) << "Couldn't find array buffer id: " << host_shm_handle_id_;
+        return PP_MakeUndefined();
+      }
+      break;
+    }
+    case ARRAY_BUFFER_SHMEM_PLUGIN: {
+      result = PpapiGlobals::Get()->GetVarTracker()->MakeArrayBufferPPVar(
+          plugin_shm_handle_.size(),
+          plugin_shm_handle_.shmem());
+      break;
+    }
+    case ARRAY_BUFFER_NO_SHMEM: {
+      result = PpapiGlobals::Get()->GetVarTracker()->MakeArrayBufferPPVar(
+          static_cast<uint32>(data_.size()), data_.data());

[dmichael (off chromium), 2013/04/12 22:40:38]

On the plugin side, if data_ is a vector<uint8_t>, you can provide a swapping
implementation to avoid this copy.

[raymes, 2013/04/14 16:32:57]

"

+      break;
+    }
+    default:
+      NOTREACHED();
+      return PP_MakeUndefined();
+  }
+  DCHECK(result.type == PP_VARTYPE_ARRAY_BUFFER);
+  return result;
+}
+
+void ArrayBufferRawVarData::InitPPVar(const PP_Var& var,
+                                      const std::vector<PP_Var>& graph) {
+}
+
+void ArrayBufferRawVarData::Write(
+    IPC::Message* m,
+    const HandleWriter& handle_writer) {
+  m->WriteInt(PP_VARTYPE_ARRAY_BUFFER);
+  m->WriteBool(valid_var_ref_);
+  if (!valid_var_ref_)
+    return;
+  m->WriteInt(type_);
+  switch (type_) {
+    case ARRAY_BUFFER_SHMEM_HOST:
+      m->WriteInt(host_shm_handle_id_);
+      break;
+    case ARRAY_BUFFER_SHMEM_PLUGIN:
+      handle_writer.Run(m, plugin_shm_handle_);
+      break;
+    case ARRAY_BUFFER_NO_SHMEM:
+      m->WriteString(data_);
+      break;
+  }
+}
+
+// static
+ArrayBufferRawVarData* ArrayBufferRawVarData::Read(PP_VarType type,
+                                                   const IPC::Message* m,
+                                                   PickleIterator* iter) {
+  bool valid_var_ref;
+  if (!m->ReadBool(iter, &valid_var_ref))
+    return NULL;
+  if (!valid_var_ref)
+    return new ArrayBufferRawVarData(false, ARRAY_BUFFER_NO_SHMEM);
+  int shmem_type;
+  if (!m->ReadInt(iter, &shmem_type))
+    return NULL;
+  scoped_ptr<ArrayBufferRawVarData> result(
+      new ArrayBufferRawVarData(true, static_cast<ShmemType>(shmem_type)));
+  switch (static_cast<ShmemType>(shmem_type)) {
+    case ARRAY_BUFFER_SHMEM_HOST:
+      if (!m->ReadInt(iter, &result->host_shm_handle_id_))
+        return NULL;
+      break;
+    case ARRAY_BUFFER_SHMEM_PLUGIN:
+      if (!IPC::ParamTraits<SerializedHandle>::Read(
+              m, iter, &result->plugin_shm_handle_)) {
+        return NULL;
+      }
+      break;
+    case ARRAY_BUFFER_NO_SHMEM:
+      if (!m->ReadString(iter, &result->data_))
+        return NULL;
+    default:
+      NOTREACHED();
+      return NULL;
+  }
+  return result.release();
+}
+
+// ArrayRawVarData -------------------------------------------------------------
+ArrayRawVarData::ArrayRawVarData(const PP_Var& var, const VarGraph& graph)
+    : valid_var_ref_(true) {
+  DCHECK(var.type == PP_VARTYPE_ARRAY);
+  ArrayVar* array_var = ArrayVar::FromPPVar(var);
+  if (!array_var) {
+    valid_var_ref_ = false;
+  } else {
+    for (ArrayVar::ElementVector::const_iterator iter =
+             array_var->elements().begin();
+         iter != array_var->elements().end();
+         ++iter) {
+      data_.push_back(VarToReference(iter->get(), graph));

[dmichael (off chromium), 2013/04/12 22:40:38]

It seems strange to me that you have two different places where you loop over
each Array and each Dictionary.

Let me see if I understand...  you do it once upfront in order to build your
"VarGraph", which really just gives you a way to find when a ref-counted Var
appears more than once. *Then*, you loop over each one again to populate the
actual "RawVarData"s. Why is that necessary? It seems like you could avoid
iterating twice. It seems potentially expensive for large objects.

[raymes1, 2013/04/12 23:21:38]

Sort of. The VarGraph gives you 2 things:
1) The transitive closure of nodes (which is the part about finding whether a
var appears more than once).
2) Translates the mapping of var id's. That is, it converts a global PP_Var
as_id value into a local index in the transitive closure. This is needed because
when you serialize you want to know which var in the transitive closure is being
referenced by a var in a container.

Looping over twice is necessary I believe. At the point where you iterate over
the elements of the array in the VarGraph, you don't know what index those
elements will be written to in the message. Basically the VarGraph attributes an
index in the message to the data associated with each Var. The "data" associated
with an ArrayVar are these local indices into the array, so they all need to be
computed first.

However, in my current code I actually do iterate an unecessary number of times
because I iterate over each element once when an ArrayVar is created and then
once again when it is written or read. I think this results in a much cleaner
design though.

[dmichael (off chromium), 2013/04/15 17:40:04]

It looks like you figured out how to avoid the two phases. Thanks, looks cleaner
to me.

+    }
+  }
+}
+
+ArrayRawVarData::ArrayRawVarData(bool valid_var_ref,
+                                 const std::vector<VarReference>& data)
+    : valid_var_ref_(valid_var_ref),
+      data_(data) {
+}
+
+ArrayRawVarData::~ArrayRawVarData() {
+}
+
+PP_Var ArrayRawVarData::CreatePPVar(PP_Instance instance) {
+  if (!valid_var_ref_)
+    return MakeInvalidRef(PP_VARTYPE_ARRAY);
+  return (new ArrayVar())->GetPPVar();
+}
+
+void ArrayRawVarData::InitPPVar(const PP_Var& var,
+                                const std::vector<PP_Var>& graph) {
+  if (var.type != PP_VARTYPE_ARRAY) {
+    NOTREACHED();
+    return;
+  }
+  ArrayVar* array_var = ArrayVar::FromPPVar(var);
+  DCHECK(array_var->elements().empty());
+  for (size_t i = 0; i < data_.size(); ++i) {
+    array_var->elements().push_back(ScopedPPVar(
+        ReferenceToVar(data_[i], graph)));
+  }
+}
+
+void ArrayRawVarData::Write(IPC::Message* m,
+                            const HandleWriter& handle_writer) {
+  m->WriteInt(PP_VARTYPE_ARRAY);
+  m->WriteBool(valid_var_ref_);
+  if (!valid_var_ref_)
+    return;
+  m->WriteInt(data_.size());
+  for (size_t i = 0; i < data_.size(); ++i)
+    WriteVarReference(m, handle_writer, data_[i]);
+}
+
+// static
+ArrayRawVarData* ArrayRawVarData::Read(PP_VarType type,
+                                       const IPC::Message* m,
+                                       PickleIterator* iter) {
+  bool valid_var_ref;
+  std::vector<VarReference> data;
+  if (!m->ReadBool(iter, &valid_var_ref))
+    return NULL;
+  if (valid_var_ref) {
+    VarReference current;
+    if (!ReadVarReference(m, iter, &current))
+      return NULL;
+    data.push_back(current);
+  }
+  return new ArrayRawVarData(valid_var_ref, data);
+}
+
+// DictionaryRawVarData --------------------------------------------------------
+DictionaryRawVarData::DictionaryRawVarData(const PP_Var& var,
+                                           const VarGraph& graph)
+    : valid_var_ref_(true) {
+  DCHECK(var.type == PP_VARTYPE_DICTIONARY);
+  DictionaryVar* dict_var = DictionaryVar::FromPPVar(var);
+  if (!dict_var) {
+    valid_var_ref_ = false;
+  } else {
+    for (DictionaryVar::KeyValueMap::const_iterator iter =
+            dict_var->key_value_map().begin();
+        iter != dict_var->key_value_map().end();
+        ++iter) {
+      std::pair<std::string, VarReference> reference;
+      reference.first = iter->first;
+      reference.second = VarToReference(iter->second.get(), graph);
+      data_.push_back(reference);
+    }
+  }
+}
+
+DictionaryRawVarData::DictionaryRawVarData(
+    bool valid_var_ref,
+    const std::vector<std::pair<std::string, VarReference> >& data)
+    : valid_var_ref_(valid_var_ref),
+      data_(data) {
+}
+
+DictionaryRawVarData::~DictionaryRawVarData() {
+}
+
+PP_Var DictionaryRawVarData::CreatePPVar(PP_Instance instance) {
+  if (!valid_var_ref_)
+    return MakeInvalidRef(PP_VARTYPE_DICTIONARY);
+  return (new DictionaryVar())->GetPPVar();
+}
+
+void DictionaryRawVarData::InitPPVar(const PP_Var& var,
+                                     const std::vector<PP_Var>& graph) {
+  if (var.type != PP_VARTYPE_DICTIONARY) {
+    NOTREACHED();
+    return;
+  }
+  DictionaryVar* dictionary_var = DictionaryVar::FromPPVar(var);
+  DCHECK(dictionary_var->key_value_map().empty());
+  for (size_t i = 0; i < data_.size(); ++i) {
+    PP_Var value = ReferenceToVar(data_[i].second, graph);
+    dictionary_var->Set(StringVar::StringToPPVar(data_[i].first), value);
+  }
+}
+
+void DictionaryRawVarData::Write(
+    IPC::Message* m,
+    const HandleWriter& handle_writer) {
+  m->WriteInt(PP_VARTYPE_DICTIONARY);
+  m->WriteBool(valid_var_ref_);
+  if (!valid_var_ref_)
+    return;
+  m->WriteInt(data_.size());
+  for (size_t i = 0; i < data_.size(); ++i) {
+    m->WriteString(data_[i].first);
+    WriteVarReference(m, handle_writer, data_[i].second);
+  }
+}
+
+// static
+DictionaryRawVarData* DictionaryRawVarData::Read(PP_VarType type,
+                                                 const IPC::Message* m,
+                                                 PickleIterator* iter) {
+  bool valid_var_ref;
+  std::vector<std::pair<std::string, VarReference> > data;
+  if (!m->ReadBool(iter, &valid_var_ref))
+    return NULL;
+  if (valid_var_ref) {
+    std::string key;
+    VarReference value;
+    if (!m->ReadString(iter, &key))
+      return NULL;
+    if (!ReadVarReference(m, iter, &value))
+      return NULL;
+    data.push_back(make_pair(key, value));
+  }
+  return new DictionaryRawVarData(valid_var_ref, data);
+}
+
+}  // namespace proxy
+}  // namespace ppapi