DevTools: FrameRecorder: add cancelRecordingFrames command

DevTools: FrameRecorder: add cancelRecordingFrames command

BUG=427418
Committed: https://crrev.com/c8d4343f0d213c041541348649ca84ddf2d194df
Cr-Commit-Position: refs/heads/master@{#313914}

[eustas, 2015-01-29 14:32:59]

eustas@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2015-01-29 17:53:07]

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
File content/browser/devtools/protocol/frame_recorder.cc (right):

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/frame_recorder.cc:26: std::pair<std::string,
double> EncodeFrame(
Looks like you are copying encoded data a number of times. Can we avoid that?

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/frame_recorder.cc:73: if (state_ == Encoding)
{
No. This should be an error. Let the caller ensure correct call sequence.

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/frame_recorder.cc:92: if (state_ != Recording)
You should be able to cancel during encoding. Perhaps, it would be better with a
separate |cancelRecordingFrames| call.

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/frame_recorder.cc:98:
callback.Run(StopRecordingFramesResponse::Create()->set_frames(no_frames));
|no_frames| has been swapped and contains collected frames, which you should not
send.

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
File content/browser/devtools/protocol/page_handler.h (right):

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/page_handler.h:81: DevToolsCommandId
command_id, const bool* cancel = nullptr);
Default values are not allowed.

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

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
File content/browser/devtools/protocol/frame_recorder.cc (right):

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/frame_recorder.cc:26: std::pair<std::string,
double> EncodeFrame(
On 2015/01/29 17:53:06, dgozman wrote:
> Looks like you are copying encoded data a number of times. Can we avoid that?

Done. Wrapped to scoped_ptr

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/frame_recorder.cc:73: if (state_ == Encoding)
{
On 2015/01/29 17:53:06, dgozman wrote:
> No. This should be an error. Let the caller ensure correct call sequence.

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.

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/frame_recorder.cc:73: if (state_ == Encoding)
{
On 2015/01/29 17:53:06, dgozman wrote:
> No. This should be an error. Let the caller ensure correct call sequence.

Done.

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/frame_recorder.cc:92: if (state_ != Recording)
On 2015/01/29 17:53:06, dgozman wrote:
> You should be able to cancel during encoding. Perhaps, it would be better with
a
> separate |cancelRecordingFrames| call.

Done.

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/frame_recorder.cc:98:
callback.Run(StopRecordingFramesResponse::Create()->set_frames(no_frames));
On 2015/01/29 17:53:06, dgozman wrote:
> |no_frames| has been swapped and contains collected frames, which you should
not
> send.

Ooops.

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
File content/browser/devtools/protocol/page_handler.h (right):

https://codereview.chromium.org/888573002/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/page_handler.h:81: DevToolsCommandId
command_id, const bool* cancel = nullptr);
On 2015/01/29 17:53:06, dgozman wrote:
> Default values are not allowed.

Done.

[dgozman, 2015-01-30 10:06:24]

https://codereview.chromium.org/888573002/diff/40001/content/browser/devtools...
File content/browser/devtools/protocol/frame_recorder.cc (right):

https://codereview.chromium.org/888573002/diff/40001/content/browser/devtools...
content/browser/devtools/protocol/frame_recorder.cc:97:
frame_encoded_callback_.Cancel();
Where do you initialize this field?

https://codereview.chromium.org/888573002/diff/40001/content/browser/devtools...
File content/browser/devtools/protocol/frame_recorder.h (right):

https://codereview.chromium.org/888573002/diff/40001/content/browser/devtools...
content/browser/devtools/protocol/frame_recorder.h:29: typedef
std::pair<std::string, double> EncodedFrame;
|using| instead of |typedef|

https://codereview.chromium.org/888573002/diff/40001/content/browser/devtools...
content/browser/devtools/protocol/frame_recorder.h:55: void FrameEncoded(const
scoped_ptr<EncodedFrame>& encoded_frame);
Why const ref? You are passing ownership, so use plain scoped_ptr.

[eustas, 2015-01-30 12:08:16]

https://codereview.chromium.org/888573002/diff/40001/content/browser/devtools...
File content/browser/devtools/protocol/frame_recorder.cc (right):

https://codereview.chromium.org/888573002/diff/40001/content/browser/devtools...
content/browser/devtools/protocol/frame_recorder.cc:97:
frame_encoded_callback_.Cancel();
On 2015/01/30 10:06:23, dgozman wrote:
> Where do you initialize this field?

In StartRecoding... Removed too much during cleanup =)

https://codereview.chromium.org/888573002/diff/40001/content/browser/devtools...
File content/browser/devtools/protocol/frame_recorder.h (right):

https://codereview.chromium.org/888573002/diff/40001/content/browser/devtools...
content/browser/devtools/protocol/frame_recorder.h:29: typedef
std::pair<std::string, double> EncodedFrame;
On 2015/01/30 10:06:23, dgozman wrote:
> |using| instead of |typedef|

Surely

https://codereview.chromium.org/888573002/diff/40001/content/browser/devtools...
content/browser/devtools/protocol/frame_recorder.h:55: void FrameEncoded(const
scoped_ptr<EncodedFrame>& encoded_frame);
On 2015/01/30 10:06:23, dgozman wrote:
> Why const ref? You are passing ownership, so use plain scoped_ptr.

Acknowledged.

[dgozman, 2015-01-30 12:29:12]

lgtm with comments

https://codereview.chromium.org/888573002/diff/60001/content/browser/devtools...
File content/browser/devtools/protocol/frame_recorder.cc (right):

https://codereview.chromium.org/888573002/diff/60001/content/browser/devtools...
content/browser/devtools/protocol/frame_recorder.cc:78:
frame_encoded_callback_.Reset(base::Bind(
Please reset this callback to nothing in stop and cancel methods.

https://codereview.chromium.org/888573002/diff/60001/content/browser/devtools...
content/browser/devtools/protocol/frame_recorder.cc:132: MaybeSendResponse();
What's the scenario where this call is required?

[eustas, 2015-01-30 12:43:23]

https://codereview.chromium.org/888573002/diff/60001/content/browser/devtools...
File content/browser/devtools/protocol/frame_recorder.cc (right):

https://codereview.chromium.org/888573002/diff/60001/content/browser/devtools...
content/browser/devtools/protocol/frame_recorder.cc:78:
frame_encoded_callback_.Reset(base::Bind(
On 2015/01/30 12:29:12, dgozman wrote:
> Please reset this callback to nothing in stop and cancel methods.

.Cancel() actually resets the callback.

We shouldn't reset it stop (that is why there is Encoding state).
I'll reset it in MaybeSend.

https://codereview.chromium.org/888573002/diff/60001/content/browser/devtools...
content/browser/devtools/protocol/frame_recorder.cc:132: MaybeSendResponse();
On 2015/01/30 12:29:11, dgozman wrote:
> What's the scenario where this call is required?

As you see - we drop the first frame in a sequence.
(Probably we should record and show it with empty time label,...)

If it was the last frame (user already stopped recording), then never after we
will enter MaybeSend and report recorded frames.

[eustas, 2015-01-30 12:43:41]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2015-01-30 12:44:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/888573002/80001

[commit-bot: I haz the power, 2015-01-30 14:20:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-30 14:21:32]

Patchset 5 (id:??) landed as
https://crrev.com/c8d4343f0d213c041541348649ca84ddf2d194df
Cr-Commit-Position: refs/heads/master@{#313914}