Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.

BUG=webrtc:5950
Committed: https://crrev.com/5d910286e1e802245eec2cd76f5f8bef6e671e51
Cr-Commit-Position: refs/heads/master@{#13058}

[Sergey Ulanov, 2016-05-17 22:55:37]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-05-17 22:56:32]

Description was changed from

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame pointers.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.
==========

to

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame pointers.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.
==========

[Sergey Ulanov, 2016-05-17 22:56:58]

The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-17 22:57:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1988783003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1988783003/20001

[Sergey Ulanov, 2016-05-17 22:57:15]

Description was changed from

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame pointers.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.
==========

to

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.
==========

[Sergey Ulanov, 2016-05-17 22:57:34]

sergeyu@chromium.org changed reviewers:
+ wez@chromium.org

[commit-bot: I haz the power, 2016-05-17 22:59:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-17 22:59:38]

Dry run: Try jobs failed on following builders:
  android_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/13396)
  android_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/13418)

[Wez, 2016-05-18 01:29:51]

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/cropped_desktop_frame.cc (right):

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/cropped_desktop_frame.cc:47: frame_ =
std::move(frame);
Why does this need to be assignment, and not initialization?

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/desktop_capturer.h (right):

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/desktop_capturer.h:41: // above pure virtual.
Is there a bug # for that work, that you can refer to here?

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/desktop_frame_win.h (right):

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/desktop_frame_win.h:30: Create(DesktopSize size,
SharedMemoryFactory* shared_memory_factory, HDC hdc);
Wrapping looks weird; is this what cl format did?

If this is the correct wrapping then please put a blank line between the dtor
and this definition, to aid readability.

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/screen_capturer_mock_objects.h (right):

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/screen_capturer_mock_objects.h:40:
void(std::unique_ptr<DesktopFrame>* frame));
In our other tests we un-wrap the |frame| so that this would be DesktopFrame* -
what's the rationale for passing unique_ptr<>* instead?

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/screen_capturer_x11.cc (right):

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/screen_capturer_x11.cc:257: (!differ_.get() ||
(differ_->width() != frame->size().width()) ||
nit: !differ_

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/shared_desktop_frame.cc (right):

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/shared_desktop_frame.cc:20: class
SharedDesktopFrame::Core {
nit: While you're here... couldn't this effectively be replaced with:

rtc::RefCountedObject<std::unique_ptr<DesktopFrame>>

Or would that lead to too much get() ugliness?

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc (right):

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc:58:
ReleaseDC(nullptr, desktop_dc_);
As per header comment, technically this should remain NULL.

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc:215: abort();
How is if (!foo) abort() different from assert(foo)?

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc:242:
assert(memory_dc_);
Why assert here, given that we already check that they're non-null earlier on?

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.h (right):

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.h:72: HDC memory_dc_
= nullptr;
nit: Technically these should still be NULL, not nullptr, since that's what
Windows defines (same for HMODULE, below).

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc
(right):

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:190: BOOL
result = SetWindowPos(magnifier_window_, nullptr, rect.left(),
nit: Technically should stay NULL.

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.h (right):

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.h:114: HWND
excluded_window_ = nullptr;
nit: See comments elsewhere re leaving Windows handles set to NULL.

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/window_capturer_mac.mm (right):

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/window_capturer_mac.mm:181:
callback_->OnCaptureCompleted(std::unique_ptr<DesktopFrame>());
Why not using nullptr here?

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/window_capturer_x11.cc (right):

https://codereview.webrtc.org/1988783003/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/window_capturer_x11.cc:277:
callback_->OnCaptureCompleted(std::unique_ptr<DesktopFrame>());
Why not using nullptr here?

[Sergey Ulanov, 2016-05-31 12:02:49]

Addressed all comments. Also added DesktopCapture::Result to return result for
each Capture() call.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/cropped_desktop_frame.cc (right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/cropped_desktop_frame.cc:47: frame_ =
std::move(frame);
On 2016/05/18 01:29:50, Wez wrote:
> Why does this need to be assignment, and not initialization?

initializers don't guarantee order in which arguments are evaluated, but |frame|
is used in the DesktopFrame() initialization above. Assigning it here guarantees
that |frame| is valid when evaluating arguments for the DesktopFrame()
constructor.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/desktop_capturer.h (right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/desktop_capturer.h:41: // above pure virtual.
On 2016/05/18 01:29:50, Wez wrote:
> Is there a bug # for that work, that you can refer to here?

Opened crbug.com/webrtc/5950

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/desktop_frame_win.h (right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/desktop_frame_win.h:30: Create(DesktopSize size,
SharedMemoryFactory* shared_memory_factory, HDC hdc);
On 2016/05/18 01:29:50, Wez wrote:
> Wrapping looks weird; is this what cl format did?
yes

> If this is the correct wrapping then please put a blank line between the dtor
> and this definition, to aid readability.
done.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/screen_capturer_mock_objects.h (right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/screen_capturer_mock_objects.h:40:
void(std::unique_ptr<DesktopFrame>* frame));
On 2016/05/18 01:29:50, Wez wrote:
> In our other tests we un-wrap the |frame| so that this would be DesktopFrame*
-
> what's the rationale for passing unique_ptr<>* instead?

When passing unique_ptr<>* the test doesn't have to delete the frame if it
doesn't need it, which makes the tests simpler.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/screen_capturer_x11.cc (right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/screen_capturer_x11.cc:257: (!differ_.get() ||
(differ_->width() != frame->size().width()) ||
On 2016/05/18 01:29:50, Wez wrote:
> nit: !differ_

Done.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/shared_desktop_frame.cc (right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/shared_desktop_frame.cc:20: class
SharedDesktopFrame::Core {
On 2016/05/18 01:29:50, Wez wrote:
> nit: While you're here... couldn't this effectively be replaced with:
> 
> rtc::RefCountedObject<std::unique_ptr<DesktopFrame>>
> 
> Or would that lead to too much get() ugliness?

Done.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc (right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc:58:
ReleaseDC(nullptr, desktop_dc_);
On 2016/05/18 01:29:50, Wez wrote:
> As per header comment, technically this should remain NULL.

Done.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc:215: abort();
On 2016/05/18 01:29:50, Wez wrote:
> How is if (!foo) abort() different from assert(foo)?

assert() is enabled only in debug builds.

capturers need to be updated to use RTC_[D]CHECK() instead of assert()/abort(),
but I'd prefer doing it in a separate CL.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc:242:
assert(memory_dc_);
On 2016/05/18 01:29:50, Wez wrote:
> Why assert here, given that we already check that they're non-null earlier on?

It's not checked in this function, so I think this asserts are useful.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.h (right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.h:72: HDC memory_dc_
= nullptr;
On 2016/05/18 01:29:50, Wez wrote:
> nit: Technically these should still be NULL, not nullptr, since that's what
> Windows defines (same for HMODULE, below).

Done.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc
(right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:190: BOOL
result = SetWindowPos(magnifier_window_, nullptr, rect.left(),
On 2016/05/18 01:29:50, Wez wrote:
> nit: Technically should stay NULL.

Done.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.h (right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.h:114: HWND
excluded_window_ = nullptr;
On 2016/05/18 01:29:50, Wez wrote:
> nit: See comments elsewhere re leaving Windows handles set to NULL.

Done.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/window_capturer_mac.mm (right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/window_capturer_mac.mm:181:
callback_->OnCaptureCompleted(std::unique_ptr<DesktopFrame>());
On 2016/05/18 01:29:51, Wez wrote:
> Why not using nullptr here?

because OnCaptureCompleted(nullptr) would call wrong constructor.

Anyway I've replaced the OnCaptureCompleted() with a new OnCaptureResult(). This
avoids the problem above and should fix compilation issues on windows bots.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/window_capturer_x11.cc (right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/window_capturer_x11.cc:277:
callback_->OnCaptureCompleted(std::unique_ptr<DesktopFrame>());
On 2016/05/18 01:29:51, Wez wrote:
> Why not using nullptr here?

same here. Replaced with OnCaptureResult(nullptr)

[Sergey Ulanov, 2016-05-31 12:03:16]

Description was changed from

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.
==========

to

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.

BUG=webrtc:5950
==========

[Wez, 2016-06-01 21:29:05]

LGTM w/ nits.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/cropped_desktop_frame.cc (right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/cropped_desktop_frame.cc:47: frame_ =
std::move(frame);
On 2016/05/31 12:02:48, Sergey Ulanov wrote:
> On 2016/05/18 01:29:50, Wez wrote:
> > Why does this need to be assignment, and not initialization?
> 
> initializers don't guarantee order in which arguments are evaluated, but
|frame|
> is used in the DesktopFrame() initialization above. Assigning it here
guarantees
> that |frame| is valid when evaluating arguments for the DesktopFrame()
> constructor.

nit: In this case DesktopFrame() (immediate base class) should be initialized
prior to |frame_| (non-static member of this derived class) so the order is
well-defined.

Nonetheless, explicit assignment in the ctor body is fine by me.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/screen_capturer_mock_objects.h (right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/screen_capturer_mock_objects.h:40:
void(std::unique_ptr<DesktopFrame>* frame));
On 2016/05/31 12:02:48, Sergey Ulanov wrote:
> On 2016/05/18 01:29:50, Wez wrote:
> > In our other tests we un-wrap the |frame| so that this would be
DesktopFrame*
> -
> > what's the rationale for passing unique_ptr<>* instead?
> 
> When passing unique_ptr<>* the test doesn't have to delete the frame if it
> doesn't need it, which makes the tests simpler. 

Ah, very nice. Sounds like we should follow this pattern in new tests in place
of the old bare-pointer approach.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc (right):

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc:215: abort();
On 2016/05/31 12:02:49, Sergey Ulanov wrote:
> On 2016/05/18 01:29:50, Wez wrote:
> > How is if (!foo) abort() different from assert(foo)?
> 
> assert() is enabled only in debug builds.
> 
> capturers need to be updated to use RTC_[D]CHECK() instead of
assert()/abort(),
> but I'd prefer doing it in a separate CL.

Acknowledged.

https://codereview.chromium.org/1988783003/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc:242:
assert(memory_dc_);
On 2016/05/31 12:02:49, Sergey Ulanov wrote:
> On 2016/05/18 01:29:50, Wez wrote:
> > Why assert here, given that we already check that they're non-null earlier
on?
> 
> It's not checked in this function, so I think this asserts are useful.

Acknowledged.

https://codereview.chromium.org/1988783003/diff/40001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/desktop_capturer.h (right):

https://codereview.chromium.org/1988783003/diff/40001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/desktop_capturer.h:33: // There was a temporary
error. The caller should retry capting again.
typo: capting

nit: IIUC what you mean here is that the caller should continue requesting
captures, in the expectation that it will eventually recover - suggesting
"retry" implies that the call should be retried immediately for some reason,
which I don't think is what you want?

Is this even an error condition, really? Or is it just a special form of SUCCESS
in which there is no actual pixel data?

https://codereview.chromium.org/1988783003/diff/40001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/desktop_capturer.h:44: // Called after a frame
has been captured. |frame| is not nullptr iff
nit: I've been told off before for using "iff". ;)

https://codereview.chromium.org/1988783003/diff/40001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/shared_desktop_frame.cc (right):

https://codereview.chromium.org/1988783003/diff/40001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/shared_desktop_frame.cc:26: new
SharedDesktopFrame(new Core(desktop_frame.release())));
nit: Can you move() here, since you're going from unique_ptr to unique_ptr?

https://codereview.chromium.org/1988783003/diff/40001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/shared_desktop_frame.cc:50: :
DesktopFrame(core->get()->size(),
nit: Could you use *core->foo() here? :P

[Sergey Ulanov, 2016-06-03 08:14:31]

https://codereview.chromium.org/1988783003/diff/40001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/desktop_capturer.h (right):

https://codereview.chromium.org/1988783003/diff/40001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/desktop_capturer.h:33: // There was a temporary
error. The caller should retry capting again.
On 2016/06/01 21:29:05, Wez wrote:
> typo: capting
> 
> nit: IIUC what you mean here is that the caller should continue requesting
> captures, in the expectation that it will eventually recover - suggesting
> "retry" implies that the call should be retried immediately for some reason,
> which I don't think is what you want?

Good point. Reworded the comment a bit.

> 
> Is this even an error condition, really? Or is it just a special form of
SUCCESS
> in which there is no actual pixel data?

I thinks it's still better to call it error, as there is no frame in the result.
It's somewhat similar to net::ERR_IO_PENDING.

https://codereview.chromium.org/1988783003/diff/40001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/desktop_capturer.h:44: // Called after a frame
has been captured. |frame| is not nullptr iff
On 2016/06/01 21:29:05, Wez wrote:
> nit: I've been told off before for using "iff". ;)

Done.

https://codereview.chromium.org/1988783003/diff/40001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/shared_desktop_frame.cc (right):

https://codereview.chromium.org/1988783003/diff/40001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/shared_desktop_frame.cc:26: new
SharedDesktopFrame(new Core(desktop_frame.release())));
On 2016/06/01 21:29:05, Wez wrote:
> nit: Can you move() here, since you're going from unique_ptr to unique_ptr?

Nope, it doesn't work because rtc::RefCountedObject constructor doesn't pass it
the argument as rvalue.
I'll try getting that fixed in a separate CL for RefCountedObject

https://codereview.chromium.org/1988783003/diff/40001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/shared_desktop_frame.cc:50: :
DesktopFrame(core->get()->size(),
On 2016/06/01 21:29:05, Wez wrote:
> nit: Could you use *core->foo() here? :P

Changed to (*core)->foo(). The parentheses are required because -> operator
takes precedence

[Sergey Ulanov, 2016-06-03 08:14:45]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-06-03 08:14:45]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1988783003/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-06-03 08:14:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1988783003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1988783003/60001

[commit-bot: I haz the power, 2016-06-03 08:18:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 08:18:42]

Try jobs failed on following builders:
  mac_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/11864)

[Sergey Ulanov, 2016-06-03 09:04:44]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-06-03 09:04:44]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.webrtc.org/1988783003/#ps80001 (title:
" ")

[commit-bot: I haz the power, 2016-06-03 09:04:48]

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

[commit-bot: I haz the power, 2016-06-03 09:06:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 09:06:59]

Try jobs failed on following builders:
  mac_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/11867)
  mac_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/3697)
  win_x64_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_rel/builds/9780)

[Sergey Ulanov, 2016-06-03 09:17:26]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-06-03 09:17:26]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.webrtc.org/1988783003/#ps100001 (title:
" ")

[commit-bot: I haz the power, 2016-06-03 09:17:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1988783003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1988783003/100001

[commit-bot: I haz the power, 2016-06-03 09:19:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 09:19:30]

Try jobs failed on following builders:
  mac_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/11868)
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/15656)

[Sergey Ulanov, 2016-06-03 09:35:43]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-06-03 09:35:44]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.webrtc.org/1988783003/#ps120001 (title:
" ")

[commit-bot: I haz the power, 2016-06-03 09:35:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1988783003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1988783003/120001

[commit-bot: I haz the power, 2016-06-03 09:39:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 09:39:12]

Try jobs failed on following builders:
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/15657)
  win_x64_clang_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)

[Sergey Ulanov, 2016-06-03 10:55:41]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-06-03 10:55:41]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.webrtc.org/1988783003/#ps140001 (title:
" ")

[commit-bot: I haz the power, 2016-06-03 10:55:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1988783003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1988783003/140001

[commit-bot: I haz the power, 2016-06-03 12:15:28]

Description was changed from

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.

BUG=webrtc:5950
==========

to

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.

BUG=webrtc:5950
==========

[commit-bot: I haz the power, 2016-06-03 12:15:29]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-06-03 12:15:37]

Description was changed from

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.

BUG=webrtc:5950
==========

to

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.

BUG=webrtc:5950

Committed: https://crrev.com/4a627a8c13554d12412cabb8f751caee6e61ee32
Cr-Commit-Position: refs/heads/master@{#13032}
==========

[commit-bot: I haz the power, 2016-06-03 12:15:38]

Patchset 7 (id:??) landed as
https://crrev.com/4a627a8c13554d12412cabb8f751caee6e61ee32
Cr-Commit-Position: refs/heads/master@{#13032}

[Sergey Ulanov, 2016-06-03 12:56:30]

A revert of this CL (patchset #7 id:140001) has been created in
https://codereview.webrtc.org/2030333003/ by sergeyu@chromium.org.

The reason for reverting is: Broke chromium builder.

[Sergey Ulanov, 2016-06-03 12:59:22]

Description was changed from

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.

BUG=webrtc:5950

Committed: https://crrev.com/4a627a8c13554d12412cabb8f751caee6e61ee32
Cr-Commit-Position: refs/heads/master@{#13032}
==========

to

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.

BUG=webrtc:5950

Committed: https://crrev.com/4a627a8c13554d12412cabb8f751caee6e61ee32
Cr-Commit-Position: refs/heads/master@{#13032}
==========

[Sergey Ulanov, 2016-06-03 13:00:06]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-06-03 13:00:06]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.webrtc.org/1988783003/#ps160001 (title:
" ")

[commit-bot: I haz the power, 2016-06-03 13:00:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1988783003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1988783003/160001

[commit-bot: I haz the power, 2016-06-03 13:02:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 13:02:55]

Try jobs failed on following builders:
  linux_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/15694)
  linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)

[Sergey Ulanov, 2016-06-03 13:15:08]

Patchset #8 (id:160001) has been deleted

[Sergey Ulanov, 2016-06-07 23:09:12]

Description was changed from

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.

BUG=webrtc:5950

Committed: https://crrev.com/4a627a8c13554d12412cabb8f751caee6e61ee32
Cr-Commit-Position: refs/heads/master@{#13032}
==========

to

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.

BUG=webrtc:5950
==========

[Sergey Ulanov, 2016-06-07 23:09:24]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2016-06-07 23:09:28]

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

[commit-bot: I haz the power, 2016-06-07 23:42:03]

Description was changed from

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.

BUG=webrtc:5950
==========

to

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.

BUG=webrtc:5950
==========

[commit-bot: I haz the power, 2016-06-07 23:42:05]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-06-07 23:42:14]

Description was changed from

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.

BUG=webrtc:5950
==========

to

==========
Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

Previously raw pointers were used for owned DesktopFrame instances.
Updated all screen and window capturer implementations to use
std::unique_ptr<>.

Also includes some other cleanups in the capturers:
 - s/NULL/nullptr
 - moved default initializers to class definition.

BUG=webrtc:5950

Committed: https://crrev.com/5d910286e1e802245eec2cd76f5f8bef6e671e51
Cr-Commit-Position: refs/heads/master@{#13058}
==========

[commit-bot: I haz the power, 2016-06-07 23:42:17]

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