Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(822)

Issue 11778049: Making DesktopEnvironment a factory class used by ClientSession to create audio/video capturers and… (Closed)

Created:
7 years, 11 months ago by alexeypa (please no reviews)
Modified:
7 years, 11 months ago
Reviewers:
Wez
CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org
Visibility:
Public.

Description

Making DesktopEnvironment a factory class used by ClientSession to create audio/video capturers and event executor for a pacticular desktop environment. Other related changes: - AudioCapturer and VideoFrameCapturer are owned by AudioScheduler and VideoScheduler correspondingly. - Both AudioScheduler and VideoScheduler can now be stopped completely asynchronously. - AudioScheduler::SetEnabled() changed to Pause() to match the corresponding method provided by VideoScheduler. - ClientSession::Stop() is synchronous now. BUG=104544 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177161

Patch Set 1 #

Total comments: 37

Patch Set 2 : CR feedback #

Total comments: 51

Patch Set 3 : CR feedback #2 #

Total comments: 22

Patch Set 4 : CR feedback #3 #

Patch Set 5 : rebased #

Patch Set 6 : - #

Unified diffs Side-by-side diffs Delta from patch set Stats (+937 lines, -850 lines) Patch
M remoting/host/audio_scheduler.h View 1 2 3 5 chunks +10 lines, -11 lines 0 comments Download
M remoting/host/audio_scheduler.cc View 5 chunks +10 lines, -14 lines 0 comments Download
A remoting/host/basic_desktop_environment.h View 1 2 3 1 chunk +58 lines, -0 lines 0 comments Download
A remoting/host/basic_desktop_environment.cc View 1 2 3 1 chunk +60 lines, -0 lines 0 comments Download
M remoting/host/chromoting_host.h View 1 2 4 chunks +5 lines, -9 lines 0 comments Download
M remoting/host/chromoting_host.cc View 1 2 6 chunks +13 lines, -14 lines 0 comments Download
M remoting/host/chromoting_host_unittest.cc View 1 2 3 4 7 chunks +40 lines, -9 lines 0 comments Download
M remoting/host/client_session.h View 1 2 3 4 7 chunks +12 lines, -15 lines 0 comments Download
M remoting/host/client_session.cc View 1 2 3 4 13 chunks +42 lines, -62 lines 0 comments Download
M remoting/host/client_session_unittest.cc View 1 2 3 4 10 chunks +60 lines, -35 lines 0 comments Download
M remoting/host/desktop_environment.h View 1 2 1 chunk +32 lines, -32 lines 0 comments Download
D remoting/host/desktop_environment.cc View 1 chunk +0 lines, -35 lines 0 comments Download
M remoting/host/desktop_environment_factory.h View 1 2 1 chunk +0 lines, -43 lines 0 comments Download
M remoting/host/desktop_environment_factory.cc View 1 2 1 chunk +0 lines, -38 lines 0 comments Download
M remoting/host/desktop_session_proxy.h View 4 chunks +19 lines, -14 lines 0 comments Download
M remoting/host/desktop_session_proxy.cc View 1 2 3 4 5 6 chunks +97 lines, -77 lines 0 comments Download
M remoting/host/host_mock_objects.h View 1 2 3 chunks +34 lines, -2 lines 0 comments Download
M remoting/host/host_mock_objects.cc View 1 2 1 chunk +32 lines, -3 lines 0 comments Download
M remoting/host/ipc_audio_capturer.h View 1 chunk +2 lines, -1 line 0 comments Download
M remoting/host/ipc_desktop_environment.h View 1 2 3 4 1 chunk +88 lines, -20 lines 0 comments Download
M remoting/host/ipc_desktop_environment.cc View 1 2 3 4 1 chunk +151 lines, -40 lines 0 comments Download
M remoting/host/ipc_desktop_environment_factory.h View 1 2 1 chunk +0 lines, -82 lines 0 comments Download
M remoting/host/ipc_desktop_environment_factory.cc View 1 2 1 chunk +0 lines, -132 lines 0 comments Download
M remoting/host/ipc_event_executor.h View 1 chunk +2 lines, -1 line 0 comments Download
M remoting/host/plugin/host_script_object.cc View 1 2 3 3 chunks +5 lines, -4 lines 0 comments Download
M remoting/host/remoting_me2me_host.cc View 1 2 3 4 5 6 chunks +9 lines, -12 lines 0 comments Download
M remoting/host/video_scheduler.h View 1 2 3 4 9 chunks +14 lines, -12 lines 0 comments Download
M remoting/host/video_scheduler.cc View 1 2 3 4 7 chunks +28 lines, -16 lines 0 comments Download
M remoting/host/video_scheduler_unittest.cc View 1 2 3 4 6 chunks +27 lines, -19 lines 0 comments Download
A remoting/host/win/session_desktop_environment.h View 1 2 3 1 chunk +55 lines, -0 lines 0 comments Download
A + remoting/host/win/session_desktop_environment.cc View 1 2 3 1 chunk +28 lines, -18 lines 0 comments Download
M remoting/host/win/session_desktop_environment_factory.h View 1 2 1 chunk +0 lines, -32 lines 0 comments Download
M remoting/host/win/session_desktop_environment_factory.cc View 1 2 1 chunk +0 lines, -41 lines 0 comments Download
M remoting/remoting.gyp View 1 2 3 4 4 chunks +4 lines, -7 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
alexeypa (please no reviews)
This is a 2nd version of https://chromiumcodereview.appspot.com/11761019/. PTAL.
7 years, 11 months ago (2013-01-08 22:37:35 UTC) #1
alexeypa (please no reviews)
Ping.
7 years, 11 months ago (2013-01-10 17:05:56 UTC) #2
Wez
Initial review comments; will follow-up tomorrow! https://chromiumcodereview.appspot.com/11778049/diff/1/remoting/host/client_session.h File remoting/host/client_session.h (right): https://chromiumcodereview.appspot.com/11778049/diff/1/remoting/host/client_session.h#newcode131 remoting/host/client_session.h:131: // Stops the ...
7 years, 11 months ago (2013-01-11 02:23:41 UTC) #3
alexeypa (please no reviews)
https://codereview.chromium.org/11778049/diff/1/remoting/host/client_session.h File remoting/host/client_session.h (right): https://codereview.chromium.org/11778049/diff/1/remoting/host/client_session.h#newcode131 remoting/host/client_session.h:131: // Stops the ClientSession. On 2013/01/11 02:23:41, Wez wrote: ...
7 years, 11 months ago (2013-01-11 19:59:46 UTC) #4
Wez
Just a couple of issues left, otherwise this is looking really good! https://chromiumcodereview.appspot.com/11778049/diff/1/remoting/host/desktop_environment.h File remoting/host/desktop_environment.h ...
7 years, 11 months ago (2013-01-12 01:58:06 UTC) #5
alexeypa (please no reviews)
PTAL. https://chromiumcodereview.appspot.com/11778049/diff/1/remoting/host/desktop_environment.h File remoting/host/desktop_environment.h (right): https://chromiumcodereview.appspot.com/11778049/diff/1/remoting/host/desktop_environment.h#newcode32 remoting/host/desktop_environment.h:32: scoped_refptr<base::SingleThreadTaskRunner> audio_task_runner) = 0; On 2013/01/12 01:58:06, Wez ...
7 years, 11 months ago (2013-01-14 20:43:09 UTC) #6
Wez
LGTM w/nits & a suggestion to simplify SessionDesktopEnvironment. https://chromiumcodereview.appspot.com/11778049/diff/12001/remoting/host/audio_scheduler.h File remoting/host/audio_scheduler.h (right): https://chromiumcodereview.appspot.com/11778049/diff/12001/remoting/host/audio_scheduler.h#newcode76 remoting/host/audio_scheduler.h:76: scoped_ptr<AudioCapturer> ...
7 years, 11 months ago (2013-01-15 05:02:52 UTC) #7
Wez
https://chromiumcodereview.appspot.com/11778049/diff/23008/remoting/host/win/session_desktop_environment.h File remoting/host/win/session_desktop_environment.h (right): https://chromiumcodereview.appspot.com/11778049/diff/23008/remoting/host/win/session_desktop_environment.h#newcode21 remoting/host/win/session_desktop_environment.h:21: // with Windows sessions. On 2013/01/15 05:02:54, Wez wrote: ...
7 years, 11 months ago (2013-01-15 05:03:52 UTC) #8
alexeypa (please no reviews)
https://chromiumcodereview.appspot.com/11778049/diff/12001/remoting/host/ipc_desktop_environment.h File remoting/host/ipc_desktop_environment.h (right): https://chromiumcodereview.appspot.com/11778049/diff/12001/remoting/host/ipc_desktop_environment.h#newcode57 remoting/host/ipc_desktop_environment.h:57: // True if |desktop_session_proxy_| has been connected to a ...
7 years, 11 months ago (2013-01-15 18:56:45 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/11778049/36002
7 years, 11 months ago (2013-01-15 19:48:09 UTC) #10
Wez
lgtm https://chromiumcodereview.appspot.com/11778049/diff/23008/remoting/host/basic_desktop_environment.h File remoting/host/basic_desktop_environment.h (right): https://chromiumcodereview.appspot.com/11778049/diff/23008/remoting/host/basic_desktop_environment.h#newcode27 remoting/host/basic_desktop_environment.h:27: // DesktopEnvironment implementation. On 2013/01/15 18:56:47, alexeypa wrote: ...
7 years, 11 months ago (2013-01-15 20:00:10 UTC) #11
Wez
BTW, it's a little misleading to describe ClientSession::Stop() as synchronous now; it won't block and ...
7 years, 11 months ago (2013-01-15 20:01:34 UTC) #12
commit-bot: I haz the power
Retried try job too often on linux_aura for step(s) aura_unittests, compositor_unittests, content_browsertests, content_unittests, interactive_ui_tests, unit_tests, ...
7 years, 11 months ago (2013-01-15 20:22:15 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/11778049/27009
7 years, 11 months ago (2013-01-16 00:22:02 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/11778049/69003
7 years, 11 months ago (2013-01-16 00:51:26 UTC) #15
commit-bot: I haz the power
Retried try job too often on win for step(s) compile
7 years, 11 months ago (2013-01-16 02:42:41 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/11778049/69003
7 years, 11 months ago (2013-01-16 16:43:57 UTC) #17
commit-bot: I haz the power
7 years, 11 months ago (2013-01-16 17:24:00 UTC) #18
Message was sent while issue was closed.
Change committed as 177161

Powered by Google App Engine
This is Rietveld 408576698