Delete ShallowCopy, in favor of copy construction and assignment.

Delete ShallowCopy, in favor of copy construction and assignment.

BUG=webrtc:6591
Committed: https://crrev.com/67dca9f12ef9e43739950c003aea882ec67cac2b
Cr-Commit-Position: refs/heads/master@{#14853}

[nisse-webrtc, 2016-10-24 09:51:24]

nisse@webrtc.org changed reviewers:
+ magjed@webrtc.org, tommi@webrtc.org

[nisse-webrtc, 2016-10-24 09:51:24]

Hi,

in particular, do the declarations of the default move and copy operations look
right?

[tommi, 2016-10-24 10:52:17]

lgtm % one request

https://codereview.chromium.org/2443123002/diff/1/webrtc/common_video/video_f...
File webrtc/common_video/video_frame.cc (right):

https://codereview.chromium.org/2443123002/diff/1/webrtc/common_video/video_f...
webrtc/common_video/video_frame.cc:96: void VideoFrame::ShallowCopy(const
VideoFrame& videoFrame) {
change variable name to video_frame? (or even just frame)

https://codereview.chromium.org/2443123002/diff/1/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.chromium.org/2443123002/diff/1/webrtc/video_frame.h#newcode83
webrtc/video_frame.h:83: void ShallowCopy(const VideoFrame& videoFrame);
video_frame or frame

[nisse-chromium (ooo August 14), 2016-10-24 11:01:19]

nisse@chromium.org changed reviewers:
+ nisse@chromium.org

[nisse-chromium (ooo August 14), 2016-10-24 11:01:20]

https://codereview.chromium.org/2443123002/diff/1/webrtc/common_video/video_f...
File webrtc/common_video/video_frame.cc (right):

https://codereview.chromium.org/2443123002/diff/1/webrtc/common_video/video_f...
webrtc/common_video/video_frame.cc:96: void VideoFrame::ShallowCopy(const
VideoFrame& videoFrame) {
On 2016/10/24 10:52:17, tommi (webrtc) wrote:
> change variable name to video_frame? (or even just frame)

Done.

https://codereview.chromium.org/2443123002/diff/1/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.chromium.org/2443123002/diff/1/webrtc/video_frame.h#newcode83
webrtc/video_frame.h:83: void ShallowCopy(const VideoFrame& videoFrame);
On 2016/10/24 10:52:17, tommi (webrtc) wrote:
> video_frame or frame

Done.

[magjed_webrtc, 2016-10-25 07:32:11]

https://codereview.webrtc.org/2443123002/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/test/video_processing_unittest.cc (right):

https://codereview.webrtc.org/2443123002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/video_processing_unittest.cc:197: out_frame
= vpm->PreprocessFrame(source);
We are completely ignoring the input value of |out_frame| and just overwriting
the pointer here. This looks broken.

https://codereview.webrtc.org/2443123002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/video_processing_unittest.cc:247: if
(out_frame == NULL)
Something is wrong. |out_frame| will always be null here, regardless of what
PreprocessFrameAndVerify does.

https://codereview.webrtc.org/2443123002/diff/20001/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.webrtc.org/2443123002/diff/20001/webrtc/video_frame.h#newc...
webrtc/video_frame.h:82: // Delete after applications are updated.
Is anyone else really using this function?

[nisse-webrtc, 2016-10-25 08:01:33]

nisse@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[nisse-webrtc, 2016-10-25 08:01:34]

+mflodman. magjed noticed that the handling of the out_frame argument of
PreprocessFrameAndVerify appear broken. It was changed in your refactoring cl
https://codereview.webrtc.org/1482913003. What do you suggest? I think the
simplest fix is to change the argument back to a pointer to pointer. 

But that can be a separate cl. Since the TestSize function has returned
prematurely for many months, we might discover additional breakage.

https://codereview.webrtc.org/2443123002/diff/20001/webrtc/modules/video_proc...
File webrtc/modules/video_processing/test/video_processing_unittest.cc (right):

https://codereview.webrtc.org/2443123002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/video_processing_unittest.cc:197: out_frame
= vpm->PreprocessFrame(source);
On 2016/10/25 07:32:11, magjed_webrtc wrote:
> We are completely ignoring the input value of |out_frame| and just overwriting
> the pointer here. This looks broken.

I agree. It was changed in cl https://codereview.webrtc.org/1482913003, and
unrelated to this cl.

https://codereview.webrtc.org/2443123002/diff/20001/webrtc/modules/video_proc...
webrtc/modules/video_processing/test/video_processing_unittest.cc:247: if
(out_frame == NULL)
On 2016/10/25 07:32:11, magjed_webrtc wrote:
> Something is wrong. |out_frame| will always be null here, regardless of what
> PreprocessFrameAndVerify does.

Same here, change from the earlier refactoring.

https://codereview.webrtc.org/2443123002/diff/20001/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.webrtc.org/2443123002/diff/20001/webrtc/video_frame.h#newc...
webrtc/video_frame.h:82: // Delete after applications are updated.
On 2016/10/25 07:32:11, magjed_webrtc wrote:
> Is anyone else really using this function?

Hmm, I can't find any. Maybe we can try to delete it right away.

[magjed_webrtc, 2016-10-25 10:05:00]

lgtm if you fix PreprocessFrameAndVerify in
https://codereview.webrtc.org/2448053003/.

https://codereview.webrtc.org/2443123002/diff/20001/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.webrtc.org/2443123002/diff/20001/webrtc/video_frame.h#newc...
webrtc/video_frame.h:82: // Delete after applications are updated.
On 2016/10/25 08:01:34, nisse-webrtc wrote:
> On 2016/10/25 07:32:11, magjed_webrtc wrote:
> > Is anyone else really using this function?
> 
> Hmm, I can't find any. Maybe we can try to delete it right away.

Yeah, try to delete it right away. I don't think it's used outside WebRTC.

[nisse-webrtc, 2016-10-27 11:44:39]

Description was changed from

==========
Deprecate ShallowCopy, in favor of copy construction and assignment.

BUG=webrtc:6591
==========

to

==========
Delete ShallowCopy, in favor of copy construction and assignment.

BUG=webrtc:6591
==========

[nisse-webrtc, 2016-10-27 11:44:39]

nisse@webrtc.org changed reviewers:
- nisse@chromium.org

[stefan-webrtc, 2016-10-31 14:00:26]

stefan@webrtc.org changed reviewers:
+ stefan@webrtc.org

[stefan-webrtc, 2016-10-31 14:00:27]

lgtm

[nisse-webrtc, 2016-10-31 14:02:04]

On 2016/10/31 14:00:27, stefan-webrtc (holmer) wrote:
> lgtm

Thanks for the review. I'm landing the other fix first, then I'll rebase and
land this.

[nisse-webrtc, 2016-10-31 14:38:46]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-10-31 14:38:46]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
magjed@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2443123002/#ps60001 (title:
"Rebase.")

[commit-bot: I haz the power, 2016-10-31 14:38:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-31 15:05:52]

Description was changed from

==========
Delete ShallowCopy, in favor of copy construction and assignment.

BUG=webrtc:6591
==========

to

==========
Delete ShallowCopy, in favor of copy construction and assignment.

BUG=webrtc:6591
==========

[commit-bot: I haz the power, 2016-10-31 15:05:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-31 15:06:04]

Description was changed from

==========
Delete ShallowCopy, in favor of copy construction and assignment.

BUG=webrtc:6591
==========

to

==========
Delete ShallowCopy, in favor of copy construction and assignment.

BUG=webrtc:6591

Committed: https://crrev.com/67dca9f12ef9e43739950c003aea882ec67cac2b
Cr-Commit-Position: refs/heads/master@{#14853}
==========

[commit-bot: I haz the power, 2016-10-31 15:06:07]

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