Readability review.

remoting/host/host_extension_session_manager.cc

 // 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.
 
 #include "remoting/host/host_extension_session_manager.h"
 
 #include "remoting/base/capabilities.h"
 #include "remoting/codec/video_encoder.h"
 #include "remoting/host/client_session_control.h"
 #include "remoting/host/host_extension.h"
 #include "remoting/host/host_extension_session.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_capturer.h"
 namespace remoting {

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

Nit: At least one blank line above this

[Wez, 2014/08/26 19:22:25]

Done.

 
 HostExtensionSessionManager::HostExtensionSessionManager(
     const std::vector<HostExtension*>& extensions,
     ClientSessionControl* client_session_control)
     : client_session_control_(client_session_control),
       client_stub_(NULL),
       extensions_(extensions) {
 }
 
 HostExtensionSessionManager::~HostExtensionSessionManager() {
 }
 
 std::string HostExtensionSessionManager::GetCapabilities() {
   std::string capabilities;
   for (HostExtensionList::const_iterator extension = extensions_.begin();
        extension != extensions_.end(); ++extension) {
     std::string capability = (*extension)->capability();

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

Nit: Could probably be a const std::string& (tiny efficiency boost in some
cases)

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

Making capability() return const std::string& risks broken code like:

const char kCapability[] = "foo";

const std::string& FooExtension::capability() const { return kCapability; }

so I'd prefer to have it return std::string instead.

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

I actually meant that the local variable here could be a const std::string&;
this is no worse than the current code if capability() returns by value, and
slightly better if it returns by const ref.  I didn't look at the prototype of
capability().

[Wez, 2014/08/26 19:22:25]

The std::string returned by capability() will go out of scope following the
assignment to the const std::string&, though, surely, leaving a danlging
reference?

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

No; if you bind a returned temporary to a const reference, the life of the
temporary is extended through the lifetime of the reference.

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

Done.

     if (capability.empty()) {
       continue;
     }
     if (!capabilities.empty()) {
       capabilities.append(" ");
     }
     capabilities.append(capability);
   }
   return capabilities;
 }
 
 scoped_ptr<webrtc::DesktopCapturer>
     HostExtensionSessionManager::OnCreateVideoCapturer(

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

Nit: Elsewhere in this CL you don't indent the function name when you've wrapped
after the return type.  Be consistent.

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

Acknowledged; that was an emacs auto-indentationism that slipped through, I
think.

         scoped_ptr<webrtc::DesktopCapturer> capturer) {
   for(HostExtensionSessionList::const_iterator it = extension_sessions_.begin();

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

Nit: for ( (2 places)

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

Done.

       it != extension_sessions_.end(); ++it) {
     if ((*it)->ModifiesVideoPipeline()) {
       capturer = (*it)->OnCreateVideoCapturer(capturer.Pass());
     }
   }
   return capturer.Pass();
 }
 
 scoped_ptr<VideoEncoder> HostExtensionSessionManager::OnCreateVideoEncoder(
     scoped_ptr<VideoEncoder> encoder) {
@@ skipping 49 matching lines @@
       it != extension_sessions_.end(); ++it) {
     if ((*it)->OnExtensionMessage(
             client_session_control_, client_stub_, message)) {
       return true;
     }
   }
   return false;
 }
 
 }  // namespace remoting
+