for readability review.

content/renderer/media/video_capture_impl.cc

Index: content/renderer/media/video_capture_impl.cc
===================================================================
--- content/renderer/media/video_capture_impl.cc	(revision 145737)
+++ content/renderer/media/video_capture_impl.cc	(working copy)
@@ -61,8 +61,7 @@
 }
 
 VideoCaptureImpl::~VideoCaptureImpl() {
-  STLDeleteContainerPairSecondPointers(cached_dibs_.begin(),
-                                       cached_dibs_.end());
+  STLDeleteValues(&cached_dibs_);
 }
 
 void VideoCaptureImpl::Init() {
@@ -71,46 +70,36 @@
     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::Bind(&VideoCaptureImpl::DoDeInitOnCaptureThread,
                  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::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));
 }
 
@@ -118,36 +107,45 @@
     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());
@@ -155,113 +153,85 @@
   if (state_ == video_capture::kError) {
     handler->OnError(this, 1);
     handler->OnRemoved(this);
-    return;
-  }
-
-  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() ) {
+  } 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.
-    return;
-  }
-
-  if (!device_id_) {
+  } else if (!device_id_) {
     clients_pending_on_filter_[handler] = capability;
-    return;
-  }
+  } 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_);
+        }
 
-  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_[handler] = capability;
+      }
+    } else if (state_ == video_capture::kStopping) {
       clients_pending_on_restart_[handler] = capability;
-      return;
-    }
+      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 << ")";
 
-    if (device_info_available_) {
-      handler->OnDeviceInfoReceived(this, device_info_);
+      StartCaptureInternal();
     }
-
-    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;
-  }
+  RemoveClient(handler, clients_pending_on_filter_) ||
+  RemoveClient(handler, clients_pending_on_restart_) ||
+  RemoveClient(handler, clients_);
 
-  if (clients_.find(handler) == clients_.end())
-    return;
-
-  clients_.erase(handler);
-
   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());
@@ -283,7 +253,8 @@
   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) {
@@ -296,13 +267,14 @@
   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) {
@@ -317,14 +289,14 @@
       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);
@@ -337,7 +309,7 @@
   }
 }
 
-void VideoCaptureImpl::DoDeviceInfoReceived(
+void VideoCaptureImpl::DoDeviceInfoReceivedOnCaptureThread(
     const media::VideoCaptureParams& device_info) {
   DCHECK(capture_message_loop_proxy_->BelongsToCurrentThread());
   DCHECK(!ClientHasDIB());
@@ -346,12 +318,12 @@
 
   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());
 
@@ -383,18 +355,14 @@
   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++);
   }
@@ -431,10 +399,27 @@
 }
 
 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;
+}