PPAPI URLRequestInfo test: port NaCl version to ppapi_tests. Fix Var and Resource leaks.

ppapi/tests/test_url_request.cc

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
-
-// Tests PPB_URLRequestInfo.
+//
+// Tests PPB_URLRequestInfo interface.
+
+#include "ppapi/tests/test_url_request.h"
 
 #include <string.h>
-
-#include "native_client/src/shared/platform/nacl_check.h"
-#include "native_client/tests/ppapi_test_lib/get_browser_interface.h"
-#include "native_client/tests/ppapi_test_lib/test_interface.h"
-#include "native_client/tests/ppapi_test_lib/testable_callback.h"
-
-#include "ppapi/c/pp_errors.h"
-#include "ppapi/c/ppb_core.h"
-#include "ppapi/c/ppb_file_io.h"
-#include "ppapi/c/ppb_file_ref.h"
-#include "ppapi/c/ppb_file_system.h"
-#include "ppapi/c/ppb_url_loader.h"
-#include "ppapi/c/ppb_url_request_info.h"
-#include "ppapi/c/ppb_url_response_info.h"
-#include "ppapi/c/ppb_var.h"
+#include <string>
+
+#include "ppapi/c/dev/ppb_testing_dev.h"
+#include "ppapi/cpp/completion_callback.h"
+#include "ppapi/cpp/instance.h"
+#include "ppapi/cpp/var.h"
+#include "ppapi/tests/test_utils.h"
+#include "ppapi/tests/testing_instance.h"
+
+REGISTER_TEST_CASE(URLRequest);
 
 namespace {
+// TODO(polina): move these to test_case.h/cc since other NaCl tests use them?
+
+const PP_Resource kInvalidResource = 0;
+const PP_Instance kInvalidInstance = 0;
+
+// These should not exist.
+// The bottom 2 bits are used to differentiate between different id types.
+// 00 - module, 01 - instance, 10 - resource, 11 - var.
+const PP_Instance kNotAnInstance = 0xFFFFF0;
+const PP_Resource kNotAResource = 0xAAAAA0;
+}
+
+TestURLRequest::TestURLRequest(TestingInstance* instance)
+    : TestCase(instance),
+      ppb_url_request_interface_(NULL),
+      ppb_url_loader_interface_(NULL),
+      ppb_url_response_interface_(NULL),
+      ppb_core_interface_(NULL),
+      ppb_var_interface_(NULL),
+      url_loader_(kInvalidResource) {
+}
+
+bool TestURLRequest::Init() {
+  ppb_url_request_interface_ = static_cast<const PPB_URLRequestInfo*>(
+      pp::Module::Get()->GetBrowserInterface(PPB_URLREQUESTINFO_INTERFACE));
+  ppb_url_loader_interface_ = static_cast<const PPB_URLLoader*>(
+      pp::Module::Get()->GetBrowserInterface(PPB_URLLOADER_INTERFACE));
+  ppb_url_response_interface_ = static_cast<const PPB_URLResponseInfo*>(
+      pp::Module::Get()->GetBrowserInterface(PPB_URLRESPONSEINFO_INTERFACE));
+  ppb_core_interface_ = static_cast<const PPB_Core*>(
+      pp::Module::Get()->GetBrowserInterface(PPB_CORE_INTERFACE));
+  ppb_var_interface_ = static_cast<const PPB_Var*>(
+      pp::Module::Get()->GetBrowserInterface(PPB_VAR_INTERFACE));
+  if (!ppb_url_request_interface_)

[dmichael (off chromium), 2012/01/17 18:38:03]

optional: You could just do
ASSERT_TRUE(foo_interface_);
for these if you wanted. (or ASSERT_NEQ(NULL,...); if that doesn't work)

[polina1, 2012/01/26 02:56:26]

Not sure why that's better for init, but in either case it does not work. It is
translated into a std::string return, but this function returns bool (contrary
to all the test functions).

[dmichael (off chromium), 2012/01/26 17:13:43]

Nevermind, wasn't reading carefully enough. Those don't make sense in Init.

+    instance_->AppendError("PPB_URLRequestInfo interface not available");
+  if (!ppb_url_response_interface_)
+    instance_->AppendError("PPB_URLResponseInfo interface not available");
+  if (!ppb_core_interface_)
+    instance_->AppendError("PPB_Core interface not available");
+  if (!ppb_var_interface_)
+    instance_->AppendError("PPB_Var interface not available");
+  if (!ppb_url_loader_interface_) {
+    instance_->AppendError("PPB_URLLoader interface not available");
+  } else {
+    url_loader_ = ppb_url_loader_interface_->Create(instance_->pp_instance());
+    if (url_loader_ == kInvalidResource)
+      instance_->AppendError("Failed to create URLLoader");
+  }
+  return EnsureRunningOverHTTP();

[bbudge, 2012/01/16 16:24:29]

Shouldn't we return false here if we couldn't get the above interfaces?

[polina1, 2012/01/26 02:56:26]

Just setting the error does the trick. That's how other Init's do it (e.g.
test_url_loader).

+}
+
+void TestURLRequest::RunTests(const std::string& filter) {
+  RUN_TEST(Create, filter);
+  RUN_TEST(IsURLRequestInfo, filter);
+  RUN_TEST(SetProperty, filter);
+  RUN_TEST(Stress, filter);
+  RUN_TEST(AppendDataToBody, filter);
+}
+
+PP_Var TestURLRequest::PP_MakeString(const char* s) {
+  return ppb_var_interface_->VarFromUtf8(s, strlen(s));
+}
 
 // Tests
 //   PP_Resource Create(PP_Instance instance).
-void TestCreate() {
-  PP_Resource url_request = kInvalidResource;
-
-  // Invalid instance -> invalid resource.
-  url_request = PPBURLRequestInfo()->Create(kInvalidInstance);
-  EXPECT(url_request == kInvalidResource);
+std::string TestURLRequest::TestCreate() {
+  // Invalid / non-existent instance -> invalid resource.
+  if (ppb_url_request_interface_->Create(kInvalidInstance) != kInvalidResource)
+    return "Create() passed with an invalid instance";

[dmichael (off chromium), 2012/01/17 18:38:03]

nit: I prefer ASSERT_* for checks, unless what the macro would spit out is
ambiguous. I find the tests more readable that way. TestURLLoader is a bad
example :-)

[polina1, 2012/01/26 02:56:26]

I think historically these tests were written not to use ASSERTs and EXPECTs. So
I just followed that, but I guess now we are converting to more gunit test
style?

Except in this test, because it is using the C interface, I sometimes need to
delay returning on an error to be able to release vars and resources. I can't do
that with ASSERTs, can I? And mixing the two styles throughout seems like it
would just create confusion. Thoughts?

[dmichael (off chromium), 2012/01/26 17:13:43]

Right, only do them where they make sense.
I still prefer using the ASSERT style where it makes sense. In 101->107, for
example, the returned strings are very boilderplate. The ASSERT gives you all
the information you need in case of a failure, and I find it more readable. It's
not a huge deal here; the other URL tests don't use ASSERT yet. But I want us to
move towards the macros to make the tests more consistent & readable. 

[polina1, 2012/01/26 21:26:59]

Done.

+  if (ppb_url_request_interface_->Create(kNotAnInstance) != kInvalidResource)
+    return "Create() passed with a non-existent instance";
 
   // Valid instance -> valid resource.
-  url_request = PPBURLRequestInfo()->Create(pp_instance());
-  EXPECT(url_request != kInvalidResource);
-
-  PPBCore()->ReleaseResource(url_request);
-  TEST_PASSED;
+  PP_Resource url_request = ppb_url_request_interface_->Create(
+      instance_->pp_instance());
+  if (url_request == kInvalidResource)
+    return "Create() failed with a valid instance";
+  ppb_core_interface_->ReleaseResource(url_request);
+
+  PASS();
 }
 
 // Tests
 //   PP_Bool IsURLRequestInfo(PP_Resource resource).
-void TestIsURLRequestInfo() {
-  const PPB_URLRequestInfo* ppb = PPBURLRequestInfo();
-
+std::string TestURLRequest::TestIsURLRequestInfo() {
   // Invalid / non-existent / non-URLRequestInfo resource -> false.
-  EXPECT(PP_FALSE == ppb->IsURLRequestInfo(kInvalidResource));
-  EXPECT(PP_FALSE == ppb->IsURLRequestInfo(kNotAResource));
-  PP_Resource url_loader = PPBURLLoader()->Create(pp_instance());
-  EXPECT(url_loader != kInvalidResource);
-  EXPECT(PP_FALSE == ppb->IsURLRequestInfo(url_loader));
-  PPBCore()->ReleaseResource(url_loader);
+  if (PP_TRUE == ppb_url_request_interface_->IsURLRequestInfo(kInvalidResource))
+    return "IsURLRequestInfo() passed with an invalid resource";
+  if (PP_TRUE == ppb_url_request_interface_->IsURLRequestInfo(kNotAResource))
+    return "IsURLRequestInfo() passed with an invalid resource";
+  if (PP_TRUE == ppb_url_request_interface_->IsURLRequestInfo(url_loader_))
+    return "IsURLRequest() passed with a non-URLRequestInfo resource";
 
   // Current URLRequestInfo resource -> true.
-  PP_Resource url_request = ppb->Create(pp_instance());
-  EXPECT(PP_TRUE == ppb->IsURLRequestInfo(url_request));
+  PP_Resource url_request = ppb_url_request_interface_->Create(
+      instance_->pp_instance());
+  std::string error;
+  if (PP_FALSE == ppb_url_request_interface_->IsURLRequestInfo(url_request))
+    error = "IsURLRequestInfo() failed with a current URLRequestInfo resource";
 
   // Released URLRequestInfo resource -> false.
-  PPBCore()->ReleaseResource(url_request);
-  EXPECT(PP_FALSE == ppb->IsURLRequestInfo(url_request));
-
-  TEST_PASSED;
+  ppb_core_interface_->ReleaseResource(url_request);
+  if (PP_TRUE == ppb_url_request_interface_->IsURLRequestInfo(url_request))
+    error = "IsURLRequestInfo() passed with a released URLRequestInfo resource";
+
+  return error;  // == PASS() if empty.
 }
 
 // Tests
 //  PP_Bool SetProperty(PP_Resource request,
 //                      PP_URLRequestProperty property,
 //                      struct PP_Var value);
-void TestSetProperty() {
-  struct PropertyTestData {
-    PropertyTestData(PP_URLRequestProperty _property,
-                     const std::string& _property_name,
-                     PP_Var _var, PP_Bool _expected_value) :
-                         property(_property), property_name(_property_name),
-                         var(_var), expected_value(_expected_value) {
-      PPBVar()->AddRef(var);
-    }
-    PP_URLRequestProperty property;
-    std::string property_name;
-    PP_Var var;
-    PP_Bool expected_value;
-  };
-
+std::string TestURLRequest::TestSetProperty() {
   // All bool properties should accept PP_TRUE and PP_FALSE, while rejecting
   // all other variable types.
-  #define ADD_BOOL_PROPERTY(_prop_name) \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeBool(PP_TRUE), PP_TRUE), \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeBool(PP_FALSE), PP_TRUE), \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeUndefined(), PP_FALSE), \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeNull(), PP_FALSE), \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeInt32(0), PP_FALSE), \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeDouble(0.0), PP_FALSE)
+  #define TEST_BOOL(_name) \

[dmichael (off chromium), 2012/01/17 18:38:03]

nit: I think the style guide says that all precompiler directives should go at
the start of the line (i.e., no 2-space indent). There's also no point in having
it in the scope of the function. It doesn't hurt anything, so feel free to leave
it in there if you like, but be aware that precompiler directives don't respect
scope.

[polina1, 2012/01/26 02:56:26]

Yeah, I know. I wanted to keep the diff with the original NaCl function (not
written by me) to a minimum, so it is easy to see how the test was ported. So is
it OK to leave things as-is?

[dmichael (off chromium), 2012/01/26 17:13:43]

How about just fixing the indentation so they are consistent with the style
guide? I don't think you need to move them out of the function body.

[polina1, 2012/01/26 21:26:59]

I shifted #define to the very left to be consistent with the style guide
(although it was talking about #ifdef and such actually). And the rest is
shifted 4 spaces. The style guide did not talk about that, but I found an
example in the C++ primer.

[dmichael (off chromium), 2012/01/26 21:36:06]

The style guide says "preprocessor directives", which includes #define. I think
4-space indent is what I'm used to seeing, too.

+    PropertyTestData(this, ID_STR(_name), PP_MakeBool(PP_TRUE), PP_TRUE), \
+    PropertyTestData(this, ID_STR(_name), PP_MakeBool(PP_FALSE), PP_TRUE), \
+    PropertyTestData(this, ID_STR(_name), PP_MakeUndefined(), PP_FALSE), \
+    PropertyTestData(this, ID_STR(_name), PP_MakeNull(), PP_FALSE), \
+    PropertyTestData(this, ID_STR(_name), PP_MakeInt32(0), PP_FALSE), \
+    PropertyTestData(this, ID_STR(_name), PP_MakeDouble(0.0), PP_FALSE)
 
   // These property types are always invalid for string properties.
-  #define ADD_STRING_INVALID_PROPERTIES(_prop_name) \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeNull(), PP_FALSE), \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeBool(PP_FALSE), PP_FALSE), \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeInt32(0), PP_FALSE), \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeDouble(0.0), PP_FALSE)
-
-  #define ADD_INT_INVALID_PROPERTIES(_prop_name) \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeUndefined(), PP_FALSE), \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeNull(), PP_FALSE), \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeBool(PP_FALSE), PP_FALSE), \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeString(""), PP_FALSE), \
-    PropertyTestData(ID_STR(_prop_name), PP_MakeDouble(0.0), PP_FALSE)
+  #define TEST_STRING_INVALID(_name) \
+    PropertyTestData(this, ID_STR(_name), PP_MakeNull(), PP_FALSE), \
+    PropertyTestData(this, ID_STR(_name), PP_MakeBool(PP_FALSE), PP_FALSE), \
+    PropertyTestData(this, ID_STR(_name), PP_MakeInt32(0), PP_FALSE), \
+    PropertyTestData(this, ID_STR(_name), PP_MakeDouble(0.0), PP_FALSE)
+
+  #define TEST_INT_INVALID(_name) \
+    PropertyTestData(this, ID_STR(_name), PP_MakeUndefined(), PP_FALSE), \
+    PropertyTestData(this, ID_STR(_name), PP_MakeNull(), PP_FALSE),  \
+    PropertyTestData(this, ID_STR(_name), PP_MakeBool(PP_FALSE), PP_FALSE), \
+    PropertyTestData(this, ID_STR(_name), PP_MakeString("notint"), PP_FALSE), \
+    PropertyTestData(this, ID_STR(_name), PP_MakeDouble(0.0), PP_FALSE)
 
   // SetProperty accepts plenty of invalid values (malformed urls, negative
   // thresholds, etc). Error checking is delayed until request opening (aka url
   // loading).
   #define ID_STR(arg) arg, #arg
   PropertyTestData test_data[] = {
-      ADD_BOOL_PROPERTY(PP_URLREQUESTPROPERTY_STREAMTOFILE),
-      ADD_BOOL_PROPERTY(PP_URLREQUESTPROPERTY_FOLLOWREDIRECTS),
-      ADD_BOOL_PROPERTY(PP_URLREQUESTPROPERTY_RECORDDOWNLOADPROGRESS),
-      ADD_BOOL_PROPERTY(PP_URLREQUESTPROPERTY_RECORDUPLOADPROGRESS),
-      ADD_BOOL_PROPERTY(PP_URLREQUESTPROPERTY_ALLOWCROSSORIGINREQUESTS),
-      ADD_BOOL_PROPERTY(PP_URLREQUESTPROPERTY_ALLOWCREDENTIALS),
-      ADD_STRING_INVALID_PROPERTIES(PP_URLREQUESTPROPERTY_URL),
-      ADD_STRING_INVALID_PROPERTIES(PP_URLREQUESTPROPERTY_METHOD),
-      ADD_STRING_INVALID_PROPERTIES(PP_URLREQUESTPROPERTY_HEADERS),
-      ADD_STRING_INVALID_PROPERTIES(
-          PP_URLREQUESTPROPERTY_CUSTOMREFERRERURL),
-      ADD_STRING_INVALID_PROPERTIES(
-          PP_URLREQUESTPROPERTY_CUSTOMCONTENTTRANSFERENCODING),
-      ADD_INT_INVALID_PROPERTIES(
-          PP_URLREQUESTPROPERTY_PREFETCHBUFFERUPPERTHRESHOLD),
-      ADD_INT_INVALID_PROPERTIES(
-          PP_URLREQUESTPROPERTY_PREFETCHBUFFERLOWERTHRESHOLD),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_URL),
+      TEST_BOOL(PP_URLREQUESTPROPERTY_STREAMTOFILE),
+      TEST_BOOL(PP_URLREQUESTPROPERTY_FOLLOWREDIRECTS),
+      TEST_BOOL(PP_URLREQUESTPROPERTY_RECORDDOWNLOADPROGRESS),
+      TEST_BOOL(PP_URLREQUESTPROPERTY_RECORDUPLOADPROGRESS),
+      TEST_BOOL(PP_URLREQUESTPROPERTY_ALLOWCROSSORIGINREQUESTS),
+      TEST_BOOL(PP_URLREQUESTPROPERTY_ALLOWCREDENTIALS),
+      TEST_STRING_INVALID(PP_URLREQUESTPROPERTY_URL),
+      TEST_STRING_INVALID(PP_URLREQUESTPROPERTY_METHOD),
+      TEST_STRING_INVALID(PP_URLREQUESTPROPERTY_HEADERS),
+      TEST_STRING_INVALID(PP_URLREQUESTPROPERTY_CUSTOMREFERRERURL),
+      TEST_STRING_INVALID(PP_URLREQUESTPROPERTY_CUSTOMCONTENTTRANSFERENCODING),
+      TEST_INT_INVALID(PP_URLREQUESTPROPERTY_PREFETCHBUFFERUPPERTHRESHOLD),
+      TEST_INT_INVALID(PP_URLREQUESTPROPERTY_PREFETCHBUFFERLOWERTHRESHOLD),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_URL),
           PP_MakeString("http://www.google.com"), PP_TRUE),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_URL),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_URL),
           PP_MakeString("foo.jpg"), PP_TRUE),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_METHOD),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_METHOD),
           PP_MakeString("GET"), PP_TRUE),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_METHOD),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_METHOD),
           PP_MakeString("POST"), PP_TRUE),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_HEADERS),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_HEADERS),
           PP_MakeString("Accept: text/plain"), PP_TRUE),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_HEADERS),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_HEADERS),
           PP_MakeString(""), PP_TRUE),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_CUSTOMREFERRERURL),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_CUSTOMREFERRERURL),
           PP_MakeString("http://www.google.com"), PP_TRUE),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_CUSTOMREFERRERURL),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_CUSTOMREFERRERURL),
           PP_MakeString(""), PP_TRUE),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_CUSTOMREFERRERURL),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_CUSTOMREFERRERURL),
           PP_MakeUndefined(), PP_TRUE),
-      PropertyTestData(
+      PropertyTestData(this,
           ID_STR(PP_URLREQUESTPROPERTY_CUSTOMCONTENTTRANSFERENCODING),
           PP_MakeString("base64"), PP_TRUE),
-      PropertyTestData(
+      PropertyTestData(this,
           ID_STR(PP_URLREQUESTPROPERTY_CUSTOMCONTENTTRANSFERENCODING),
           PP_MakeString(""), PP_TRUE),
-      PropertyTestData(
+      PropertyTestData(this,
           ID_STR(PP_URLREQUESTPROPERTY_CUSTOMCONTENTTRANSFERENCODING),
           PP_MakeUndefined(), PP_TRUE),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_URL),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_URL),
           PP_MakeUndefined(), PP_FALSE),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_METHOD),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_METHOD),
           PP_MakeUndefined(), PP_FALSE),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_HEADERS),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_HEADERS),
           PP_MakeString("Proxy-Authorization: Basic dXNlcjpwYXNzd29yZA=="),
           PP_TRUE),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_HEADERS),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_HEADERS),
           PP_MakeString("Accept-Encoding: *\n"
                         "Accept-Charset: iso-8859-5, unicode-1-1;q=0.8"),
           PP_TRUE),
-      PropertyTestData(
+      PropertyTestData(this,
           ID_STR(PP_URLREQUESTPROPERTY_PREFETCHBUFFERUPPERTHRESHOLD),
           PP_MakeInt32(0), PP_TRUE),
-      PropertyTestData(
+      PropertyTestData(this,
           ID_STR(PP_URLREQUESTPROPERTY_PREFETCHBUFFERUPPERTHRESHOLD),
           PP_MakeInt32(100), PP_TRUE),
-      PropertyTestData(
+      PropertyTestData(this,
           ID_STR(PP_URLREQUESTPROPERTY_PREFETCHBUFFERLOWERTHRESHOLD),
           PP_MakeInt32(0), PP_TRUE),
-      PropertyTestData(
+      PropertyTestData(this,
           ID_STR(PP_URLREQUESTPROPERTY_PREFETCHBUFFERLOWERTHRESHOLD),
           PP_MakeInt32(100), PP_TRUE),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_URL),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_URL),
           PP_MakeString("::::::::::::"), PP_TRUE),
-      PropertyTestData(ID_STR(PP_URLREQUESTPROPERTY_METHOD),
+      PropertyTestData(this, ID_STR(PP_URLREQUESTPROPERTY_METHOD),
           PP_MakeString("INVALID"), PP_TRUE),
-      PropertyTestData(
+      PropertyTestData(this,
           ID_STR(PP_URLREQUESTPROPERTY_CUSTOMCONTENTTRANSFERENCODING),
           PP_MakeString("invalid"), PP_TRUE),
-      PropertyTestData(
+      PropertyTestData(this,
           ID_STR(PP_URLREQUESTPROPERTY_PREFETCHBUFFERUPPERTHRESHOLD),
           PP_MakeInt32(-100), PP_TRUE),
-      PropertyTestData(
+      PropertyTestData(this,
           ID_STR(PP_URLREQUESTPROPERTY_PREFETCHBUFFERLOWERTHRESHOLD),
           PP_MakeInt32(-100), PP_TRUE),
-
   };
-
-  const PPB_URLRequestInfo* ppb = PPBURLRequestInfo();
-  PP_Resource url_request = ppb->Create(pp_instance());
-  EXPECT(url_request != kInvalidResource);
-
+  std::string error;
+
+  PP_Resource url_request = ppb_url_request_interface_->Create(
+      instance_->pp_instance());
+  if (url_request == kInvalidResource)
+    error = "Failed to create a URLRequestInfo";
+
+  // Loop over all test data even if we encountered an error to release vars.
   for (size_t i = 0;
-       i < sizeof(test_data)/sizeof(PropertyTestData);
+       i < sizeof(test_data) / sizeof(PropertyTestData);

[dmichael (off chromium), 2012/01/17 18:38:03]

nit: best practice is to use sizeof(test_data[0]) in the denominator, in case
test_data's type changes.

[polina1, 2012/01/26 02:56:26]

Done.

        ++i) {
-    if (test_data[i].expected_value !=
-            ppb->SetProperty(url_request,
-                             test_data[i].property,
-                             test_data[i].var)) {
-        nacl::string error_string = nacl::string("Setting property ") +
-            test_data[i].property_name.c_str() + " to " +
-            StringifyVar(test_data[i].var) + " did not return " +
-            StringifyVar(PP_MakeBool(test_data[i].expected_value));
-        // PostTestMessage will signal test failure here.
-        PostTestMessage(__FUNCTION__, error_string.c_str());
+    if (error.empty() && test_data[i].expected_value !=
+        ppb_url_request_interface_->SetProperty(url_request,
+                                                test_data[i].property,
+                                                test_data[i].var)) {
+      pp::Var var(pp::Var::DontManage(), test_data[i].var);
+      error = std::string("Setting property ") +
+          test_data[i].property_name + " to " + var.DebugString() +
+          " did not return " + (test_data[i].expected_value ? "True" : "False");
+      error = test_data[i].property_name;

[dmichael (off chromium), 2012/01/17 18:38:03]

FWIW, I think this is a perfect example where building a custom error string is
better than using the ASSERT_* macros. My suggestion about using ASSERT_*
doesn't apply here.

[polina1, 2012/01/26 02:56:26]

Done.

     }
-    PPBVar()->Release(test_data[i].var);
-  }
-
-  PPBCore()->ReleaseResource(url_request);
-  TEST_PASSED;
-}
-
-
-void LoadAndCompareBody(PP_Resource request,
-                        const nacl::string& expected_body) {
-  PP_Resource url_loader = PPBURLLoader()->Create(pp_instance());
-  EXPECT(url_loader != kInvalidResource);
-
-  TestableCallback callback(pp_instance(), true);
-
-  int32_t result = PPBURLLoader()->Open(url_loader,
-                                        request,
-                                        callback.GetCallback());
-  EXPECT(PP_OK_COMPLETIONPENDING == result);
-  result = callback.WaitForResult();
-  EXPECT(PP_OK == result);
-
-  PP_Resource response = PPBURLLoader()->GetResponseInfo(url_loader);
-  EXPECT(response != kInvalidResource);
-
-  PP_Var status =
-      PPBURLResponseInfo()->GetProperty(response,
-                                        PP_URLRESPONSEPROPERTY_STATUSCODE);
-  EXPECT_VAR_INT(status, 200);
-
-  nacl::string actual_body;
-
-  // Read the entire body in this loop.
-  for (;;) {
-    callback.Reset();
-    const size_t kBufferSize = 32;
-    char buf[kBufferSize];
-    result = PPBURLLoader()->ReadResponseBody(url_loader,
-                                              buf, kBufferSize,
-                                              callback.GetCallback());
-    EXPECT(PP_OK_COMPLETIONPENDING == result);
-    result = callback.WaitForResult();
-    EXPECT(result >= PP_OK);
-    if (result < 0) {
-      return;
+    ppb_var_interface_->Release(test_data[i].var);

[dmichael (off chromium), 2012/01/17 18:38:03]

Ah, you can ignore my prior comment about releasing the var in the destructor,
if you want :-)

[polina1, 2012/01/26 02:56:26]

I already responded to that comment before reading this, so I'll just leave the
reply in.

+  }
+
+  ppb_core_interface_->ReleaseResource(url_request);
+  return error;  // == PASS() if empty.
+}
+
+std::string TestURLRequest::LoadAndCompareBody(
+    PP_Resource url_request, const std::string& expected_body) {
+  TestCompletionCallback test_callback(instance_->pp_instance(), true);
+  pp::CompletionCallback callback =
+      static_cast<pp::CompletionCallback>(test_callback);
+  int32_t result = ppb_url_loader_interface_->Open(
+      url_loader_, url_request, callback.pp_completion_callback());
+  if (PP_OK_COMPLETIONPENDING != result)
+    return ReportError("PPB_URLLoader::Open()", result);
+  result = test_callback.WaitForResult();
+  if (PP_OK != result)
+    return ReportError("PPB_URLLoader::Open()", result);
+
+  std::string error;
+  PP_Resource url_response =
+      ppb_url_loader_interface_->GetResponseInfo(url_loader_);
+  if (url_response == kInvalidResource) {
+    error = "PPB_URLLoader::GetResponseInfo() returned invalid resource";
+  } else {
+    PP_Var status = ppb_url_response_interface_->GetProperty(
+        url_response, PP_URLRESPONSEPROPERTY_STATUSCODE);
+    if (status.type != PP_VARTYPE_INT32 && status.value.as_int != 200)
+      error = ReportError("PPB_URLLoader::Open() status", status.value.as_int);
+
+    std::string actual_body;
+    for (; error.empty();) {  // Read the entire body in this loop.
+      const size_t kBufferSize = 32;
+      char buf[kBufferSize];
+      result = ppb_url_loader_interface_->ReadResponseBody(
+          url_loader_, buf, kBufferSize,
+          callback.pp_completion_callback());
+      if (PP_OK_COMPLETIONPENDING != result) {
+        error = ReportError("PPB_URLLoader::ReadResponseBody()", result);
+        break;
+      }
+      result = test_callback.WaitForResult();
+      if (result < PP_OK)
+        error = ReportError("PPB_URLLoader::ReadResponseBody()", result);
+      if (result <= PP_OK)
+        break;
+      actual_body.append(buf, result);
     }
-    if (PP_OK == result) {
+    if (actual_body != expected_body)
+      error = "PPB_URLLoader::ReadResponseBody() read unexpected response";
+  }
+  ppb_core_interface_->ReleaseResource(url_response);
+  return error;
+}
+
+// Tests
+//   PP_Bool AppendDataToBody(
+//       PP_Resource request, const void* data, uint32_t len);
+std::string TestURLRequest::TestAppendDataToBody() {
+  PP_Resource url_request = ppb_url_request_interface_->Create(
+      instance_->pp_instance());
+  if (url_request == kInvalidResource)
+    return "Create() failed\n";
+
+  std::string postdata("sample postdata");
+  std::string error;
+  PP_Var post_string_var = PP_MakeString("POST");
+  PP_Var echo_string_var = PP_MakeString("/echo");
+
+  // NULL pointer causes a crash. In general PPAPI implementation does not
+  // test for NULL because they are just a special case of bad pointers that
+  // are not detectable if set to point to an object that does not exist.
+
+  // Invalid resource should fail.
+  if (PP_TRUE == ppb_url_request_interface_->AppendDataToBody(
+      kInvalidResource, postdata.data(), postdata.length())) {
+    error = "AppendDataToBody() succeeded with invalid resource";
+  // Append data and POST to echoing web server.
+  } else if (PP_FALSE == ppb_url_request_interface_->SetProperty(
+      url_request, PP_URLREQUESTPROPERTY_METHOD, post_string_var)) {
+    error = "SetProperty(METHOD) failed\n";
+  } else if (PP_FALSE == ppb_url_request_interface_->SetProperty(
+      url_request, PP_URLREQUESTPROPERTY_URL, echo_string_var)) {
+    error = "SetProperty(URL) failed\n";
+  } else if (PP_FALSE == ppb_url_request_interface_->AppendDataToBody(
+      url_request, postdata.data(), postdata.length())) {
+    error = "AppendDataToBody() failed";
+  } else {
+    // Check for success.
+    error = LoadAndCompareBody(url_request, postdata);
+  }
+
+  ppb_var_interface_->Release(post_string_var);
+  ppb_var_interface_->Release(echo_string_var);
+  ppb_core_interface_->ReleaseResource(url_request);
+  return error;  // == PASS() if empty.
+}
+
+// TODO(elijahtaylor): add TestAppendFileToBody based on a broken disabled
+// version from a NaCl test - see crbug.com/110242 for details.
+
+// Allocates and manipulates a large number of resources.
+std::string TestURLRequest::TestStress() {
+  const int kManyResources = 500;
+  PP_Resource url_request_info[kManyResources];
+
+  std::string error;
+  int num_created = kManyResources;
+  for (int i = 0; i < kManyResources; i++) {
+    url_request_info[i] = ppb_url_request_interface_->Create(
+        instance_->pp_instance());
+    if (url_request_info[i] == kInvalidResource) {
+      error = "Create() failed";
+    } else if (PP_FALSE == ppb_url_request_interface_->IsURLRequestInfo(
+        url_request_info[i])) {
+      error = "IsURLRequestInfo() failed";
+    } else if (PP_FALSE == ppb_url_request_interface_->SetProperty(
+        url_request_info[i],
+        PP_URLREQUESTPROPERTY_STREAMTOFILE,
+        PP_MakeBool(PP_FALSE))) {
+      error = "SetProperty() failed";
+    }
+    if (!error.empty()) {
+      num_created = i + 1;
       break;
     }
-    actual_body.append(buf, result);
-  }
-
-  EXPECT(actual_body == expected_body);
-
-  PPBCore()->ReleaseResource(response);
-  PPBCore()->ReleaseResource(url_loader);
-}
-
-void SetupPOSTURLRequest(PP_Resource request) {
-  const PPB_URLRequestInfo* ppb = PPBURLRequestInfo();
-  EXPECT(PP_TRUE == ppb->SetProperty(request,
-                                     PP_URLREQUESTPROPERTY_METHOD,
-                                     PP_MakeString("POST")));
-  EXPECT(PP_TRUE == ppb->SetProperty(request,
-                                     PP_URLREQUESTPROPERTY_URL,
-                                     PP_MakeString("/echo")));
-}
-
-void TestAppendDataToBody() {
-  const PPB_URLRequestInfo* ppb = PPBURLRequestInfo();
-  PP_Resource url_request = ppb->Create(pp_instance());
-  EXPECT(url_request != kInvalidResource);
-  nacl::string postdata("sample postdata");
-
-  // Bad pointer passed in should fail to proxy.
-  EXPECT(PP_FALSE == ppb->AppendDataToBody(url_request,
-                                           NULL,
-                                           1));
-  // Invalid resource should fail.
-  EXPECT(PP_FALSE == ppb->AppendDataToBody(kInvalidResource,
-                                          postdata.data(),
-                                          postdata.length()));
-  // Append data and POST to echoing web server.
-  SetupPOSTURLRequest(url_request);
-  EXPECT(PP_TRUE == ppb->AppendDataToBody(url_request,
-                                          postdata.data(),
-                                          postdata.length()));
-  // Check for success.
-  LoadAndCompareBody(url_request, postdata);
-
-  PPBCore()->ReleaseResource(url_request);
-  TEST_PASSED;
-}
-
-// TODO(elijahtaylor): Revisit this function when it is enabled. Parts could be
-// factored out and it could be smaller and more readable in general.
-void TestAppendFileToBody() {
-  const int64_t kFileSystemSize = 1024;
-  const PPB_URLRequestInfo* ppb = PPBURLRequestInfo();
-  PP_Resource url_request = ppb->Create(pp_instance());
-  EXPECT(url_request != kInvalidResource);
-
-  // Creating a file reference PP_Resource is a multi-stage process.
-  // First, create and open a new file system.
-  const PPB_FileSystem* ppb_file_sys = PPBFileSystem();
-  PP_Resource file_sys =
-      ppb_file_sys->Create(pp_instance(),
-                           PP_FILESYSTEMTYPE_EXTERNAL);
-  EXPECT(file_sys != kInvalidResource);
-
-  TestableCallback callback(pp_instance(), true);
-
-  int32_t result = ppb_file_sys->Open(file_sys,
-                                      kFileSystemSize,
-                                      callback.GetCallback());
-  EXPECT(PP_OK_COMPLETIONPENDING == result);
-  result = callback.WaitForResult();
-
-  EXPECT(PP_OK == result);
-
-  // Create a file reference, then tie it to a new writable file io resource.
-  PP_Resource file_ref = PPBFileRef()->Create(file_sys, "/foo");
-  EXPECT(file_ref != kInvalidResource);
-  PP_Resource file_io = PPBFileIO()->Create(pp_instance());
-  EXPECT(file_ref != kInvalidResource);
-
-  callback.Reset();
-  result = PPBFileIO()->Open(file_io,
-                             file_ref,
-                             PP_FILEOPENFLAG_WRITE | PP_FILEOPENFLAG_CREATE |
-                             PP_FILEOPENFLAG_TRUNCATE,
-                             callback.GetCallback());
-
-  EXPECT(PP_OK_COMPLETIONPENDING == result);
-  result = callback.WaitForResult();
-  EXPECT(PP_OK == result);
-
-  // Now we can write the contents of this string to the file io resource.
-  nacl::string postdata("sample postdata from file");
-  size_t total_written = 0;
-  for (;;) {
-    callback.Reset();
-    result = PPBFileIO()->Write(file_io,
-                                total_written,
-                                postdata.data() + total_written,
-                                postdata.length() - total_written,
-                                callback.GetCallback());
-
-    EXPECT(PP_OK_COMPLETIONPENDING == result);
-    result = callback.WaitForResult();
-
-    EXPECT(result >= PP_OK);
-    if (result < 0)
-      return;
-    if (result == PP_OK)
-      break;
-    total_written += result;
-  }
-  PPBFileIO()->Close(file_io);
-
-  const int64_t kReadUntilEOF = -1;
-  const PP_Time kDummyTimeValue = 5;  // seconds since epoch
-
-  // Finally, set up the url_request to post to an echoing web server.
-  SetupPOSTURLRequest(url_request);
-  EXPECT(PP_TRUE == ppb->AppendFileToBody(url_request,
-                                          file_ref,
-                                          0,    // start offset
-                                          postdata.length(),
-                                          0));  // expected last modified time
-  LOG_TO_BROWSER("testing ordinary 'AppendFileToBody'");
-  // Check for success.
-  LoadAndCompareBody(url_request, postdata);
-
-  // Use a new url_request for another POST request
-  PPBCore()->ReleaseResource(url_request);
-  url_request = ppb->Create(pp_instance());
-  EXPECT(url_request != kInvalidResource);
-
-  callback.Reset();
-  // Set the timestamps so we can test 'expected last modified time'
-  result = PPBFileIO()->Touch(file_ref,
-                              kDummyTimeValue+1,
-                              kDummyTimeValue,
-                              callback.GetCallback());
-
-  SetupPOSTURLRequest(url_request);
-  EXPECT(PP_TRUE == ppb->AppendFileToBody(url_request,
-                                          file_ref,
-                                          0,  // start offset
-                                          postdata.length(),
-                                          kDummyTimeValue));
-  LOG_TO_BROWSER("testing 'expected last modified time'");
-  LoadAndCompareBody(url_request, postdata);
-
-  PPBCore()->ReleaseResource(url_request);
-  url_request = ppb->Create(pp_instance());
-  EXPECT(url_request != kInvalidResource);
-
-  SetupPOSTURLRequest(url_request);
-  // Test for failure on expected last modified time, it won't fail until an
-  // actual request is made.
-  EXPECT(PP_TRUE == ppb->AppendFileToBody(url_request,
-                                          file_ref,
-                                          0,  // start offset
-                                          kReadUntilEOF,  // number of bytes
-                                          kDummyTimeValue - 1));
-  LOG_TO_BROWSER("testing 'expected last modified time' failure");
-  PP_Resource url_loader = PPBURLLoader()->Create(pp_instance());
-  EXPECT(url_loader != kInvalidResource);
-  callback.Reset();
-
-  result = PPBURLLoader()->Open(url_loader,
-                                url_request,
-                                callback.GetCallback());
-  EXPECT(PP_OK_COMPLETIONPENDING == result);
-  result = callback.WaitForResult();
-  EXPECT(PP_ERROR_FILECHANGED == result);
-
-  // Invalid Resource is failure.
-  EXPECT(PP_FALSE == ppb->AppendFileToBody(url_request,
-                                           kInvalidResource,
-                                           0,    // start offset
-                                           1,    // length
-                                           0));  // expected last modified time
-  // Test bad start offset.
-  EXPECT(PP_FALSE == ppb->AppendFileToBody(url_request,
-                                           file_ref,
-                                           -1,   // start offset
-                                           1,    // length
-                                           0));  // expected last modified time
-  // Test invalid length. (-1 means read until end of file, hence -2)
-  EXPECT(PP_FALSE == ppb->AppendFileToBody(url_request,
-                                           kInvalidResource,
-                                           0,    // start offset
-                                           -2,   // length
-                                           0));  // expected last modified time
-  PPBCore()->ReleaseResource(url_request);
-  PPBCore()->ReleaseResource(file_ref);
-  PPBCore()->ReleaseResource(file_sys);
-  TEST_PASSED;
-}
-
-// Allocates and manipulates a large number of resources.
-void TestStress() {
-  const int kManyResources = 500;
-  PP_Resource url_request_info[kManyResources];
-  const PPB_URLRequestInfo* ppb = PPBURLRequestInfo();
-
-  for (int i = 0; i < kManyResources; i++) {
-    url_request_info[i] = ppb->Create(pp_instance());
-    EXPECT(url_request_info[i] != kInvalidResource);
-    EXPECT(PP_TRUE == ppb->IsURLRequestInfo(url_request_info[i]));
-    EXPECT(PP_TRUE == ppb->SetProperty(url_request_info[i],
-                                       PP_URLREQUESTPROPERTY_STREAMTOFILE,
-                                       PP_MakeBool(PP_FALSE)));
-  }
-  for (int i = 0; i < kManyResources; i++) {
-    PPBCore()->ReleaseResource(url_request_info[i]);
-    EXPECT(PP_FALSE == ppb->IsURLRequestInfo(url_request_info[i]));
-  }
-
-  TEST_PASSED;
-}
-
-}  // namespace
-
-void SetupTests() {
-  RegisterTest("TestCreate", TestCreate);
-  RegisterTest("TestIsURLRequestInfo", TestIsURLRequestInfo);
-  RegisterTest("TestSetProperty", TestSetProperty);
-  RegisterTest("TestAppendDataToBody", TestAppendDataToBody);
-  RegisterTest("TestAppendFileToBody", TestAppendFileToBody);
-  RegisterTest("TestStress", TestStress);
-}
-
-void SetupPluginInterfaces() {
-  // none
-}
+  }
+  for (int i = 0; i < num_created; i++) {
+    ppb_core_interface_->ReleaseResource(url_request_info[i]);
+    if (PP_TRUE ==
+        ppb_url_request_interface_->IsURLRequestInfo(url_request_info[i]))
+      error = "IsURLREquestInfo() succeeded after release";
+  }
+  return error;  // == PASS() if empty.
+}