Issue 7635005: Properly handle screen recorder shutdown in ChromotingHost.

Issue 7635005: Properly handle screen recorder shutdown in ChromotingHost. (Closed)

Created:
9 years, 4 months ago by Sergey Ulanov

Modified:
9 years, 4 months ago

Reviewers:
dmac, Wez

CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, Paweł Hajdan Jr., dmaclach+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, ajwong+watch_chromium.org, sergeyu+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

Description

Properly handle screen recorder shutdown in ChromotingHost. BUG=91620 TEST=Host doesn't crash when shutting down. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96645

Patch Set 1 : - #

Total comments: 6

Patch Set 2 : - #

Created: 9 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+71 lines, -47 lines)			Patch
M	remoting/host/chromoting_host.h	View	1	2 chunks	+9 lines, -0 lines	0 comments	Download
M	remoting/host/chromoting_host.cc	View	1	4 chunks	+35 lines, -7 lines	0 comments	Download
M	remoting/host/screen_recorder.h	View		5 chunks	+4 lines, -5 lines	0 comments	Download
M	remoting/host/screen_recorder.cc	View		6 chunks	+18 lines, -31 lines	0 comments	Download
M	remoting/host/screen_recorder_unittest.cc	View	1	3 chunks	+5 lines, -4 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

dmac

http://codereview.chromium.org/7635005/diff/2001/remoting/host/chromoting_host.cc File remoting/host/chromoting_host.cc (right): http://codereview.chromium.org/7635005/diff/2001/remoting/host/chromoting_host.cc#newcode548 remoting/host/chromoting_host.cc:548: recorder_->Stop(base::Bind(&ChromotingHost::OnScreenRecorderStopped, this)); should we check recorder_.get() != NULL? Could ...

9 years, 4 months ago (2011-08-11 23:56:33 UTC) #3

Sergey Ulanov

9 years, 4 months ago (2011-08-12 00:29:48 UTC) #4

http://codereview.chromium.org/7635005/diff/2001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/7635005/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:548:
recorder_->Stop(base::Bind(&ChromotingHost::OnScreenRecorderStopped, this));
On 2011/08/11 23:56:34, dmac wrote:
> should we check recorder_.get() != NULL? Could we get in here again?

Done.

http://codereview.chromium.org/7635005/diff/2001/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/7635005/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:218: // Number of screen recorders that are
currently being stopped.
On 2011/08/11 23:56:34, dmac wrote:
> Maybe expand this comment to explain the case that you explained to me? eg we
> are we worried about this?

Done.

http://codereview.chromium.org/7635005/diff/2001/remoting/host/screen_recorde...
File remoting/host/screen_recorder_unittest.cc (right):

http://codereview.chromium.org/7635005/diff/2001/remoting/host/screen_recorde...
remoting/host/screen_recorder_unittest.cc:10: #include
"remoting/host/screen_recorder.h"
On 2011/08/11 23:56:34, dmac wrote:
> NIT: this include should be on top...

Done.

Expand Messages | Collapse Messages