[Chromoting] Implement new APIs in DesktopCapturer

[Chromoting] Implement new APIs in DesktopCapturer

This change implements GetSourceList() and SelectSource() function in all
DesktopCapturer derived classes.

After this change, and https://codereview.chromium.org/2479553006/,
GetSourceList() and SelectSource() can be changed to pure virtual functions in
WebRTC.

BUG=webrtc:6513
Committed: https://crrev.com/c034cd404edfaa2f6769101fe993f24ccabbe2f9
Cr-Commit-Position: refs/heads/master@{#430183}

[Hzj_jie, 2016-11-04 20:42:31]

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

[commit-bot: I haz the power, 2016-11-04 20:43:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 20:53:39]

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

[commit-bot: I haz the power, 2016-11-04 20:53:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Hzj_jie, 2016-11-04 21:08:16]

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

[Hzj_jie, 2016-11-04 21:08:31]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-11-04 21:08:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2016-11-04 21:24:34]

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

[commit-bot: I haz the power, 2016-11-04 21:24:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2016-11-04 21:45:29]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-11-04 22:34:11]

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

[commit-bot: I haz the power, 2016-11-04 22:34:13]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2016-11-04 22:52:50]

Description was changed from

==========
[Chromoting] Implement new APIs in DesktopCapturer

This change implements GetSourceList() and SelectSource() function in all
DesktopCapturer derived classes.

After this change, GetSourceList() and SelectSource() can be changed to pure
virtual functions in WebRTC.

BUG=webrtc:6513
==========

to

==========
[Chromoting] Implement new APIs in DesktopCapturer

This change implements GetSourceList() and SelectSource() function in all
DesktopCapturer derived classes.

After this change, and https://codereview.chromium.org/2479553006/,
GetSourceList() and SelectSource() can be changed to pure virtual functions in
WebRTC.

BUG=webrtc:6513
==========

[Hzj_jie, 2016-11-04 22:52:50]

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

[Hzj_jie, 2016-11-04 22:59:05]

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

[Sergey Ulanov, 2016-11-05 00:16:26]

lgtm when my comments are addressed.

https://codereview.chromium.org/2483483004/diff/40001/remoting/host/desktop_c...
File remoting/host/desktop_capturer_proxy.h (right):

https://codereview.chromium.org/2483483004/diff/40001/remoting/host/desktop_c...
remoting/host/desktop_capturer_proxy.h:49: // Following two functions are not
supported, they always return false.
Put this in the comment above class definition please

https://codereview.chromium.org/2483483004/diff/40001/remoting/host/desktop_s...
File remoting/host/desktop_session_proxy.h (right):

https://codereview.chromium.org/2483483004/diff/40001/remoting/host/desktop_s...
remoting/host/desktop_session_proxy.h:113: bool
GetSourceList(webrtc::DesktopCapturer::SourceList* sources);
I don't think we want these methods here, see my comments for the IPC capturer.

https://codereview.chromium.org/2483483004/diff/40001/remoting/host/ipc_video...
File remoting/host/ipc_video_frame_capturer.cc (right):

https://codereview.chromium.org/2483483004/diff/40001/remoting/host/ipc_video...
remoting/host/ipc_video_frame_capturer.cc:45: return
desktop_session_proxy_->GetSourceList(sources);
Problem here is that the API is synchronous, but all IPCs need to be
asynchronous. OTOH the host currently doesn't need these functions.
Please add NOTIMPLEMENTED() and return false here. We can figure out what to do
about it later, when we need these functions.

https://codereview.chromium.org/2483483004/diff/40001/remoting/protocol/conne...
File remoting/protocol/connection_unittest.cc (right):

https://codereview.chromium.org/2483483004/diff/40001/remoting/protocol/conne...
remoting/protocol/connection_unittest.cc:98: return true;
Need to put something in sources if you return success here. Alternatively add
NOTIMPLEMENTED() and return false.

https://codereview.chromium.org/2483483004/diff/40001/remoting/protocol/fake_...
File remoting/protocol/fake_desktop_capturer.h (right):

https://codereview.chromium.org/2483483004/diff/40001/remoting/protocol/fake_...
remoting/protocol/fake_desktop_capturer.h:44: // Following two functions are not
supported, they always return false.
Don't need this comment. Instead add NOTIMPLEMENTED() in the implementation.

https://codereview.chromium.org/2483483004/diff/40001/remoting/protocol/video...
File remoting/protocol/video_frame_pump_unittest.cc (right):

https://codereview.chromium.org/2483483004/diff/40001/remoting/protocol/video...
remoting/protocol/video_frame_pump_unittest.cc:126: return true;
return false;

[Hzj_jie, 2016-11-06 00:58:00]

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

[commit-bot: I haz the power, 2016-11-06 00:58:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-06 01:26:44]

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

[commit-bot: I haz the power, 2016-11-06 01:26:44]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2016-11-06 04:27:24]

https://codereview.chromium.org/2483483004/diff/40001/remoting/host/desktop_c...
File remoting/host/desktop_capturer_proxy.h (right):

https://codereview.chromium.org/2483483004/diff/40001/remoting/host/desktop_c...
remoting/host/desktop_capturer_proxy.h:49: // Following two functions are not
supported, they always return false.
On 2016/11/05 00:16:26, Sergey Ulanov wrote:
> Put this in the comment above class definition please

Done.

https://codereview.chromium.org/2483483004/diff/40001/remoting/host/desktop_s...
File remoting/host/desktop_session_proxy.h (right):

https://codereview.chromium.org/2483483004/diff/40001/remoting/host/desktop_s...
remoting/host/desktop_session_proxy.h:113: bool
GetSourceList(webrtc::DesktopCapturer::SourceList* sources);
On 2016/11/05 00:16:26, Sergey Ulanov wrote:
> I don't think we want these methods here, see my comments for the IPC
capturer.

Done.

https://codereview.chromium.org/2483483004/diff/40001/remoting/host/ipc_video...
File remoting/host/ipc_video_frame_capturer.cc (right):

https://codereview.chromium.org/2483483004/diff/40001/remoting/host/ipc_video...
remoting/host/ipc_video_frame_capturer.cc:45: return
desktop_session_proxy_->GetSourceList(sources);
On 2016/11/05 00:16:26, Sergey Ulanov wrote:
> Problem here is that the API is synchronous, but all IPCs need to be
> asynchronous. OTOH the host currently doesn't need these functions.
> Please add NOTIMPLEMENTED() and return false here. We can figure out what to
do
> about it later, when we need these functions.

Yes, that's the reason I put the TODO in DesktopSessionProxy instead of this
class. DesktopSessionProxy needs to be able to get source list or select source
without invoking RPC.

Done.

https://codereview.chromium.org/2483483004/diff/40001/remoting/protocol/conne...
File remoting/protocol/connection_unittest.cc (right):

https://codereview.chromium.org/2483483004/diff/40001/remoting/protocol/conne...
remoting/protocol/connection_unittest.cc:98: return true;
On 2016/11/05 00:16:26, Sergey Ulanov wrote:
> Need to put something in sources if you return success here. Alternatively add
> NOTIMPLEMENTED() and return false.

Done.

https://codereview.chromium.org/2483483004/diff/40001/remoting/protocol/fake_...
File remoting/protocol/fake_desktop_capturer.h (right):

https://codereview.chromium.org/2483483004/diff/40001/remoting/protocol/fake_...
remoting/protocol/fake_desktop_capturer.h:44: // Following two functions are not
supported, they always return false.
On 2016/11/05 00:16:26, Sergey Ulanov wrote:
> Don't need this comment. Instead add NOTIMPLEMENTED() in the implementation.

Done.

https://codereview.chromium.org/2483483004/diff/40001/remoting/protocol/video...
File remoting/protocol/video_frame_pump_unittest.cc (right):

https://codereview.chromium.org/2483483004/diff/40001/remoting/protocol/video...
remoting/protocol/video_frame_pump_unittest.cc:126: return true;
On 2016/11/05 00:16:26, Sergey Ulanov wrote:
> return false;

Done.

[Hzj_jie, 2016-11-06 04:33:20]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2016-11-06 04:33:20]

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/2483483004/#ps60001
(title: "Resolve review comments")

[commit-bot: I haz the power, 2016-11-06 04:33:24]

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

[commit-bot: I haz the power, 2016-11-06 04:42:33]

Description was changed from

==========
[Chromoting] Implement new APIs in DesktopCapturer

This change implements GetSourceList() and SelectSource() function in all
DesktopCapturer derived classes.

After this change, and https://codereview.chromium.org/2479553006/,
GetSourceList() and SelectSource() can be changed to pure virtual functions in
WebRTC.

BUG=webrtc:6513
==========

to

==========
[Chromoting] Implement new APIs in DesktopCapturer

This change implements GetSourceList() and SelectSource() function in all
DesktopCapturer derived classes.

After this change, and https://codereview.chromium.org/2479553006/,
GetSourceList() and SelectSource() can be changed to pure virtual functions in
WebRTC.

BUG=webrtc:6513
==========

[commit-bot: I haz the power, 2016-11-06 04:42:34]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-11-06 04:44:55]

Description was changed from

==========
[Chromoting] Implement new APIs in DesktopCapturer

This change implements GetSourceList() and SelectSource() function in all
DesktopCapturer derived classes.

After this change, and https://codereview.chromium.org/2479553006/,
GetSourceList() and SelectSource() can be changed to pure virtual functions in
WebRTC.

BUG=webrtc:6513
==========

to

==========
[Chromoting] Implement new APIs in DesktopCapturer

This change implements GetSourceList() and SelectSource() function in all
DesktopCapturer derived classes.

After this change, and https://codereview.chromium.org/2479553006/,
GetSourceList() and SelectSource() can be changed to pure virtual functions in
WebRTC.

BUG=webrtc:6513

Committed: https://crrev.com/c034cd404edfaa2f6769101fe993f24ccabbe2f9
Cr-Commit-Position: refs/heads/master@{#430183}
==========

[commit-bot: I haz the power, 2016-11-06 04:44:56]

Patchset 2 (id:??) landed as
https://crrev.com/c034cd404edfaa2f6769101fe993f24ccabbe2f9
Cr-Commit-Position: refs/heads/master@{#430183}