DevTools: FrameRecorder: add cancelRecordingFrames command

content/browser/devtools/protocol/frame_recorder.cc

Index: content/browser/devtools/protocol/frame_recorder.cc
diff --git a/content/browser/devtools/protocol/frame_recorder.cc b/content/browser/devtools/protocol/frame_recorder.cc
index 34ea03301d7b5fbcb36ec91f70209bc38e05479b..5b64c045ef61c72f1dbbf88ce20ce197da6c4ddf 100644
--- a/content/browser/devtools/protocol/frame_recorder.cc
+++ b/content/browser/devtools/protocol/frame_recorder.cc
@@ -23,7 +23,8 @@ namespace {
 
 static int kMaxRecordFrameCount = 180;
 
-std::string EncodeFrame(const SkBitmap& bitmap) {
+std::pair<std::string, double> EncodeFrame(

[dgozman, 2015/01/29 17:53:06]

Looks like you are copying encoded data a number of times. Can we avoid that?

[eustas, 2015/01/30 09:26:30]

Done. Wrapped to scoped_ptr

+    const SkBitmap& bitmap, double timestamp) {
   std::vector<unsigned char> data;
   SkAutoLockPixels lock_image(bitmap);
   bool encoded = gfx::PNGCodec::Encode(
@@ -34,14 +35,14 @@ std::string EncodeFrame(const SkBitmap& bitmap) {
       false, std::vector<gfx::PNGCodec::Comment>(), &data);
 
   if (!encoded)
-    return std::string();
+    return std::make_pair(std::string(), timestamp);
 
   std::string base_64_data;
   base::Base64Encode(
       base::StringPiece(reinterpret_cast<char*>(&data[0]), data.size()),
       &base_64_data);
 
-  return base_64_data;
+  return std::make_pair(base_64_data, timestamp);
 }
 } // namespace
 
@@ -67,8 +68,14 @@ void FrameRecorder::SetRenderViewHost(RenderViewHostImpl* host) {
 Response FrameRecorder::StartRecordingFrames(int max_frame_count) {
   if (max_frame_count <= 0 || max_frame_count > kMaxRecordFrameCount)
     return Response::InvalidParams("maxFrameCount");
-  if (state_ != Ready)
+  if (state_ != Ready && state_ != Encoding)
     return Response::InternalError("Already recording");
+  if (state_ == Encoding) {

[dgozman, 2015/01/29 17:53:06]

No. This should be an error. Let the caller ensure correct call sequence.

[eustas, 2015/01/30 09:26:30]

Workflow:
(Ready) + Start -> (Recording)
(Recording) + Stop -> (Encoding)
(Encoding) -> (Ready)

There is transition that is not controlled directly.

Scenario: user starts recording frames shortly after they stopped. Obviously,
previous recording is out of interest.

[eustas, 2015/01/30 09:26:30]

Done.

+    std::vector<scoped_refptr<devtools::page::RecordedFrame>> no_frames;
+    callback_.Run(StopRecordingFramesResponse::Create()->set_frames(no_frames));
+  }
+  frame_encoded_callback_.Reset(
+      base::Bind(&FrameRecorder::FrameEncoded, weak_factory_.GetWeakPtr()));
   state_ = Recording;
   max_frame_count_ = max_frame_count;
   captured_frames_count_ = 0;
@@ -81,9 +88,17 @@ Response FrameRecorder::StartRecordingFrames(int max_frame_count) {
 }
 
 Response FrameRecorder::StopRecordingFrames(
-    StopRecordingFramesCallback callback) {
+    StopRecordingFramesCallback callback, bool cancel) {
   if (state_ != Recording)

[dgozman, 2015/01/29 17:53:06]

You should be able to cancel during encoding. Perhaps, it would be better with a
separate |cancelRecordingFrames| call.

[eustas, 2015/01/30 09:26:30]

Done.

     return Response::InternalError("Not recording");
+  if (cancel) {
+    frame_encoded_callback_.Cancel();
+    std::vector<scoped_refptr<devtools::page::RecordedFrame>> no_frames;
+    frames_.swap(no_frames);
+    callback.Run(StopRecordingFramesResponse::Create()->set_frames(no_frames));

[dgozman, 2015/01/29 17:53:06]

|no_frames| has been swapped and contains collected frames, which you should not
send.

[eustas, 2015/01/30 09:26:30]

Ooops.

+    state_ = Ready;
+    return Response::OK();
+  }
   state_ = Encoding;
   callback_ = callback;
   MaybeSendResponse();
@@ -120,16 +135,15 @@ void FrameRecorder::FrameCaptured(
   base::PostTaskAndReplyWithResult(
       base::WorkerPool::GetTaskRunner(true).get(),
       FROM_HERE,
-      base::Bind(&EncodeFrame, bitmap),
-      base::Bind(&FrameRecorder::FrameEncoded, weak_factory_.GetWeakPtr(),
-                 timestamp.ToDoubleT()));
+      base::Bind(&EncodeFrame, bitmap, timestamp.ToDoubleT()),
+      frame_encoded_callback_.callback());
 }
 
 void FrameRecorder::FrameEncoded(
-    double timestamp, const std::string& encoded_frame) {
+    const std::pair<std::string, double>& encoded_frame) {
   frames_.push_back(RecordedFrame::Create()
-      ->set_data(encoded_frame)
-      ->set_timestamp(timestamp));
+      ->set_data(encoded_frame.first)
+      ->set_timestamp(encoded_frame.second));
   MaybeSendResponse();
 }