Add a done task to ScreenRecorder::Stop()

Add a done task to ScreenRecorder::Stop()

ScreenRecorder::Stop() need a done task to indicate it is actually paused.
This is needed so that we can shutdown threads safely.

BUG=69997
TEST=remoting_unittests --gtest_filter=ScreenRecorder.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72188

[Alpha Left Google, 2011-01-19 02:25:57]

Please take a look of this code. This adds a pause task to ScreenRecorder.
However I'm also changing ChromotingHost at the same time so leaving Pause
invocation as Pause(NULL) so those two patches will be submitted together
instead of this going in alone.

[Alpha Left Google, 2011-01-19 02:44:13]

hm.. fixed a bug in my code.. test didn't catch that..

[Alpha Left Google, 2011-01-19 23:50:19]

This is ready for review. I changed pause to stop and so now it's safe to wait
for stop task, release ScreenRecorder and terminates threads.

[Sergey Ulanov, 2011-01-20 19:14:17]

http://codereview.chromium.org/6282006/diff/7001/remoting/host/screen_recorde...
File remoting/host/screen_recorder.cc (right):

http://codereview.chromium.org/6282006/diff/7001/remoting/host/screen_recorde...
remoting/host/screen_recorder.cc:152: if (task) {
Call DoCompleteStop()?

http://codereview.chromium.org/6282006/diff/7001/remoting/host/screen_recorde...
remoting/host/screen_recorder.cc:270: // more than one connection.
I think we should just remove support for parallel connections

http://codereview.chromium.org/6282006/diff/7001/remoting/host/screen_recorde...
remoting/host/screen_recorder.cc:283: if (network_stopped_)
Do we really need network_stopped_? It should be safe to post
DoFinishOneRecording task on capture_loop_, it doesn't do anything when started_
== false. If you remove network_stopped_, then we also don't need
DoStopOnNetworkThread, and the whole shutdown process becomes much simpler.

Is the problem that capture_loop_ may not exist at this point? Maybe we should
use MessageLoopProxy instead of MessageLoop?

http://codereview.chromium.org/6282006/diff/7001/remoting/host/screen_recorde...
remoting/host/screen_recorder.cc:348: // TODO(hclam): Invalid the full screen if
there is a new connection added.
Invalidate

http://codereview.chromium.org/6282006/diff/7001/remoting/host/screen_recorder.h
File remoting/host/screen_recorder.h (right):

http://codereview.chromium.org/6282006/diff/7001/remoting/host/screen_recorde...
remoting/host/screen_recorder.h:81: void Stop(Task* task);
I think we should call it |done_task|, or |done|, or |stop_done|. |task| is not
very meaningful.

http://codereview.chromium.org/6282006/diff/7001/remoting/protocol/buffered_s...
File remoting/protocol/buffered_socket_writer.cc (right):

http://codereview.chromium.org/6282006/diff/7001/remoting/protocol/buffered_s...
remoting/protocol/buffered_socket_writer.cc:73: // Don't try to write if there
is another write pending.
Remove extra spaces

http://codereview.chromium.org/6282006/diff/7001/remoting/protocol/buffered_s...
File remoting/protocol/buffered_socket_writer.h (left):

http://codereview.chromium.org/6282006/diff/7001/remoting/protocol/buffered_s...
remoting/protocol/buffered_socket_writer.h:93: scoped_ptr<WriteFailedCallback>
write_failed_callback_;
Why do you need to remove this?

[Alpha Left Google, 2011-01-20 19:24:17]

Addressing style problems later.

http://codereview.chromium.org/6282006/diff/7001/remoting/host/screen_recorde...
File remoting/host/screen_recorder.cc (right):

http://codereview.chromium.org/6282006/diff/7001/remoting/host/screen_recorde...
remoting/host/screen_recorder.cc:270: // more than one connection.
On 2011/01/20 19:14:17, sergeyu wrote:
> I think we should just remove support for parallel connections

I'd leave the decision later, not really related to this patch.

http://codereview.chromium.org/6282006/diff/7001/remoting/host/screen_recorde...
remoting/host/screen_recorder.cc:283: if (network_stopped_)
On 2011/01/20 19:14:17, sergeyu wrote:
> Do we really need network_stopped_? It should be safe to post
> DoFinishOneRecording task on capture_loop_, it doesn't do anything when
started_
> == false. If you remove network_stopped_, then we also don't need
> DoStopOnNetworkThread, and the whole shutdown process becomes much simpler.
> 
> Is the problem that capture_loop_ may not exist at this point? Maybe we should
> use MessageLoopProxy instead of MessageLoop?

The problem that needs network_stopped_ here is that when we are shutting down
capture loop, network loop can receive a callback and needs to post back to the
capture loop and that causes race conditions in MessageLoop code.

I think we've gone through this argument before, the reason that we're not using
MessageLoopProxy was because we thought we can order destruction nicely and
doesn't need refcounting. Since we made that decision in the first place we
should try to make it happen.

http://codereview.chromium.org/6282006/diff/7001/remoting/protocol/buffered_s...
File remoting/protocol/buffered_socket_writer.h (left):

http://codereview.chromium.org/6282006/diff/7001/remoting/protocol/buffered_s...
remoting/protocol/buffered_socket_writer.h:93: scoped_ptr<WriteFailedCallback>
write_failed_callback_;
On 2011/01/20 19:14:17, sergeyu wrote:
> Why do you need to remove this?

Going through I found this is not used at all so I cleaned it up at the same
time.

[Alpha Left Google, 2011-01-20 19:32:08]

Fixed style problems too.

http://codereview.chromium.org/6282006/diff/7001/remoting/host/screen_recorde...
File remoting/host/screen_recorder.cc (right):

http://codereview.chromium.org/6282006/diff/7001/remoting/host/screen_recorde...
remoting/host/screen_recorder.cc:152: if (task) {
On 2011/01/20 19:14:17, sergeyu wrote:
> Call DoCompleteStop()?

Done.

http://codereview.chromium.org/6282006/diff/7001/remoting/host/screen_recorder.h
File remoting/host/screen_recorder.h (right):

http://codereview.chromium.org/6282006/diff/7001/remoting/host/screen_recorde...
remoting/host/screen_recorder.h:81: void Stop(Task* task);
On 2011/01/20 19:14:17, sergeyu wrote:
> I think we should call it |done_task|, or |done|, or |stop_done|. |task| is
not
> very meaningful.

Done.

http://codereview.chromium.org/6282006/diff/7001/remoting/protocol/buffered_s...
File remoting/protocol/buffered_socket_writer.cc (right):

http://codereview.chromium.org/6282006/diff/7001/remoting/protocol/buffered_s...
remoting/protocol/buffered_socket_writer.cc:73: // Don't try to write if there
is another write pending.
On 2011/01/20 19:14:17, sergeyu wrote:
> Remove extra spaces

weird... done.

[Sergey Ulanov, 2011-01-20 20:00:06]

LGTM

http://codereview.chromium.org/6282006/diff/15001/remoting/host/screen_record...
File remoting/host/screen_recorder.cc (right):

http://codereview.chromium.org/6282006/diff/15001/remoting/host/screen_record...
remoting/host/screen_recorder.cc:346: // TODO(hclam): Invalid the full screen if
there is a new connection added.
s/Invalid/Invalidate/

[awong, 2011-01-20 20:14:00]

I got acouple more comments...

http://codereview.chromium.org/6282006/diff/21001/remoting/host/screen_record...
File remoting/host/screen_recorder.cc (right):

http://codereview.chromium.org/6282006/diff/21001/remoting/host/screen_record...
remoting/host/screen_recorder.cc:69: void ScreenRecorder::Stop(Task* done_task)
{
Can we fix this combine the all the Stop/DoStop, SetMaxRate/DoSetMaxRate
functions?

Can be a separate CL, but we should fix the convention.

http://codereview.chromium.org/6282006/diff/21001/remoting/host/screen_record...
remoting/host/screen_recorder.cc:218: if (!started_)
Is this a normal condition?

If so, should we warn?

http://codereview.chromium.org/6282006/diff/21001/remoting/host/screen_record...
remoting/host/screen_recorder.cc:252: if (network_stopped_) {
This class's comment needs a description of the various states that it can be
in, and what that logically means.  We don't need to list out exactly which
functions are allowed, but having the states listed out would help

http://codereview.chromium.org/6282006/diff/21001/remoting/host/screen_record...
File remoting/host/screen_recorder_unittest.cc (right):

http://codereview.chromium.org/6282006/diff/21001/remoting/host/screen_record...
remoting/host/screen_recorder_unittest.cc:40: delete arg0;
I hate Chromium Tasks. :(

http://codereview.chromium.org/6282006/diff/21001/remoting/host/screen_record...
remoting/host/screen_recorder_unittest.cc:145: TEST_F(ScreenRecorderTest,
StartAndStop) {
What's the invariant that this is testing?  Can you add a 1-line comment above
the TEST_F?

[Sergey Ulanov, 2011-01-20 20:56:26]

http://codereview.chromium.org/6282006/diff/21001/remoting/protocol/buffered_...
File remoting/protocol/buffered_socket_writer.h (left):

http://codereview.chromium.org/6282006/diff/21001/remoting/protocol/buffered_...
remoting/protocol/buffered_socket_writer.h:93: scoped_ptr<WriteFailedCallback>
write_failed_callback_;
After looking at this again: This is supposed to be set in Init(). It's better
to fix Init() so that it sets this callback instead of removing it completely.

[Alpha Left Google, 2011-01-20 22:48:47]

http://codereview.chromium.org/6282006/diff/21001/remoting/host/screen_record...
File remoting/host/screen_recorder.cc (right):

http://codereview.chromium.org/6282006/diff/21001/remoting/host/screen_record...
remoting/host/screen_recorder.cc:69: void ScreenRecorder::Stop(Task* done_task)
{
On 2011/01/20 20:14:00, awong wrote:
> Can we fix this combine the all the Stop/DoStop, SetMaxRate/DoSetMaxRate
> functions?
> 
> Can be a separate CL, but we should fix the convention.

will do separately.

http://codereview.chromium.org/6282006/diff/21001/remoting/host/screen_record...
remoting/host/screen_recorder.cc:218: if (!started_)
On 2011/01/20 20:14:00, awong wrote:
> Is this a normal condition?
> 
> If so, should we warn?

You're right, this should be a DCHECK. The check in the next method is valid
though.

http://codereview.chromium.org/6282006/diff/21001/remoting/host/screen_record...
remoting/host/screen_recorder.cc:252: if (network_stopped_) {
On 2011/01/20 20:14:00, awong wrote:
> This class's comment needs a description of the various states that it can be
> in, and what that logically means.  We don't need to list out exactly which
> functions are allowed, but having the states listed out would help

Done.

http://codereview.chromium.org/6282006/diff/21001/remoting/host/screen_record...
File remoting/host/screen_recorder_unittest.cc (right):

http://codereview.chromium.org/6282006/diff/21001/remoting/host/screen_record...
remoting/host/screen_recorder_unittest.cc:145: TEST_F(ScreenRecorderTest,
StartAndStop) {
On 2011/01/20 20:14:00, awong wrote:
> What's the invariant that this is testing?  Can you add a 1-line comment above
> the TEST_F?

Done.

http://codereview.chromium.org/6282006/diff/21001/remoting/protocol/buffered_...
File remoting/protocol/buffered_socket_writer.h (left):

http://codereview.chromium.org/6282006/diff/21001/remoting/protocol/buffered_...
remoting/protocol/buffered_socket_writer.h:93: scoped_ptr<WriteFailedCallback>
write_failed_callback_;
On 2011/01/20 20:56:26, sergeyu wrote:
> After looking at this again: This is supposed to be set in Init(). It's better
> to fix Init() so that it sets this callback instead of removing it completely.

Removed from this patch.

[Alpha Left Google, 2011-01-21 18:58:44]

ping.

[awong, 2011-01-21 19:09:40]

One more comment.

http://codereview.chromium.org/6282006/diff/26001/remoting/host/screen_record...
File remoting/host/screen_recorder.h (right):

http://codereview.chromium.org/6282006/diff/26001/remoting/host/screen_record...
remoting/host/screen_recorder.h:65: // |started_| - If this is set to false
there should be no activity on the
started_ and network_stopped_ are negations of each other.  One says the thread
should be used, and one says the thread should not.

Can they both be "started" or "stopped"?

Also, maybe capture_started_ instead of just started_

[Alpha Left Google, 2011-01-21 19:18:39]

On 2011/01/21 19:09:40, awong wrote:
> One more comment.
> 
>
http://codereview.chromium.org/6282006/diff/26001/remoting/host/screen_record...
> File remoting/host/screen_recorder.h (right):
> 
>
http://codereview.chromium.org/6282006/diff/26001/remoting/host/screen_record...
> remoting/host/screen_recorder.h:65: // |started_| - If this is set to false
> there should be no activity on the
> started_ and network_stopped_ are negations of each other.  One says the
thread
> should be used, and one says the thread should not.
> 
> Can they both be "started" or "stopped"?
> 
> Also, maybe capture_started_ instead of just started_

renamed it to capture_started but leave it as started. One says capture thread
may be used, other says network thread must not be used, so negation doesn't
work in this case.

[Alpha Left Google, 2011-01-21 19:37:28]

changed variable names and fixed comments.

[awong, 2011-01-21 19:42:19]

LGTM