Web MIDI: add an unit test to check MidiManager instantiation

media/midi/midi_manager.h

 // Copyright (c) 2013 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.
 
 #ifndef MEDIA_MIDI_MIDI_MANAGER_H_
 #define MEDIA_MIDI_MIDI_MANAGER_H_
 
 #include <map>
 #include <set>
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/memory/ref_counted.h"
 #include "base/synchronization/lock.h"
 #include "base/time/time.h"
 #include "media/base/media_export.h"
 #include "media/midi/midi_port_info.h"
 #include "media/midi/midi_result.h"
 
+namespace base {
+class SingleThreadTaskRunner;
+}  // namespace base
+
 namespace media {
 
 // A MidiManagerClient registers with the MidiManager to receive MIDI data.
 // See MidiManager::RequestAccess() and MidiManager::ReleaseAccess()
 // for details.
 class MEDIA_EXPORT MidiManagerClient {
  public:
   virtual ~MidiManagerClient() {}
 
   // CompleteStartSession() is called when platform dependent preparation is
@@ skipping 14 matching lines @@
   // AccumulateMidiBytesSent() is called to acknowledge when bytes have
   // successfully been sent to the hardware.
   // This happens as a result of the client having previously called
   // MidiManager::DispatchSendMidiData().
   virtual void AccumulateMidiBytesSent(size_t n) = 0;
 };
 
 // Manages access to all MIDI hardware.
 class MEDIA_EXPORT MidiManager {
  public:
+  // The constructor and the destructor will be called on the CrBrowserMain
+  // thread.
   static MidiManager* Create();
 
   MidiManager();
   virtual ~MidiManager();
 
   // A client calls StartSession() to receive and send MIDI data.
   // If the session is ready to start, the MIDI system is lazily initialized
   // and the client is registered to receive MIDI data.
   // CompleteStartSession() is called with MIDI_OK if the session is started.
   // Otherwise CompleteStartSession() is called with proper MidiResult code.
+  // StartSession() and EndSession() can be called on the Chrome_IOThread.
+  // CompleteStartSession() will be invoked on the same Chrome_IOThread.
   void StartSession(MidiManagerClient* client, int client_id);
 
   // A client calls EndSession() to stop receiving MIDI data.
   void EndSession(MidiManagerClient* client);
 
   // DispatchSendMidiData() is called when MIDI data should be sent to the MIDI
   // system.
   // This method is supposed to return immediately and should not block.
   // |port_index| represents the specific output port from output_ports().
   // |data| represents a series of bytes encoding one or more MIDI messages.
@@ skipping 12 matching lines @@
   const MidiPortInfoList& input_ports() { return input_ports_; }
 
   // output_ports() is a list of MIDI ports for sending MIDI data.
   // Each individual port in this list can be identified by its
   // integer index into this list.
   const MidiPortInfoList& output_ports() { return output_ports_; }
 
  protected:
   friend class MidiManagerUsb;
 
-  // Initializes the platform dependent MIDI system. It will call
-  // CompleteInitialization() asynchronously when initialization is finished.
+  // Initializes the platform dependent MIDI system. It is called on
+  // Chrome_IOThread, but will invoke CompleteInitialization() asynchronously
+  // on the same thread or another thread when initialization is finished.
   // |result| of CompleteInitialization() will be MIDI_OK on success.
-  // MidiManager has a default implementation that calls
-  // CompleteInitialization() with MIDI_NOT_SUPPORTED.
+  // MidiManager has a default implementation that synchronously calls
+  // CompleteInitialization() with MIDI_NOT_SUPPORTED on the caller thread.
   virtual void StartInitialization();

[yukawa, 2014/05/05 04:07:36]

It would be nice if the comment here is written particularly for developers who
are implementing their derived class of MidiManager for their platforms, rather
than for general audience.

We might want to cover following topics.
- This is an extension point for each platform-dependent class derived from
MidiManager class.
- Guaranteed to be called on Chrome_IOThread when ....
- Developers who want to port MidiManager for their platform must do ...

What do you think?

[Takashi Toyoshima, 2014/05/06 01:58:18]

Done.

 
   // Called from a platform dependent implementation of StartInitialization().
-  // It will distribute |result| to MIDIManagerClient objects in
+  // It invokes CompleteInitializationInternal() on the thread that calls
+  // StartSession() and distributes |result| to MIDIManagerClient objects in
   // |pending_clients_|.
   void CompleteInitialization(MidiResult result);
 
   void AddInputPort(const MidiPortInfo& info);
   void AddOutputPort(const MidiPortInfo& info);
 
   // Dispatches to all clients.
   // TODO(toyoshim): Fix the mac implementation to use
   // |ReceiveMidiData(..., base::TimeTicks)|.
   void ReceiveMidiData(uint32 port_index,
                        const uint8* data,
                        size_t length,
                        double timestamp);
 
   void ReceiveMidiData(uint32 port_index,
                        const uint8* data,
                        size_t length,
                        base::TimeTicks time) {
     ReceiveMidiData(port_index, data, length,
                     (time - base::TimeTicks()).InSecondsF());
   }
 
-  bool initialized_;
-  MidiResult result_;
-
+  // Following |clients_| and |pending_clients_| are protected only for testing.
+  // Do not access to them for other purposes.

[yukawa, 2014/05/05 04:07:36]

Thank you for the comment.
Another possible options are
1. introducing some getter methods like
GetClientsForTesting()/GetPendingClientsForTesting().
2. FRIEND_TEST_ALL_PREFIXES or similar approach
What do you think?

[Takashi Toyoshima, 2014/05/06 01:58:18]

Currently, we adopt anonymous namespace for all unit tests. So,
FRIEND_TEST_ALL_PREFIXES like approach does not work. I feel it makes testing
difficult like this, but I'm not sure the pros and cons. Let's discuss it later
with yhirano@.

For this case, 1. sounds better. Maybe having GetClientSizeForTesting() and
GetPendingClientSizeForTesting() is enough.

   // Keeps track of all clients who wish to receive MIDI data.
   typedef std::set<MidiManagerClient*> ClientList;
   ClientList clients_;
 
   // Keeps track of all clients who are waiting for CompleteStartSession().
   typedef std::map<int, MidiManagerClient*> PendingClientMap;
   PendingClientMap pending_clients_;
 
-  // Protects access to our clients, |clients_| and |pending_clients_|.
-  base::Lock clients_lock_;
+ private:
+  void CompleteInitializationInternal(MidiResult result);
+
+  // Keeps a SingleThreadTaskRunner of the thread that calls StartSession in
+  // order to invoke CompleteStartSession() on the thread.
+  scoped_refptr<base::SingleThreadTaskRunner> session_thread_runner_;
+
+  // Keeps true if platform dependent initialization is already completed.
+  bool initialized_;
+
+  // Keeps the platform dependent initialization result if initialization is
+  // completed. Otherwise keeps MIDI_NOT_SUPPORTED.
+  MidiResult result_;
+
+  // Protects access to |clients_|, |pending_clients_|, |initialized_|, and
+  // |result_|.

[Takashi Toyoshima, 2014/05/05 00:49:02]

Sorry, this comment was incomplete.

+  base::Lock lock_;
 
   MidiPortInfoList input_ports_;
   MidiPortInfoList output_ports_;
 
- private:
   DISALLOW_COPY_AND_ASSIGN(MidiManager);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_MIDI_MIDI_MANAGER_H_