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

Issue 11761019: Tiny little refactoring of DesktopEnvironment. (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 single interface that ClientSession needs to interact with the desktop (i.e. capture audio/video and inject input). DesktopEnvironment is a single-threaded, synchronous interface. Implemenattions are free to use multithreaded/asynchronous backends as long as synchronous nature of the interface is preserved. For example such a backend should be able to shutdown without any assistance from the DesktopEnviroment's owner. Specific changes: - AudioCapturer and VideoFrameCapturer are owned by AudioScheduler and VideoScheduler correspondingly. - Both AudioScheduler and VideoScheduler can now be stopped completely asynchronously. - Both AudioScheduler and VideoScheduler are owned by a specific DesktopEnviroment implementation. - AudioScheduler::SetEnabled() changed to Pause() to match the corresponding method provided by VideoScheduler. - ClientSession::Stop() is synchronous now. - MouseClampingFilter now gets the screen size from DesktopEnvironment::GetScreenSize() which does not try to access the cached screen size from a wrong thread. BUG=104544, 134694

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1231 lines, -692 lines) Patch
M remoting/host/audio_scheduler.h View 5 chunks +6 lines, -7 lines 0 comments Download
M remoting/host/audio_scheduler.cc View 5 chunks +10 lines, -14 lines 0 comments Download
M remoting/host/chromoting_host.h View 3 chunks +5 lines, -4 lines 0 comments Download
M remoting/host/chromoting_host.cc View 4 chunks +12 lines, -10 lines 0 comments Download
M remoting/host/chromoting_host_unittest.cc View 6 chunks +11 lines, -12 lines 0 comments Download
M remoting/host/client_session.h View 7 chunks +8 lines, -30 lines 0 comments Download
M remoting/host/client_session.cc View 8 chunks +30 lines, -93 lines 0 comments Download
M remoting/host/client_session_unittest.cc View 14 chunks +40 lines, -43 lines 0 comments Download
M remoting/host/desktop_environment.h View 1 chunk +62 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 2 chunks +18 lines, -7 lines 0 comments Download
M remoting/host/desktop_environment_factory.cc View 1 chunk +13 lines, -13 lines 0 comments Download
A remoting/host/desktop_environment_fake.h View 1 chunk +59 lines, -0 lines 0 comments Download
A remoting/host/desktop_environment_fake.cc View 1 chunk +87 lines, -0 lines 0 comments Download
M remoting/host/desktop_session_proxy.h View 5 chunks +49 lines, -23 lines 0 comments Download
M remoting/host/desktop_session_proxy.cc View 7 chunks +168 lines, -74 lines 0 comments Download
M remoting/host/host_mock_objects.h View 2 chunks +49 lines, -2 lines 0 comments Download
M remoting/host/host_mock_objects.cc View 1 chunk +29 lines, -3 lines 0 comments Download
M remoting/host/ipc_audio_capturer.h View 1 chunk +3 lines, -6 lines 0 comments Download
M remoting/host/ipc_audio_capturer.cc View 2 chunks +9 lines, -17 lines 0 comments Download
M remoting/host/ipc_desktop_environment.h View 2 chunks +48 lines, -19 lines 0 comments Download
M remoting/host/ipc_desktop_environment.cc View 3 chunks +102 lines, -32 lines 0 comments Download
M remoting/host/ipc_desktop_environment_factory.h View 3 chunks +12 lines, -16 lines 0 comments Download
M remoting/host/ipc_desktop_environment_factory.cc View 4 chunks +17 lines, -25 lines 0 comments Download
D remoting/host/ipc_event_executor.h View 1 chunk +0 lines, -44 lines 0 comments Download
D remoting/host/ipc_event_executor.cc View 1 chunk +0 lines, -37 lines 0 comments Download
A remoting/host/local_desktop_environment.h View 1 chunk +91 lines, -0 lines 0 comments Download
A remoting/host/local_desktop_environment.cc View 1 chunk +148 lines, -0 lines 0 comments Download
M remoting/host/mouse_clamping_filter.h View 2 chunks +3 lines, -3 lines 0 comments Download
M remoting/host/mouse_clamping_filter.cc View 2 chunks +8 lines, -6 lines 0 comments Download
M remoting/host/plugin/host_script_object.cc View 2 chunks +5 lines, -3 lines 0 comments Download
M remoting/host/remoting_me2me_host.cc View 2 chunks +6 lines, -9 lines 0 comments Download
M remoting/host/video_scheduler.h View 11 chunks +18 lines, -12 lines 0 comments Download
M remoting/host/video_scheduler.cc View 8 chunks +37 lines, -17 lines 0 comments Download
M remoting/host/video_scheduler_unittest.cc View 6 chunks +36 lines, -20 lines 0 comments Download
M remoting/host/win/session_desktop_environment_factory.h View 1 chunk +4 lines, -3 lines 0 comments Download
M remoting/host/win/session_desktop_environment_factory.cc View 1 chunk +24 lines, -18 lines 0 comments Download
M remoting/remoting.gyp View 4 chunks +4 lines, -3 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
alexeypa (please no reviews)
Rubber stamp, please. ;-)
7 years, 11 months ago (2013-01-03 20:47:18 UTC) #1
Wez
On 2013/01/03 20:47:18, alexeypa wrote: > Rubber stamp, please. ;-) Yeah, right... :P Aside from ...
7 years, 11 months ago (2013-01-03 20:57:03 UTC) #2
Wez
I don't like the fact that this pushes the AudioScheduler and VideoScheduler into each DesktopEnvironment ...
7 years, 11 months ago (2013-01-03 21:05:48 UTC) #3
alexeypa (please no reviews)
7 years, 11 months ago (2013-01-03 21:26:08 UTC) #4
On 2013/01/03 21:05:48, Wez wrote:
> I don't like the fact that this pushes the AudioScheduler and VideoScheduler
> into each DesktopEnvironment implementation; the intent was that
> DesktopEnvironment should become a factory for the desktop integration
> components (audio & video capturer, event executor, etc).  What's your
reasoning
> for abandoning that approach?

The problem is that in the IPC case the audio/video capturers and event executor
are owned by two parties each: ClientSession (or the corresponding scheduler) on
one side and the component handling the IPC I/O on the other side. In order to
manage them properly we would need to create two separate front-end interfaces
(used by ClientSession and the IPC component correspondingly) and a ref-counted
core, actually implementing the functionality and dealing with threading issues.

Packaging schedulers to the desktop environment implementation makes it much
easier. We can make assumptions about lifetime of the capturers, so they remain
simple plain objects.

Powered by Google App Engine
This is Rietveld 408576698