MediaStreamDispatcher

content/renderer/media/media_stream_dispatcher.cc

+// 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.
+
+#include "content/renderer/media/media_stream_dispatcher.h"
+
+#include "base/logging.h"
+#include "content/common/media_stream_messages.h"
+
+MediaStreamDispatcher::MediaStreamDispatcher(RenderView* render_view)
+    : RenderViewObserver(render_view),
+      next_ipc_id_(0),
+      main_message_loop_(NULL) {

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

what about putting MessageLoop::current() in here

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

Done.

+  main_message_loop_ = MessageLoop::current();
+}
+
+MediaStreamDispatcher::~MediaStreamDispatcher() {}
+
+void MediaStreamDispatcher::GenerateStream(
+    int request_id,
+    EventHandler* event_handler,
+    media_stream::GenerateStreamOptions components,
+    const std::string& security_origin) {
+  DCHECK(MessageLoop::current()== main_message_loop_);

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

DCHECK_EQ

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

Done.

+  VLOG(1) << "MediaStreamDispatcher::GenerateStream" << request_id << ")";
+
+  requests_.push_back(Request(event_handler, request_id, next_ipc_id_));
+  Send(new MediaStreamHostMsg_GenerateStream(
+      routing_id(),

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

indentation

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

Done.

+      next_ipc_id_++,
+      components,
+      security_origin));
+}
+
+void MediaStreamDispatcher::StopStream(const std::string &label) {
+  DCHECK(MessageLoop::current()== main_message_loop_);

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

DCHECK_EQ

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

Done.

+  VLOG(1) << "MediaStreamDispatcher::StopStream"
+          << ", {label = " << label;
+
+  LabelStreamMap::iterator it = label_stream_map_.find(label);
+  if (it == label_stream_map_.end())
+    return;
+
+  Send(new MediaStreamHostMsg_StopGeneratedStream(
+      routing_id(),

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

nit: indentation

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

Done.

+      label));
+
+  label_stream_map_.erase(it);
+}
+
+bool MediaStreamDispatcher::OnMessageReceived(const IPC::Message& message) {
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(MediaStreamDispatcher, message)
+    IPC_MESSAGE_HANDLER(MediaStreamMsg_StreamGenerated,
+                        OnStreamGenerated)
+    IPC_MESSAGE_HANDLER(MediaStreamMsg_StreamGenerationFailed,
+                        OnStreamGenerationFailed)
+    IPC_MESSAGE_HANDLER(MediaStreamHostMsg_VideoDeviceFailed,
+                        OnVideoDeviceFailed)
+    IPC_MESSAGE_HANDLER(MediaStreamHostMsg_AudioDeviceFailed,
+                        OnAudioDeviceFailed)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+}
+
+void MediaStreamDispatcher::OnStreamGenerated(
+    int request_id,
+    const std::string &label,

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

& at the wrong side.

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

Done.

+    const media_stream::StreamDeviceInfoArray audio_array,

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

This is an STL vector. Is there any reason not to pass it by reference? Why is
it const if it's a copy?

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

Nope. My mistake.

+    const media_stream::StreamDeviceInfoArray video_array) {
+
+  for (RequestList::iterator it = requests_.begin();
+      it != requests_.end(); ++it) {

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

indentation

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

Done.

+    Request& request =  *it;

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

nit: get rid of extra space

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

Done.

+    if (request.ipc_request == request_id) {
+      Stream new_stream;
+      new_stream.handler = request.handler;
+      new_stream.audio_array = audio_array;
+      new_stream.video_array = video_array;
+      label_stream_map_[label] = new_stream;
+      request.handler->OnStreamGenerated(request.request_id, label,
+                                         audio_array, video_array);
+      VLOG(1) << "MediaStreamDispatcher::OnStreamGenerated("
+              << request.request_id << ", " << label << ")";
+      requests_.erase(it);
+      break;
+    }
+  }
+}
+
+void MediaStreamDispatcher::OnStreamGenerationFailed(int request_id) {
+  for (RequestList::iterator it = requests_.begin();
+      it != requests_.end(); ++it) {

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

indentation

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

Done.

+    Request& request =  *it;

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

nit: get rid of extra space

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

Done.

+    if (request.ipc_request == request_id) {
+      request.handler->OnStreamGenerationFailed(request.request_id);
+      VLOG(1) << "MediaStreamDispatcher::OnStreamGenerationFailed("
+              << request.request_id << ")\n";
+      requests_.erase(it);
+      break;
+    }
+  }
+}
+
+void MediaStreamDispatcher::OnVideoDeviceFailed(std::string label, int index) {

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

Are you sure this can't be const std::string& ?
Same for the other methods.

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

Done.

+  LabelStreamMap::iterator it = label_stream_map_.find(label);
+  if (it == label_stream_map_.end())
+    return;
+
+  media_stream::StreamDeviceInfoArray::iterator device_it =
+      it->second.video_array.begin();
+  it->second.video_array.erase(device_it + index);
+  it->second.handler->OnVideoDeviceFailed(label, index);
+}
+
+void MediaStreamDispatcher::OnAudioDeviceFailed(std::string label, int index) {
+  LabelStreamMap::iterator it = label_stream_map_.find(label);
+  if (it == label_stream_map_.end())
+    return;
+
+  media_stream::StreamDeviceInfoArray::iterator device_it =

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

nit: drop = to next line

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

http://www.chromium.org/developers/coding-style
bool result =
    YetAnotherLongFunctionCall(first_argument, ???

+      it->second.audio_array.begin();
+  it->second.audio_array.erase(device_it + index);

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

I think a few DCHECKs here would be good. If for some reason the message
ordering fails or something strange happens this can do weird things.

Is there anything else you're assuming when doing this?

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

Done.

+  it->second.handler->OnAudioDeviceFailed(label, index);
+}
+
+const media_stream::StreamDeviceInfoArray*
+MediaStreamDispatcher::audio_device_array(
+    const std::string& label) {

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

nit: this can fit on one line

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

Done.

+  DCHECK(MessageLoop::current()== main_message_loop_);

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

DCHECK_EQ

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

Done.

+  if (label.empty())
+    return NULL;
+
+  if (label_stream_map_.find(label) == label_stream_map_.end())
+    return NULL;
+
+  return &label_stream_map_[label].audio_array;
+}
+
+const media_stream::StreamDeviceInfoArray*
+MediaStreamDispatcher::video_device_array(
+    const std::string& label) {

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

nit: this can fit on one line

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

Done.

+  DCHECK(MessageLoop::current()== main_message_loop_);

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

DCHECK_EQ

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

Done.

+  if (label.empty())
+    return NULL;
+
+  if (label_stream_map_.find(label) == label_stream_map_.end())
+    return NULL;
+
+  return &label_stream_map_[label].video_array;

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

I don't think this is safe at all. Maybe I'm wrong, but AFAIK STL maps can
change the address where they store their contents and re-organize themselves.

Also, you're returning a pointer to an STL vector. Why not return always a const
reference and have a local static empty vector for the cases where you return
NULL? I'm not sure if that's usual in Chromium but I've seen it many times in
WebKit. It should make your test code cleaner too.

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

Done.

+}