MidiManager: Trivial cleanups

This patch makes several trivial changes:
 - Enforce GUARDED_BY against |clients_| and modify test only
   methods for that.
 - Initialize members in the header rather than constructor
   as we can as possible.
 - |finalized_| and |initialization_state_| do not have to be
   protected by |lock_|.
 - Enable GUARDED_BY in MidiManagerWinrt.

Bug: 672793
Change-Id: I726882c502b1d5b2414c5baa33ca31c3f630072e
Reviewed-on: https://chromium-review.googlesource.com/1249061Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595398}

media/midi/midi_manager.cc

@@ -58,20 +58,11 @@ void ReportUsage(Usage usage) {
 
 }  // namespace
 
-MidiManager::MidiManager(MidiService* service)
-    : initialization_state_(InitializationState::NOT_STARTED),
-      finalized_(false),
-      result_(Result::NOT_INITIALIZED),
-      data_sent_(false),
-      data_received_(false),
-      service_(service) {
+MidiManager::MidiManager(MidiService* service) : service_(service) {
   ReportUsage(Usage::CREATED);
 }
 
 MidiManager::~MidiManager() {
-  // Make sure that Finalize() is called to clean up resources allocated on
-  // the Chrome_IOThread.
-  base::AutoLock auto_lock(lock_);
   CHECK(finalized_);
 }
 
@@ -85,12 +76,13 @@ MidiManager* MidiManager::Create(MidiService* service) {
 
 void MidiManager::Shutdown() {
   Finalize();
+  finalized_ = true;
 
   base::AutoLock auto_lock(lock_);
-  if (session_thread_runner_)
+  if (session_thread_runner_) {
     DCHECK(session_thread_runner_->BelongsToCurrentThread());
-
-  finalized_ = true;
+    session_thread_runner_ = nullptr;
+  }
 
   UMA_HISTOGRAM_ENUMERATION("Media.Midi.ResultOnShutdown", result_,
                             static_cast<Sample>(Result::MAX) + 1);
@@ -110,18 +102,16 @@ void MidiManager::Shutdown() {
   for (auto* client : clients_)
     client->Detach();
   clients_.clear();
-
-  session_thread_runner_ = nullptr;
 }
 
 void MidiManager::StartSession(MidiManagerClient* client) {
+  DCHECK(!finalized_);
   ReportUsage(Usage::SESSION_STARTED);
 
   bool needs_initialization = false;
 
   {
     base::AutoLock auto_lock(lock_);
-    DCHECK(!finalized_);
 
     if (clients_.find(client) != clients_.end() ||
         pending_clients_.find(client) != pending_clients_.end()) {
@@ -207,22 +197,23 @@ void MidiManager::StartInitialization() {
 }
 
 void MidiManager::CompleteInitialization(Result result) {
+  DCHECK(!finalized_);
+  DCHECK_EQ(InitializationState::STARTED, initialization_state_);
+
   TRACE_EVENT0("midi", "MidiManager::CompleteInitialization");
   ReportUsage(Usage::INITIALIZED);
 
   base::AutoLock auto_lock(lock_);
+  if (!session_thread_runner_)
+    return;
+  DCHECK(session_thread_runner_->BelongsToCurrentThread());
+
   UMA_HISTOGRAM_ENUMERATION("Media.Midi.InputPorts", input_ports_.size(),
                             kMaxUmaDevices + 1);
   UMA_HISTOGRAM_ENUMERATION("Media.Midi.OutputPorts", output_ports_.size(),
                             kMaxUmaDevices + 1);
 
-  if (!session_thread_runner_)
-    return;
-  DCHECK(session_thread_runner_->BelongsToCurrentThread());
-
-  DCHECK(!finalized_);
   DCHECK(clients_.empty());
-  DCHECK_EQ(initialization_state_, InitializationState::STARTED);
   initialization_state_ = InitializationState::COMPLETED;
   result_ = result;
 
@@ -300,4 +291,13 @@ void MidiManager::ReceiveMidiData(uint32_t port_index,
     client->ReceiveMidiData(port_index, data, length, timestamp);
 }
 
+size_t MidiManager::GetClientCountForTesting() {
+  base::AutoLock auto_lock(lock_);
+  return clients_.size();
+}
+
+size_t MidiManager::GetPendingClientCountForTesting() {
+  return pending_clients_.size();
+}
+
 }  // namespace midi

media/midi/midi_manager.h

@@ -165,10 +165,8 @@ class MIDI_EXPORT MidiManager {
                        base::TimeTicks time);
 
   // Only for testing.
-  size_t clients_size_for_testing() const { return clients_.size(); }
-  size_t pending_clients_size_for_testing() const {
-    return pending_clients_.size();
-  }
+  size_t GetClientCountForTesting();
+  size_t GetPendingClientCountForTesting();
 
   MidiService* service() { return service_; }
 
@@ -179,34 +177,38 @@ class MIDI_EXPORT MidiManager {
     COMPLETED,
   };
 
-  // Keeps track of all clients who wish to receive MIDI data.
-  // TODO(toyoshim): Enable GUARDED_BY once a testing function is fixed.
-  std::set<MidiManagerClient*> clients_;  // GUARDED_BY(lock_);
-
-  // Keeps track of all clients who are waiting for CompleteStartSession().
-  std::set<MidiManagerClient*> pending_clients_;
-
-  // Keeps a SingleThreadTaskRunner of the thread that calls StartSession in
-  // order to invoke CompleteStartSession() on the thread.
-  scoped_refptr<base::SingleThreadTaskRunner> session_thread_runner_;
+  // Note: Members that are not protected by any lock should be accessed only on
+  // the I/O thread.
 
   // Tracks platform dependent initialization state.
-  InitializationState initialization_state_;
+  InitializationState initialization_state_ = InitializationState::NOT_STARTED;
 
   // Keeps false until Finalize() is called.
-  bool finalized_;
+  bool finalized_ = false;
 
   // Keeps the platform dependent initialization result if initialization is
   // completed. Otherwise keeps mojom::Result::NOT_INITIALIZED.
-  mojom::Result result_;
+  mojom::Result result_ = mojom::Result::NOT_INITIALIZED;
+
+  // Keeps track of all clients who are waiting for CompleteStartSession().
+  std::set<MidiManagerClient*> pending_clients_;
+
+  // Keeps track of all clients who wish to receive MIDI data.
+  std::set<MidiManagerClient*> clients_ GUARDED_BY(lock_);
+
+  // Keeps a SingleThreadTaskRunner of the thread that calls StartSession in
+  // order to invoke CompleteStartSession() on the thread. This is touched only
+  // on the IO thread usually, but to be guarded by |lock_| for thread checks.
+  scoped_refptr<base::SingleThreadTaskRunner> session_thread_runner_
+      GUARDED_BY(lock_);
 
   // Keeps all MidiPortInfo.
   MidiPortInfoList input_ports_ GUARDED_BY(lock_);
   MidiPortInfoList output_ports_ GUARDED_BY(lock_);
 
   // Tracks if actual data transmission happens.
-  bool data_sent_ GUARDED_BY(lock_);
-  bool data_received_ GUARDED_BY(lock_);
+  bool data_sent_ GUARDED_BY(lock_) = false;
+  bool data_received_ GUARDED_BY(lock_) = false;
 
   // Protects members above.
   base::Lock lock_;

media/midi/midi_manager_unittest.cc

@@ -59,13 +59,9 @@ class FakeMidiManager : public MidiManager {
     CompleteInitialization(result);
   }
 
-  size_t GetClientCount() const {
-    return clients_size_for_testing();
-  }
+  size_t GetClientCount() { return GetClientCountForTesting(); }
 
-  size_t GetPendingClientCount() const {
-    return pending_clients_size_for_testing();
-  }
+  size_t GetPendingClientCount() { return GetPendingClientCountForTesting(); }
 
   bool IsInitialized() const { return initialized_; }
 

media/midi/midi_manager_winrt.cc

@@ -850,6 +850,7 @@ void MidiManagerWinrt::SendOnComRunner(uint32_t port_index,
                                        const std::vector<uint8_t>& data) {
   DCHECK(service()->task_service()->IsOnTaskRunner(kComTaskRunner));
 
+  base::AutoLock auto_lock(lazy_init_member_lock_);
   MidiPort<IMidiOutPort>* port = port_manager_out_->GetPortByIndex(port_index);
   if (!(port && port->handle)) {
     VLOG(1) << "Port not available: " << port_index;

media/midi/midi_manager_winrt.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/strings/string16.h"
+#include "base/thread_annotations.h"
 #include "media/midi/midi_manager.h"
 
 namespace midi {
@@ -52,10 +53,10 @@ class MIDI_EXPORT MidiManagerWinrt final : public MidiManager {
   base::Lock lazy_init_member_lock_;
 
   // All operations to Midi{In|Out}PortManager should be done on kComTaskRunner.
-  std::unique_ptr<MidiInPortManager>
-      port_manager_in_;  // GUARDED_BY(lazy_init_member_lock_)
-  std::unique_ptr<MidiOutPortManager>
-      port_manager_out_;  // GUARDED_BY(lazy_init_member_lock_)
+  std::unique_ptr<MidiInPortManager> port_manager_in_
+      GUARDED_BY(lazy_init_member_lock_);
+  std::unique_ptr<MidiOutPortManager> port_manager_out_
+      GUARDED_BY(lazy_init_member_lock_);
 
   // Incremented when a MidiPortManager is ready.
   uint8_t port_manager_ready_count_ = 0;