MediaStreamDispatcher

content/renderer/media/media_stream_dispatcher.h

Index: content/renderer/media/media_stream_dispatcher.h
===================================================================
--- content/renderer/media/media_stream_dispatcher.h	(revision 0)
+++ content/renderer/media/media_stream_dispatcher.h	(revision 0)
@@ -0,0 +1,121 @@
+// Copyright (c) 2011 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 CONTENT_RENDERER_MEDIA_MEDIA_STREAM_DISPATCHER_H_
+#define CONTENT_RENDERER_MEDIA_MEDIA_STREAM_DISPATCHER_H_
+
+#include <list>
+#include <map>
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/message_loop.h"
+#include "content/common/media/media_stream_options.h"
+#include "content/renderer/render_view_observer.h"
+
+// MediaStreamDispatcher is a delegate for the Media Stream API messages.
+// MediaStreams are used by WebKit to open media devices such as Video Capture
+// and Audio input devices.
+// It's the complement of MediaStreamDispatcherHost (owned by
+// BrowserRenderProcessHost).
+class MediaStreamDispatcher : public RenderViewObserver {
+ public:
+  class EventHandler {

[scherkus (not reviewing), 2011/06/17 03:31:59]

as discussed before (not sure if you were on that CL thread or not...) but as
per style guide inner classes are only permitted if they're used internally by
the class

in other words, please split this out to a separate .h file

[Per K, 2011/06/17 15:47:54]

I heard about the problem with VideoCaptureController on VS2005 where the class
both inherited from a class called EventHandler and had a pointer to another
class called Eventhandler.
But does this mean that ex AudioMessageFilter::Delegate is violating the style?
To me, it looks much neater to have them as inner classes.

Anyway - I changed.

+   public:
+    // A new media stream have been created.
+    virtual void OnStreamGenerated(
+        int request_id,
+        const std::string &label,

[Leandro Graciá Gil, 2011/06/16 17:40:01]

& in the wrong side.

[Per K, 2011/06/17 15:47:54]

Done.

+        const media_stream::StreamDeviceInfoArray audio_device_array,

[Leandro Graciá Gil, 2011/06/16 17:40:01]

This should be a reference since you're passing an STL vector.

[Per K, 2011/06/17 15:47:54]

Done.

+        const media_stream::StreamDeviceInfoArray video_device_array) = 0;
+
+    // Creation of a new media stream failed. The user might have denied access
+    // to the requested devices or no device is available.
+    virtual void OnStreamGenerationFailed(int request_id) = 0;
+
+    // An error have occurred on a video device. This is called if a runtime
+    // error occur.

[Leandro Graciá Gil, 2011/06/16 17:40:01]

Minor nit: occurs.

[Per K, 2011/06/17 15:47:54]

Done.

+    virtual void OnVideoDeviceFailed(
+        const std::string &label,
+        int index) = 0;
+
+    // An error have occurred on an audio device. This is called if a runtime
+    // error occur.

[Leandro Graciá Gil, 2011/06/16 17:40:01]

Same as before.

[Per K, 2011/06/17 15:47:54]

Done.

+    virtual void OnAudioDeviceFailed(
+        const std::string &label,
+        int index) = 0;
+
+   protected:
+    virtual ~EventHandler() {}
+  };
+
+  explicit MediaStreamDispatcher(RenderView* render_view);
+  virtual ~MediaStreamDispatcher();
+
+  // Request a new media stream to be created.
+  // This can be used either of WebKit or a plugin.
+  // Note: The event_handler must be valid for as long as the stream exists.
+  void GenerateStream(int request_id,
+                      EventHandler* event_handler,

[Leandro Graciá Gil, 2011/06/16 17:40:01]

What is this EventHandler exactly for and who's providing it? I'm especially
interested in its lifetime and ownership.

[Per K, 2011/06/17 15:47:54]

We wanted to make MediaStreamDispatcher independent of the WebKit types in order
to be able to support for example Pepper if they want to be able to open a video
capture and audio microphone. 

Therefore we split your first implementation in two. There will be a new class
with the following declaration: 
class MediaStreamImpl : public WebKit::WebMediaStreamClient,
                        public media_stream::StreamClient,
                        public MediaStreamDispatcher::EventHandler

It can not be submitted yet since WebMediaStreamClient is still under
development and also the peer connection part of 
WebMediaStreamClient is not implemented. Henrik G is hopefully working on in.

In the case of MediaStreamImpl the lifetime is the same as the renderview to my
understanding.

+                      media_stream::GenerateStreamOptions components,
+                      const std::string& security_origin);
+
+  // Stop a started stream. Label is the label provided in OnStreamGenerated.
+  void StopStream(const std::string& label);
+
+  // Returns a StreamDeviceInfoArray given a label. This can be used for
+  // verifying if a label is an opened media stream.
+  const media_stream::StreamDeviceInfoArray* audio_device_array(

[scherkus (not reviewing), 2011/06/17 03:31:59]

const ptr to an array?

if the point is to verify if a label is open why not provide that method
directly?

IsAudioDeviceOpen(const std::string& label)

etc

[Per K, 2011/06/17 15:47:54]

Changed to reference.

+      const std::string& label);
+  const media_stream::StreamDeviceInfoArray* video_device_array(
+    const std::string& label);

[scherkus (not reviewing), 2011/06/17 03:31:59]

nit: indentation

[Per K, 2011/06/17 15:47:54]

Done.

+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(MediaStreamDispatcherTest, Basic);
+  FRIEND_TEST_ALL_PREFIXES(MediaStreamDispatcherTest, TestFailure);
+
+  typedef int IPC_REQUEST_ID;

[scherkus (not reviewing), 2011/06/17 03:31:59]

nit: why is this type name ALL_CAPS?

we use CamelCase for types

[Per K, 2011/06/17 15:47:54]

Done.

+  struct Request {

[scherkus (not reviewing), 2011/06/17 03:31:59]

this is an example where inner classes is ok :)

[Per K, 2011/06/17 15:47:54]

Done.

+    Request(EventHandler* handler, int request_id, IPC_REQUEST_ID ipc_request)
+        : handler(handler),
+          request_id(request_id),
+          ipc_request(ipc_request) {
+    }
+    EventHandler* handler;
+    int request_id;
+    int ipc_request;
+  };
+
+  // Private class for keeping track of opened devices and who have
+  // opened it.
+  struct Stream {
+    EventHandler* handler;
+    media_stream::StreamDeviceInfoArray audio_array;
+    media_stream::StreamDeviceInfoArray video_array;
+  };
+
+  // Messages from the browser.
+  virtual bool OnMessageReceived(const IPC::Message& message);
+  void OnStreamGenerated(
+      int request_id,
+      const std::string &label,

[Leandro Graciá Gil, 2011/06/16 17:40:01]

This should be const std::string& label  (the & on the other side).

[Per K, 2011/06/17 15:47:54]

Done.

+      const media_stream::StreamDeviceInfoArray audio_array,
+      const media_stream::StreamDeviceInfoArray video_array);
+  void OnStreamGenerationFailed(int requestId);
+  void OnVideoDeviceFailed(std::string label, int index);
+  void OnAudioDeviceFailed(std::string label, int index);
+
+  IPC_REQUEST_ID next_ipc_id_;
+  typedef std::map<std::string, Stream> LabelStreamMap;
+  LabelStreamMap label_stream_map_;
+
+  // List of calls made to GenerateStream that has not yet completed.
+  typedef std::list <Request> RequestList;

[Leandro Graciá Gil, 2011/06/16 17:40:01]

Not sure, but I think this has an extra space.

[scherkus (not reviewing), 2011/06/17 03:31:59]

yes it does :)

[Per K, 2011/06/17 15:47:54]

Done.

+  RequestList requests_;
+  MessageLoop* main_message_loop_;

[scherkus (not reviewing), 2011/06/17 03:31:59]

nit: s/main_message_loop_/message_loop_/

(no need to say it's the "main" one)

[Per K, 2011/06/17 15:47:54]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(MediaStreamDispatcher);
+};
+
+#endif  // CONTENT_RENDERER_MEDIA_MEDIA_STREAM_DISPATCHER_H_