Converted VideoFrameCapturer callbacks into a VideoFrameCapturer::Delegate interface.

Converted VideoFrameCapturer callbacks into a VideoFrameCapturer::Delegate interface.

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

[alexeypa (please no reviews), 2012-11-07 19:14:27]

PTAL.

[Wez, 2012-11-07 19:58:29]

The change to VideoFrameCapturer's interface fundamentally looks good to me.

https://chromiumcodereview.appspot.com/11363128/diff/1/remoting/host/video_fr...
File remoting/host/video_frame_capturer.h (right):

https://chromiumcodereview.appspot.com/11363128/diff/1/remoting/host/video_fr...
remoting/host/video_frame_capturer.h:61: virtual void OnCaptureCompleted(
nit: OnCaptureFrameCompleted; I'd like to rename CaptureInvalidRegion to
CaptureFrame in a future CL.

https://chromiumcodereview.appspot.com/11363128/diff/1/remoting/host/video_fr...
remoting/host/video_frame_capturer.h:89: // Called at the beginning of a
capturing session.
nit: |delegate| remain valid until Stop() is called.

[simonmorris, 2012-11-07 20:35:20]

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
File remoting/host/video_frame_capturer_linux.cc (right):

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_linux.cc:311: void
VideoFrameCapturerLinux::CaptureInvalidRegion() {
DCHECK(delegate_ != NULL) ?

Or, looking at the removed line 371, return here if delegate_ is NULL?

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_linux.cc:368: DCHECK(has_xfixes_);
Is it clearly OK to remove the check? It's probably right to check, or DCHECK,
whether delegate_ is NULL.

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
File remoting/host/video_frame_capturer_mac.mm (right):

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_mac.mm:313: void
VideoFrameCapturerMac::CaptureInvalidRegion() {
DCHECK(delegate_ != NULL) ?

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_mac.mm:366: void
VideoFrameCapturerMac::CaptureCursor() {
Return if delegate_ is NULL?

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
File remoting/host/video_frame_capturer_win.cc (right):

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_win.cc:350: void
VideoFrameCapturerWin::CaptureRegion(const SkRegion& region) {
Check whether delegate_ is NULL?

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_win.cc:404: void
VideoFrameCapturerWin::CaptureCursor() {
This should check whether the delegate is NULL if the other platforms'
implementations do.

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_scheduler_...
File remoting/host/video_scheduler_unittest.cc (right):

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_scheduler_...
remoting/host/video_scheduler_unittest.cc:110: SkISize size_;
I think SkISize's default constructor doesn't initialize it: if so, initialize
size_ in the VideoSchedulerTest constructor.

[alexeypa (please no reviews), 2012-11-07 20:54:40]

PTAL.

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
File remoting/host/video_frame_capturer.h (right):

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer.h:61: virtual void OnCaptureCompleted(
On 2012/11/07 19:58:29, Wez wrote:
> nit: OnCaptureFrameCompleted; I'd like to rename CaptureInvalidRegion to
> CaptureFrame in a future CL.

Ack.

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer.h:89: // Called at the beginning of a
capturing session.
On 2012/11/07 19:58:29, Wez wrote:
> nit: |delegate| remain valid until Stop() is called.

Done.

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
File remoting/host/video_frame_capturer_linux.cc (right):

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_linux.cc:311: void
VideoFrameCapturerLinux::CaptureInvalidRegion() {
On 2012/11/07 20:35:21, simonmorris wrote:
> DCHECK(delegate_ != NULL) ?
> 
> Or, looking at the removed line 371, return here if delegate_ is NULL?

No, why? It will crash nicely if |delegate_| is NULL.

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_linux.cc:368: DCHECK(has_xfixes_);
On 2012/11/07 20:35:21, simonmorris wrote:
> Is it clearly OK to remove the check? It's probably right to check, or DCHECK,
> whether delegate_ is NULL.

Yes. It will crash if |delegate_| is NULL. No need for a separate CHECK or
DCHECK. This class is useless if |delegate_| is NULL, so crashing is a good way
to say what went wrong.

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
File remoting/host/video_frame_capturer_mac.mm (right):

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_mac.mm:313: void
VideoFrameCapturerMac::CaptureInvalidRegion() {
On 2012/11/07 20:35:21, simonmorris wrote:
> DCHECK(delegate_ != NULL) ?

Same as before.

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_mac.mm:366: void
VideoFrameCapturerMac::CaptureCursor() {
On 2012/11/07 20:35:21, simonmorris wrote:
> Return if delegate_ is NULL?

|delegate_| cannot be NULL. That check was a development artifact of cursor
shape feature.

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
File remoting/host/video_frame_capturer_win.cc (right):

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_win.cc:350: void
VideoFrameCapturerWin::CaptureRegion(const SkRegion& region) {
On 2012/11/07 20:35:21, simonmorris wrote:
> Check whether delegate_ is NULL?

Same as before.

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_win.cc:404: void
VideoFrameCapturerWin::CaptureCursor() {
On 2012/11/07 20:35:21, simonmorris wrote:
> This should check whether the delegate is NULL if the other platforms'
> implementations do.

Same as before.

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_scheduler_...
File remoting/host/video_scheduler_unittest.cc (right):

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_scheduler_...
remoting/host/video_scheduler_unittest.cc:110: SkISize size_;
On 2012/11/07 20:35:21, simonmorris wrote:
> I think SkISize's default constructor doesn't initialize it: if so, initialize
> size_ in the VideoSchedulerTest constructor.

Done.

[simonmorris, 2012-11-07 21:08:57]

lgtm

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
File remoting/host/video_frame_capturer_linux.cc (right):

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_linux.cc:368: DCHECK(has_xfixes_);
On 2012/11/07 20:54:41, alexeypa wrote:
> On 2012/11/07 20:35:21, simonmorris wrote:
> > Is it clearly OK to remove the check? It's probably right to check, or
DCHECK,
> > whether delegate_ is NULL.
> 
> Yes. It will crash if |delegate_| is NULL. No need for a separate CHECK or
> DCHECK. This class is useless if |delegate_| is NULL, so crashing is a good
way
> to say what went wrong.

There is a check for delegate_ being NULL, so I agree there's no real need
to check it again.

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
File remoting/host/video_frame_capturer_win.cc (right):

http://codereview.chromium.org/11363128/diff/1/remoting/host/video_frame_capt...
remoting/host/video_frame_capturer_win.cc:350: void
VideoFrameCapturerWin::CaptureRegion(const SkRegion& region) {
On 2012/11/07 20:54:41, alexeypa wrote:
> On 2012/11/07 20:35:21, simonmorris wrote:
> > Check whether delegate_ is NULL?
> 
> Same as before.

Stop() sets delegate_ to NULL, so it isn't quite the same as before. But if
you're as happy with crashes as DCHECKs, it's OK.

[commit-bot: I haz the power, 2012-11-07 21:16:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/11363128/11001

[commit-bot: I haz the power, 2012-11-07 22:19:52]

Retried try job too often for step(s) remoting_unittests

[commit-bot: I haz the power, 2012-11-07 22:40:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/11363128/26001

[commit-bot: I haz the power, 2012-11-08 00:57:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/11363128/28002

[commit-bot: I haz the power, 2012-11-08 06:19:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-08 17:13:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/11363128/42005

[commit-bot: I haz the power, 2012-11-08 19:36:16]

Change committed as 166728