Readability review.

remoting/host/video_frame_recorder_host_extension.cc

Index: remoting/host/video_frame_recorder_host_extension.cc
diff --git a/remoting/host/video_frame_recorder_host_extension.cc b/remoting/host/video_frame_recorder_host_extension.cc
index b0b4d47d84bdef289c35b713e094c16218172a9c..40ec78724ae0f68b0ae937a63ef770a84479e724 100644
--- a/remoting/host/video_frame_recorder_host_extension.cc
+++ b/remoting/host/video_frame_recorder_host_extension.cc
@@ -21,26 +21,13 @@ namespace remoting {
 
 namespace {
 
-const char kVideoRecorderCapabilities[] = "videoRecorder";
-
-const char kVideoRecorderType[] = "video-recorder";
-
-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";
-
 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,29 +35,42 @@ 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);
 }
 
-scoped_ptr<VideoEncoder>
-VideoFrameRecorderHostExtensionSession::OnCreateVideoEncoder(
-    scoped_ptr<VideoEncoder> encoder) {
-  video_frame_recorder.DetachVideoEncoderWrapper();
-  return video_frame_recorder.WrapVideoEncoder(encoder.Pass());
+VideoFrameRecorderHostExtensionSession::~VideoFrameRecorderHostExtensionSession(
+    ) {

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

Nit: I have absolutely no idea how the Google style guide would suggest wrapping
this -- I don't think there is a rule -- but I think if the function name were
too long we'd normally break after the "::", so maybe doing that instead is
better?  Up to you.

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

Done.

+}
+
+void VideoFrameRecorderHostExtensionSession::OnCreateVideoEncoder(
+    scoped_ptr<VideoEncoder>* encoder) {
+  video_frame_recorder_.DetachVideoEncoderWrapper();
+  *encoder = video_frame_recorder_.WrapVideoEncoder(encoder->Pass());
 }
 
 bool VideoFrameRecorderHostExtensionSession::ModifiesVideoPipeline() const {
   return true;
 }
 
+// Name of the extension message type field, and its value for this extension.
+const char kType[] = "type";
+const char kVideoRecorderType[] = "video-recorder";

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

Nit: Since these two are used by multiple functions, it might be clearer to
leave them atop the file where they used to be, or else maybe declare them in
the class declaration.  I don't think there are rules about this, though.

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

Declaring them in the class complicates things with "constexpr" subtlety, so
I've moved them back above the session class declaration, to keep things simple.

+
 bool VideoFrameRecorderHostExtensionSession::OnExtensionMessage(
     ClientSessionControl* client_session_control,
     protocol::ClientStub* client_stub,
@@ -85,80 +85,108 @@ 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);
+  } else {
+    VLOG(1) << "Unknown video-recorder message";

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

Nit: As noted before, it's usually better to avoid logging.  As a reader, I
don't know whether there actually are other message types, or this is just
trying to be future-proof, and if control _does_ reach here, whether that's
something bad that we ought to know about, or not.

I think either of the following is probably clearer: just eliminate this "else"
clause entirely (unknown messages are fine, drop them on the floor) or make the
last handler more like:

  } else {
    DCHECK_EQ(kNextFrameType, type);
    OnNextFrame(client_stub);
  }

(to indicate that we need to explicitly handle all messages and if someone adds
one later without touching this it's a bug).

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

Removed the logging.

   }
 
   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>