Readability review.

These CLs implement a simple frame recording and fetch "extension" for the Chromoting protocol, to allow sample sessions to be captured for performance testing.

The three main classes introduced are:
- HostExtensionSessionManager
  This pulls logic for handling Chromoting extensions out of
  ClientSession instances into a dedicated manager class.
  It also introduces hooks through which extensions can
  wrap or replace the Chromoting video encoder or capturer.
- VideoFrameRecorder
  This allows a VideoEncoder to be wrapped to return a new
  encoder that will optionally copy & deliver frames to the
  VideoFrameRecorder before supplying them to the real
  encoder.
- VideoFrameRecorderHostExtension
  This extension uses a VideoFrameRecorder to allow a
  connected client to start/stop recording, and to retrieve
  the resulting frame data.

Original CLs:
crrev.com/402233003
crrev.com/339073002
crrev.com/372943002
crrev.com/462503002

BUG=260879

Review URL: https://codereview.chromium.org/468613002

Cr-Commit-Position: refs/heads/master@{#292541}

15 files changed, 355 insertions, 280 deletions

diff --git a/remoting/host/cast_extension_session.cc b/remoting/host/cast_extension_session.cc
index 8842c3e..bff8751 100644
--- a/remoting/host/cast_extension_session.cc
+++ b/remoting/host/cast_extension_session.cc
@@ -234,28 +234,26 @@ void CastExtensionSession::OnCreateSessionDescriptionFailure(
 // stream from the peer connection here, and then attempt to re-setup the
 // peer connection in the OnRenegotiationNeeded() callback.
 // See crbug.com/403843.
-scoped_ptr<webrtc::DesktopCapturer> CastExtensionSession::OnCreateVideoCapturer(
-    scoped_ptr<webrtc::DesktopCapturer> capturer) {
+void CastExtensionSession::OnCreateVideoCapturer(
+    scoped_ptr<webrtc::DesktopCapturer>* capturer) {
   if (has_grabbed_capturer_) {
     LOG(ERROR) << "The video pipeline was reset unexpectedly.";
     has_grabbed_capturer_ = false;
     peer_connection_->RemoveStream(stream_.release());
-    return capturer.Pass();
+    return;
   }
 
   if (received_offer_) {
     has_grabbed_capturer_ = true;
-    if (SetupVideoStream(capturer.Pass())) {
+    if (SetupVideoStream(capturer->Pass())) {
       peer_connection_->CreateAnswer(create_session_desc_observer_, NULL);
     } else {
       has_grabbed_capturer_ = false;
       // Ignore the received offer, since we failed to setup a video stream.
       received_offer_ = false;
     }
-    return scoped_ptr<webrtc::DesktopCapturer>();
+    return;
   }
-
-  return capturer.Pass();
 }
 
 bool CastExtensionSession::ModifiesVideoPipeline() const {
@@ -672,4 +670,3 @@ void CastExtensionSession::OnIceCandidate(
 }
 
 }  // namespace remoting
-
diff --git a/remoting/host/cast_extension_session.h b/remoting/host/cast_extension_session.h
index 03501bf..83e477a 100644
--- a/remoting/host/cast_extension_session.h
+++ b/remoting/host/cast_extension_session.h
@@ -61,8 +61,8 @@ class CastExtensionSession : public HostExtensionSession,
   void OnCreateSessionDescriptionFailure(const std::string& error);
 
   // HostExtensionSession interface.
-  virtual scoped_ptr<webrtc::DesktopCapturer> OnCreateVideoCapturer(
-      scoped_ptr<webrtc::DesktopCapturer> capturer) OVERRIDE;
+  virtual void OnCreateVideoCapturer(
+      scoped_ptr<webrtc::DesktopCapturer>* capturer) OVERRIDE;
   virtual bool ModifiesVideoPipeline() const OVERRIDE;
   virtual bool OnExtensionMessage(
       ClientSessionControl* client_session_control,
@@ -243,4 +243,3 @@ class CastExtensionSession : public HostExtensionSession,
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_CAST_EXTENSION_SESSION_H_
-
diff --git a/remoting/host/client_session.cc b/remoting/host/client_session.cc
index d741e1c..2af7f68 100644
--- a/remoting/host/client_session.cc
+++ b/remoting/host/client_session.cc
@@ -456,11 +456,11 @@ void ClientSession::ResetVideoPipeline() {
   // Create VideoEncoder and DesktopCapturer to match the session's video
   // channel configuration.
   scoped_ptr<webrtc::DesktopCapturer> video_capturer =
-      extension_manager_->OnCreateVideoCapturer(
-          desktop_environment_->CreateVideoCapturer());
+      desktop_environment_->CreateVideoCapturer();
+  extension_manager_->OnCreateVideoCapturer(&video_capturer);
   scoped_ptr<VideoEncoder> video_encoder =
-      extension_manager_->OnCreateVideoEncoder(
-          CreateVideoEncoder(connection_->session()->config()));
+      CreateVideoEncoder(connection_->session()->config());
+  extension_manager_->OnCreateVideoEncoder(&video_encoder);
 
   // Don't start the VideoScheduler if either capturer or encoder are missing.
   if (!video_capturer || !video_encoder)
diff --git a/remoting/host/fake_host_extension.cc b/remoting/host/fake_host_extension.cc
index 7f5c8c3..c16cd03 100644
--- a/remoting/host/fake_host_extension.cc
+++ b/remoting/host/fake_host_extension.cc
@@ -19,10 +19,10 @@ class FakeExtension::Session : public HostExtensionSession {
   Session(FakeExtension* extension, const std::string& message_type);
   virtual ~Session() {}
 
-  virtual scoped_ptr<webrtc::DesktopCapturer> OnCreateVideoCapturer(
-      scoped_ptr<webrtc::DesktopCapturer> encoder) OVERRIDE;
-  virtual scoped_ptr<VideoEncoder> OnCreateVideoEncoder(
-      scoped_ptr<VideoEncoder> encoder) OVERRIDE;
+  // HostExtensionSession interface.
+  virtual void OnCreateVideoCapturer(
+      scoped_ptr<webrtc::DesktopCapturer>* encoder) OVERRIDE;
+  virtual void OnCreateVideoEncoder(scoped_ptr<VideoEncoder>* encoder) OVERRIDE;
   virtual bool ModifiesVideoPipeline() const OVERRIDE;
   virtual bool OnExtensionMessage(
       ClientSessionControl* client_session_control,
@@ -42,20 +42,17 @@ FakeExtension::Session::Session(
     message_type_(message_type) {
 }
 
-scoped_ptr<webrtc::DesktopCapturer>
-FakeExtension::Session::OnCreateVideoCapturer(
-    scoped_ptr<webrtc::DesktopCapturer> capturer) {
+void FakeExtension::Session::OnCreateVideoCapturer(
+    scoped_ptr<webrtc::DesktopCapturer>* capturer) {
   extension_->has_wrapped_video_capturer_ = true;
   if (extension_->steal_video_capturer_) {
-    capturer.reset();
+    capturer->reset();
   }
-  return capturer.Pass();
 }
 
-scoped_ptr<VideoEncoder> FakeExtension::Session::OnCreateVideoEncoder(
-    scoped_ptr<VideoEncoder> encoder) {
+void FakeExtension::Session::OnCreateVideoEncoder(
+    scoped_ptr<VideoEncoder>* encoder) {
   extension_->has_wrapped_video_encoder_ = true;
-  return encoder.Pass();
 }
 
 bool FakeExtension::Session::ModifiesVideoPipeline() const {
@@ -100,23 +97,4 @@ scoped_ptr<HostExtensionSession> FakeExtension::CreateExtensionSession(
   return session.Pass();
 }
 
-void FakeExtension::set_steal_video_capturer(bool steal_video_capturer) {
-  steal_video_capturer_ = steal_video_capturer;
-}
-
-bool FakeExtension::has_wrapped_video_encoder() {
-  DCHECK(was_instantiated());
-  return has_wrapped_video_encoder_;
-}
-
-bool FakeExtension::has_wrapped_video_capturer() {
-  DCHECK(was_instantiated());
-  return has_wrapped_video_capturer_;
-}
-
-bool FakeExtension::has_handled_message() {
-  DCHECK(was_instantiated());
-  return has_handled_message_;
-}
-
 } // namespace remoting
diff --git a/remoting/host/fake_host_extension.h b/remoting/host/fake_host_extension.h
index e61828c..c52376c 100644
--- a/remoting/host/fake_host_extension.h
+++ b/remoting/host/fake_host_extension.h
@@ -33,26 +33,40 @@ class FakeExtension : public HostExtension {
       protocol::ClientStub* client_stub) OVERRIDE;
 
   // Controls for testing.
-  void set_steal_video_capturer(bool steal_video_capturer);
+  void set_steal_video_capturer(bool steal_video_capturer) {
+    steal_video_capturer_ = steal_video_capturer;
+  }
 
   // Accessors for testing.
-  bool has_handled_message();
-  bool has_wrapped_video_encoder();
-  bool has_wrapped_video_capturer();
-  bool was_instantiated() { return was_instantiated_; }
+  bool has_handled_message() const { return has_handled_message_; }
+  bool has_wrapped_video_encoder() const { return has_wrapped_video_encoder_; }
+  bool has_wrapped_video_capturer() const {
+    return has_wrapped_video_capturer_;
+  }
+  bool was_instantiated() const { return was_instantiated_; }
 
  private:
   class Session;
   friend class Session;
 
+  // The type name of extension messages that this fake consumes.
   std::string message_type_;
+
+  // The capability this fake reports, and requires clients to support, if any.
   std::string capability_;
 
+  // True if this extension should intercept creation of the session's video
+  // capturer and consume it, preventing the video pipeline being created.
   bool steal_video_capturer_;
 
+  // True if a message of |message_type_| has been processed by this extension.
   bool has_handled_message_;
+
+  // True if this extension had the opportunity to modify the video pipeline.
   bool has_wrapped_video_encoder_;
   bool has_wrapped_video_capturer_;
+
+  // True if CreateExtensionSession() was called on this extension.
   bool was_instantiated_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeExtension);
diff --git a/remoting/host/host_extension_session.cc b/remoting/host/host_extension_session.cc
index 5f54865..c2d8076 100644
--- a/remoting/host/host_extension_session.cc
+++ b/remoting/host/host_extension_session.cc
@@ -9,14 +9,12 @@
 
 namespace remoting {
 
-scoped_ptr<webrtc::DesktopCapturer> HostExtensionSession::OnCreateVideoCapturer(
-    scoped_ptr<webrtc::DesktopCapturer> capturer) {
-  return capturer.Pass();
+void HostExtensionSession::OnCreateVideoCapturer(
+    scoped_ptr<webrtc::DesktopCapturer>* capturer) {
 }
 
-scoped_ptr<VideoEncoder> HostExtensionSession::OnCreateVideoEncoder(
-    scoped_ptr<VideoEncoder> encoder) {
-  return encoder.Pass();
+void HostExtensionSession::OnCreateVideoEncoder(
+    scoped_ptr<VideoEncoder>* encoder) {
 }
 
 bool HostExtensionSession::ModifiesVideoPipeline() const {
diff --git a/remoting/host/host_extension_session.h b/remoting/host/host_extension_session.h
index a4a0bd7..1c51644 100644
--- a/remoting/host/host_extension_session.h
+++ b/remoting/host/host_extension_session.h
@@ -27,13 +27,18 @@ class HostExtensionSession {
  public:
   virtual ~HostExtensionSession() {}
 
-  // Optional hook functions for HostExtensions which need to wrap or replace
-  // parts of the video, audio, input, etc pipelines.
-  // These are called in response to ResetVideoPipeline().
-  virtual scoped_ptr<webrtc::DesktopCapturer> OnCreateVideoCapturer(
-      scoped_ptr<webrtc::DesktopCapturer> capturer);
-  virtual scoped_ptr<VideoEncoder> OnCreateVideoEncoder(
-      scoped_ptr<VideoEncoder> encoder);
+  // Hook functions called when the video pipeline is being (re)constructed.
+  // Implementations will receive these calls only if they express the need to
+  // modify the pipeline (see below). They may replace or wrap |capturer| and/or
+  // |encoder|, e.g. to filter video frames in some way.
+  // If either |capturer| or |encoder| are reset then the video pipeline is not
+  // constructed.
+  virtual void OnCreateVideoCapturer(
+      scoped_ptr<webrtc::DesktopCapturer>* capturer);
+  virtual void OnCreateVideoEncoder(scoped_ptr<VideoEncoder>* encoder);
+
+  // Must return true if the HostExtensionSession needs the opportunity to
+  // modify the video pipeline.
   virtual bool ModifiesVideoPipeline() const;
 
   // Called when the host receives an |ExtensionMessage| for the |ClientSession|
diff --git a/remoting/host/host_extension_session_manager.cc b/remoting/host/host_extension_session_manager.cc
index 31ed590..9fd2717 100644
--- a/remoting/host/host_extension_session_manager.cc
+++ b/remoting/host/host_extension_session_manager.cc
@@ -23,11 +23,11 @@ HostExtensionSessionManager::HostExtensionSessionManager(
 HostExtensionSessionManager::~HostExtensionSessionManager() {
 }
 
-std::string HostExtensionSessionManager::GetCapabilities() {
+std::string HostExtensionSessionManager::GetCapabilities() const {
   std::string capabilities;
-  for (HostExtensionList::const_iterator extension = extensions_.begin();
+  for (HostExtensions::const_iterator extension = extensions_.begin();
        extension != extensions_.end(); ++extension) {
-    std::string capability = (*extension)->capability();
+    const std::string& capability = (*extension)->capability();
     if (capability.empty()) {
       continue;
     }
@@ -39,27 +39,24 @@ std::string HostExtensionSessionManager::GetCapabilities() {
   return capabilities;
 }
 
-scoped_ptr<webrtc::DesktopCapturer>
-    HostExtensionSessionManager::OnCreateVideoCapturer(
-        scoped_ptr<webrtc::DesktopCapturer> capturer) {
-  for(HostExtensionSessionList::const_iterator it = extension_sessions_.begin();
+void HostExtensionSessionManager::OnCreateVideoCapturer(
+    scoped_ptr<webrtc::DesktopCapturer>* capturer) {
+  for (HostExtensionSessions::const_iterator it = extension_sessions_.begin();
       it != extension_sessions_.end(); ++it) {
     if ((*it)->ModifiesVideoPipeline()) {
-      capturer = (*it)->OnCreateVideoCapturer(capturer.Pass());
+      (*it)->OnCreateVideoCapturer(capturer);
     }
   }
-  return capturer.Pass();
 }
 
-scoped_ptr<VideoEncoder> HostExtensionSessionManager::OnCreateVideoEncoder(
-    scoped_ptr<VideoEncoder> encoder) {
-  for(HostExtensionSessionList::const_iterator it = extension_sessions_.begin();
+void HostExtensionSessionManager::OnCreateVideoEncoder(
+    scoped_ptr<VideoEncoder>* encoder) {
+  for (HostExtensionSessions::const_iterator it = extension_sessions_.begin();
       it != extension_sessions_.end(); ++it) {
     if ((*it)->ModifiesVideoPipeline()) {
-      encoder = (*it)->OnCreateVideoEncoder(encoder.Pass());
+      (*it)->OnCreateVideoEncoder(encoder);
     }
   }
-  return encoder.Pass();
 }
 
 void HostExtensionSessionManager::OnNegotiatedCapabilities(
@@ -72,7 +69,7 @@ void HostExtensionSessionManager::OnNegotiatedCapabilities(
 
   bool reset_video_pipeline = false;
 
-  for (HostExtensionList::const_iterator extension = extensions_.begin();
+  for (HostExtensions::const_iterator extension = extensions_.begin();
        extension != extensions_.end(); ++extension) {
     // If the extension requires a capability that was not negotiated then do
     // not instantiate it.
@@ -101,7 +98,7 @@ void HostExtensionSessionManager::OnNegotiatedCapabilities(
 
 bool HostExtensionSessionManager::OnExtensionMessage(
     const protocol::ExtensionMessage& message) {
-  for(HostExtensionSessionList::const_iterator it = extension_sessions_.begin();
+  for(HostExtensionSessions::const_iterator it = extension_sessions_.begin();
       it != extension_sessions_.end(); ++it) {
     if ((*it)->OnExtensionMessage(
             client_session_control_, client_stub_, message)) {
diff --git a/remoting/host/host_extension_session_manager.h b/remoting/host/host_extension_session_manager.h
index b84d520..44c28ea 100644
--- a/remoting/host/host_extension_session_manager.h
+++ b/remoting/host/host_extension_session_manager.h
@@ -32,45 +32,47 @@ class ExtensionMessage;
 // set of capabilities negotiated between client and host.
 class HostExtensionSessionManager {
  public:
+  typedef std::vector<HostExtension*> HostExtensions;
+
   // Creates an extension manager for the specified |extensions|.
-  HostExtensionSessionManager(const std::vector<HostExtension*>& extensions,
+  HostExtensionSessionManager(const HostExtensions& extensions,
                               ClientSessionControl* client_session_control);
   virtual ~HostExtensionSessionManager();
 
   // Returns the union of all capabilities supported by registered extensions.
-  std::string GetCapabilities();
+  std::string GetCapabilities() const;
 
-  // Calls the corresponding hook functions in each extension in turn, to give
-  // them an opportunity to wrap or replace video components.
-  scoped_ptr<webrtc::DesktopCapturer> OnCreateVideoCapturer(
-      scoped_ptr<webrtc::DesktopCapturer> capturer);
-  scoped_ptr<VideoEncoder> OnCreateVideoEncoder(
-      scoped_ptr<VideoEncoder> encoder);
+  // Calls the corresponding hook functions for each extension, to allow them
+  // to wrap or replace video pipeline components. Only extensions which return
+  // true from ModifiesVideoPipeline() will be called.
+  // The order in which extensions are called is undefined.
+  void OnCreateVideoCapturer(scoped_ptr<webrtc::DesktopCapturer>* capturer);
+  void OnCreateVideoEncoder(scoped_ptr<VideoEncoder>* encoder);
 
   // Handles completion of authentication and capabilities negotiation, creating
-  // the set of |HostExtensionSession| to match the client's capabilities.
+  // the set of HostExtensionSessions to match the client's capabilities.
   void OnNegotiatedCapabilities(protocol::ClientStub* client_stub,
                                 const std::string& capabilities);
 
-  // Passes |message| to each |HostExtensionSession| in turn until the message
+  // Passes |message| to each HostExtensionSession in turn until the message
   // is handled, or none remain. Returns true if the message was handled.
+  // It is not valid for more than one extension to handle the same message.
   bool OnExtensionMessage(const protocol::ExtensionMessage& message);
 
  private:
-  typedef std::vector<HostExtension*> HostExtensionList;
-  typedef ScopedVector<HostExtensionSession> HostExtensionSessionList;
+  typedef ScopedVector<HostExtensionSession> HostExtensionSessions;
 
   // Passed to HostExtensionSessions to allow them to send messages,
   // disconnect the session, etc.
   ClientSessionControl* client_session_control_;
   protocol::ClientStub* client_stub_;
 
-  // List of HostExtensions to attach to the session, if it reaches the
+  // The HostExtensions to instantiate for the session, if it reaches the
   // authenticated state.
-  HostExtensionList extensions_;
+  HostExtensions extensions_;
 
-  // List of HostExtensionSessions, used to handle extension messages.
-  HostExtensionSessionList extension_sessions_;
+  // The instantiated HostExtensionSessions, used to handle extension messages.
+  HostExtensionSessions extension_sessions_;
 
   DISALLOW_COPY_AND_ASSIGN(HostExtensionSessionManager);
 };
diff --git a/remoting/host/host_extension_session_manager_unittest.cc b/remoting/host/host_extension_session_manager_unittest.cc
index f2c875c..7a3592e 100644
--- a/remoting/host/host_extension_session_manager_unittest.cc
+++ b/remoting/host/host_extension_session_manager_unittest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/strings/string_util.h"
 #include "remoting/codec/video_encoder.h"
 #include "remoting/host/fake_host_extension.h"
 #include "remoting/host/host_extension_session_manager.h"
@@ -17,7 +18,7 @@ class HostExtensionSessionManagerTest : public testing::Test {
  public:
   HostExtensionSessionManagerTest()
       : extension1_("ext1", "cap1"),
-        extension2_("ext2", ""),
+        extension2_("ext2", std::string()),
         extension3_("ext3", "cap3") {
     extensions_.push_back(&extension1_);
     extensions_.push_back(&extension2_);
@@ -30,14 +31,16 @@ class HostExtensionSessionManagerTest : public testing::Test {
   FakeExtension extension1_;
   FakeExtension extension2_;
   FakeExtension extension3_;
-  std::vector<HostExtension*> extensions_;
+  HostExtensionSessionManager::HostExtensions extensions_;
 
   // Mocks of interfaces provided by ClientSession.
   MockClientSessionControl client_session_control_;
   protocol::MockClientStub client_stub_;
+
+  DISALLOW_COPY_AND_ASSIGN(HostExtensionSessionManagerTest);
 };
 
-// Verifies that messages are passed to be handled by the correct extension.
+// Verifies that messages are handled by the correct extension.
 TEST_F(HostExtensionSessionManagerTest, ExtensionMessages_MessageHandled) {
   HostExtensionSessionManager extension_manager(extensions_,
                                                 &client_session_control_);
@@ -76,7 +79,13 @@ TEST_F(HostExtensionSessionManagerTest, ExtensionCapabilities_AreReported) {
   HostExtensionSessionManager extension_manager(extensions_,
                                                 &client_session_control_);
 
-  EXPECT_EQ(extension_manager.GetCapabilities(), "cap1 cap3");
+  std::vector<std::string> reported_caps;
+  Tokenize(extension_manager.GetCapabilities(), " ", &reported_caps);
+  std::sort(reported_caps.begin(), reported_caps.end());
+
+  ASSERT_EQ(2U, reported_caps.size());
+  EXPECT_EQ("cap1", reported_caps[0]);
+  EXPECT_EQ("cap3", reported_caps[1]);
 }
 
 // Verifies that an extension is not instantiated if the client does not
@@ -107,8 +116,8 @@ TEST_F(HostExtensionSessionManagerTest, CanWrapVideoCapturer) {
   extension3_.set_steal_video_capturer(true);
   extension_manager.OnNegotiatedCapabilities(&client_stub_, "cap1");
 
-  extension_manager.OnCreateVideoCapturer(
-      scoped_ptr<webrtc::DesktopCapturer>());
+  scoped_ptr<webrtc::DesktopCapturer> dummy_capturer;
+  extension_manager.OnCreateVideoCapturer(&dummy_capturer);
 
   EXPECT_FALSE(extension1_.has_wrapped_video_encoder());
   EXPECT_TRUE(extension1_.has_wrapped_video_capturer());
@@ -129,7 +138,8 @@ TEST_F(HostExtensionSessionManagerTest, CanWrapVideoEncoder) {
   extension3_.set_steal_video_capturer(true);
   extension_manager.OnNegotiatedCapabilities(&client_stub_, "cap1");
 
-  extension_manager.OnCreateVideoEncoder(scoped_ptr<VideoEncoder>());
+  scoped_ptr<VideoEncoder> dummy_encoder;
+  extension_manager.OnCreateVideoEncoder(&dummy_encoder);
 
   EXPECT_TRUE(extension1_.has_wrapped_video_encoder());
   EXPECT_FALSE(extension1_.has_wrapped_video_capturer());
@@ -148,9 +158,10 @@ TEST_F(HostExtensionSessionManagerTest, RespectModifiesVideoPipeline) {
   extension2_.set_steal_video_capturer(true);
   extension_manager.OnNegotiatedCapabilities(&client_stub_, "cap1");
 
-  extension_manager.OnCreateVideoCapturer(
-      scoped_ptr<webrtc::DesktopCapturer>());
-  extension_manager.OnCreateVideoEncoder(scoped_ptr<VideoEncoder>());
+  scoped_ptr<webrtc::DesktopCapturer> dummy_capturer;
+  extension_manager.OnCreateVideoCapturer(&dummy_capturer);
+  scoped_ptr<VideoEncoder> dummy_encoder;
+  extension_manager.OnCreateVideoEncoder(&dummy_encoder);
 
   EXPECT_FALSE(extension1_.has_wrapped_video_encoder());
   EXPECT_FALSE(extension1_.has_wrapped_video_capturer());
@@ -159,7 +170,7 @@ TEST_F(HostExtensionSessionManagerTest, RespectModifiesVideoPipeline) {
   EXPECT_FALSE(extension3_.was_instantiated());
 }
 
-// Verifies that if an extension reports that they modify the video pipeline
+// Verifies that if an extension reports that it modifies the video pipeline
 // then ResetVideoPipeline() is called on the ClientSessionControl interface.
 TEST_F(HostExtensionSessionManagerTest, CallsResetVideoPipeline) {
   HostExtensionSessionManager extension_manager(extensions_,
diff --git a/remoting/host/video_frame_recorder.cc b/remoting/host/video_frame_recorder.cc
index 5303a80..8883b10 100644
--- a/remoting/host/video_frame_recorder.cc
+++ b/remoting/host/video_frame_recorder.cc
@@ -27,64 +27,21 @@ class VideoFrameRecorder::RecordingVideoEncoder : public VideoEncoder {
  public:
   RecordingVideoEncoder(scoped_ptr<VideoEncoder> encoder,
                         scoped_refptr<base::TaskRunner> recorder_task_runner,
-                        base::WeakPtr<VideoFrameRecorder> recorder)
-      : encoder_(encoder.Pass()),
-        recorder_task_runner_(recorder_task_runner),
-        recorder_(recorder),
-        enable_recording_(false),
-        weak_factory_(this) {
-    DCHECK(encoder_);
-    DCHECK(recorder_task_runner_.get());
-  }
+                        base::WeakPtr<VideoFrameRecorder> recorder);
 
-  base::WeakPtr<RecordingVideoEncoder> AsWeakPtr() {
-    return weak_factory_.GetWeakPtr();
-  }
+  base::WeakPtr<RecordingVideoEncoder> AsWeakPtr();
 
-  void SetEnableRecording(bool enable_recording) {
+  void set_enable_recording(bool enable_recording) {
     DCHECK(!encoder_task_runner_.get() ||
            encoder_task_runner_->BelongsToCurrentThread());
     enable_recording_ = enable_recording;
   }
 
   // remoting::VideoEncoder interface.
-  virtual void SetLosslessEncode(bool want_lossless) OVERRIDE {
-    encoder_->SetLosslessEncode(want_lossless);
-  }
-  virtual void SetLosslessColor(bool want_lossless) OVERRIDE {
-    encoder_->SetLosslessColor(want_lossless);
-  }
+  virtual void SetLosslessEncode(bool want_lossless) OVERRIDE;
+  virtual void SetLosslessColor(bool want_lossless) OVERRIDE;
   virtual scoped_ptr<VideoPacket> Encode(
-      const webrtc::DesktopFrame& frame) OVERRIDE {
-    // If this is the first Encode() then store the TaskRunner and inform the
-    // VideoFrameRecorder so it can post SetEnableRecording() on it.
-    if (!encoder_task_runner_.get()) {
-      encoder_task_runner_ = base::ThreadTaskRunnerHandle::Get();
-      recorder_task_runner_->PostTask(FROM_HERE,
-          base::Bind(&VideoFrameRecorder::SetEncoderTaskRunner,
-                     recorder_,
-                     encoder_task_runner_));
-    }
-
-    DCHECK(encoder_task_runner_->BelongsToCurrentThread());
-
-    if (enable_recording_) {
-      // Copy the frame and post it to the VideoFrameRecorder to store.
-      scoped_ptr<webrtc::DesktopFrame> frame_copy(
-          new webrtc::BasicDesktopFrame(frame.size()));
-      *frame_copy->mutable_updated_region() = frame.updated_region();
-      frame_copy->set_dpi(frame.dpi());
-      frame_copy->CopyPixelsFrom(frame.data(),
-                                 frame.stride(),
-                                 webrtc::DesktopRect::MakeSize(frame.size()));
-      recorder_task_runner_->PostTask(FROM_HERE,
-          base::Bind(&VideoFrameRecorder::RecordFrame,
-                     recorder_,
-                     base::Passed(&frame_copy)));
-    }
-
-    return encoder_->Encode(frame);
-  }
+      const webrtc::DesktopFrame& frame) OVERRIDE;
 
  private:
   scoped_ptr<VideoEncoder> encoder_;
@@ -99,6 +56,66 @@ class VideoFrameRecorder::RecordingVideoEncoder : public VideoEncoder {
   DISALLOW_COPY_AND_ASSIGN(RecordingVideoEncoder);
 };
 
+VideoFrameRecorder::RecordingVideoEncoder::RecordingVideoEncoder(
+    scoped_ptr<VideoEncoder> encoder,
+    scoped_refptr<base::TaskRunner> recorder_task_runner,
+    base::WeakPtr<VideoFrameRecorder> recorder)
+    : encoder_(encoder.Pass()),
+      recorder_task_runner_(recorder_task_runner),
+      recorder_(recorder),
+      enable_recording_(false),
+      weak_factory_(this) {
+  DCHECK(encoder_);
+  DCHECK(recorder_task_runner_.get());
+}
+
+base::WeakPtr<VideoFrameRecorder::RecordingVideoEncoder>
+VideoFrameRecorder::RecordingVideoEncoder::AsWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
+void VideoFrameRecorder::RecordingVideoEncoder::SetLosslessEncode(
+    bool want_lossless) {
+  encoder_->SetLosslessEncode(want_lossless);
+}
+
+void VideoFrameRecorder::RecordingVideoEncoder::SetLosslessColor(
+    bool want_lossless) {
+  encoder_->SetLosslessColor(want_lossless);
+}
+
+scoped_ptr<VideoPacket> VideoFrameRecorder::RecordingVideoEncoder::Encode(
+    const webrtc::DesktopFrame& frame) {
+  // If this is the first Encode() then store the TaskRunner and inform the
+  // VideoFrameRecorder so it can post set_enable_recording() on it.
+  if (!encoder_task_runner_.get()) {
+    encoder_task_runner_ = base::ThreadTaskRunnerHandle::Get();
+    recorder_task_runner_->PostTask(FROM_HERE,
+        base::Bind(&VideoFrameRecorder::SetEncoderTaskRunner,
+                   recorder_,
+                   encoder_task_runner_));
+  }
+
+  DCHECK(encoder_task_runner_->BelongsToCurrentThread());
+
+  if (enable_recording_) {
+    // Copy the frame and post it to the VideoFrameRecorder to store.
+    scoped_ptr<webrtc::DesktopFrame> frame_copy(
+        new webrtc::BasicDesktopFrame(frame.size()));
+    *frame_copy->mutable_updated_region() = frame.updated_region();
+    frame_copy->set_dpi(frame.dpi());
+    frame_copy->CopyPixelsFrom(frame.data(),
+                               frame.stride(),
+                               webrtc::DesktopRect::MakeSize(frame.size()));
+    recorder_task_runner_->PostTask(FROM_HERE,
+        base::Bind(&VideoFrameRecorder::RecordFrame,
+                   recorder_,
+                   base::Passed(&frame_copy)));
+  }
+
+  return encoder_->Encode(frame);
+}
+
 VideoFrameRecorder::VideoFrameRecorder()
     : content_bytes_(0),
       max_content_bytes_(0),
@@ -139,7 +156,7 @@ void VideoFrameRecorder::DetachVideoEncoderWrapper() {
   // Tell the wrapper to stop recording and posting frames to us.
   if (encoder_task_runner_.get()) {
     encoder_task_runner_->PostTask(FROM_HERE,
-        base::Bind(&RecordingVideoEncoder::SetEnableRecording,
+        base::Bind(&RecordingVideoEncoder::set_enable_recording,
                    recording_encoder_, false));
   }
 
@@ -159,7 +176,7 @@ void VideoFrameRecorder::SetEnableRecording(bool enable_recording) {
 
   if (encoder_task_runner_.get()) {
     encoder_task_runner_->PostTask(FROM_HERE,
-        base::Bind(&RecordingVideoEncoder::SetEnableRecording,
+        base::Bind(&RecordingVideoEncoder::set_enable_recording,
                    recording_encoder_,
                    enable_recording_));
   }
@@ -198,7 +215,7 @@ void VideoFrameRecorder::SetEncoderTaskRunner(
   // If the caller already enabled recording, inform the recording encoder.
   if (enable_recording_ && encoder_task_runner_.get()) {
     encoder_task_runner_->PostTask(FROM_HERE,
-        base::Bind(&RecordingVideoEncoder::SetEnableRecording,
+        base::Bind(&RecordingVideoEncoder::set_enable_recording,
                    recording_encoder_,
                    enable_recording_));
   }
diff --git a/remoting/host/video_frame_recorder.h b/remoting/host/video_frame_recorder.h
index 9d31722..a162373 100644
--- a/remoting/host/video_frame_recorder.h
+++ b/remoting/host/video_frame_recorder.h
@@ -28,11 +28,11 @@ class VideoEncoder;
 // or "control" thread.
 //
 // On the control thread:
-// 1. Create the VideoFrameRecorder on the controlling thread.
+// 1. Create the VideoFrameRecorder.
 // 2. Specify the amount of memory that may be used for recording.
 // 3. Call WrapVideoEncoder(), passing the actual VideoEncoder that will be
 //    used to encode frames.
-// 4. Hand the returned wrapper VideoEncoder of to the video encoding thread,
+// 4. Hand off the returned wrapper VideoEncoder to the video encoding thread,
 //    to call in place of the actual VideoEncoder.
 // 5. Start/stop frame recording as necessary.
 // 6. Use NextFrame() to read each recorded frame in sequence.
@@ -50,19 +50,21 @@ class VideoFrameRecorder {
 
   // Wraps the supplied VideoEncoder, returning a replacement VideoEncoder that
   // will route frames to the recorder, as well as passing them for encoding.
-  // The caller must delete the previous recording VideoEncoder, or call
-  // DetachVideoEncoderWrapper() before calling WrapVideoEncoder() to create
-  // a new wrapper.
+  // Each VideoFrameRecorder supports at most one wrapper at a time; if a new
+  // wrapper is required then any existing one must be deleted, or detached via
+  // DetachVideoEncoderWrapper(), before WrapVideoEncoder() is called again.
   scoped_ptr<VideoEncoder> WrapVideoEncoder(scoped_ptr<VideoEncoder> encoder);
 
   // Detaches the existing VideoEncoder wrapper, stopping it from recording.
+  // The detached wrapper remains owned by the caller and will continue to
+  // pass-through frames to the wrapped encoder, without recording them.
   void DetachVideoEncoderWrapper();
 
   // Enables/disables frame recording. Frame recording is initially disabled.
   void SetEnableRecording(bool enable_recording);
 
   // Sets the maximum number of bytes of pixel data that may be recorded.
-  // When this maximum is reached older frames will be discard to make space
+  // When this maximum is reached older frames will be discarded to make space
   // for new ones.
   void SetMaxContentBytes(int64_t max_content_bytes);
 
diff --git a/remoting/host/video_frame_recorder_host_extension.cc b/remoting/host/video_frame_recorder_host_extension.cc
index b0b4d47..477f74f 100644
--- a/remoting/host/video_frame_recorder_host_extension.cc
+++ b/remoting/host/video_frame_recorder_host_extension.cc
@@ -21,26 +21,17 @@ namespace remoting {
 
 namespace {
 
-const char kVideoRecorderCapabilities[] = "videoRecorder";
-
-const char kVideoRecorderType[] = "video-recorder";
-
+// Name of the extension message type field, and its value for this extension.
 const char kType[] = "type";
-const char kData[] = "data";
-
-const char kStartType[] = "start";
-const char kStopType[] = "stop";
-const char kNextFrameType[] = "next-frame";
-const char kNextFrameReplyType[] = "next-frame-reply";
+const char kVideoRecorderType[] = "video-recorder";
 
 class VideoFrameRecorderHostExtensionSession : public HostExtensionSession {
  public:
   explicit VideoFrameRecorderHostExtensionSession(int64_t max_content_bytes);
-  virtual ~VideoFrameRecorderHostExtensionSession() {}
+  virtual ~VideoFrameRecorderHostExtensionSession();
 
   // remoting::HostExtensionSession interface.
-  virtual scoped_ptr<VideoEncoder> OnCreateVideoEncoder(
-      scoped_ptr<VideoEncoder> encoder) OVERRIDE;
+  virtual void OnCreateVideoEncoder(scoped_ptr<VideoEncoder>* encoder) OVERRIDE;
   virtual bool ModifiesVideoPipeline() const OVERRIDE;
   virtual bool OnExtensionMessage(
       ClientSessionControl* client_session_control,
@@ -48,23 +39,32 @@ class VideoFrameRecorderHostExtensionSession : public HostExtensionSession {
       const protocol::ExtensionMessage& message) OVERRIDE;
 
  private:
-  VideoEncoderVerbatim verbatim_encoder;
-  VideoFrameRecorder video_frame_recorder;
+  // Handlers for the different frame recorder extension message types.
+  void OnStart();
+  void OnStop();
+  void OnNextFrame(protocol::ClientStub* client_stub);
+
+  VideoEncoderVerbatim verbatim_encoder_;
+  VideoFrameRecorder video_frame_recorder_;
   bool first_frame_;
 
   DISALLOW_COPY_AND_ASSIGN(VideoFrameRecorderHostExtensionSession);
 };
 
 VideoFrameRecorderHostExtensionSession::VideoFrameRecorderHostExtensionSession(
-    int64_t max_content_bytes) : first_frame_(false) {
-  video_frame_recorder.SetMaxContentBytes(max_content_bytes);
+    int64_t max_content_bytes)
+    : first_frame_(false) {
+  video_frame_recorder_.SetMaxContentBytes(max_content_bytes);
+}
+
+VideoFrameRecorderHostExtensionSession::
+    ~VideoFrameRecorderHostExtensionSession() {
 }
 
-scoped_ptr<VideoEncoder>
-VideoFrameRecorderHostExtensionSession::OnCreateVideoEncoder(
-    scoped_ptr<VideoEncoder> encoder) {
-  video_frame_recorder.DetachVideoEncoderWrapper();
-  return video_frame_recorder.WrapVideoEncoder(encoder.Pass());
+void VideoFrameRecorderHostExtensionSession::OnCreateVideoEncoder(
+    scoped_ptr<VideoEncoder>* encoder) {
+  video_frame_recorder_.DetachVideoEncoderWrapper();
+  *encoder = video_frame_recorder_.WrapVideoEncoder(encoder->Pass());
 }
 
 bool VideoFrameRecorderHostExtensionSession::ModifiesVideoPipeline() const {
@@ -85,80 +85,106 @@ bool VideoFrameRecorderHostExtensionSession::OnExtensionMessage(
 
   scoped_ptr<base::Value> value(base::JSONReader::Read(message.data()));
   base::DictionaryValue* client_message;
-  if (value && value->GetAsDictionary(&client_message)) {
-    std::string type;
-    if (!client_message->GetString(kType, &type)) {
-      LOG(ERROR) << "Invalid video-recorder message";
-      return true;
-    }
+  if (!value || !value->GetAsDictionary(&client_message)) {
+    return true;
+  }
 
-    if (type == kStartType) {
-      video_frame_recorder.SetEnableRecording(true);
-      first_frame_ = true;
-    } else if (type == kStopType) {
-      video_frame_recorder.SetEnableRecording(false);
-    } else if (type == kNextFrameType) {
-      scoped_ptr<webrtc::DesktopFrame> frame(video_frame_recorder.NextFrame());
-
-      // TODO(wez): This involves six copies of the entire frame.
-      // See if there's some way to optimize at least a few of them out.
-      base::DictionaryValue reply_message;
-      reply_message.SetString(kType, kNextFrameReplyType);
-      if (frame) {
-        // If this is the first frame then override the updated region so that
-        // the encoder will send the whole frame's contents.
-        if (first_frame_) {
-          first_frame_ = false;
-
-          frame->mutable_updated_region()->SetRect(
-              webrtc::DesktopRect::MakeSize(frame->size()));
-        }
-
-        // Encode the frame into a raw ARGB VideoPacket.
-        scoped_ptr<VideoPacket> encoded_frame(
-            verbatim_encoder.Encode(*frame));
-
-        // Serialize that packet into a string.
-        std::string data;
-        data.resize(encoded_frame->ByteSize());
-        encoded_frame->SerializeWithCachedSizesToArray(
-            reinterpret_cast<uint8_t*>(&data[0]));
-
-        // Convert that string to Base64, so it's JSON-friendly.
-        std::string base64_data;
-        base::Base64Encode(data, &base64_data);
-
-        // Copy the Base64 data into the message.
-        reply_message.SetString(kData, base64_data);
-      }
-
-      // JSON-encode the reply into a string.
-      std::string reply_json;
-      if (!base::JSONWriter::Write(&reply_message, &reply_json)) {
-        LOG(ERROR) << "Failed to create reply json";
-        return true;
-      }
-
-      // Return the frame (or a 'data'-less reply) to the client.
-      protocol::ExtensionMessage message;
-      message.set_type(kVideoRecorderType);
-      message.set_data(reply_json);
-      client_stub->DeliverHostMessage(message);
-    }
+  std::string type;
+  if (!client_message->GetString(kType, &type)) {
+    LOG(ERROR) << "Invalid video-recorder message";
+    return true;
+  }
+
+  const char kStartType[] = "start";
+  const char kStopType[] = "stop";
+  const char kNextFrameType[] = "next-frame";
+
+  if (type == kStartType) {
+    OnStart();
+  } else if (type == kStopType) {
+    OnStop();
+  } else if (type == kNextFrameType) {
+    OnNextFrame(client_stub);
   }
 
   return true;
 }
 
+void VideoFrameRecorderHostExtensionSession::OnStart() {
+  video_frame_recorder_.SetEnableRecording(true);
+  first_frame_ = true;
+}
+
+void VideoFrameRecorderHostExtensionSession::OnStop() {
+  video_frame_recorder_.SetEnableRecording(false);
+}
+
+void VideoFrameRecorderHostExtensionSession::OnNextFrame(
+    protocol::ClientStub* client_stub) {
+  scoped_ptr<webrtc::DesktopFrame> frame(video_frame_recorder_.NextFrame());
+
+  // TODO(wez): This involves six copies of the entire frame.
+  // See if there's some way to optimize at least a few of them out.
+  const char kNextFrameReplyType[] = "next-frame-reply";
+  base::DictionaryValue reply_message;
+  reply_message.SetString(kType, kNextFrameReplyType);
+  if (frame) {
+    // If this is the first frame then override the updated region so that
+    // the encoder will send the whole frame's contents.
+    if (first_frame_) {
+      first_frame_ = false;
+
+      frame->mutable_updated_region()->SetRect(
+          webrtc::DesktopRect::MakeSize(frame->size()));
+    }
+
+    // Encode the frame into a raw ARGB VideoPacket.
+    scoped_ptr<VideoPacket> encoded_frame(
+        verbatim_encoder_.Encode(*frame));
+
+    // Serialize that packet into a string.
+    std::string data(encoded_frame->ByteSize(), 0);
+    encoded_frame->SerializeWithCachedSizesToArray(
+        reinterpret_cast<uint8_t*>(&data[0]));
+
+    // Convert that string to Base64, so it's JSON-friendly.
+    std::string base64_data;
+    base::Base64Encode(data, &base64_data);
+
+    // Copy the Base64 data into the message.
+    const char kData[] = "data";
+    reply_message.SetString(kData, base64_data);
+  }
+
+  // JSON-encode the reply into a string.
+  // Note that JSONWriter::Write() can only fail due to invalid inputs, and will
+  // DCHECK in Debug builds in that case.
+  std::string reply_json;
+  if (!base::JSONWriter::Write(&reply_message, &reply_json)) {
+    return;
+  }
+
+  // Return the frame (or a 'data'-less reply) to the client.
+  protocol::ExtensionMessage message;
+  message.set_type(kVideoRecorderType);
+  message.set_data(reply_json);
+  client_stub->DeliverHostMessage(message);
+}
+
 } // namespace
 
+VideoFrameRecorderHostExtension::VideoFrameRecorderHostExtension() {}
+
+VideoFrameRecorderHostExtension::~VideoFrameRecorderHostExtension() {}
+
 void VideoFrameRecorderHostExtension::SetMaxContentBytes(
     int64_t max_content_bytes) {
   max_content_bytes_ = max_content_bytes;
 }
 
 std::string VideoFrameRecorderHostExtension::capability() const {
-  return kVideoRecorderCapabilities;
+  const char kVideoRecorderCapability[] = "videoRecorder";
+  return kVideoRecorderCapability;
 }
 
 scoped_ptr<HostExtensionSession>
diff --git a/remoting/host/video_frame_recorder_host_extension.h b/remoting/host/video_frame_recorder_host_extension.h
index 4a974e3..8bac525 100644
--- a/remoting/host/video_frame_recorder_host_extension.h
+++ b/remoting/host/video_frame_recorder_host_extension.h
@@ -15,8 +15,8 @@ namespace remoting {
 // sequences of frames to run tests against.
 class VideoFrameRecorderHostExtension : public HostExtension {
  public:
-  VideoFrameRecorderHostExtension() {}
-  virtual ~VideoFrameRecorderHostExtension() {}
+  VideoFrameRecorderHostExtension();
+  virtual ~VideoFrameRecorderHostExtension();
 
   // Sets the maximum number of bytes that each session may record.
   void SetMaxContentBytes(int64_t max_content_bytes);
diff --git a/remoting/host/video_frame_recorder_unittest.cc b/remoting/host/video_frame_recorder_unittest.cc
index f0f5b9d4..9e4139b 100644
--- a/remoting/host/video_frame_recorder_unittest.cc
+++ b/remoting/host/video_frame_recorder_unittest.cc
@@ -18,29 +18,34 @@ namespace webrtc {
 // Define equality operator for DesktopFrame to allow use of EXPECT_EQ().
 static bool operator==(const DesktopFrame& a,
                        const DesktopFrame& b) {
-  if ((a.size().equals(b.size())) &&
-      (a.updated_region().Equals(b.updated_region())) &&
-      (a.dpi().equals(b.dpi()))) {
-    for (int i = 0; i < a.size().height(); ++i) {
-      if (memcmp(a.data() + a.stride() * i,
-                 b.data() + b.stride() * i,
-                 a.size().width() * DesktopFrame::kBytesPerPixel) != 0) {
-        return false;
-      }
+  if ((!a.size().equals(b.size())) ||
+      (!a.updated_region().Equals(b.updated_region())) ||
+      (!a.dpi().equals(b.dpi()))) {
+    return false;
+  }
+
+  for (int i = 0; i < a.size().height(); ++i) {
+    if (memcmp(a.data() + a.stride() * i,
+               b.data() + b.stride() * i,
+               a.size().width() * DesktopFrame::kBytesPerPixel) != 0) {
+      return false;
     }
-    return true;
   }
-  return false;
+
+  return true;
 }
 
 } // namespace
 
 namespace remoting {
 
-const int64_t kMaxContentBytes = 10 * 1024 * 1024;
-const int kWidth = 640;
-const int kHeight = 480;
-const int kTestFrameCount = 6;
+namespace {
+const int kFrameWidth = 640;
+const int kFrameHeight = 480;
+const size_t kTestFrameCount = 6;
+const int64 kTestFrameBytes =
+    kFrameWidth * kFrameHeight * webrtc::DesktopFrame::kBytesPerPixel;
+} // namespace
 
 class VideoFrameRecorderTest : public testing::Test {
  public:
@@ -49,27 +54,51 @@ class VideoFrameRecorderTest : public testing::Test {
   virtual void SetUp() OVERRIDE;
   virtual void TearDown() OVERRIDE;
 
+  // Creates a new VideoEncoder, wraps it using |recorder_|, and stores the
+  // newly wrapped encoder in |encoder_|.
   void CreateAndWrapEncoder();
+
+  // Creates the next test frame to pass to |encoder_|. Each test frame's pixel
+  // values are set uniquely, so that tests can verify that the correct set of
+  // frames were recorded.
   scoped_ptr<webrtc::DesktopFrame> CreateNextFrame();
+
+  // Calls CreateNextFrame() to create kTextFrameCount test frames, and stores
+  // them to |test_frames_|.
   void CreateTestFrames();
+
+  // Passes the frames in |test_frames_| to |encoder_|, in order, to encode.
   void EncodeTestFrames();
+
+  // Creates a frame and passes it to |encoder_| without adding it to
+  // |test_frames_|.
   void EncodeDummyFrame();
+
+  // Configures |recorder_| to start recording, and pumps events to ensure that
+  // |encoder_| is ready to record frames.
   void StartRecording();
+
+  // Reads frames from |recorder_| and compares them to the |test_frames_|.
   void VerifyTestFrames();
 
  protected:
+  typedef std::list<webrtc::DesktopFrame*> DesktopFrames;
+
   base::MessageLoop message_loop_;
 
   scoped_ptr<VideoFrameRecorder> recorder_;
   scoped_ptr<VideoEncoder> encoder_;
 
-  std::list<webrtc::DesktopFrame*> test_frames_;
+  DesktopFrames test_frames_;
   int frame_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(VideoFrameRecorderTest);
 };
 
 VideoFrameRecorderTest::VideoFrameRecorderTest() : frame_count_(0) {}
 
 void VideoFrameRecorderTest::SetUp() {
+  const int64_t kMaxContentBytes = 10 * 1024 * 1024;
   recorder_.reset(new VideoFrameRecorder());
   recorder_->SetMaxContentBytes(kMaxContentBytes);
 }
@@ -97,11 +126,12 @@ void VideoFrameRecorderTest::CreateAndWrapEncoder() {
 
 scoped_ptr<webrtc::DesktopFrame> VideoFrameRecorderTest::CreateNextFrame() {
   scoped_ptr<webrtc::DesktopFrame> frame(
-      new webrtc::BasicDesktopFrame(webrtc::DesktopSize(kWidth, kHeight)));
+      new webrtc::BasicDesktopFrame(webrtc::DesktopSize(kFrameWidth,
+                                                        kFrameHeight)));
 
   // Fill content, DPI and updated-region based on |frame_count_| so that each
   // generated frame is different.
-  memset(frame->data(), frame_count_, frame->stride() * kHeight);
+  memset(frame->data(), frame_count_, frame->stride() * kFrameHeight);
   frame->set_dpi(webrtc::DesktopVector(frame_count_, frame_count_));
   frame->mutable_updated_region()->SetRect(
       webrtc::DesktopRect::MakeWH(frame_count_, frame_count_));
@@ -111,15 +141,15 @@ scoped_ptr<webrtc::DesktopFrame> VideoFrameRecorderTest::CreateNextFrame() {
 }
 
 void VideoFrameRecorderTest::CreateTestFrames() {
-  for (int i=0; i < kTestFrameCount; ++i) {
+  for (size_t i = 0; i < kTestFrameCount; ++i) {
     test_frames_.push_back(CreateNextFrame().release());
   }
 }
 
 void VideoFrameRecorderTest::EncodeTestFrames() {
-  std::list<webrtc::DesktopFrame*>::iterator i;
-  for (i = test_frames_.begin(); i != test_frames_.end(); ++i) {
-    scoped_ptr<VideoPacket> packet = encoder_->Encode(*(*i));
+  for (DesktopFrames::iterator i = test_frames_.begin();
+       i != test_frames_.end(); ++i) {
+    ASSERT_TRUE(encoder_->Encode(**i));
 
     // Process tasks to let the recorder pick up the frame.
     base::RunLoop().RunUntilIdle();
@@ -127,8 +157,9 @@ void VideoFrameRecorderTest::EncodeTestFrames() {
 }
 
 void VideoFrameRecorderTest::EncodeDummyFrame() {
-  webrtc::BasicDesktopFrame dummy_frame(webrtc::DesktopSize(kWidth, kHeight));
-  scoped_ptr<VideoPacket> packet = encoder_->Encode(dummy_frame);
+  webrtc::BasicDesktopFrame dummy_frame(
+      webrtc::DesktopSize(kFrameWidth, kFrameHeight));
+  ASSERT_TRUE(encoder_->Encode(dummy_frame));
   base::RunLoop().RunUntilIdle();
 }
 
@@ -158,7 +189,7 @@ TEST_F(VideoFrameRecorderTest, CreateDestroy) {
 }
 
 // Basic test that creating, starting, stopping and destroying a
-// VideoFrameRecorder don't end the world.
+// VideoFrameRecorder succeeds (e.g. does not crash or DCHECK).
 TEST_F(VideoFrameRecorderTest, StartStop) {
   StartRecording();
   recorder_->SetEnableRecording(false);
@@ -218,8 +249,7 @@ TEST_F(VideoFrameRecorderTest, MaxContentBytesEnforced) {
   CreateAndWrapEncoder();
 
   // Configure a maximum content size sufficient for five and a half frames.
-  int64 frame_bytes = kWidth * kHeight * webrtc::DesktopFrame::kBytesPerPixel;
-  recorder_->SetMaxContentBytes((frame_bytes * 11) / 2);
+  recorder_->SetMaxContentBytes((kTestFrameBytes * 11) / 2);
 
   // Start the recorder, so that the wrapper will push frames to it.
   StartRecording();
@@ -238,14 +268,13 @@ TEST_F(VideoFrameRecorderTest, MaxContentBytesEnforced) {
   VerifyTestFrames();
 }
 
-// Test that when asked to record more frames than the maximum content bytes
-// limit allows, the first encoded frames are dropped.
+// Test that when frames are consumed the corresponding space is freed up in
+// the content buffer, allowing subsequent frames to be recorded.
 TEST_F(VideoFrameRecorderTest, ContentBytesUpdatedByNextFrame) {
   CreateAndWrapEncoder();
 
   // Configure a maximum content size sufficient for kTestFrameCount frames.
-  int64 frame_bytes = kWidth * kHeight * webrtc::DesktopFrame::kBytesPerPixel;
-  recorder_->SetMaxContentBytes(frame_bytes * kTestFrameCount);
+  recorder_->SetMaxContentBytes(kTestFrameBytes * kTestFrameCount);
 
   // Start the recorder, so that the wrapper will push frames to it.
   StartRecording();