Readability review.

remoting/host/host_extension_session_manager.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_MANAGER_H_
 #define REMOTING_HOST_HOST_EXTENSION_SESSION_MANAGER_H_
 
 #include <string>
 #include <vector>
 
@@ skipping 15 matching lines @@
 class ClientStub;
 class ExtensionMessage;
 }
 
 // Helper class used to create and manage a set of HostExtensionSession
 // instances depending upon the set of registered HostExtensions, and the
 // set of capabilities negotiated between client and host.
 class HostExtensionSessionManager {
  public:
   // Creates an extension manager for the specified |extensions|.
   HostExtensionSessionManager(const std::vector<HostExtension*>& extensions,

[Peter Kasting, 2014/08/22 06:38:28]

Nit: Since you already have a typedef for this in the private section below,
consider making it public and using it here, so callers can benefit from it.

[Wez, 2014/08/25 23:23:10]

Done.

                               ClientSessionControl* client_session_control);
   virtual ~HostExtensionSessionManager();
 
   // Returns the union of all capabilities supported by registered extensions.
   std::string GetCapabilities();

[Peter Kasting, 2014/08/22 06:38:28]

This can probably be const.

[Wez, 2014/08/25 23:23:11]

Done.

 
   // Calls the corresponding hook functions in each extension in turn, to give
   // them an opportunity to wrap or replace video components.

[Peter Kasting, 2014/08/22 06:38:29]

It seems like more comments could be useful.  For example, what if multiple
different extensions want to replace the relevant component -- what order do
they go in?

[Wez, 2014/08/25 23:23:10]

Clarified that the order is undefined.

   scoped_ptr<webrtc::DesktopCapturer> OnCreateVideoCapturer(

[Peter Kasting, 2014/08/22 06:38:29]

Nit: It might be clearer to have these functions look more like:

  void OnCreateVideoCapturer(scoped_ptr<webrtc::DesktopCapturer>* capturer);

This makes it more obvious that the function is not so much taking ownership of
the argument as potentially modifying it.  (2 places)

[Wez, 2014/08/25 23:23:11]

Done.

       scoped_ptr<webrtc::DesktopCapturer> capturer);
   scoped_ptr<VideoEncoder> OnCreateVideoEncoder(
       scoped_ptr<VideoEncoder> encoder);
 
   // Handles completion of authentication and capabilities negotiation, creating
   // the set of |HostExtensionSession| to match the client's capabilities.

[Peter Kasting, 2014/08/22 06:38:29]

Nit: |HostExtensionSession|s

[Wez, 2014/08/25 23:23:10]

Acknowledged.  Removed the || since we don't seem to use those for type names,
only variables & members.

   void OnNegotiatedCapabilities(protocol::ClientStub* client_stub,
                                 const std::string& capabilities);
 
   // Passes |message| to each |HostExtensionSession| in turn until the message
   // is handled, or none remain. Returns true if the message was handled.

[Peter Kasting, 2014/08/22 06:38:29]

Again, what order do these go in?

[Wez, 2014/08/25 23:23:10]

It's also not valid for two extensions to handle the same message - the function
currently exits as soon as an extension handles the message. Added comment to
clarify that.

If the function instead called all extensions even after one has handled the
message then it could DCHECK() that at most one handles it, plus every extension
would always be called for every message, which allows things like adding an
extension-message-logging extension, for example. WDYT?

[Peter Kasting, 2014/08/25 23:39:06]

I don't have an opinion -- that's less of a readability change and more of a
functional one, so it'd be better posed to the relevant OWNERS here.

   bool OnExtensionMessage(const protocol::ExtensionMessage& message);
 
  private:
   typedef std::vector<HostExtension*> HostExtensionList;

[Peter Kasting, 2014/08/22 06:38:29]

Nit: Don't use "List" in a typename, especially if it isn't actually a
std::list<>.  For example, "HostExtensions" would be a more generic/less
misleading name here. (2 places)

[Wez, 2014/08/25 23:23:11]

Done.

   typedef ScopedVector<HostExtensionSession> HostExtensionSessionList;
 
   // 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

[Peter Kasting, 2014/08/22 06:38:28]

Nit: Again, avoid "list" except perhaps where you're actually using a list.  (2
places)

[Wez, 2014/08/25 23:23:11]

Done.

   // authenticated state.
   HostExtensionList extensions_;
 
   // List of HostExtensionSessions, used to handle extension messages.
   HostExtensionSessionList extension_sessions_;
 
   DISALLOW_COPY_AND_ASSIGN(HostExtensionSessionManager);
 };
 
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_HOST_EXTENSION_SESSION_MANAGER_H_
+