Web MIDI: add an unit test to check MidiManager instantiation

media/midi/midi_manager.h

Index: media/midi/midi_manager.h
diff --git a/media/midi/midi_manager.h b/media/midi/midi_manager.h
index 156a2465cb424bb943a271fdd4b7f99c53a669f5..ec6fe3eea1da700455a6bcbdb06696f264b25c2e 100644
--- a/media/midi/midi_manager.h
+++ b/media/midi/midi_manager.h
@@ -10,12 +10,17 @@
 #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.
@@ -50,6 +55,8 @@ class MEDIA_EXPORT MidiManagerClient {
 // 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();
@@ -60,6 +67,8 @@ class MEDIA_EXPORT MidiManager {
   // 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.
@@ -92,15 +101,17 @@ class MEDIA_EXPORT MidiManager {
  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);
 
@@ -123,9 +134,8 @@ class MEDIA_EXPORT MidiManager {
                     (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_;
@@ -134,13 +144,27 @@ class MEDIA_EXPORT MidiManager {
   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);
 };