[PPAPI] Added VarResource_Dev class.

ppapi/cpp/dev/var_resource_dev.cc

+// Copyright 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/cpp/dev/var_resource_dev.h"
+
+#include "ppapi/c/dev/ppb_var_resource_dev.h"
+#include "ppapi/cpp/logging.h"
+#include "ppapi/cpp/module_impl.h"
+
+namespace pp {
+
+namespace {
+
+template <> const char* interface_name<PPB_VarResource_Dev_0_1>() {
+  return PPB_VAR_RESOURCE_DEV_INTERFACE_0_1;
+}
+
+}  // namespace
+
+VarResource_Dev::VarResource_Dev(const pp::Resource& resource) : Var(Null()) {
+  if (!has_interface<PPB_VarResource_Dev_0_1>()) {
+    PP_NOTREACHED();
+    return;
+  }
+
+  // TODO(mgiuca): Think about reference counting.
+  var_ = get_interface<PPB_VarResource_Dev_0_1>()->VarFromResource(
+      resource.pp_resource());

[dmichael (off chromium), 2013/10/30 18:03:10]

This should be fine, but you also have to set "is_managed_" to true. :'-(

It might be worthwhile adding something like:
void TakeOwnership(PP_Var var)
as a protected member of Var, so you can use it here and in VarArrayBuffer,
VarArray, and VarDictionary. These latter two actually look buggy to me, because
they don't set is_managed_.

(Note...  I have an old bug sitting around that we should get rid of is_managed_
entirely https://code.google.com/p/chromium/issues/detail?id=52106 )

[yzshen1, 2013/10/30 18:10:35]

Var(Null) constructor sets |is_managed_| to true, right?

[Matt Giuca, 2013/10/31 06:33:46]

Correct. So this code should be fine as it is.

+}
+
+VarResource_Dev::VarResource_Dev(const Var& var) : Var(var) {
+  if (!var.is_resource()) {
+    PP_NOTREACHED();
+
+    // This takes care of releasing the reference that this object holds.
+    Var::operator=(Var(Null()));
+  }
+}
+
+VarResource_Dev::VarResource_Dev(const PP_Var& var) : Var(var) {

[dmichael (off chromium), 2013/10/30 18:03:10]

Are you using this for anything? If you can avoid it, I'd rather leave it out.
We should keep the PP_Var stuff to a minimum in the C++ API, because it opens
the door to ref-counting mistakes.

[Matt Giuca, 2013/10/31 06:33:46]

Done.

+  if (var.type != PP_VARTYPE_RESOURCE) {
+    PP_NOTREACHED();
+
+    // This takes care of releasing the reference that this object holds.
+    Var::operator=(Var(Null()));
+  }
+}
+
+VarResource_Dev::VarResource_Dev(const VarResource_Dev& other) : Var(other) {}
+
+VarResource_Dev::~VarResource_Dev() {}
+
+VarResource_Dev& VarResource_Dev::operator=(const VarResource_Dev& other) {
+  Var::operator=(other);
+  return *this;
+}
+
+Var& VarResource_Dev::operator=(const Var& other) {
+  if (other.is_array()) {

[yzshen1, 2013/10/30 16:50:01]

resource :)

[Matt Giuca, 2013/10/30 21:54:41]

Oops, as you can see, I made this in a hurry and didn't have time last night to
write any test code. I just wanted to get a first opinion from you guys as
quickly as possible. Sorry!

[Matt Giuca, 2013/10/31 06:33:46]

Done.

+    Var::operator=(other);
+  } else {
+    PP_NOTREACHED();
+    Var::operator=(Var(Null()));
+  }
+  return *this;
+}
+
+pp::Resource VarResource_Dev::ToResource() {

[yzshen1, 2013/10/30 16:50:01]

Maybe we could also consider something like:

template <class ResourceSubclassType>
bool ToResource(ResourceSubclassType* resource) const

or
template <class ResourceSubclassType>
ResourceSubclassType ToResource() const?

[Matt Giuca, 2013/10/30 21:54:41]

I'll have to think about this. What would the usage look like?

VarResource_Dev var_resource = ...;
FileSystem filesystem;
if (!var_resource.ToResource(&filesystem))
  // ERROR: Not a file system.
// Use filesystem.

?

Seems like an OK user interface, but I have some questions:
- Is the second version (nicer) actually possible? I thought you couldn't
control a template based on the return type of the function.
- What would happen if you just passed a pp::Resource base class? Would it still
be expected to work?
- How would this be implemented? Would we need a template specialization for
each class? I thought we were trying to avoid having code in VarResource_Dev for
each subclass. The original design was for each of the resource types such as
FileSystem to have a converter from Resource to itself.
- I'm typically wary of template magic, especially exposing it to the user. I
think this method is going to behave quite differently to any other method, as
it will automatically downcast depending on the type you give it. I feel like a
simpler non template-based one is going to be much easier to document and easier
for users to think about, even if they have to do a bit more work.

Thoughts?

[Matt Giuca, 2013/10/31 06:33:46]

As discussed, saving for another CL, if at all. Let's see how we go with the API
discussed in the design doc.

+  if (!has_interface<PPB_VarResource_Dev_0_1>())
+    return pp::Resource();
+
+  // TODO(mgiuca): Think about reference counting.
+  return pp::Resource(

[yzshen1, 2013/10/30 16:50:01]

I think you need to use Resource(PassRef, PP_Resource).

[dmichael (off chromium), 2013/10/30 18:03:10]

+1

[Matt Giuca, 2013/10/31 06:33:46]

Done.

+      get_interface<PPB_VarResource_Dev_0_1>()->VarToResource(var_));
+}
+
+}  // namespace pp