for readability review.

content/renderer/media/video_capture_impl.cc

 // Copyright (c) 2012 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/video_capture_impl.h"
 
 #include "base/bind.h"
 #include "base/stl_util.h"
 #include "content/common/child_process.h"
 #include "content/common/media/video_capture_messages.h"
@@ skipping 43 matching lines @@
       video_type_(media::VideoCaptureCapability::kI420),
       device_info_available_(false),
       state_(video_capture::kStopped) {
   DCHECK(filter);
   memset(&current_params_, 0, sizeof(current_params_));
   memset(&device_info_, 0, sizeof(device_info_));
   current_params_.session_id = id;
 }
 
 VideoCaptureImpl::~VideoCaptureImpl() {
-  STLDeleteContainerPairSecondPointers(cached_dibs_.begin(),
-                                       cached_dibs_.end());
+  STLDeleteValues(&cached_dibs_);
 }
 
 void VideoCaptureImpl::Init() {
   io_message_loop_proxy_ = ChildProcess::current()->io_message_loop_proxy();
   if (!io_message_loop_proxy_->BelongsToCurrentThread()) {
     io_message_loop_proxy_->PostTask(FROM_HERE,
         base::Bind(&VideoCaptureImpl::AddDelegateOnIOThread,
                    base::Unretained(this)));
-    return;
+  } else {
+    AddDelegateOnIOThread();
   }
-
-  AddDelegateOnIOThread();
 }
 
 void VideoCaptureImpl::DeInit(base::Closure task) {
   capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoDeInit,
-                 base::Unretained(this), task));
-}
-
-void VideoCaptureImpl::DoDeInit(base::Closure task) {
-  if (state_ == video_capture::kStarted)
-    Send(new VideoCaptureHostMsg_Stop(device_id_));
-
-  io_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::RemoveDelegateOnIOThread,
+      base::Bind(&VideoCaptureImpl::DoDeInitOnCaptureThread,
                  base::Unretained(this), task));
 }
 
 void VideoCaptureImpl::StartCapture(
     media::VideoCapture::EventHandler* handler,
     const media::VideoCaptureCapability& capability) {
   DCHECK_EQ(capability.color, media::VideoCaptureCapability::kI420);
 
   capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoStartCapture,
+      base::Bind(&VideoCaptureImpl::DoStartCaptureOnCaptureThread,
                  base::Unretained(this), handler, capability));
 }
 
 void VideoCaptureImpl::StopCapture(media::VideoCapture::EventHandler* handler) {
   capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoStopCapture,
+      base::Bind(&VideoCaptureImpl::DoStopCaptureOnCaptureThread,
                  base::Unretained(this), handler));
 }
 
 void VideoCaptureImpl::FeedBuffer(scoped_refptr<VideoFrameBuffer> buffer) {
   capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoFeedBuffer,
+      base::Bind(&VideoCaptureImpl::DoFeedBufferOnCaptureThread,
                  base::Unretained(this), buffer));
 }
 
 void VideoCaptureImpl::OnBufferCreated(
     base::SharedMemoryHandle handle,
     int length, int buffer_id) {
   capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoBufferCreated,
+      base::Bind(&VideoCaptureImpl::DoBufferCreatedOnCaptureThread,
                  base::Unretained(this), handle, length, buffer_id));
 }
 
 void VideoCaptureImpl::OnBufferReceived(int buffer_id, base::Time timestamp) {
   capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoBufferReceived,
+      base::Bind(&VideoCaptureImpl::DoBufferReceivedOnCaptureThread,
                  base::Unretained(this), buffer_id, timestamp));
 }
 
 void VideoCaptureImpl::OnStateChanged(video_capture::State state) {
   capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoStateChanged,
+      base::Bind(&VideoCaptureImpl::DoStateChangedOnCaptureThread,
                  base::Unretained(this), state));
 }
 
 void VideoCaptureImpl::OnDeviceInfoReceived(
     const media::VideoCaptureParams& device_info) {
   capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoDeviceInfoReceived,
+      base::Bind(&VideoCaptureImpl::DoDeviceInfoReceivedOnCaptureThread,
                  base::Unretained(this), device_info));
 }
 
 void VideoCaptureImpl::OnDelegateAdded(int32 device_id) {
   capture_message_loop_proxy_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureImpl::DoDelegateAdded,
+      base::Bind(&VideoCaptureImpl::DoDelegateAddedOnCaptureThread,
                  base::Unretained(this), device_id));
 }
 
-void VideoCaptureImpl::DoStartCapture(
+void VideoCaptureImpl::DoDeInitOnCaptureThread(base::Closure task) {
+  if (state_ == video_capture::kStarted)
+    Send(new VideoCaptureHostMsg_Stop(device_id_));
+
+  io_message_loop_proxy_->PostTask(FROM_HERE,
+      base::Bind(&VideoCaptureImpl::RemoveDelegateOnIOThread,
+                 base::Unretained(this), task));
+}
+
+void VideoCaptureImpl::DoStartCaptureOnCaptureThread(
     media::VideoCapture::EventHandler* handler,
     const media::VideoCaptureCapability& capability) {
   DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
 
   if (state_ == video_capture::kError) {
     handler->OnError(this, 1);
     handler->OnRemoved(this);
-    return;
+  } else if ((clients_pending_on_filter_.find(handler) !=

[zunger, 2012/07/10 23:34:30]

Oh, wow. This function is a *lot* clearer now.

+              clients_pending_on_filter_.end()) ||
+             (clients_pending_on_restart_.find(handler) !=
+              clients_pending_on_restart_.end()) ||
+             clients_.find(handler) != clients_.end() ) {
+    // This client has started.
+  } else if (!device_id_) {
+    clients_pending_on_filter_[handler] = capability;
+  } else {
+    handler->OnStarted(this);
+    if (state_ == video_capture::kStarted) {
+      if (capability.width > current_params_.width ||
+          capability.height > current_params_.height) {
+        StopDevice();
+        DVLOG(1) << "StartCapture: Got client with higher resolution ("
+                 << capability.width << ", " << capability.height << ") "
+                 << "after started, try to restart.";
+        clients_pending_on_restart_[handler] = capability;
+      } else {
+        if (device_info_available_) {
+          handler->OnDeviceInfoReceived(this, device_info_);
+        }
+
+        clients_[handler] = capability;
+      }
+    } else if (state_ == video_capture::kStopping) {
+      clients_pending_on_restart_[handler] = capability;
+      DVLOG(1) << "StartCapture: Got new resolution ("
+               << capability.width << ", " << capability.height << ") "
+               << ", during stopping.";
+    } else {
+      clients_[handler] = capability;
+      DCHECK_EQ(clients_.size(), 1ul);

[zunger, 2012/07/10 23:34:30]

Expected value has to be on the left or the error messages won't come out right.
Also, what's with the 1ul business? Is '1' not compiling properly?

[wjia(left Chromium), 2012/07/12 22:57:30]

I checked how DCHECK_EQ is defined in Chromium and couldn't find the difference
of putting expected value on the left or right. Anyhow, the expected value has
been moved to left.

The default type for "1" is int, while size() is uint. Therefore "1ul" is used.

+      video_type_ = capability.color;
+      current_params_.width = capability.width;
+      current_params_.height = capability.height;
+      current_params_.frame_per_second = capability.frame_rate;
+      DVLOG(1) << "StartCapture: starting with first resolution ("
+               << current_params_.width << "," << current_params_.height << ")";
+
+      StartCaptureInternal();
+    }
   }
-
-  ClientInfo::iterator it1 = clients_pending_on_filter_.find(handler);
-  ClientInfo::iterator it2 = clients_pending_on_restart_.find(handler);
-  if (it1 != clients_pending_on_filter_.end() ||
-      it2 != clients_pending_on_restart_.end() ||
-      clients_.find(handler) != clients_.end() ) {
-    // This client has started.
-    return;
-  }
-
-  if (!device_id_) {
-    clients_pending_on_filter_[handler] = capability;
-    return;
-  }
-
-  handler->OnStarted(this);
-  if (state_ == video_capture::kStarted) {
-    if (capability.width > current_params_.width ||
-        capability.height > current_params_.height) {
-      StopDevice();
-      DVLOG(1) << "StartCapture: Got client with higher resolution ("
-               << capability.width << ", " << capability.height << ") "
-               << "after started, try to restart.";
-      clients_pending_on_restart_[handler] = capability;
-      return;
-    }
-
-    if (device_info_available_) {
-      handler->OnDeviceInfoReceived(this, device_info_);
-    }
-
-    clients_[handler] = capability;
-    return;
-  }
-
-  if (state_ == video_capture::kStopping) {
-    clients_pending_on_restart_[handler] = capability;
-    DVLOG(1) << "StartCapture: Got new resolution ("
-             << capability.width << ", " << capability.height << ") "
-             << ", during stopping.";
-    return;
-  }
-
-  clients_[handler] = capability;
-  DCHECK_EQ(clients_.size(), 1ul);
-  video_type_ = capability.color;
-  current_params_.width = capability.width;
-  current_params_.height = capability.height;
-  current_params_.frame_per_second = capability.frame_rate;
-  DVLOG(1) << "StartCapture: starting with first resolution ("
-           << current_params_.width << ", " << current_params_.height << ")";
-
-  StartCaptureInternal();
 }
 
-void VideoCaptureImpl::DoStopCapture(
+void VideoCaptureImpl::DoStopCaptureOnCaptureThread(
     media::VideoCapture::EventHandler* handler) {
   DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
 

[zunger, 2012/07/10 23:34:30]

You may want to comment why this is an OR and we're short-circuiting.

[wjia(left Chromium), 2012/07/12 22:57:30]

I am not sure if I understand the comment. This function should be run on
capture thread. This DCHECK is a safety checking for that.

-  ClientInfo::iterator it = clients_pending_on_filter_.find(handler);
-  if (it != clients_pending_on_filter_.end()) {
-    handler->OnStopped(this);
-    handler->OnRemoved(this);
-    clients_pending_on_filter_.erase(it);
-    return;
-  }
-  it = clients_pending_on_restart_.find(handler);
-  if (it != clients_pending_on_restart_.end()) {
-    handler->OnStopped(this);
-    handler->OnRemoved(this);
-    clients_pending_on_filter_.erase(it);
-    return;
-  }
-
-  if (clients_.find(handler) == clients_.end())
-    return;
-
-  clients_.erase(handler);
+  RemoveClient(handler, clients_pending_on_filter_) ||
+  RemoveClient(handler, clients_pending_on_restart_) ||
+  RemoveClient(handler, clients_);
 
   if (clients_.empty()) {
     DVLOG(1) << "StopCapture: No more client, stopping ...";
     StopDevice();
   }
-  handler->OnStopped(this);
-  handler->OnRemoved(this);
 }
 
-void VideoCaptureImpl::DoFeedBuffer(scoped_refptr<VideoFrameBuffer> buffer) {
+void VideoCaptureImpl::DoFeedBufferOnCaptureThread(
+    scoped_refptr<VideoFrameBuffer> buffer) {
   DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
 
   CachedDIB::iterator it;
-  for (it = cached_dibs_.begin(); it != cached_dibs_.end(); it++) {
+  for (it = cached_dibs_.begin(); it != cached_dibs_.end(); ++it) {
     if (buffer == it->second->mapped_memory)
       break;
   }
 
-  DCHECK(it != cached_dibs_.end());
-  DCHECK_GT(it->second->references, 0);
-  it->second->references--;
-  if (it->second->references == 0) {
-    Send(new VideoCaptureHostMsg_BufferReady(device_id_, it->first));
+  if (it != cached_dibs_.end() && it->second) {
+    DCHECK_GT(it->second->references, 0);
+    --it->second->references;
+    if (it->second->references == 0) {
+      Send(new VideoCaptureHostMsg_BufferReady(device_id_, it->first));
+    }
   }
 }
 
-void VideoCaptureImpl::DoBufferCreated(
+void VideoCaptureImpl::DoBufferCreatedOnCaptureThread(
     base::SharedMemoryHandle handle,
     int length, int buffer_id) {
   DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
   DCHECK(device_info_available_);
 
   media::VideoCapture::VideoFrameBuffer* buffer;
   DCHECK(cached_dibs_.find(buffer_id) == cached_dibs_.end());
 
   base::SharedMemory* dib = new base::SharedMemory(handle, false);
   dib->Map(length);
   buffer = new VideoFrameBuffer();
   buffer->memory_pointer = static_cast<uint8*>(dib->memory());
   buffer->buffer_size = length;
   buffer->width = device_info_.width;
   buffer->height = device_info_.height;
   buffer->stride = device_info_.width;
 
   DIBBuffer* dib_buffer = new DIBBuffer(dib, buffer);
   cached_dibs_[buffer_id] = dib_buffer;
 }
 
-void VideoCaptureImpl::DoBufferReceived(int buffer_id, base::Time timestamp) {
+void VideoCaptureImpl::DoBufferReceivedOnCaptureThread(
+    int buffer_id, base::Time timestamp) {
   DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
 
   if (state_ != video_capture::kStarted) {
     Send(new VideoCaptureHostMsg_BufferReady(device_id_, buffer_id));
     return;
   }
 
   media::VideoCapture::VideoFrameBuffer* buffer;
   DCHECK(cached_dibs_.find(buffer_id) != cached_dibs_.end());
   buffer = cached_dibs_[buffer_id]->mapped_memory;
   buffer->timestamp = timestamp;
 
-  for (ClientInfo::iterator it = clients_.begin(); it != clients_.end(); it++) {
+  for (ClientInfo::iterator it = clients_.begin(); it != clients_.end(); ++it) {
     it->first->OnBufferReady(this, buffer);
   }
   cached_dibs_[buffer_id]->references = clients_.size();
 }
 
-void VideoCaptureImpl::DoStateChanged(video_capture::State state) {
+void VideoCaptureImpl::DoStateChangedOnCaptureThread(
+    video_capture::State state) {
   DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
 
   switch (state) {
     case video_capture::kStarted:
       break;
     case video_capture::kStopped:
       state_ = video_capture::kStopped;
       DVLOG(1) << "OnStateChanged: stopped!, device_id = " << device_id_;
       STLDeleteValues(&cached_dibs_);
       if (!clients_.empty() || !clients_pending_on_restart_.empty())
         RestartCapture();
       break;
     case video_capture::kPaused:
       for (ClientInfo::iterator it = clients_.begin();
-           it != clients_.end(); it++) {
+           it != clients_.end(); ++it) {
         it->first->OnPaused(this);
       }
       break;
     case video_capture::kError:
       DVLOG(1) << "OnStateChanged: error!, device_id = " << device_id_;
       for (ClientInfo::iterator it = clients_.begin();
-           it != clients_.end(); it++) {
+           it != clients_.end(); ++it) {
         // TODO(wjia): browser process would send error code.
         it->first->OnError(this, 1);
         it->first->OnRemoved(this);
       }
       clients_.clear();
       state_ = video_capture::kError;
       break;
     default:
       break;
   }
 }
 
-void VideoCaptureImpl::DoDeviceInfoReceived(
+void VideoCaptureImpl::DoDeviceInfoReceivedOnCaptureThread(
     const media::VideoCaptureParams& device_info) {
   DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
   DCHECK(!ClientHasDIB());
 
   STLDeleteValues(&cached_dibs_);
 
   device_info_ = device_info;
   device_info_available_ = true;
-  for (ClientInfo::iterator it = clients_.begin(); it != clients_.end(); it++) {
+  for (ClientInfo::iterator it = clients_.begin(); it != clients_.end(); ++it) {
     it->first->OnDeviceInfoReceived(this, device_info);
   }
 }
 
-void VideoCaptureImpl::DoDelegateAdded(int32 device_id) {
+void VideoCaptureImpl::DoDelegateAddedOnCaptureThread(int32 device_id) {
   DVLOG(1) << "DoDelegateAdded: device_id " << device_id;
   DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
 
   device_id_ = device_id;
   for (ClientInfo::iterator it = clients_pending_on_filter_.begin();
        it != clients_pending_on_filter_.end(); ) {
     media::VideoCapture::EventHandler* handler = it->first;
     const media::VideoCaptureCapability capability = it->second;
     clients_pending_on_filter_.erase(it++);
     StartCapture(handler, capability);
@@ skipping 11 matching lines @@
   }
 }
 
 void VideoCaptureImpl::RestartCapture() {
   DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
   DCHECK_EQ(state_, video_capture::kStopped);
 
   int width = 0;
   int height = 0;
   for (ClientInfo::iterator it = clients_.begin();
-       it != clients_.end(); it++) {
-    if (it->second.width > width)
-      width = it->second.width;
-    if (it->second.height > height)
-      height = it->second.height;
+       it != clients_.end(); ++it) {
+    width = std::max(width, it->second.width);
+    height = std::max(height, it->second.height);
   }
   for (ClientInfo::iterator it = clients_pending_on_restart_.begin();
        it != clients_pending_on_restart_.end(); ) {
-    if (it->second.width > width)
-      width = it->second.width;
-    if (it->second.height > height)
-      height = it->second.height;
+    width = std::max(width, it->second.width);
+    height = std::max(height, it->second.height);
     clients_[it->first] = it->second;
     clients_pending_on_restart_.erase(it++);
   }
   current_params_.width = width;
   current_params_.height = height;
   DVLOG(1) << "RestartCapture, " << current_params_.width << ", "
            << current_params_.height;
   StartCaptureInternal();
 }
 
@@ skipping 16 matching lines @@
   capture_message_loop_proxy_->PostTask(FROM_HERE, task);
 }
 
 void VideoCaptureImpl::Send(IPC::Message* message) {
   io_message_loop_proxy_->PostTask(FROM_HERE,
       base::Bind(base::IgnoreResult(&VideoCaptureMessageFilter::Send),
                  message_filter_.get(), message));
 }
 
 bool VideoCaptureImpl::ClientHasDIB() {
+  DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
   CachedDIB::iterator it;
-  for (it = cached_dibs_.begin(); it != cached_dibs_.end(); it++) {
+  for (it = cached_dibs_.begin(); it != cached_dibs_.end(); ++it) {

[zunger, 2012/07/10 23:34:30]

for (CachedDIB::iterator it ...)

or even

for (const auto& it : cached_dibs_) {
  if (it->second->references > 0) return true;
}

Keep variable scopes as tight as possible.

[wjia(left Chromium), 2012/07/12 22:57:30]

Done. Can not use auto since the compiler doesn't support it.

     if (it->second->references > 0)
       return true;
   }
   return false;
 }
+
+bool VideoCaptureImpl::RemoveClient(
+    media::VideoCapture::EventHandler* handler,
+    ClientInfo& clients) {

[zunger, 2012/07/10 23:34:30]

No non-const references! Use a pointer.

[zunger, 2012/07/10 23:34:30]

This function can be const. (Make every function const unless there's an amazing
reason for it not to be)

[wjia(left Chromium), 2012/07/12 22:57:30]

Done.

[wjia(left Chromium), 2012/07/12 22:57:30]

This function RemoveClient() can't be const since handler->OnStopped() needs
non-const pointer. I made ClientHasDIB() const.

+  DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
+  bool found = false;
+
+  ClientInfo::iterator it = clients.find(handler);
+  if (it != clients.end()) {
+    handler->OnStopped(this);
+    handler->OnRemoved(this);
+    clients.erase(it);
+    found = true;
+  }
+  return found;
+}