WebSocket Pepper API: allow to release in completion callbacks

ppapi/tests/test_websocket.cc

Index: ppapi/tests/test_websocket.cc
diff --git a/ppapi/tests/test_websocket.cc b/ppapi/tests/test_websocket.cc
index d3b189a64b7df871f9fe28db962f0b9190f66405..6ba79bf52471d740ec1d2b5583f41dc58f3739de 100644
--- a/ppapi/tests/test_websocket.cc
+++ b/ppapi/tests/test_websocket.cc
@@ -68,16 +68,81 @@ struct WebSocketEvent {
   WebSocketEvent(EventType type,
                  bool was_clean,
                  uint16_t close_code,
-                 const pp::Var& var)
+                 const pp::Var& var,
+                 int32_t result)
       : event_type(type),
         was_clean(was_clean),
         close_code(close_code),
-        var(var) {
+        var(var),
+        result(result) {
   }
   EventType event_type;
   bool was_clean;
   uint16_t close_code;
   pp::Var var;
+  int32_t result;
+};
+
+class CallbackChecker {
+ public:
+  class CallbackDelegate : public TestCompletionCallback::Delegate {
+   public:
+    explicit CallbackDelegate(CallbackChecker* owner,
+                              WebSocketEvent::EventType type,
+                              bool release)
+        : owner_(owner),
+          type_(type),
+          release_(release) {
+    }
+
+    // TestCompletionCallback::Delegate implementation.
+    virtual void OnCallback(void* user_data, int32_t result) {
+      owner_->RecordCallback(type_, result);
+      if (release_)
+        owner_->ReleaseResource();
+    }
+
+   private:
+    CallbackChecker* owner_;
+    WebSocketEvent::EventType type_;
+    bool release_;
+  };
+
+  explicit CallbackChecker(const PPB_Core* core_interface,
+                           PP_Resource resource)
+      : core_interface_(core_interface),
+        resource_(resource) {
+  }
+
+  ~CallbackChecker() {
+    for (std::vector<CallbackDelegate*>::iterator it = delegates_.begin();
+        it < delegates_.end(); it++)
+      delete *it;
+  }
+
+  CallbackDelegate* GetDelegateFor(WebSocketEvent::EventType type,

[dmichael (off chromium), 2012/06/27 19:52:17]

I don't think you need CallbackChecker (see below). But if you keep it, this
should be named something more like "CreateDelegateFor"

+                                   bool release) {
+    delegates_.push_back(new CallbackDelegate(this, type, release));
+    return delegates_.back();
+  }
+
+  const std::vector<WebSocketEvent>& GetSeenEvents() const {
+    return events_;
+  }
+
+  void RecordCallback(WebSocketEvent::EventType type, int32_t result) {
+    events_.push_back(WebSocketEvent(type, true, 0U, pp::Var(), result));
+  }
+
+  void ReleaseResource() {
+    core_interface_->ReleaseResource(resource_);
+  }
+
+ private:
+  const PPB_Core* core_interface_;
+  PP_Resource resource_;
+  std::vector<WebSocketEvent> events_;
+  std::vector<CallbackDelegate*> delegates_;
 };
 
 class TestWebSocketAPI : public pp::WebSocketAPI {
@@ -95,7 +160,7 @@ class TestWebSocketAPI : public pp::WebSocketAPI {
 
   virtual void WebSocketDidOpen() {
     events_.push_back(
-        WebSocketEvent(WebSocketEvent::EVENT_OPEN, true, 0U, pp::Var()));
+        WebSocketEvent(WebSocketEvent::EVENT_OPEN, true, 0U, pp::Var(), 0));
     connected_ = true;
     if (wait_for_connected_) {
       GetTestingInterface()->QuitMessageLoop(instance_);
@@ -105,8 +170,8 @@ class TestWebSocketAPI : public pp::WebSocketAPI {
 
   virtual void WebSocketDidClose(
       bool was_clean, uint16_t code, const pp::Var& reason) {
-    events_.push_back(
-        WebSocketEvent(WebSocketEvent::EVENT_CLOSE, was_clean, code, reason));
+    events_.push_back(WebSocketEvent(
+        WebSocketEvent::EVENT_CLOSE, was_clean, code, reason, 0));
     connected_ = true;
     closed_ = true;
     if (wait_for_connected_ || wait_for_closed_) {
@@ -118,7 +183,7 @@ class TestWebSocketAPI : public pp::WebSocketAPI {
 
   virtual void HandleWebSocketMessage(const pp::Var &message) {
     events_.push_back(
-        WebSocketEvent(WebSocketEvent::EVENT_MESSAGE, true, 0U, message));
+        WebSocketEvent(WebSocketEvent::EVENT_MESSAGE, true, 0U, message, 0));
     received_ = true;
     if (wait_for_received_) {
       GetTestingInterface()->QuitMessageLoop(instance_);
@@ -129,7 +194,7 @@ class TestWebSocketAPI : public pp::WebSocketAPI {
 
   virtual void HandleWebSocketError() {
     events_.push_back(
-        WebSocketEvent(WebSocketEvent::EVENT_ERROR, true, 0U, pp::Var()));
+        WebSocketEvent(WebSocketEvent::EVENT_ERROR, true, 0U, pp::Var(), 0));
   }
 
   void WaitForConnected() {
@@ -938,6 +1003,57 @@ std::string TestWebSocket::TestAbortCalls() {
   receive_callback.WaitForResult(result);
   ASSERT_EQ(PP_ERROR_ABORTED, receive_callback.result());
 
+  // Release the resource in the close completion callback.
+  ws = Connect(url, &result, "");
+  ASSERT_TRUE(ws);
+  ASSERT_EQ(PP_OK, result);
+  result = websocket_interface_->Close(
+      ws, PP_WEBSOCKETSTATUSCODE_NORMAL_CLOSURE, PP_MakeUndefined(),
+      close_callback.GetCallback().pp_completion_callback());
+  CallbackChecker close_checker(core_interface_, ws);
+  close_callback.SetDelegate(close_checker.GetDelegateFor(
+      WebSocketEvent::EVENT_CLOSE, true));
+  close_callback.WaitForResult(result);
+  CHECK_CALLBACK_BEHAVIOR(close_callback);
+  ASSERT_EQ(PP_OK, close_callback.result());
+
+  const std::vector<WebSocketEvent>& close_events =
+      close_checker.GetSeenEvents();
+  ASSERT_EQ(1U, close_events.size());
+  ASSERT_EQ(WebSocketEvent::EVENT_CLOSE, close_events[0].event_type);
+  ASSERT_EQ(PP_OK, close_events[0].result);

[dmichael (off chromium), 2012/06/27 19:52:17]

I'm having trouble understanding why you wanted the "checker"? You're already
seeing whether the callback was fired with the right value when you do
close_callback.WaitForResult(result);
ASSERT_EQ(PP_OK, close_callback.result());

It seems overly complicated to check for it two different ways. Same comment for
the receive_checker below. Can you get rid of the CallbackChecker and just have
a Delegate that releases the resource?

[Takashi Toyoshima, 2012/06/28 08:05:37]

Ah... you are right.
Firstly, I'd like to check when the second callback is invoked. That's the
original reason why I try to introduce this CallbackChecker mechanism.
Using this, I checked its behavior locally, but I stopped to apply the check
because I think it will introduce flakiness. As a result, only meaningless
redundant checks remains here.

I remove CallbackChecker.

+
+  // Release the resource in the aborting receive completion callback which is
+  // introduced by calling Close().
+  ws = Connect(url, &result, "");
+  ASSERT_TRUE(ws);
+  ASSERT_EQ(PP_OK, result);
+  CallbackChecker receive_checker(core_interface_, ws);
+  result = websocket_interface_->ReceiveMessage(
+      ws, &receive_var,
+      receive_callback.GetCallback().pp_completion_callback());
+  receive_callback.SetDelegate(receive_checker.GetDelegateFor(
+      WebSocketEvent::EVENT_MESSAGE, true));
+  result = websocket_interface_->Close(
+      ws, PP_WEBSOCKETSTATUSCODE_NORMAL_CLOSURE, PP_MakeUndefined(),
+      close_callback.GetCallback().pp_completion_callback());
+  close_callback.SetDelegate(receive_checker.GetDelegateFor(
+      WebSocketEvent::EVENT_CLOSE, false));
+  receive_callback.WaitForResult(result);
+  CHECK_CALLBACK_BEHAVIOR(receive_callback);
+  ASSERT_EQ(PP_ERROR_ABORTED, receive_callback.result());
+  close_callback.WaitForResult(result);
+  CHECK_CALLBACK_BEHAVIOR(close_callback);
+  ASSERT_EQ(PP_ERROR_ABORTED, close_callback.result());
+
+  const std::vector<WebSocketEvent>& receive_events =
+      receive_checker.GetSeenEvents();
+  ASSERT_EQ(2U, receive_events.size());
+  ASSERT_EQ(WebSocketEvent::EVENT_MESSAGE, receive_events[0].event_type);
+  ASSERT_EQ(PP_ERROR_ABORTED, receive_events[0].result);
+  ASSERT_EQ(WebSocketEvent::EVENT_CLOSE, receive_events[1].event_type);
+  ASSERT_EQ(PP_ERROR_ABORTED, receive_events[1].result);
+
   // Test the behavior where receive process might be in-flight.
   const char* text = "yukarin";
   PP_Var text_var = CreateVarString(text);