Web MIDI: introduce pending client count limit to start sessions

media/midi/midi_manager_unittest.cc

Index: media/midi/midi_manager_unittest.cc
diff --git a/media/midi/midi_manager_unittest.cc b/media/midi/midi_manager_unittest.cc
index 178b0f66d0382863d3d005127902114d994f2882..034cf58498815c10fde29da7607199ab62da56eb 100644
--- a/media/midi/midi_manager_unittest.cc
+++ b/media/midi/midi_manager_unittest.cc
@@ -4,6 +4,8 @@
 
 #include "media/midi/midi_manager.h"
 
+#include <vector>
+
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -53,13 +55,18 @@ class FakeMidiManager : public MidiManager {
 
 class FakeMidiManagerClient : public MidiManagerClient {
  public:
-  FakeMidiManagerClient(int client_id) : client_id_(client_id) {}
+  FakeMidiManagerClient(int client_id)

[yukawa, 2014/05/04 22:44:54]

explicit?

[Takashi Toyoshima, 2014/05/04 23:28:44]

Done.

+      : client_id_(client_id),
+        result_(MIDI_NOT_SUPPORTED),
+        start_session_is_completed_(false) {}
   virtual ~FakeMidiManagerClient() {}
 
   // MidiManagerClient implementation.
   virtual void CompleteStartSession(int client_id, MidiResult result) OVERRIDE {
-    DCHECK_EQ(client_id_, client_id);
+    EXPECT_FALSE(start_session_is_completed_);
+    EXPECT_EQ(client_id_, client_id);
     result_ = result;
+    start_session_is_completed_ = true;
   }
 
   virtual void ReceiveMidiData(uint32 port_index, const uint8* data,
@@ -77,6 +84,7 @@ class FakeMidiManagerClient : public MidiManagerClient {
  private:
   int client_id_;
   MidiResult result_;
+  bool start_session_is_completed_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeMidiManagerClient);
 };
@@ -110,15 +118,17 @@ class MidiManagerTest : public ::testing::Test {
     }
   }
 
-  void StartTheSecondSession(FakeMidiManagerClient* client) {
-    EXPECT_TRUE(manager_->start_initialization_is_called_);
+  void StartTheNthSession(FakeMidiManagerClient* client, size_t nth) {
+    EXPECT_EQ(nth != 1, manager_->start_initialization_is_called_);
     EXPECT_EQ(0U, manager_->GetClientCount());
-    EXPECT_EQ(1U, manager_->GetPendingClientCount());
+    EXPECT_EQ(nth - 1, manager_->GetPendingClientCount());
 
-    // StartInitialization() should not be called for the second session.
+    // StartInitialization() should not be called for the second and later
+    // sessions.
     manager_->start_initialization_is_called_ = false;
     manager_->StartSession(client, client->GetClientId());
-    EXPECT_FALSE(manager_->start_initialization_is_called_);
+    EXPECT_EQ(nth == 1, manager_->start_initialization_is_called_);
+    manager_->start_initialization_is_called_ = true;
   }
 
   void EndSession(FakeMidiManagerClient* client, size_t before, size_t after) {
@@ -131,9 +141,10 @@ class MidiManagerTest : public ::testing::Test {
     manager_->CallCompleteInitialization(result);
   }
 
- private:
+ protected:
   scoped_ptr<FakeMidiManager> manager_;
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(MidiManagerTest);
 };
 
@@ -174,7 +185,7 @@ TEST_F(MidiManagerTest, StartMultipleSessions) {
   client2.reset(new FakeMidiManagerClient(1));
 
   StartTheFirstSession(client1.get(), false);
-  StartTheSecondSession(client2.get());
+  StartTheNthSession(client2.get(), 2);
   CompleteInitialization(MIDI_OK);
   EXPECT_EQ(MIDI_OK, client1->GetResult());
   EXPECT_EQ(MIDI_OK, client2->GetResult());
@@ -182,6 +193,39 @@ TEST_F(MidiManagerTest, StartMultipleSessions) {
   EndSession(client2.get(), 1U, 0U);
 }
 
+TEST_F(MidiManagerTest, TooManyPendingSessions) {
+  // Push as many client requests for starting session as possible.
+  std::vector<scoped_ptr<FakeMidiManagerClient> > clients;
+  clients.resize(MidiManager::kMaxPendingClientCount);
+  for (size_t i = 0; i < MidiManager::kMaxPendingClientCount; ++i) {
+    clients[i].reset(new FakeMidiManagerClient(i));
+    StartTheNthSession(clients[i].get(), i + 1);
+  }
+
+  // Push the last client that should be rejected for too many pending requests.
+  scoped_ptr<FakeMidiManagerClient> client(
+      new FakeMidiManagerClient(MidiManager::kMaxPendingClientCount));
+  manager_->start_initialization_is_called_ = false;

[yukawa, 2014/05/04 22:44:54]

These direct interactions with |manager_| could be moved into MidiManagerTest
like MidiManagerTest::StartTheNthSession. I think this is minor issue and
basically it's up to you and other reviews but it looks a bit inconsistent if we
relax the access rights of |manager_| just for this test case.
What do you think?

[Takashi Toyoshima, 2014/05/04 23:28:44]

These checks are too specific on this test case, and hard to reuse. E.g., it
will introduce new method, or new arguments to share the implementation. So I'm
slightly prefer to current one for readability.

[yukawa, 2014/05/04 23:45:55]

Got it. Thanks.

+  manager_->StartSession(client.get(), client->GetClientId());
+  EXPECT_FALSE(manager_->start_initialization_is_called_);
+  EXPECT_EQ(MIDI_INITIALIZATION_ERROR, client->GetResult());
+
+  // Other clients still should not receive a result.
+  for (size_t i = 0; i < clients.size(); ++i)
+    EXPECT_EQ(MIDI_NOT_SUPPORTED, clients[i]->GetResult());
+
+  // The result MIDI_OK should be distributed to other clients.
+  CompleteInitialization(MIDI_OK);
+  for (size_t i = 0; i < clients.size(); ++i)
+    EXPECT_EQ(MIDI_OK, clients[i]->GetResult());
+
+  // Close all successful sessions in FIFO order.
+  size_t sessions = clients.size();
+  for (size_t i = 0; i < clients.size(); ++i, --sessions) {
+    EndSession(clients[i].get(), sessions, sessions - 1);
+  }

[yukawa, 2014/05/04 22:44:54]

Curly brackets can be omitted.

[Takashi Toyoshima, 2014/05/04 23:28:44]

Done.

+}
+
 }  // namespace
 
 }  // namespace media