Readability review.

remoting/host/host_extension_session.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef REMOTING_HOST_HOST_EXTENSION_SESSION_H_
 #define REMOTING_HOST_HOST_EXTENSION_SESSION_H_
 
 #include "base/memory/scoped_ptr.h"
 
 namespace webrtc {
 class DesktopCapturer;
 }
 
 namespace remoting {
 
 class ClientSessionControl;
 class VideoEncoder;
 
 namespace protocol {
 class ExtensionMessage;
 class ClientStub;
 }  // namespace protocol
 
 // Created by an |HostExtension| to store |ClientSession| specific state, and to
 // handle extension messages.
 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, wrap or take ownership

[Peter Kasting, 2014/08/26 20:58:15]

Nit: For clarity, I think maybe "take ownership" should be removed, as it's
redundant with the rest of your comment in the context of e.g. wrapping the
provided object, and it doesn't make much sense in the context of an ownership
transfer (since, as you note, the pipeline won't be constructed in that case).

[Wez, 2014/08/28 00:13:20]

Done.

+  // of |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;

[Peter Kasting, 2014/08/26 20:58:15]

Tiny nit: I wonder if "WantsToModifyVideoPipeline" would be clearer?  Not sure.

[Wez, 2014/08/28 00:13:20]

Acknowledged. I think Modifies* is sufficiently expressive, and more concise. :)

 
   // Called when the host receives an |ExtensionMessage| for the |ClientSession|
   // associated with this |HostExtensionSession|.
   // It returns |true| if the message was handled, and |false| otherwise.
   virtual bool OnExtensionMessage(
       ClientSessionControl* client_session_control,
       protocol::ClientStub* client_stub,
       const protocol::ExtensionMessage& message) = 0;
 };
 
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_HOST_EXTENSION_SESSION_H_