Web MIDI: call AccumulateMidiBytesSent() after operation finished

For better flow control, call AccumulateMidiBytesSent() after
send operation finished.

BUG=467442

Review URL: https://codereview.chromium.org/1060553003

Cr-Commit-Position: refs/heads/master@{#324037}

6 files changed, 39 insertions, 21 deletions

diff --git a/media/midi/midi_manager.cc b/media/midi/midi_manager.cc
index f7de36f..183988e 100644
--- a/media/midi/midi_manager.cc
+++ b/media/midi/midi_manager.cc
@@ -94,6 +94,15 @@ void MidiManager::EndSession(MidiManagerClient* client) {
   pending_clients_.erase(client);
 }
 
+void MidiManager::AccumulateMidiBytesSent(MidiManagerClient* client, size_t n) {
+  {
+    base::AutoLock auto_lock(lock_);
+    if (clients_.find(client) == clients_.end())
+      return;
+  }
+  client->AccumulateMidiBytesSent(n);
+}
+
 void MidiManager::DispatchSendMidiData(MidiManagerClient* client,
                                        uint32 port_index,
                                        const std::vector<uint8>& data,
diff --git a/media/midi/midi_manager.h b/media/midi/midi_manager.h
index a7c5262..2904c11 100644
--- a/media/midi/midi_manager.h
+++ b/media/midi/midi_manager.h
@@ -86,6 +86,10 @@ class MEDIA_EXPORT MidiManager {
   // A client calls EndSession() to stop receiving MIDI data.
   void EndSession(MidiManagerClient* client);
 
+  // Invoke AccumulateMidiBytesSent() for |client| safely. If the session was
+  // already closed, do nothing.
+  void AccumulateMidiBytesSent(MidiManagerClient* client, size_t n);
+
   // DispatchSendMidiData() is called when MIDI data should be sent to the MIDI
   // system.
   // This method is supposed to return immediately and should not block.
diff --git a/media/midi/midi_manager_usb.cc b/media/midi/midi_manager_usb.cc
index 2571727..cefbc1a 100644
--- a/media/midi/midi_manager_usb.cc
+++ b/media/midi/midi_manager_usb.cc
@@ -33,7 +33,7 @@ void MidiManagerUsb::StartInitialization() {
 void MidiManagerUsb::Initialize(
     base::Callback<void(MidiResult result)> callback) {
   initialize_callback_ = callback;
-  scheduler_.reset(new MidiScheduler);
+  scheduler_.reset(new MidiScheduler(this));
   // This is safe because EnumerateDevices cancels the operation on destruction.
   device_factory_->EnumerateDevices(
       this,
diff --git a/media/midi/midi_manager_usb_unittest.cc b/media/midi/midi_manager_usb_unittest.cc
index b699576..7f5bb27 100644
--- a/media/midi/midi_manager_usb_unittest.cc
+++ b/media/midi/midi_manager_usb_unittest.cc
@@ -320,8 +320,8 @@ TEST_F(MidiManagerUsbTest, InitializeFailBecauseOfInvalidDescriptor) {
 }
 
 TEST_F(MidiManagerUsbTest, Send) {
+  Initialize();
   scoped_ptr<FakeUsbMidiDevice> device(new FakeUsbMidiDevice(&logger_));
-  FakeMidiManagerClient client(&logger_);
   uint8 descriptor[] = {
     0x12, 0x01, 0x10, 0x01, 0x00, 0x00, 0x00, 0x08, 0x86, 0x1a,
     0x2d, 0x75, 0x54, 0x02, 0x00, 0x02, 0x00, 0x01, 0x09, 0x02,
@@ -345,7 +345,6 @@ TEST_F(MidiManagerUsbTest, Send) {
     0xf0, 0x00, 0x01, 0xf7,
   };
 
-  Initialize();
   ScopedVector<UsbMidiDevice> devices;
   devices.push_back(device.release());
   EXPECT_FALSE(IsInitializationCallbackInvoked());
@@ -353,25 +352,22 @@ TEST_F(MidiManagerUsbTest, Send) {
   EXPECT_EQ(MIDI_OK, GetInitializationResult());
   ASSERT_EQ(2u, manager_->output_streams().size());
 
-  manager_->DispatchSendMidiData(&client, 1, ToVector(data), 0);
+  manager_->DispatchSendMidiData(client_.get(), 1, ToVector(data), 0);
   // Since UsbMidiDevice::Send is posted as a task, RunLoop should run to
   // invoke the task.
-  // TODO(crbug.com/467442): AccumulateMidiBytesSent is recorded before
-  // UsbMidiDevice is invoked for now, but this should be after the invocation.
   base::RunLoop run_loop;
   run_loop.RunUntilIdle();
   EXPECT_EQ("UsbMidiDevice::GetDescriptor\n"
-            "MidiManagerClient::AccumulateMidiBytesSent size = 7\n"
             "UsbMidiDevice::Send endpoint = 2 data = "
             "0x19 0x90 0x45 0x7f "
             "0x14 0xf0 0x00 0x01 "
-            "0x15 0xf7 0x00 0x00\n",
+            "0x15 0xf7 0x00 0x00\n"
+            "MidiManagerClient::AccumulateMidiBytesSent size = 7\n",
             logger_.TakeLog());
 }
 
 TEST_F(MidiManagerUsbTest, SendFromCompromizedRenderer) {
   scoped_ptr<FakeUsbMidiDevice> device(new FakeUsbMidiDevice(&logger_));
-  FakeMidiManagerClient client(&logger_);
   uint8 descriptor[] = {
     0x12, 0x01, 0x10, 0x01, 0x00, 0x00, 0x00, 0x08, 0x86, 0x1a,
     0x2d, 0x75, 0x54, 0x02, 0x00, 0x02, 0x00, 0x01, 0x09, 0x02,
@@ -405,11 +401,11 @@ TEST_F(MidiManagerUsbTest, SendFromCompromizedRenderer) {
   EXPECT_EQ("UsbMidiDevice::GetDescriptor\n", logger_.TakeLog());
 
   // The specified port index is invalid. The manager must ignore the request.
-  manager_->DispatchSendMidiData(&client, 99, ToVector(data), 0);
+  manager_->DispatchSendMidiData(client_.get(), 99, ToVector(data), 0);
   EXPECT_EQ("", logger_.TakeLog());
 
   // The specified port index is invalid. The manager must ignore the request.
-  manager_->DispatchSendMidiData(&client, 2, ToVector(data), 0);
+  manager_->DispatchSendMidiData(client_.get(), 2, ToVector(data), 0);
   EXPECT_EQ("", logger_.TakeLog());
 }
 
diff --git a/media/midi/midi_scheduler.cc b/media/midi/midi_scheduler.cc
index cc93733..a65e703 100644
--- a/media/midi/midi_scheduler.cc
+++ b/media/midi/midi_scheduler.cc
@@ -11,7 +11,9 @@
 
 namespace media {
 
-MidiScheduler::MidiScheduler() : weak_factory_(this) {
+MidiScheduler::MidiScheduler(MidiManager* manager)
+  : manager_(manager),
+    weak_factory_(this) {
 }
 
 MidiScheduler::~MidiScheduler() {
@@ -26,7 +28,11 @@ void MidiScheduler::PostSendDataTask(MidiManagerClient* client,
   DCHECK(client);
 
   const base::Closure& weak_closure = base::Bind(
-      &MidiScheduler::InvokeClosure, weak_factory_.GetWeakPtr(), closure);
+      &MidiScheduler::InvokeClosure,
+      weak_factory_.GetWeakPtr(),
+      client,
+      length,
+      closure);
 
   base::TimeDelta delay;
   if (timestamp != 0.0) {
@@ -37,15 +43,13 @@ void MidiScheduler::PostSendDataTask(MidiManagerClient* client,
   }
   base::MessageLoop::current()->task_runner()->PostDelayedTask(
       FROM_HERE, weak_closure, delay);
-
-  // TODO(crbug.com/467442): AccumulateMidiBytesSent should be called in
-  // InvokeClosure. But for now, we call it here since |client| may be deleted
-  // at that time.
-  client->AccumulateMidiBytesSent(length);
 }
 
-void MidiScheduler::InvokeClosure(const base::Closure& closure) {
+void MidiScheduler::InvokeClosure(MidiManagerClient* client,
+                                  size_t length,
+                                  const base::Closure& closure) {
   closure.Run();
+  manager_->AccumulateMidiBytesSent(client, length);
 }
 
 }  // namespace media
diff --git a/media/midi/midi_scheduler.h b/media/midi/midi_scheduler.h
index 3cb8433..06f6b57 100644
--- a/media/midi/midi_scheduler.h
+++ b/media/midi/midi_scheduler.h
@@ -10,12 +10,13 @@
 
 namespace media {
 
+class MidiManager;
 class MidiManagerClient;
 
 // TODO(crbug.com/467442): Make tasks cancelable per client.
 class MidiScheduler final {
  public:
-  MidiScheduler();
+  MidiScheduler(MidiManager* manager);
   ~MidiScheduler();
 
   // Post |closure| to the current message loop safely. The |closure| will not
@@ -27,8 +28,12 @@ class MidiScheduler final {
                         const base::Closure& closure);
 
  private:
-  void InvokeClosure(const base::Closure& closure);
+  void InvokeClosure(MidiManagerClient* client,
+                     size_t length,
+                     const base::Closure& closure);
 
+  // MidiManager should own the MidiScheduler and be alive longer.
+  MidiManager* manager_;
   base::WeakPtrFactory<MidiScheduler> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(MidiScheduler);