Remove DesktopRegion parameter in DesktopCapturer::Capture.

Remove DesktopRegion parameter from DesktopCapturer::Capture.

To ensure this change won't break Chromium, this is the first change, to add a
new CaptureFrame() function, and let Capture(DesktopRegion) and CaptureFrame()
call each other. So both a legacy consumer or a legacy implementation won't be
broken.

BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=6513

Committed: https://crrev.com/91902cb6c0e1753c1bebb2848992092fb1e5d419
Cr-Commit-Position: refs/heads/master@{#14635}

[Hzj_jie, 2016-10-10 22:46:23]

Description was changed from

==========
Remove DesktopRegion parameter in DesktopCapturer::Capture.

BUG=6513
==========

to

==========
Remove DesktopRegion parameter in DesktopCapturer::Capture.

BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=6513
==========

[Hzj_jie, 2016-10-10 22:49:26]

Description was changed from

==========
Remove DesktopRegion parameter in DesktopCapturer::Capture.

BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=6513
==========

to

==========
Remove DesktopRegion parameter from DesktopCapturer::Capture.

To ensure this change won't break Chromium, this is the first change, to add a
new Capture() function, and let Capture(DesktopRegion) and Capture() call each
other. So both a legacy consumer or a legacy implementation won't be broken.

BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=6513
==========

[Hzj_jie, 2016-10-10 22:49:53]

Patchset #1 (id:1) has been deleted

[Hzj_jie, 2016-10-10 23:31:15]

Patchset #1 (id:20001) has been deleted

[Hzj_jie, 2016-10-10 23:34:11]

Patchset #1 (id:40001) has been deleted

[Hzj_jie, 2016-10-10 23:44:55]

Patchset #1 (id:60001) has been deleted

[Hzj_jie, 2016-10-11 00:10:35]

Patchset #1 (id:80001) has been deleted

[Hzj_jie, 2016-10-11 00:49:15]

Description was changed from

==========
Remove DesktopRegion parameter from DesktopCapturer::Capture.

To ensure this change won't break Chromium, this is the first change, to add a
new Capture() function, and let Capture(DesktopRegion) and Capture() call each
other. So both a legacy consumer or a legacy implementation won't be broken.

BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=6513
==========

to

==========
Remove DesktopRegion parameter from DesktopCapturer::Capture.

To ensure this change won't break Chromium, this is the first change, to add a
new Capture2() function, and let Capture(DesktopRegion) and Capture2() call each
other. So both a legacy consumer or a legacy implementation won't be broken.

BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=6513
==========

[Hzj_jie, 2016-10-11 01:07:51]

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

[Sergey Ulanov, 2016-10-11 18:42:42]

I don't think there is any reason to add the suffix to the method name.
Otherwise LGTM.

https://codereview.chromium.org/2409833002/diff/100001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/desktop_capturer.h (right):

https://codereview.chromium.org/2409833002/diff/100001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/desktop_capturer.h:73: // Pending capture
requests will be ignored once DesktopCapturer instance has
There is no way to issue a capture request after DesktopCapturer is deleted, so
this sentence is confusing. Keep the old wording? "Pending capture operations
are canceled when DesktopCapturer is deleted."

https://codereview.chromium.org/2409833002/diff/100001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/desktop_capturer.h:75: virtual void Capture2() {
Capture(DesktopRegion()); }
Call this Capture(). I don't think there are any issues with overloading the old
Capture().

[Hzj_jie, 2016-10-11 19:32:05]

https://codereview.chromium.org/2409833002/diff/100001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/desktop_capturer.h (right):

https://codereview.chromium.org/2409833002/diff/100001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/desktop_capturer.h:73: // Pending capture
requests will be ignored once DesktopCapturer instance has
On 2016/10/11 18:42:41, Sergey Ulanov wrote:
> There is no way to issue a capture request after DesktopCapturer is deleted,
so
> this sentence is confusing. Keep the old wording? "Pending capture operations
> are canceled when DesktopCapturer is deleted."

Done.

https://codereview.chromium.org/2409833002/diff/100001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/desktop_capturer.h:75: virtual void Capture2() {
Capture(DesktopRegion()); }
On 2016/10/11 18:42:41, Sergey Ulanov wrote:
> Call this Capture(). I don't think there are any issues with overloading the
old
> Capture(). 

That won't work, because http://www.stroustrup.com/bs_faq2.html#overloadderived.
I can surely add using DesktopCapturer::Capture in all derived classes. But
since the original Capture(const DesktopRegion&) will be removed anyway, we can
rename the Capture2 back to Capture when Capture(const DesktopRegion&) has been
removed.

[Hzj_jie, 2016-10-12 19:12:50]

On 2016/10/11 19:32:05, Hzj_jie wrote:
>
https://codereview.chromium.org/2409833002/diff/100001/webrtc/modules/desktop...
> File webrtc/modules/desktop_capture/desktop_capturer.h (right):
> 
>
https://codereview.chromium.org/2409833002/diff/100001/webrtc/modules/desktop...
> webrtc/modules/desktop_capture/desktop_capturer.h:73: // Pending capture
> requests will be ignored once DesktopCapturer instance has
> On 2016/10/11 18:42:41, Sergey Ulanov wrote:
> > There is no way to issue a capture request after DesktopCapturer is deleted,
> so
> > this sentence is confusing. Keep the old wording? "Pending capture
operations
> > are canceled when DesktopCapturer is deleted."
> 
> Done.
> 
>
https://codereview.chromium.org/2409833002/diff/100001/webrtc/modules/desktop...
> webrtc/modules/desktop_capture/desktop_capturer.h:75: virtual void Capture2()
{
> Capture(DesktopRegion()); }
> On 2016/10/11 18:42:41, Sergey Ulanov wrote:
> > Call this Capture(). I don't think there are any issues with overloading the
> old
> > Capture(). 
> 
> That won't work, because
http://www.stroustrup.com/bs_faq2.html#overloadderived.
> I can surely add using DesktopCapturer::Capture in all derived classes. But
> since the original Capture(const DesktopRegion&) will be removed anyway, we
can
> rename the Capture2 back to Capture when Capture(const DesktopRegion&) has
been
> removed.

If you do not have other concern, I will submit this change later.

[Sergey Ulanov, 2016-10-12 21:22:52]

Please try renaming Capture2->Capture

https://codereview.chromium.org/2409833002/diff/100001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/desktop_capturer.h (right):

https://codereview.chromium.org/2409833002/diff/100001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/desktop_capturer.h:75: virtual void Capture2() {
Capture(DesktopRegion()); }
On 2016/10/11 19:32:05, Hzj_jie wrote:
> On 2016/10/11 18:42:41, Sergey Ulanov wrote:
> > Call this Capture(). I don't think there are any issues with overloading the
> old
> > Capture(). 
> 
> That won't work, because
http://www.stroustrup.com/bs_faq2.html#overloadderived.

This looks like a different issue. Here both overloads are in the same
interface.

> I can surely add using DesktopCapturer::Capture in all derived classes. But
> since the original Capture(const DesktopRegion&) will be removed anyway, we
can
> rename the Capture2 back to Capture when Capture(const DesktopRegion&) has
been
> removed.

I think overloading should work properly here.

[Hzj_jie, 2016-10-12 21:54:19]

On 2016/10/12 21:22:52, Sergey Ulanov wrote:
> Please try renaming Capture2->Capture
> 
>
https://codereview.chromium.org/2409833002/diff/100001/webrtc/modules/desktop...
> File webrtc/modules/desktop_capture/desktop_capturer.h (right):
> 
>
https://codereview.chromium.org/2409833002/diff/100001/webrtc/modules/desktop...
> webrtc/modules/desktop_capture/desktop_capturer.h:75: virtual void Capture2()
{
> Capture(DesktopRegion()); }
> On 2016/10/11 19:32:05, Hzj_jie wrote:
> > On 2016/10/11 18:42:41, Sergey Ulanov wrote:
> > > Call this Capture(). I don't think there are any issues with overloading
the
> > old
> > > Capture(). 
> > 
> > That won't work, because
> http://www.stroustrup.com/bs_faq2.html#overloadderived.
> 
> This looks like a different issue. Here both overloads are in the same
> interface.
> 
> > I can surely add using DesktopCapturer::Capture in all derived classes. But
> > since the original Capture(const DesktopRegion&) will be removed anyway, we
> can
> > rename the Capture2 back to Capture when Capture(const DesktopRegion&) has
> been
> > removed.
> 
> I think overloading should work properly here.

http://cpp.sh/5y54i is an example. The two commented lines are not workable. But
you need to actively add using in both derived classes or use c.B::F().
This is do out of my expectation when I first realized it. A common modern
language, such as Java, does not have this behavior (An java example,
https://ideone.com/wx50bn).

[Sergey Ulanov, 2016-10-12 22:07:58]

On 2016/10/12 21:54:19, Hzj_jie wrote:
> On 2016/10/12 21:22:52, Sergey Ulanov wrote:
> > I think overloading should work properly here.
> 
> http://cpp.sh/5y54i is an example. The two commented lines are not workable.
But
> you need to actively add using in both derived classes or use c.B::F().
> This is do out of my expectation when I first realized it. A common modern
> language, such as Java, does not have this behavior (An java example,
> https://ideone.com/wx50bn).

I don't think your example matches what we have here. Please take a look at this
version: cpp.sh/4xrwb
All code that calls Capture() uses DesktopCapturer/WindowCapturer/ScreenCapturer
_interfaces_ and never direct implementations (such as ScreenCapturerWin).

[Sergey Ulanov, 2016-10-12 22:10:53]

On 2016/10/12 22:07:58, Sergey Ulanov wrote:
> On 2016/10/12 21:54:19, Hzj_jie wrote:
> > On 2016/10/12 21:22:52, Sergey Ulanov wrote:
> > > I think overloading should work properly here.
> > 
> > http://cpp.sh/5y54i is an example. The two commented lines are not workable.
> But
> > you need to actively add using in both derived classes or use c.B::F().
> > This is do out of my expectation when I first realized it. A common modern
> > language, such as Java, does not have this behavior (An java example,
> > https://ideone.com/wx50bn).
> 
> I don't think your example matches what we have here. Please take a look at
this
> version: cpp.sh/4xrwb
> All code that calls Capture() uses
DesktopCapturer/WindowCapturer/ScreenCapturer
> _interfaces_ and never direct implementations (such as ScreenCapturerWin).

FWIW there are other examples of overloaded methods when updating parameters
list, e.g. see RtpRtcp::SendOutgoingData()
https://codesearch.chromium.org/chromium/src/third_party/webrtc/modules/rtp_r...

[Hzj_jie, 2016-10-12 22:27:07]

On 2016/10/12 22:10:53, Sergey Ulanov wrote:
> On 2016/10/12 22:07:58, Sergey Ulanov wrote:
> > On 2016/10/12 21:54:19, Hzj_jie wrote:
> > > On 2016/10/12 21:22:52, Sergey Ulanov wrote:
> > > > I think overloading should work properly here.
> > > 
> > > http://cpp.sh/5y54i is an example. The two commented lines are not
workable.
> > But
> > > you need to actively add using in both derived classes or use c.B::F().
> > > This is do out of my expectation when I first realized it. A common modern
> > > language, such as Java, does not have this behavior (An java example,
> > > https://ideone.com/wx50bn).
> > 
> > I don't think your example matches what we have here. Please take a look at
> this
> > version: cpp.sh/4xrwb
> > All code that calls Capture() uses
> DesktopCapturer/WindowCapturer/ScreenCapturer
> > _interfaces_ and never direct implementations (such as ScreenCapturerWin).
This is not quite true. DesktopAndCursorComposerTest is one of the example we
are using implementation detail.
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/desktop_captu...

And the sample you provided will trigger overloaded-virtual warning, (Mine is
also.)
a.cpp:10:18: warning: ‘virtual void B::F(double)’ was hidden
[-Woverloaded-virtual]
     virtual void F(double) { F(); }
                  ^
a.cpp:16:10: warning:   by ‘virtual void C::F()’ [-Woverloaded-virtual]
     void F() override { cout << "F()" << endl; }
          ^
a.cpp:9:18: warning: ‘virtual void B::F()’ was hidden [-Woverloaded-virtual]
     virtual void F() { F(double()); }
                  ^
a.cpp:22:10: warning:   by ‘virtual void D::F(double)’ [-Woverloaded-virtual]
     void F(double) override { cout << "F(double)" << endl; }

And -Woverloaded-virtual is enabled by default in webrtc @
https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=0&l=176.
> 
> FWIW there are other examples of overloaded methods when updating parameters
> list, e.g. see RtpRtcp::SendOutgoingData()
>
https://codesearch.chromium.org/chromium/src/third_party/webrtc/modules/rtp_r...
This is a different example, one of the two overloading functions is not
virtual. So -Woverloaded-virtual won't be triggered.

[Sergey Ulanov, 2016-10-12 23:10:55]

I see. Then I suggest calling new function CaptureFrame() instead of Capture2().
This way you won't have to rename the function back.

[Hzj_jie, 2016-10-12 23:12:39]

On 2016/10/12 23:10:55, Sergey Ulanov wrote:
> I see. Then I suggest calling new function CaptureFrame() instead of
Capture2().
> This way you won't have to rename the function back.

Sure, I will update the change.

[Hzj_jie, 2016-10-12 23:45:51]

On 2016/10/12 23:12:39, Hzj_jie wrote:
> On 2016/10/12 23:10:55, Sergey Ulanov wrote:
> > I see. Then I suggest calling new function CaptureFrame() instead of
> Capture2().
> > This way you won't have to rename the function back.
> 
> Sure, I will update the change.

The test failures do not relate to my change, good luck. :)

[Hzj_jie, 2016-10-13 19:45:36]

Description was changed from

==========
Remove DesktopRegion parameter from DesktopCapturer::Capture.

To ensure this change won't break Chromium, this is the first change, to add a
new Capture2() function, and let Capture(DesktopRegion) and Capture2() call each
other. So both a legacy consumer or a legacy implementation won't be broken.

BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=6513
==========

to

==========
Remove DesktopRegion parameter from DesktopCapturer::Capture.

To ensure this change won't break Chromium, this is the first change, to add a
new CaptureFrame() function, and let Capture(DesktopRegion) and CaptureFrame()
call each other. So both a legacy consumer or a legacy implementation won't be
broken.

BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=6513
==========

[Hzj_jie, 2016-10-13 23:46:05]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2016-10-13 23:46:05]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2409833002/#ps140001
(title: "Change Capture2 to CaptureFrame")

[commit-bot: I haz the power, 2016-10-13 23:46:13]

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-13 23:47:54]

Description was changed from

==========
Remove DesktopRegion parameter from DesktopCapturer::Capture.

To ensure this change won't break Chromium, this is the first change, to add a
new CaptureFrame() function, and let Capture(DesktopRegion) and CaptureFrame()
call each other. So both a legacy consumer or a legacy implementation won't be
broken.

BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=6513
==========

to

==========
Remove DesktopRegion parameter from DesktopCapturer::Capture.

To ensure this change won't break Chromium, this is the first change, to add a
new CaptureFrame() function, and let Capture(DesktopRegion) and CaptureFrame()
call each other. So both a legacy consumer or a legacy implementation won't be
broken.

BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=6513
==========

[commit-bot: I haz the power, 2016-10-13 23:47:55]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2016-10-13 23:47:58]

Description was changed from

==========
Remove DesktopRegion parameter from DesktopCapturer::Capture.

To ensure this change won't break Chromium, this is the first change, to add a
new CaptureFrame() function, and let Capture(DesktopRegion) and CaptureFrame()
call each other. So both a legacy consumer or a legacy implementation won't be
broken.

BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=6513
==========

to

==========
Remove DesktopRegion parameter from DesktopCapturer::Capture.

To ensure this change won't break Chromium, this is the first change, to add a
new CaptureFrame() function, and let Capture(DesktopRegion) and CaptureFrame()
call each other. So both a legacy consumer or a legacy implementation won't be
broken.

BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=6513

Committed: https://crrev.com/91902cb6c0e1753c1bebb2848992092fb1e5d419
Cr-Commit-Position: refs/heads/master@{#14635}
==========

[commit-bot: I haz the power, 2016-10-13 23:47:59]

Patchset 3 (id:??) landed as
https://crrev.com/91902cb6c0e1753c1bebb2848992092fb1e5d419
Cr-Commit-Position: refs/heads/master@{#14635}