Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
422503004:
Adding ability to stream windows and inject events to them (Closed)
|
Created:
6 years, 4 months ago by ronakvora do not use Modified:
6 years, 4 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionI've created a new desktop environment, Me2MeWindowDesktopEnvironment. This environment has creates a WindowCapturerScreenWrapper instead of a
ScreenCapturer with its CreateVideoCapturer() method. This wrapper wraps a
WindowCapturer and delegates messages sent to it to the wrapped WindowCapturer.
When the command line flag window-Id is specified the remoting_me2me_host.cc choses Me2MeWindowDesktopEnvironment instead of the regular desktop environment.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289825
Patch Set 1 #Patch Set 2 : uploaded to remove lint errors #
Total comments: 65
Patch Set 3 : commented code #Patch Set 4 : I updated the code with almost all of the comments Lambros had. The exceptions are the redesign of … #
Total comments: 72
Patch Set 5 : I updated the design of input_injector so we now have a single_window_input_injector subclassing in… #Patch Set 6 : removed enablewindowinjection from input_injector_win and input_injector_linux #Patch Set 7 : trying to make input_injectors unchanged #Patch Set 8 : removed input_injector diffs #
Total comments: 20
Patch Set 9 : Added comments, removed extraneous commented out code, and reformatted. #
Total comments: 108
Patch Set 10 : Updated so CFRefs use CFCast. Updated --window-id flag description. Changed single_window_input_inj… #Patch Set 11 : Refactoring file names #Patch Set 12 : Updated single_window_desktop_environment.h and .cc #Patch Set 13 : Addressed all comments from Lambros and Wez. Updated file names, comments, nits, etc. #Patch Set 14 : Small nit changes #
Total comments: 10
Patch Set 15 : Just a few nits. #
Total comments: 7
Patch Set 16 : Removed enable_window_capture_ = false in remoting_me2me_host.cc #Patch Set 17 : added exit code to remoting_host when unable to parse window_id #
Total comments: 16
Patch Set 18 : Removed window_capturer_screen_wrapper #Patch Set 19 : Small changes to linux and windows single_window_input_injector #Patch Set 20 : Small changes to remoting_me2me_host.cc #Patch Set 21 : removed unnecessary include #
Total comments: 4
Patch Set 22 : Small changes #
Total comments: 2
Patch Set 23 : Updated if directive #
Total comments: 6
Patch Set 24 : Lambros nits #Patch Set 25 : Changes to get this thing compiling on windows and linux #Patch Set 26 : Changed method in single_window_input_injector to be called CreateForWindow so that the method is n… #Patch Set 27 : compiler issues #Messages
Total messages: 41 (0 generated)
You guys know what I've done already, but in case anyone else wants a better description. I've created a new desktop environment, Me2MeWindowDesktopEnvironment. This environment has creates a WindowCapturerScreenWrapper instead of a ScreenCapturer with its CreateVideoCapturer() method. This wrapper wraps a WindowCapturer and delegates messages sent to it to the wrapped WindowCapturer. When the command line flag window-Id is specified the remoting_me2me_host.cc choses Me2MeWindowDesktopEnvironment instead of the regular desktop environment.
Per-window remoting is a great feature to add. Thanks for doing this work! This is just a first round of comments. We might want to re-think the design in a couple of areas. Particularly, the WindowCapturerScreenWrapper is confusing, as it derives from ScreenCapturer, and I'm not sure ScreenCapturer qualifies as an interface. We generally discourage inheritance from things that aren't interfaces. https://codereview.chromium.org/422503004/diff/20001/remoting/host/desktop_en... File remoting/host/desktop_environment.h (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/desktop_en... remoting/host/desktop_environment.h:15: #include "third_party/webrtc/modules/desktop_capture/screen_capturer.h" Undo this, and restore "class ScreenCapturer". https://codereview.chromium.org/422503004/diff/20001/remoting/host/host_main.cc File remoting/host/host_main.cc (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/host_main.... remoting/host/host_main.cc:80: " --window-Id=<id> - Specifies that a window should be streamed.\n"; Please use lower-case: window-id Also, you don't need the extra spaces here. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... File remoting/host/input_injector.h (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector.h:30: static scoped_ptr<InputInjector> Create( Name this "CreateForWindow". Chromium style doesn't allow overloading. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector.h:34: bool enable_window_capture); This boolean flag is redundant. The calling code should call this method or the other method to specify what kind of InputInjector to create. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... File remoting/host/input_injector_mac.cc (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:79: virtual void SetWindowId(webrtc::WindowId windowId); Do these need to be virtual? https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:80: virtual void SetEnableWindowCapture(bool enable_window_capture); Don't say "Capture" here. Maybe replace these methods with a single method called EnableWindowInjection(webrtc::WindowId window_id) or similar? You could maybe add this method to the InputInjector interface, and make it a no-op with NOTIMPLEMENTED() in the Windows and Linux implementations? https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:83: // TODO(sidj, sskhandp) put these inside of WindowInputInjectorMac. Remove TODO - I don't think it's worth defining a separate class for this. It should be a simple modification to the existing classes to support per-window injection on each platform. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:84: CGWindowID windowId_input_injector_mac_; nit: window_id_ https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:85: bool enable_window_capture_injector_mac_; nit: window_injection_enabled_ - don't use "capture" or "mac". And field names should be nouns, not verbs. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:102: // TODO(sidj, sskhandp) Remove these methods and use Remove TODO. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:120: // TODO(sidj, sskhandp) put these inside of WindowInputInjectorMac. Remove TODO. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:277: CFRelease(window_id_array); Use ScopedCFTypeRef from base/mac/scoped_cftyperef.h instead of manually calling CFRelease here. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:425: // TODO(sskhandp, sidj) remove this method and make WindowInputInjector Remove TODO. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:431: InputInjectorMac* injector = new InputInjectorMac(main_task_runner); nit: Declare |injector| as scoped_ptr instead of raw pointer. If you want, you could use the existing Create() method here. https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... File remoting/host/me2me_window_desktop_environment.cc (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. "(c) 2012" -> "2014" https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:7: #if defined(OS_POSIX) Don't need this OS_POSIX block. https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:12: #include "base/logging.h" Do we really need all these #includes? For example, this file doesn't seem to use gnubby_auth_handler.h. https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... File remoting/host/me2me_window_desktop_environment.h (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. I'm not sure if we need this new class? Maybe it's possible to tweak the existing BasicDesktopEnvironment to allow some kind of dependency-injection of Window-based capturers and injectors? https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:9: #include "remoting/host/input_injector.h" nit: Don't need full definition of InputInjector class. Remove this #include, and add forward-declaration: "class InputInjector;" instead, just below "namespace remoting {" separated by a blank line. https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:42: class Me2MeWindowDesktopEnvironmentFactory : public BasicDesktopEnvironmentFactory { This breaks 80-char limit. Put ": public BasicD..." on next line, indented by 4 spaces. https://codereview.chromium.org/422503004/diff/20001/remoting/host/remoting_m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:312: webrtc::WindowId windowId_; nit: window_id_ https://codereview.chromium.org/422503004/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:462: unsigned temp; nit: window_id is a better variable name. https://codereview.chromium.org/422503004/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:466: // any precision here. You could verify this with an assertion. Use COMPILE_ASSERT from base/macros.h, together with the sizeof() operator. https://codereview.chromium.org/422503004/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:467: windowId_ = (webrtc::WindowId) temp; nit: static_cast<webrtc::WindowId>(temp) https://codereview.chromium.org/422503004/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:705: new Me2MeWindowDesktopEnvironmentFactory( This is the reason I'd prefer not to have yet another new Factory class. It complicates code like this. On Windows, will we need to create a new IpcWindowDesktopEnvironmentFactory class to deal with both IPC *and* windowing? It's getting out of hand :) https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... File remoting/host/window_capturer_screen_wrapper.cc (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:2: * Copyright (c) 2013 The WebRTC project authors. All Rights Reserved. This should be the normal Chromium copyright. Anything coming under a different copyright must go in a third_party directory. https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:17: #include "third_party/webrtc/modules/desktop_capture/desktop_capture_options.h" You shouldn't need most of these #includes. Particularly the mac-specific headers. https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:42: /* probably not needed for now since these methods don't get called Remove this commented-out block. It's Mac-specific so it doesn't belong here anyway. https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... File remoting/host/window_capturer_screen_wrapper.h (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:1: #include "third_party/webrtc/modules/desktop_capture/screen_capturer.h" Missing copyright notice. https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:8: explicit WindowCapturerScreenWrapper(); "explicit" not needed. https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:16: // WindowCapturer interface. This is confusing, because this class doesn't actually derive in any way from WindowCapturer. I don't think these methods should be declared virtual. https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:29: static WindowCapturerScreenWrapper* Create(); This method isn't needed. Remove it, and update the call-site to call "new WindowCapturerScreenWrapper()" directly.
Also, please update the Subject so instead of saying "Description" it says something like "Add command-line option to remoting_me2me_host to enable window capturing." Use the "Edit Issue" link in the top-left corner.
I updated the code with comments and Lambros' comments. I updated everything except for the redesign. I also was a bit confused about the scoped_ptrs for CFRefTypes. I had a question about that for you and it is in the comment replies. https://codereview.chromium.org/422503004/diff/20001/remoting/host/desktop_en... File remoting/host/desktop_environment.h (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/desktop_en... remoting/host/desktop_environment.h:15: #include "third_party/webrtc/modules/desktop_capture/screen_capturer.h" On 2014/07/30 00:14:48, Lambros wrote: > Undo this, and restore "class ScreenCapturer". Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/host_main.cc File remoting/host/host_main.cc (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/host_main.... remoting/host/host_main.cc:80: " --window-Id=<id> - Specifies that a window should be streamed.\n"; On 2014/07/30 00:14:48, Lambros wrote: > Please use lower-case: window-id > Also, you don't need the extra spaces here. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... File remoting/host/input_injector.h (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector.h:30: static scoped_ptr<InputInjector> Create( On 2014/07/30 00:14:48, Lambros wrote: > Name this "CreateForWindow". Chromium style doesn't allow overloading. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector.h:34: bool enable_window_capture); On 2014/07/30 00:14:48, Lambros wrote: > This boolean flag is redundant. The calling code should call this method or the > other method to specify what kind of InputInjector to create. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... File remoting/host/input_injector_mac.cc (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:79: virtual void SetWindowId(webrtc::WindowId windowId); On 2014/07/30 00:14:48, Lambros wrote: > Do these need to be virtual? Nope, changed. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:80: virtual void SetEnableWindowCapture(bool enable_window_capture); On 2014/07/30 00:14:48, Lambros wrote: > Don't say "Capture" here. Maybe replace these methods with a single method > called EnableWindowInjection(webrtc::WindowId window_id) > or similar? > > You could maybe add this method to the InputInjector interface, and make it a > no-op with NOTIMPLEMENTED() in the Windows and Linux implementations? > Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:83: // TODO(sidj, sskhandp) put these inside of WindowInputInjectorMac. On 2014/07/30 00:14:49, Lambros wrote: > Remove TODO - I don't think it's worth defining a separate class for this. It > should be a simple modification to the existing classes to support per-window > injection on each platform. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:84: CGWindowID windowId_input_injector_mac_; On 2014/07/30 00:14:49, Lambros wrote: > nit: window_id_ Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:85: bool enable_window_capture_injector_mac_; On 2014/07/30 00:14:48, Lambros wrote: > nit: window_injection_enabled_ - don't use "capture" or "mac". And field names > should be nouns, not verbs. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:102: // TODO(sidj, sskhandp) Remove these methods and use On 2014/07/30 00:14:48, Lambros wrote: > Remove TODO. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:120: // TODO(sidj, sskhandp) put these inside of WindowInputInjectorMac. On 2014/07/30 00:14:48, Lambros wrote: > Remove TODO. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:277: CFRelease(window_id_array); On 2014/07/30 00:14:49, Lambros wrote: > Use ScopedCFTypeRef from base/mac/scoped_cftyperef.h instead of manually calling > CFRelease here. I've been a bit confused on the scoped_X types. Do these types take care of the releasing and retaining for me? https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:425: // TODO(sskhandp, sidj) remove this method and make WindowInputInjector On 2014/07/30 00:14:48, Lambros wrote: > Remove TODO. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:431: InputInjectorMac* injector = new InputInjectorMac(main_task_runner); On 2014/07/30 00:14:49, Lambros wrote: > nit: Declare |injector| as scoped_ptr instead of raw pointer. If you want, you > could use the existing Create() method here. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... File remoting/host/me2me_window_desktop_environment.cc (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/07/30 00:14:49, Lambros wrote: > "(c) 2012" -> "2014" Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:7: #if defined(OS_POSIX) On 2014/07/30 00:14:49, Lambros wrote: > Don't need this OS_POSIX block. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:12: #include "base/logging.h" On 2014/07/30 00:14:49, Lambros wrote: > Do we really need all these #includes? For example, this file doesn't seem to > use gnubby_auth_handler.h. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... File remoting/host/me2me_window_desktop_environment.h (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/07/30 00:14:49, Lambros wrote: > I'm not sure if we need this new class? Maybe it's possible to tweak the > existing BasicDesktopEnvironment to allow some kind of dependency-injection of > Window-based capturers and injectors? See my other comment in remoting_me2me_host.cc :)! https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:9: #include "remoting/host/input_injector.h" On 2014/07/30 00:14:49, Lambros wrote: > nit: Don't need full definition of InputInjector class. Remove this #include, > and add forward-declaration: "class InputInjector;" instead, just below > "namespace remoting {" separated by a blank line. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:42: class Me2MeWindowDesktopEnvironmentFactory : public BasicDesktopEnvironmentFactory { On 2014/07/30 00:14:49, Lambros wrote: > This breaks 80-char limit. Put ": public BasicD..." on next line, indented by 4 > spaces. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/remoting_m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:312: webrtc::WindowId windowId_; On 2014/07/30 00:14:49, Lambros wrote: > nit: window_id_ Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:462: unsigned temp; On 2014/07/30 00:14:49, Lambros wrote: > nit: window_id is a better variable name. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:466: // any precision here. On 2014/07/30 00:14:49, Lambros wrote: > You could verify this with an assertion. Use COMPILE_ASSERT from base/macros.h, > together with the sizeof() operator. What exactly would I want to check? That window_id_ == window_id? https://codereview.chromium.org/422503004/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:467: windowId_ = (webrtc::WindowId) temp; On 2014/07/30 00:14:49, Lambros wrote: > nit: static_cast<webrtc::WindowId>(temp) Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:705: new Me2MeWindowDesktopEnvironmentFactory( On 2014/07/30 00:14:49, Lambros wrote: > This is the reason I'd prefer not to have yet another new Factory class. It > complicates code like this. On Windows, will we need to create a new > IpcWindowDesktopEnvironmentFactory class to deal with both IPC *and* windowing? > It's getting out of hand :) > I do suppose we could add window functionality to the current environment. Sergey and I had discussed this design pattern so this is how I approached it. Perhaps you, Sundeep, and Sid could have a discussion on the best way to design this? Let me know how urgent it would be. I can try to get this done in another CL before I leave. https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... File remoting/host/window_capturer_screen_wrapper.cc (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:2: * Copyright (c) 2013 The WebRTC project authors. All Rights Reserved. On 2014/07/30 00:14:50, Lambros wrote: > This should be the normal Chromium copyright. Anything coming under a different > copyright must go in a third_party directory. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:17: #include "third_party/webrtc/modules/desktop_capture/desktop_capture_options.h" On 2014/07/30 00:14:50, Lambros wrote: > You shouldn't need most of these #includes. Particularly the mac-specific > headers. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:42: /* probably not needed for now since these methods don't get called On 2014/07/30 00:14:50, Lambros wrote: > Remove this commented-out block. It's Mac-specific so it doesn't belong here > anyway. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... File remoting/host/window_capturer_screen_wrapper.h (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:1: #include "third_party/webrtc/modules/desktop_capture/screen_capturer.h" On 2014/07/30 00:14:50, Lambros wrote: > Missing copyright notice. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:8: explicit WindowCapturerScreenWrapper(); On 2014/07/30 00:14:50, Lambros wrote: > "explicit" not needed. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:16: // WindowCapturer interface. On 2014/07/30 00:14:50, Lambros wrote: > This is confusing, because this class doesn't actually derive in any way from > WindowCapturer. I don't think these methods should be declared virtual. Done. https://codereview.chromium.org/422503004/diff/20001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:29: static WindowCapturerScreenWrapper* Create(); On 2014/07/30 00:14:50, Lambros wrote: > This method isn't needed. Remove it, and update the call-site to call "new > WindowCapturerScreenWrapper()" directly. Done.
Forget to "Done" one of the comments. https://codereview.chromium.org/422503004/diff/20001/remoting/host/host_main.cc File remoting/host/host_main.cc (right): https://codereview.chromium.org/422503004/diff/20001/remoting/host/host_main.... remoting/host/host_main.cc:80: " --window-Id=<id> - Specifies that a window should be streamed.\n"; On 2014/07/30 00:14:48, Lambros wrote: > Please use lower-case: window-id > Also, you don't need the extra spaces here. Done.
In terms of design the changes I'd like to see are: 1. Strip down the ScreenCapturer-from-WindowCapturer adapter. - Chromoting should remove that once the mouse shape changes have landed, since we can use the DesktopCapturer interface directly. 2. Move the windowed input injector into a wrapper around the desktop input injector. - Chromoting should simplify that to use InputFilter once we've got rid of InputInjector. 3. Give the various classes more appropriate names, e.g. SingleWindowDesktopEnvironment. https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... File remoting/host/input_injector.h (right): https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... remoting/host/input_injector.h:37: webrtc::WindowId window_id); InputInjector is an interface; you can pull the input injection implementation for windows out to a separate header/implementation file. https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... remoting/host/input_injector.h:37: webrtc::WindowId window_id); All your windowed input-injection implementation does is find the top-left corner of the window and adjust the input event coordinates accordingly. You can break that out into a separate WindowedInputInjector that creates an InputInjector internally and just proxies ClipboardStub and InputStub APIs back and forth (that bit is an annoying artefact of InputInjector bundling both APIs into one object - you should be able to use InputFilter for this instead ): and adjusts the mouse coordinates before passing them on to the real InputInjector. https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... remoting/host/input_injector.h:45: void EnableWindowInjection(webrtc::WindowId window_id); Looks like this got here by mistake? https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... File remoting/host/input_injector_mac.cc (right): https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:80: CGWindowID window_id_input_injector_mac_; Members appear after type definitions (see style guide :) https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:153: window_id_input_injector_mac_ = (CGWindowID) window_id; Why do you need to store this in the InputInjector - isn't the Core storing it already? https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:329: CGRectGetMinY(windowRect))); Do you mean to clamp input to the window width & height as well? What happens if the window is resized on the host, mid-session? https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... File remoting/host/me2me_window_desktop_environment.cc (right): https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:16: DCHECK(caller_task_runner()->BelongsToCurrentThread()); ~BasicDesktopEnvironment already checks this. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:21: LOG(INFO) << "video capturer created"; Remove this log line. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:22: DCHECK(caller_task_runner()->BelongsToCurrentThread()); Please add blank lines to break this function up into logical blocks, e.g. one after this DCHECK, one after the |options| are created and configured and one before returning the result, to make it easier to read. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:50: DCHECK(caller_task_runner->BelongsToCurrentThread()); This check probably belongs in the BasicDesktopEnvironment ctor! https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:54: (webrtc::WindowId windowIdEnvironment) { Check the thread here. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:54: (webrtc::WindowId windowIdEnvironment) { Wrap after the ( not before. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:74: LOG(INFO) << "desktop environment created"; nit: Remove this logging. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... File remoting/host/me2me_window_desktop_environment.h (right): https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: No (c) please. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:9: #include "remoting/host/input_injector.h" Why do you need this? https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:15: class Me2MeWindowDesktopEnvironment : Suggest SingleWindowDesktopEnvironment https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:15: class Me2MeWindowDesktopEnvironment : This is a definition of a class that is only creatable via the factory, below - does move it into the cc? https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:34: webrtc::WindowId windowIdEnvironment_; Do you mean window_id_? https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:40: class Me2MeWindowDesktopEnvironmentFactory : public BasicDesktopEnvironmentFactory { Line too long; presubmit should have caught this? https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:54: webrtc::WindowId windowIdEnvironmentFactory_; Do you mean window_id_? https://codereview.chromium.org/422503004/diff/60001/remoting/host/remoting_m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:462: unsigned window_id; Unsigned what? Why not uint32_t? https://codereview.chromium.org/422503004/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:464: kWindowIdSwitchName), &window_id)) { kWindowIdSwitchName should be indented by 4 spaces from the start of the call name, if need be, i.e. from level with |cmd_line->...| However, in this case kWindowIdSwitchName should fit on the line after ASCII, and then wrap for &window_id, or just wrap before cmd_line. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... File remoting/host/window_capturer_screen_wrapper.cc (right): https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:9: WindowCapturerScreenWrapper::WindowCapturerScreenWrapper() { You're not initializing the |window_capturer_| bare pointer! https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:25: window_capturer_->Start(callback); When you switch |window_capturer_| to scoped_ptr<>, not only will it be auto-initialized to NULL, it'll also DCHECK if you try to dereference it when it's NULL. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:33: MouseShapeObserver* mouse_shape_observer) { NOTIMPLEMENTED(); https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:36: bool WindowCapturerScreenWrapper::GetScreenList(ScreenList* screens) { NOTIMPLENTED(); https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:40: bool WindowCapturerScreenWrapper::SelectScreen(webrtc::ScreenId id) { NOTIMPLEMENTED(); https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... File remoting/host/window_capturer_screen_wrapper.h (right): https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. Lose the (c), please. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:6: #include "third_party/webrtc/modules/desktop_capture/window_capturer.h" You don't need this include, I don't think; just forward-declare webrtc::WindowCapturer. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:15: // wrapped WindowCapturer. Do you actually need to proxy these calls through? Couldn't you just have the caller create the underlying webrtc::WindowCapturer and construct this class to wrap it, thereby avoiding this class needing any WindowCapturer-like methods? https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:15: // wrapped WindowCapturer. You shouldn't need this class at all, in fact; the only reason the Chromoting host requires a ScreenCapturer is so that it can register for mouse-shape change notifications - that's something we're in the process of cleaning up, though, at which point you'll be able to return a DesktopCapturer from DesktopEnvironment rather than a ScreenCapturer, so you can then return the WindowCapturer directly. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:16: class WindowCapturerScreenWrapper : public ::webrtc::ScreenCapturer { Lose the :: before webrtc. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:21: // WindowCapturer interface. This class doesn't provide the WindowCapturer, interface, though? https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:23: void SetWindow(const webrtc::DesktopCaptureOptions& options); What's the differencing between "selecting" and "setting" the window? Why does SetWindow actually take a set of DesktopCaptureOptions? https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:25: // DesktopCapturer interface. nit: webrtc::DesktopCapturer for consistency, or just fold this under the webrtc::ScreenCapturer interface comment. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:36: webrtc::WindowCapturer* window_capturer_; scoped_ptr<WindowCapturer>
P.S. The CL description also needs a more helpful title than "Description" ;)
I've updated the design of the InputInjector so that we now have a SingleWindowInputInjector. I also addressed all of Wez's comments. I updated the CFTypeRefs to be ScopedCFTypeRefs for all CFTypeRef types except for dictionaries. These seem to segfault/ get an error: "Trace/BPT trap: 5". I'm not sure what the deal is with that. Lambros, can you please review the FindCGRectOfWindow method again (it's in single_window_input_injector_mac.mm now) and let me know if you wanted me to update the dictionaries to ScopedCFTypeRef as well? https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... File remoting/host/input_injector.h (right): https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... remoting/host/input_injector.h:37: webrtc::WindowId window_id); On 2014/08/01 23:41:53, Wez wrote: > All your windowed input-injection implementation does is find the top-left > corner of the window and adjust the input event coordinates accordingly. > > You can break that out into a separate WindowedInputInjector that creates an > InputInjector internally and just proxies ClipboardStub and InputStub APIs back > and forth (that bit is an annoying artefact of InputInjector bundling both APIs > into one object - you should be able to use InputFilter for this instead ): and > adjusts the mouse coordinates before passing them on to the real InputInjector. Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... remoting/host/input_injector.h:37: webrtc::WindowId window_id); On 2014/08/01 23:41:53, Wez wrote: > All your windowed input-injection implementation does is find the top-left > corner of the window and adjust the input event coordinates accordingly. > > You can break that out into a separate WindowedInputInjector that creates an > InputInjector internally and just proxies ClipboardStub and InputStub APIs back > and forth (that bit is an annoying artefact of InputInjector bundling both APIs > into one object - you should be able to use InputFilter for this instead ): and > adjusts the mouse coordinates before passing them on to the real InputInjector. Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... remoting/host/input_injector.h:45: void EnableWindowInjection(webrtc::WindowId window_id); On 2014/08/01 23:41:53, Wez wrote: > Looks like this got here by mistake? I use this in the CreateForWindow method; I think it needs to be public? Should it just only be declared in the cc? https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... File remoting/host/input_injector_mac.cc (right): https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:80: CGWindowID window_id_input_injector_mac_; On 2014/08/01 23:41:53, Wez wrote: > Members appear after type definitions (see style guide :) Got it. Removed anyhow since it was not needed, as you noted below :). https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:153: window_id_input_injector_mac_ = (CGWindowID) window_id; On 2014/08/01 23:41:53, Wez wrote: > Why do you need to store this in the InputInjector - isn't the Core storing it > already? Don't. Removed! https://codereview.chromium.org/422503004/diff/60001/remoting/host/input_inje... remoting/host/input_injector_mac.cc:329: CGRectGetMinY(windowRect))); On 2014/08/01 23:41:54, Wez wrote: > Do you mean to clamp input to the window width & height as well? > > What happens if the window is resized on the host, mid-session? I don't know what you mean by clamp input to window width & height. What I'm trying to do is basically adjust for the fact that the the host receives coordinates in respect to the entire desktop rather than the window. I do this by calculating the top-left of the window we are capturing and adding those coordinates to the coordinates sent to the host from the client (which, again, are in respect to the desktop). This way the events are injected to the correct place on the host side (correct place = place on the window we are capturing that the user clicked on in the client). If the window gets resized, that is fine because I call FindCGRectOfWindow() before adjusting every time (right under the if (window_injection_enabled_) statement. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... File remoting/host/me2me_window_desktop_environment.cc (right): https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:16: DCHECK(caller_task_runner()->BelongsToCurrentThread()); On 2014/08/01 23:41:54, Wez wrote: > ~BasicDesktopEnvironment already checks this. Removed. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:21: LOG(INFO) << "video capturer created"; On 2014/08/01 23:41:54, Wez wrote: > Remove this log line. Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:22: DCHECK(caller_task_runner()->BelongsToCurrentThread()); On 2014/08/01 23:41:54, Wez wrote: > Please add blank lines to break this function up into logical blocks, e.g. one > after this DCHECK, one after the |options| are created and configured and one > before returning the result, to make it easier to read. Done. I also refactored the SetWindow method of WindowCapturerScreenWrapper to be called CreateWindowCapturerInstance so it does not get confused with the SelectWindow method. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:50: DCHECK(caller_task_runner->BelongsToCurrentThread()); On 2014/08/01 23:41:54, Wez wrote: > This check probably belongs in the BasicDesktopEnvironment ctor! Removed. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:54: (webrtc::WindowId windowIdEnvironment) { On 2014/08/01 23:41:54, Wez wrote: > Wrap after the ( not before. Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:54: (webrtc::WindowId windowIdEnvironment) { On 2014/08/01 23:41:54, Wez wrote: > Check the thread here. Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.cc:74: LOG(INFO) << "desktop environment created"; On 2014/08/01 23:41:54, Wez wrote: > nit: Remove this logging. Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... File remoting/host/me2me_window_desktop_environment.h (right): https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/08/01 23:41:54, Wez wrote: > nit: No (c) please. Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:9: #include "remoting/host/input_injector.h" On 2014/08/01 23:41:54, Wez wrote: > Why do you need this? I needed it for me2me_window_desktop_environment.cc. I've moved it over to that file instead. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:15: class Me2MeWindowDesktopEnvironment : On 2014/08/01 23:41:54, Wez wrote: > This is a definition of a class that is only creatable via the factory, below - > does move it into the cc? The reason I did it this way is because it's declared in the BasicDesktopEnvironment.h in this manner. I was told consistency trumps for coding style. Let me know if you'd still want me to change it. Reference: https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/basi... https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:15: class Me2MeWindowDesktopEnvironment : On 2014/08/01 23:41:54, Wez wrote: > Suggest SingleWindowDesktopEnvironment Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:34: webrtc::WindowId windowIdEnvironment_; On 2014/08/01 23:41:54, Wez wrote: > Do you mean window_id_? Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:40: class Me2MeWindowDesktopEnvironmentFactory : public BasicDesktopEnvironmentFactory { On 2014/08/01 23:41:54, Wez wrote: > Line too long; presubmit should have caught this? It did. Thought I had changed it. Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/me2me_wind... remoting/host/me2me_window_desktop_environment.h:54: webrtc::WindowId windowIdEnvironmentFactory_; On 2014/08/01 23:41:54, Wez wrote: > Do you mean window_id_? Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/remoting_m... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:462: unsigned window_id; On 2014/08/01 23:41:55, Wez wrote: > Unsigned what? Why not uint32_t? Not sure. I was following an example from somewhere I believe. Updated. https://codereview.chromium.org/422503004/diff/60001/remoting/host/remoting_m... remoting/host/remoting_me2me_host.cc:464: kWindowIdSwitchName), &window_id)) { On 2014/08/01 23:41:55, Wez wrote: > kWindowIdSwitchName should be indented by 4 spaces from the start of the call > name, if need be, i.e. from level with |cmd_line->...| > > However, in this case kWindowIdSwitchName should fit on the line after ASCII, > and then wrap for &window_id, or just wrap before cmd_line. Done. Wrapped after cmd_line. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... File remoting/host/window_capturer_screen_wrapper.cc (right): https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:9: WindowCapturerScreenWrapper::WindowCapturerScreenWrapper() { On 2014/08/01 23:41:55, Wez wrote: > You're not initializing the |window_capturer_| bare pointer! Changed to scoped_ptr. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:25: window_capturer_->Start(callback); On 2014/08/01 23:41:55, Wez wrote: > When you switch |window_capturer_| to scoped_ptr<>, not only will it be > auto-initialized to NULL, it'll also DCHECK if you try to dereference it when > it's NULL. Got it. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:33: MouseShapeObserver* mouse_shape_observer) { On 2014/08/01 23:41:55, Wez wrote: > NOTIMPLEMENTED(); This method gets called since we're using it as a ScreenCapturer. So, you can't have it call NOTIMPLEMENTED(), can you? https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:36: bool WindowCapturerScreenWrapper::GetScreenList(ScreenList* screens) { On 2014/08/01 23:41:55, Wez wrote: > NOTIMPLENTED(); Even though it returns a boolean? https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.cc:40: bool WindowCapturerScreenWrapper::SelectScreen(webrtc::ScreenId id) { On 2014/08/01 23:41:55, Wez wrote: > NOTIMPLEMENTED(); Even though it returns a boolean? https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... File remoting/host/window_capturer_screen_wrapper.h (right): https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/08/01 23:41:55, Wez wrote: > Lose the (c), please. Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:6: #include "third_party/webrtc/modules/desktop_capture/window_capturer.h" On 2014/08/01 23:41:55, Wez wrote: > You don't need this include, I don't think; just forward-declare > webrtc::WindowCapturer. Done. Added the include to the cc file. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:15: // wrapped WindowCapturer. On 2014/08/01 23:41:55, Wez wrote: > You shouldn't need this class at all, in fact; the only reason the Chromoting > host requires a ScreenCapturer is so that it can register for mouse-shape change > notifications - that's something we're in the process of cleaning up, though, at > which point you'll be able to return a DesktopCapturer from DesktopEnvironment > rather than a ScreenCapturer, so you can then return the WindowCapturer > directly. Yep. The original plan was to change the interface to a DesktopCapturer, rather than ScreenCapturer, but then I realized that the SetMouseShapeObserver method was needed for ScreenCapturer, and so Sergey and I discussed this solution. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:15: // wrapped WindowCapturer. On 2014/08/01 23:41:55, Wez wrote: > Do you actually need to proxy these calls through? > > Couldn't you just have the caller create the underlying webrtc::WindowCapturer > and construct this class to wrap it, thereby avoiding this class needing any > WindowCapturer-like methods? Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:16: class WindowCapturerScreenWrapper : public ::webrtc::ScreenCapturer { On 2014/08/01 23:41:55, Wez wrote: > Lose the :: before webrtc. Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:21: // WindowCapturer interface. On 2014/08/01 23:41:55, Wez wrote: > This class doesn't provide the WindowCapturer, interface, though? Removed. I used to have all the methods of WindowCapturer here. When I realized I didn't need them, I removed them. I removed the comment. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:23: void SetWindow(const webrtc::DesktopCaptureOptions& options); On 2014/08/01 23:41:55, Wez wrote: > What's the differencing between "selecting" and "setting" the window? > Why does SetWindow actually take a set of DesktopCaptureOptions? The SetWindow just creates the wrapped WindowCapturer instance. I updated this method to be called CreateWindowCapturerInstance. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:25: // DesktopCapturer interface. On 2014/08/01 23:41:55, Wez wrote: > nit: webrtc::DesktopCapturer for consistency, or just fold this under the > webrtc::ScreenCapturer interface comment. Done. https://codereview.chromium.org/422503004/diff/60001/remoting/host/window_cap... remoting/host/window_capturer_screen_wrapper.h:36: webrtc::WindowCapturer* window_capturer_; On 2014/08/01 23:41:55, Wez wrote: > scoped_ptr<WindowCapturer> Done.
https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... File remoting/host/single_window_input_injector_mac.mm (right): https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:1: // Copyright 2014 The Chromium Authors. All rights reserved. Does this need to be a .mm file? It doesn't seem to have any ObjC in it. https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:49: scoped_ptr<InputInjector> input_injector): nit: Move ':' to next line, and format like this: SingleWindowInputInjectorMac::SingleWindowInputInjectorMac( webrtc::WindowId window_id, scoped_ptr<InputInjector> input_injector) : window_id_(static_cast<CGWindowID>(window_id)), input_injector_(input_injector.Pass()) { } https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:50: window_id_((CGWindowID)window_id), nit: static_cast<CGWindowID>(window_id) (see formatting above) https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:123: if (window_array == NULL || CFArrayGetCount(window_array) == 0) { I think you can do 'window_array.is_null()' ? https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:130: CFDictionaryRef window = To answer your question, the raw CFDictonaryRef is correct here. A scoper would be wrong, as the ref is owned by the containing |window_array|, and it would be wrong for you to call CFRelease() on it. https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:131: reinterpret_cast<CFDictionaryRef>( nit: Use CFCast<> from base/mac/foundation_util.h https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:134: if (CFDictionaryContainsKey(window, kCGWindowBounds)) { nit: Use GetValueFromDictionary() from base/mac/foundation_util.h https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:135: CFDictionaryRef bounds = Same here. This ref is owned by the |window| dictionary. https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:139: CGRectMakeWithDictionaryRepresentation(bounds, &rect); Check that CGRectMakeWithDictionaryRepresentation succeeded before returning |rect|. Also, you can simplify this by returning the rect here instead of at the end of the method. Then you won't need the 2 'else' blocks below, and you can return CGRectNull at the end of this method. https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:153: scoped_ptr<SingleWindowInputInjectorMac>injector( nit: space between '>' and injector https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... File remoting/host/me2me_single_window_desktop_environment.cc (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:34: scoped_ptr<WindowCapturerScreenWrapper>window_capturer_wrapper( nit: space after '>'. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:44: scoped_ptr<InputInjector>input_injector( nit: space after '>' https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:60: webrtc::WindowId windowIdEnvironment) { window_id https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:86: input_task_runner(), nit: indentation https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... File remoting/host/me2me_single_window_desktop_environment.h (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:5: #ifndef REMOTING_HOST_WINDOW_DESKTOP_ENVIRONMENT_H_ nit: REMOTING_HOST_ME2ME_SINGLE_WINDOW_DESKTOP_ENVIRONMENT_H_ https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:15: public BasicDesktopEnvironment { nit: This will fit on one line. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:31: void SetWindowId(webrtc::WindowId windowIdEnvironment); nit: s/windowIdEnvironment/window_id (and also in .cc file). https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:131: extern const char kWindowIdSwitchName[] = "window-id"; nit: remove 'extern' https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:311: // Used to specify which window to stream, if enabled. nit: Blank line before this comment. https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:437: optional: remove this blank line https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:469: // Set enable_window_capture_ to false because we failed to get optional: I would prefer to avoid 'we' in comments and logging. It can be confusing for non-native English speakers. Maybe say 'The window id from the command line was not found. Log an error and stream the entire desktop instead.'? https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:472: LOG(ERROR) << "We wanted to stream a window but could not find its id."; Suggestion: LOG(ERROR) << "Window " << window_id_ << " not found, streaming entire desktop instead." https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... File remoting/host/single_window_input_injector.h (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector.h:8: #include "base/memory/ref_counted.h" Many of these headers aren't needed. Here, you just need: * scoped_ptr.h for scoped_ptr. * input_injector.h for base class definition. * webrtc header for webrtc::WindowId typedef. Any other headers you need for the .cc belong in the .cc. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector.h:16: class SingleThreadTaskRunner; Remove this. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector.h:21: class SingleWindowInputInjector : public InputInjector { Please add a comment here explaining the purpose of this class. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... File remoting/host/single_window_input_injector_mac.mm (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:93: // find the window on the host it returns is coordinates in Density s/is/its s/conver/convert https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:134: // We don't use ScopedCFTypeRef for |window| because the I don't think this comment is needed. https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... File remoting/host/window_capturer_screen_wrapper.cc (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.cc:5: #include "remoting/host/window_capturer_screen_wrapper.h" nit: Blank line after this, as it's logically distinct from the other headers. https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.cc:29: return; Should be NOT_IMPLEMENTED() ? nit: remove 'return;' https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.cc:31: bool WindowCapturerScreenWrapper::GetScreenList(ScreenList* screens) { nit: Blank line between methods. https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... File remoting/host/window_capturer_screen_wrapper.h (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.h:16: // is becaues the remoting_me2me_host uses a screen capturer interface. typo: because https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.h:25: virtual ~WindowCapturerScreenWrapper(); nit: one space https://codereview.chromium.org/422503004/diff/160001/remoting/remoting_host.... File remoting/remoting_host.gypi (right): https://codereview.chromium.org/422503004/diff/160001/remoting/remoting_host.... remoting/remoting_host.gypi:253: 'host/window_capturer_screen_wrapper.cc', nit: I think these should come after 'host/win/' ?
https://codereview.chromium.org/422503004/diff/160001/remoting/host/host_main.cc File remoting/host/host_main.cc (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/host_main... remoting/host/host_main.cc:80: " --window-id=<id> - Specifies that a window should be streamed.\n"; nit: "Specifies a window to remote, instead of the whole desktop." https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... File remoting/host/me2me_single_window_desktop_environment.cc (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:5: #include "remoting/host/me2me_single_window_desktop_environment.h" As for the header, this file needs to move to single_window_desktop_environment.cc https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:23: // Use the default capturing options with the WindowCapturer nit: You don't really need this comment. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:28: // Create a WindowCapturer Nor this one. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:33: // Wrap WindowCapturer in a ScreenCapturer interface Update this comment to explicitly state that we have to do this because DesktopEnvironment returns one, and that in future we can return it directly. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:69: webrtc::WindowId windowId) window_id https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:88: desktop_environment->SetWindowId(window_id_); Can this go in the ctor? https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... File remoting/host/me2me_single_window_desktop_environment.h (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. This file should be called single_window_desktop_environment.h, and similarly for the .cc; ideally re-name the files in a separate patch-set from any other changes, so that reviewers can see the diffs clearly. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:5: #ifndef REMOTING_HOST_WINDOW_DESKTOP_ENVIRONMENT_H_ On 2014/08/05 22:47:24, Lambros wrote: > nit: REMOTING_HOST_ME2ME_SINGLE_WINDOW_DESKTOP_ENVIRONMENT_H_ REMOTING_HOST_SINGLE_WINDOW_DESKTOP_ENVIRONMENT_H_ https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:14: class SingleWindowDesktopEnvironment : This doesn't need to be defined in the header - it can be moved into the cc file. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:31: void SetWindowId(webrtc::WindowId windowIdEnvironment); Why not pass this directly to the ctor, like you do with the factory? https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:36: DISALLOW_COPY_AND_ASSIGN(SingleWindowDesktopEnvironment); nit: Indentation should be two spaces. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:39: // Used to create |SingleWindowDesktopEnvironment| instances. This is already implicit in the name of the class. Suggest: "Passed to the ChromotingHost to remote an individual window's contents, rather than a whole desktop." https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:40: class SingleWindowDesktopEnvironmentFactory : nit: Wrap before the : https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:47: webrtc::WindowId windowId); window_id https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:57: DISALLOW_COPY_AND_ASSIGN(SingleWindowDesktopEnvironmentFactory); nit: Indentation should be two spaces. https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:458: enable_window_capture_ = cmd_line->HasSwitch(kWindowIdSwitchName); nit: Blank line before this, to keep the window-capture option block of code visually separated from signal_parent https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:460: // CGWindowId is a uint32_t This comment doesn't seem relevant? https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:465: // window_id_ is of type long int, so we should not lose Doesn't its type depend upon the platform, since it's the WindowId define? https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... File remoting/host/single_window_input_injector.h (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector.h:25: scoped_ptr<InputInjector> input_injector); nit: Either clarify that |input_injector| normally needs to be passed a full-desktop InputInjector, or just create it internally in the SingleWindowInputInjector. :) https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... File remoting/host/single_window_input_injector_mac.mm (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:11: #include "remoting/proto/internal.pb.h" I think you want to include the event.pb.h header, rather than internal https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:21: // A class to generate events to a window on Mac. nit: Suggest: "Implements InputInjector to direct input events to a specific window." Also consider clarifying that clipboard injection will still be global. This comment seems more appropriate on the SingleWindowInputInjector base class, in the header, in fact. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:73: LOG(ERROR) << "Failing silently and just forwarding event"; nit: Suggest "Window rect is null, so ignoring MouseEvent" https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:76: } nit: Blank line after this, to separate it from the next logical block of code. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:82: webrtc::DesktopVector mouse_pos(CGRectGetMinX(windowRect), mouse_pos -> window_origin, then? https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:86: // desktops. They get added in already in InputInjector. Suggest: "The underlying InputInjector expects coordinates relative to the top-left of the top-left-most monitor, so translate the window origin to that coordinate scheme." https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... File remoting/host/window_capturer_screen_wrapper.h (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.h:21: class WindowCapturerScreenWrapper : public webrtc::ScreenCapturer { Suggest WindowToScreenCapturerAdapter https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.h:21: class WindowCapturerScreenWrapper : public webrtc::ScreenCapturer { Note that we should have the change landed within the next day or two that will remove the need for this adapter.
I refactored the me2me_single_window_desktop_environment to single_window_desktop_environment. I updated and addressed the other comments as well. I did not refactor the window_capturer_screen_wrapper since it looks like this will be removed in the next day or so. https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... File remoting/host/single_window_input_injector_mac.mm (right): https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/08/05 22:47:24, Lambros wrote: > Does this need to be a .mm file? It doesn't seem to have any ObjC in it. Guess not! I just saw that every mac file was .mm so I made it .mm. https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:49: scoped_ptr<InputInjector> input_injector): On 2014/08/05 22:47:24, Lambros wrote: > nit: Move ':' to next line, and format like this: > SingleWindowInputInjectorMac::SingleWindowInputInjectorMac( > webrtc::WindowId window_id, > scoped_ptr<InputInjector> input_injector) > : window_id_(static_cast<CGWindowID>(window_id)), > input_injector_(input_injector.Pass()) { > } Done. https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:50: window_id_((CGWindowID)window_id), On 2014/08/05 22:47:24, Lambros wrote: > nit: static_cast<CGWindowID>(window_id) > (see formatting above) Done. https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:123: if (window_array == NULL || CFArrayGetCount(window_array) == 0) { On 2014/08/05 22:47:23, Lambros wrote: > I think you can do 'window_array.is_null()' ? I don't think ScopedCFTypeRef has this method implemented and neither does CFArrayRef. https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:130: CFDictionaryRef window = On 2014/08/05 22:47:23, Lambros wrote: > To answer your question, the raw CFDictonaryRef is correct here. A scoper would > be wrong, as the ref is owned by the containing |window_array|, and it would be > wrong for you to call CFRelease() on it. Acknowledged. https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:131: reinterpret_cast<CFDictionaryRef>( On 2014/08/05 22:47:23, Lambros wrote: > nit: Use CFCast<> from base/mac/foundation_util.h Done. https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:134: if (CFDictionaryContainsKey(window, kCGWindowBounds)) { On 2014/08/05 22:47:24, Lambros wrote: > nit: Use GetValueFromDictionary() from base/mac/foundation_util.h Done. https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:135: CFDictionaryRef bounds = On 2014/08/05 22:47:23, Lambros wrote: > Same here. This ref is owned by the |window| dictionary. Acknowledged. https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:139: CGRectMakeWithDictionaryRepresentation(bounds, &rect); On 2014/08/05 22:47:24, Lambros wrote: > Check that CGRectMakeWithDictionaryRepresentation succeeded before returning > |rect|. > > Also, you can simplify this by returning the rect here instead of at the end of > the method. Then you won't need the 2 'else' blocks below, and you can return > CGRectNull at the end of this method. Done. https://codereview.chromium.org/422503004/diff/140001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:153: scoped_ptr<SingleWindowInputInjectorMac>injector( On 2014/08/05 22:47:24, Lambros wrote: > nit: space between '>' and injector Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/host_main.cc File remoting/host/host_main.cc (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/host_main... remoting/host/host_main.cc:80: " --window-id=<id> - Specifies that a window should be streamed.\n"; On 2014/08/06 04:10:15, Wez wrote: > nit: "Specifies a window to remote, instead of the whole desktop." It seems like there's been an exception made when these lines are > 80 characters. Is this true? https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... File remoting/host/me2me_single_window_desktop_environment.cc (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:5: #include "remoting/host/me2me_single_window_desktop_environment.h" On 2014/08/06 04:10:15, Wez wrote: > As for the header, this file needs to move to > single_window_desktop_environment.cc What do you mean? Just needs to be renamed? https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:23: // Use the default capturing options with the WindowCapturer On 2014/08/06 04:10:15, Wez wrote: > nit: You don't really need this comment. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:28: // Create a WindowCapturer On 2014/08/06 04:10:15, Wez wrote: > Nor this one. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:33: // Wrap WindowCapturer in a ScreenCapturer interface On 2014/08/06 04:10:15, Wez wrote: > Update this comment to explicitly state that we have to do this because > DesktopEnvironment returns one, and that in future we can return it directly. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:34: scoped_ptr<WindowCapturerScreenWrapper>window_capturer_wrapper( On 2014/08/05 22:47:24, Lambros wrote: > nit: space after '>'. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:44: scoped_ptr<InputInjector>input_injector( On 2014/08/05 22:47:24, Lambros wrote: > nit: space after '>' Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:60: webrtc::WindowId windowIdEnvironment) { On 2014/08/05 22:47:24, Lambros wrote: > window_id removed method. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:69: webrtc::WindowId windowId) On 2014/08/06 04:10:15, Wez wrote: > window_id Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:86: input_task_runner(), On 2014/08/05 22:47:24, Lambros wrote: > nit: indentation Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.cc:88: desktop_environment->SetWindowId(window_id_); On 2014/08/06 04:10:15, Wez wrote: > Can this go in the ctor? Yep, done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... File remoting/host/me2me_single_window_desktop_environment.h (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/08/06 04:10:16, Wez wrote: > This file should be called single_window_desktop_environment.h, and similarly > for the .cc; ideally re-name the files in a separate patch-set from any other > changes, so that reviewers can see the diffs clearly. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:5: #ifndef REMOTING_HOST_WINDOW_DESKTOP_ENVIRONMENT_H_ On 2014/08/06 04:10:16, Wez wrote: > On 2014/08/05 22:47:24, Lambros wrote: > > nit: REMOTING_HOST_ME2ME_SINGLE_WINDOW_DESKTOP_ENVIRONMENT_H_ > > REMOTING_HOST_SINGLE_WINDOW_DESKTOP_ENVIRONMENT_H_ Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:5: #ifndef REMOTING_HOST_WINDOW_DESKTOP_ENVIRONMENT_H_ On 2014/08/05 22:47:24, Lambros wrote: > nit: REMOTING_HOST_ME2ME_SINGLE_WINDOW_DESKTOP_ENVIRONMENT_H_ Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:14: class SingleWindowDesktopEnvironment : On 2014/08/06 04:10:16, Wez wrote: > This doesn't need to be defined in the header - it can be moved into the cc > file. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:15: public BasicDesktopEnvironment { On 2014/08/05 22:47:24, Lambros wrote: > nit: This will fit on one line. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:31: void SetWindowId(webrtc::WindowId windowIdEnvironment); On 2014/08/05 22:47:24, Lambros wrote: > nit: s/windowIdEnvironment/window_id > (and also in .cc file). Just removed it as wez suggested. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:31: void SetWindowId(webrtc::WindowId windowIdEnvironment); On 2014/08/06 04:10:16, Wez wrote: > Why not pass this directly to the ctor, like you do with the factory? I believe this was just easier for me at the time before I had figured out/ was comfortable with initialization lists. Updated it. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:36: DISALLOW_COPY_AND_ASSIGN(SingleWindowDesktopEnvironment); On 2014/08/06 04:10:16, Wez wrote: > nit: Indentation should be two spaces. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:39: // Used to create |SingleWindowDesktopEnvironment| instances. On 2014/08/06 04:10:16, Wez wrote: > This is already implicit in the name of the class. Suggest: > > "Passed to the ChromotingHost to remote an individual window's contents, rather > than a whole desktop." Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:40: class SingleWindowDesktopEnvironmentFactory : On 2014/08/06 04:10:15, Wez wrote: > nit: Wrap before the : Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:47: webrtc::WindowId windowId); On 2014/08/06 04:10:15, Wez wrote: > window_id Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/me2me_sin... remoting/host/me2me_single_window_desktop_environment.h:57: DISALLOW_COPY_AND_ASSIGN(SingleWindowDesktopEnvironmentFactory); On 2014/08/06 04:10:15, Wez wrote: > nit: Indentation should be two spaces. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:131: extern const char kWindowIdSwitchName[] = "window-id"; On 2014/08/05 22:47:24, Lambros wrote: > nit: remove 'extern' Ah, I had this for when I thought I was going to use this directly in input_injector_mac.mm to redirect the flow of the code. https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:311: // Used to specify which window to stream, if enabled. On 2014/08/05 22:47:24, Lambros wrote: > nit: Blank line before this comment. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:437: On 2014/08/05 22:47:25, Lambros wrote: > optional: remove this blank line Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:458: enable_window_capture_ = cmd_line->HasSwitch(kWindowIdSwitchName); On 2014/08/06 04:10:16, Wez wrote: > nit: Blank line before this, to keep the window-capture option block of code > visually separated from signal_parent Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:460: // CGWindowId is a uint32_t On 2014/08/06 04:10:16, Wez wrote: > This comment doesn't seem relevant? i'm not sure how window ids are represented on other platforms. Will that matter in the future? What if someone decides to make it a short since that's how windows or linux gets their window ids? https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:465: // window_id_ is of type long int, so we should not lose On 2014/08/06 04:10:16, Wez wrote: > Doesn't its type depend upon the platform, since it's the WindowId define? I'm not sure what the best way would be to get the window_id_ in correctly for all platforms. Should I have a #if defined(...) for each platform? https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:469: // Set enable_window_capture_ to false because we failed to get On 2014/08/05 22:47:25, Lambros wrote: > optional: I would prefer to avoid 'we' in comments and logging. It can be > confusing for non-native English speakers. Maybe say 'The window id from the > command line was not found. Log an error and stream the entire desktop > instead.'? > Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:472: LOG(ERROR) << "We wanted to stream a window but could not find its id."; On 2014/08/05 22:47:25, Lambros wrote: > Suggestion: > LOG(ERROR) << "Window " << window_id_ > << " not found, streaming entire desktop instead." Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... File remoting/host/single_window_input_injector.h (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector.h:8: #include "base/memory/ref_counted.h" On 2014/08/05 22:47:25, Lambros wrote: > Many of these headers aren't needed. > > Here, you just need: > * scoped_ptr.h for scoped_ptr. > * input_injector.h for base class definition. > * webrtc header for webrtc::WindowId typedef. > > Any other headers you need for the .cc belong in the .cc. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector.h:16: class SingleThreadTaskRunner; On 2014/08/05 22:47:25, Lambros wrote: > Remove this. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector.h:21: class SingleWindowInputInjector : public InputInjector { On 2014/08/05 22:47:25, Lambros wrote: > Please add a comment here explaining the purpose of this class. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector.h:25: scoped_ptr<InputInjector> input_injector); On 2014/08/06 04:10:16, Wez wrote: > nit: Either clarify that |input_injector| normally needs to be passed a > full-desktop InputInjector, or just create it internally in the > SingleWindowInputInjector. :) Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... File remoting/host/single_window_input_injector_mac.mm (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:11: #include "remoting/proto/internal.pb.h" On 2014/08/06 04:10:16, Wez wrote: > I think you want to include the event.pb.h header, rather than internal Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:21: // A class to generate events to a window on Mac. On 2014/08/06 04:10:16, Wez wrote: > nit: Suggest: > > "Implements InputInjector to direct input events to a specific window." > > Also consider clarifying that clipboard injection will still be global. > > This comment seems more appropriate on the SingleWindowInputInjector base class, > in the header, in fact. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:73: LOG(ERROR) << "Failing silently and just forwarding event"; On 2014/08/06 04:10:16, Wez wrote: > nit: Suggest "Window rect is null, so ignoring MouseEvent" Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:76: } On 2014/08/06 04:10:16, Wez wrote: > nit: Blank line after this, to separate it from the next logical block of code. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:82: webrtc::DesktopVector mouse_pos(CGRectGetMinX(windowRect), On 2014/08/06 04:10:16, Wez wrote: > mouse_pos -> window_origin, then? Yep, you're right. We were doing it this way in our VNC server, so I just kept that same style. This is more explicit though. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:86: // desktops. They get added in already in InputInjector. On 2014/08/06 04:10:16, Wez wrote: > Suggest: "The underlying InputInjector expects coordinates relative to the > top-left of the top-left-most monitor, so translate the window origin to that > coordinate scheme." Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:93: // find the window on the host it returns is coordinates in Density On 2014/08/05 22:47:25, Lambros wrote: > s/is/its > s/conver/convert Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.mm:134: // We don't use ScopedCFTypeRef for |window| because the On 2014/08/05 22:47:25, Lambros wrote: > I don't think this comment is needed. When I had a discussion with Wez, he asked me to document this. https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... File remoting/host/window_capturer_screen_wrapper.cc (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.cc:5: #include "remoting/host/window_capturer_screen_wrapper.h" On 2014/08/05 22:47:25, Lambros wrote: > nit: Blank line after this, as it's logically distinct from the other headers. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.cc:29: return; On 2014/08/05 22:47:25, Lambros wrote: > Should be NOT_IMPLEMENTED() ? > nit: remove 'return;' Right now, we call this method. Is it alright if NOTIMPLEMENTED() gets called in the code? It logs an error, but I'm not sure if that's okay for our purposes right now. https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.cc:31: bool WindowCapturerScreenWrapper::GetScreenList(ScreenList* screens) { On 2014/08/05 22:47:25, Lambros wrote: > nit: Blank line between methods. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... File remoting/host/window_capturer_screen_wrapper.h (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.h:16: // is becaues the remoting_me2me_host uses a screen capturer interface. On 2014/08/05 22:47:25, Lambros wrote: > typo: because Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.h:21: class WindowCapturerScreenWrapper : public webrtc::ScreenCapturer { On 2014/08/06 04:10:16, Wez wrote: > Suggest WindowToScreenCapturerAdapter I'll just leave it for now since it is going to be changed very soon. https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.h:21: class WindowCapturerScreenWrapper : public webrtc::ScreenCapturer { On 2014/08/06 04:10:16, Wez wrote: > Suggest WindowToScreenCapturerAdapter Acknowledged. https://codereview.chromium.org/422503004/diff/160001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.h:25: virtual ~WindowCapturerScreenWrapper(); On 2014/08/05 22:47:25, Lambros wrote: > nit: one space Done. https://codereview.chromium.org/422503004/diff/160001/remoting/remoting_host.... File remoting/remoting_host.gypi (right): https://codereview.chromium.org/422503004/diff/160001/remoting/remoting_host.... remoting/remoting_host.gypi:253: 'host/window_capturer_screen_wrapper.cc', On 2014/08/05 22:47:25, Lambros wrote: > nit: I think these should come after 'host/win/' ? Done.
LGTM :) https://codereview.chromium.org/422503004/diff/160001/remoting/host/host_main.cc File remoting/host/host_main.cc (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/host_main... remoting/host/host_main.cc:80: " --window-id=<id> - Specifies that a window should be streamed.\n"; On 2014/08/06 20:56:17, ronakvora wrote: > On 2014/08/06 04:10:15, Wez wrote: > > nit: "Specifies a window to remote, instead of the whole desktop." > > It seems like there's been an exception made when these lines are > 80 > characters. Is this true? Not sure what you mean? An exception to what? https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:460: // CGWindowId is a uint32_t On 2014/08/06 20:56:19, ronakvora wrote: > On 2014/08/06 04:10:16, Wez wrote: > > This comment doesn't seem relevant? > > i'm not sure how window ids are represented on other platforms. Will that matter > in the future? What if someone decides to make it a short since that's how > windows or linux gets their window ids? This is cross-platform code; it shouldn't have a comment that assumes it's being built on a particular platform - you could leave a comment to clarify that unit32_t is large enough to hold window-ids on all of our currently-supported platforms, if you like. https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:465: // window_id_ is of type long int, so we should not lose On 2014/08/06 20:56:18, ronakvora wrote: > On 2014/08/06 04:10:16, Wez wrote: > > Doesn't its type depend upon the platform, since it's the WindowId define? > > I'm not sure what the best way would be to get the window_id_ in correctly for > all platforms. Should I have a #if defined(...) for each platform? What's important is the effect of truncation in precision - in this case it'll mean we get a bogus window Id. If we were going to use it as an index into a buffer that would be a different situation. https://codereview.chromium.org/422503004/diff/260001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/260001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:471: // instead. Sorry, I was confused by the original error message here. This is a bad-input case - if you ended up here then there _was_ a --window-id flag specified, but you couldn't parse a window-Id from it. So just log it as an invalid parameter and bail. https://codereview.chromium.org/422503004/diff/260001/remoting/host/single_wi... File remoting/host/single_window_input_injector_mac.cc (right): https://codereview.chromium.org/422503004/diff/260001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.cc:24: // will be local to the window. This comment really just duplicates what is described in the header, so you don't really need it. https://codereview.chromium.org/422503004/diff/260001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.cc:121: // This method is used if we are capturing a window instead of a screen. We're always doing that if this class is used, so isn't this comment redundant? https://codereview.chromium.org/422503004/diff/260001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.cc:144: // window_array. The same is true of the |bounds|. nit: |window_array| https://codereview.chromium.org/422503004/diff/260001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.cc:156: if (success) { nit: if (CGRectMakeWith...()) { ?
A few nits. https://codereview.chromium.org/422503004/diff/160001/remoting/host/host_main.cc File remoting/host/host_main.cc (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/host_main... remoting/host/host_main.cc:80: " --window-id=<id> - Specifies that a window should be streamed.\n"; On 2014/08/07 23:14:32, Wez wrote: > On 2014/08/06 20:56:17, ronakvora wrote: > > On 2014/08/06 04:10:15, Wez wrote: > > > nit: "Specifies a window to remote, instead of the whole desktop." > > > > It seems like there's been an exception made when these lines are > 80 > > characters. Is this true? > > Not sure what you mean? An exception to what? Ah, sorry, when I had increased the spacing between the options and their descriptions one of the lines that was in the code previously (before my changes) became > 80 characters. I thought it was an exception that you guys allowed through. https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:460: // CGWindowId is a uint32_t On 2014/08/07 23:14:32, Wez wrote: > On 2014/08/06 20:56:19, ronakvora wrote: > > On 2014/08/06 04:10:16, Wez wrote: > > > This comment doesn't seem relevant? > > > > i'm not sure how window ids are represented on other platforms. Will that > matter > > in the future? What if someone decides to make it a short since that's how > > windows or linux gets their window ids? > > This is cross-platform code; it shouldn't have a comment that assumes it's being > built on a particular platform - you could leave a comment to clarify that > unit32_t is large enough to hold window-ids on all of our currently-supported > platforms, if you like. Done. https://codereview.chromium.org/422503004/diff/160001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:465: // window_id_ is of type long int, so we should not lose On 2014/08/07 23:14:32, Wez wrote: > On 2014/08/06 20:56:18, ronakvora wrote: > > On 2014/08/06 04:10:16, Wez wrote: > > > Doesn't its type depend upon the platform, since it's the WindowId define? > > > > I'm not sure what the best way would be to get the window_id_ in correctly for > > all platforms. Should I have a #if defined(...) for each platform? > > What's important is the effect of truncation in precision - in this case it'll > mean we get a bogus window Id. If we were going to use it as an index into a > buffer that would be a different situation. Got it. https://codereview.chromium.org/422503004/diff/260001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/260001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:471: // instead. On 2014/08/07 23:14:33, Wez wrote: > Sorry, I was confused by the original error message here. > > This is a bad-input case - if you ended up here then there _was_ a --window-id > flag specified, but you couldn't parse a window-Id from it. So just log it as an > invalid parameter and bail. Got it. returning false now. https://codereview.chromium.org/422503004/diff/260001/remoting/host/single_wi... File remoting/host/single_window_input_injector_mac.cc (right): https://codereview.chromium.org/422503004/diff/260001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.cc:24: // will be local to the window. On 2014/08/07 23:14:33, Wez wrote: > This comment really just duplicates what is described in the header, so you > don't really need it. Removed. https://codereview.chromium.org/422503004/diff/260001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.cc:121: // This method is used if we are capturing a window instead of a screen. On 2014/08/07 23:14:33, Wez wrote: > We're always doing that if this class is used, so isn't this comment redundant? Yeah. Removed. https://codereview.chromium.org/422503004/diff/260001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.cc:144: // window_array. The same is true of the |bounds|. On 2014/08/07 23:14:33, Wez wrote: > nit: |window_array| Done. https://codereview.chromium.org/422503004/diff/260001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.cc:156: if (success) { On 2014/08/07 23:14:33, Wez wrote: > nit: if (CGRectMakeWith...()) { ? Done.
https://codereview.chromium.org/422503004/diff/280001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/280001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:470: // instead. This comment is out of date. https://codereview.chromium.org/422503004/diff/280001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:473: enable_window_capture_ = false; No need to set this flag in this case, since you're going to quit. You need to make sure that the process exits with a meaningful exit code, though.
removed enable_window_capture = false in remoting_me2me_host.cc https://codereview.chromium.org/422503004/diff/280001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/280001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:470: // instead. On 2014/08/08 22:12:13, Wez wrote: > This comment is out of date. Done. https://codereview.chromium.org/422503004/diff/280001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:473: enable_window_capture_ = false; On 2014/08/08 22:12:14, Wez wrote: > No need to set this flag in this case, since you're going to quit. > > You need to make sure that the process exits with a meaningful exit code, > though. Isn't the meaningful exit code to return false?
https://codereview.chromium.org/422503004/diff/280001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/280001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:473: enable_window_capture_ = false; On 2014/08/08 22:28:26, ronakvora wrote: > On 2014/08/08 22:12:14, Wez wrote: > > No need to set this flag in this case, since you're going to quit. > > > > You need to make sure that the process exits with a meaningful exit code, > > though. > > Isn't the meaningful exit code to return false? That's the function return code - what will the process *exit code* be when it quits because of a bad window-id parameter?
Looks mostly good. Please wait for https://codereview.chromium.org/455073004/ to land and then remove WindowCapturerScreenWrapper. https://codereview.chromium.org/422503004/diff/280001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/280001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:473: enable_window_capture_ = false; On 2014/08/11 06:40:36, Wez wrote: > On 2014/08/08 22:28:26, ronakvora wrote: > > On 2014/08/08 22:12:14, Wez wrote: > > > No need to set this flag in this case, since you're going to quit. > > > > > > You need to make sure that the process exits with a meaningful exit code, > > > though. > > > > Isn't the meaningful exit code to return false? > > That's the function return code - what will the process *exit code* be when it > quits because of a bad window-id parameter? Host should exit with kUsageExitCode after returning false here (see HostProcess::StartOnUiThread()), and that's an appropriate error code in that case. https://codereview.chromium.org/422503004/diff/320001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/320001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:356: self_(this), Add initializers for enable_window_capture_ and window_id_ here. https://codereview.chromium.org/422503004/diff/320001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:470: std::exit(EXIT_FAILURE); Don't need this. Host process should exit with kUsageExitCode after the next line returns false. https://codereview.chromium.org/422503004/diff/320001/remoting/host/single_wi... File remoting/host/single_window_input_injector.h (right): https://codereview.chromium.org/422503004/diff/320001/remoting/host/single_wi... remoting/host/single_window_input_injector.h:23: static scoped_ptr<InputInjector> Create( You need to add stubs for this method for Linux and Windows. Otherwise the host won't link. Just add _linux.cc and _win.cc files and return NULL from Create(). Also please add a warning (LOG(WARNING) << ...) when --window-id is specified in remoting_me2me_host.cc for Linux and Win to warn that window mode is not fully supported on these platforms. https://codereview.chromium.org/422503004/diff/320001/remoting/host/single_wi... File remoting/host/single_window_input_injector_mac.cc (right): https://codereview.chromium.org/422503004/diff/320001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.cc:71: CGRect windowRect = FindCGRectOfWindow(); window_rect https://codereview.chromium.org/422503004/diff/320001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.cc:83: webrtc::DesktopVector window_mouse_pos(windowRect.origin.x, call this window_pos? https://codereview.chromium.org/422503004/diff/320001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.cc:168: remove extra empty lines. https://codereview.chromium.org/422503004/diff/320001/remoting/host/window_ca... File remoting/host/window_capturer_screen_wrapper.h (right): https://codereview.chromium.org/422503004/diff/320001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.h:21: class WindowCapturerScreenWrapper : public webrtc::ScreenCapturer { You will not need this class after Wez's CL: https://codereview.chromium.org/455073004/ https://codereview.chromium.org/422503004/diff/320001/remoting/remoting_host.... File remoting/remoting_host.gypi (right): https://codereview.chromium.org/422503004/diff/320001/remoting/remoting_host.... remoting/remoting_host.gypi:237: 'host/ingle_window_desktop_environment.h', typo (gyp effectively ignores .h files in the list of files, that's why it still compiles)
Removed window capturer screen wrapper https://codereview.chromium.org/422503004/diff/280001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/280001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:473: enable_window_capture_ = false; On 2014/08/12 18:18:00, Sergey Ulanov wrote: > On 2014/08/11 06:40:36, Wez wrote: > > On 2014/08/08 22:28:26, ronakvora wrote: > > > On 2014/08/08 22:12:14, Wez wrote: > > > > No need to set this flag in this case, since you're going to quit. > > > > > > > > You need to make sure that the process exits with a meaningful exit code, > > > > though. > > > > > > Isn't the meaningful exit code to return false? > > > > That's the function return code - what will the process *exit code* be when it > > quits because of a bad window-id parameter? > > Host should exit with kUsageExitCode after returning false here (see > HostProcess::StartOnUiThread()), and that's an appropriate error code in that > case. Got it. removed. https://codereview.chromium.org/422503004/diff/320001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/320001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:470: std::exit(EXIT_FAILURE); On 2014/08/12 18:18:00, Sergey Ulanov wrote: > Don't need this. Host process should exit with kUsageExitCode after the next > line returns false. Removed. https://codereview.chromium.org/422503004/diff/320001/remoting/host/single_wi... File remoting/host/single_window_input_injector.h (right): https://codereview.chromium.org/422503004/diff/320001/remoting/host/single_wi... remoting/host/single_window_input_injector.h:23: static scoped_ptr<InputInjector> Create( On 2014/08/12 18:18:00, Sergey Ulanov wrote: > You need to add stubs for this method for Linux and Windows. Otherwise the host > won't link. Just add _linux.cc and _win.cc files and return NULL from Create(). > > Also please add a warning (LOG(WARNING) << ...) when --window-id is specified in > remoting_me2me_host.cc for Linux and Win to warn that window mode is not fully > supported on these platforms. Done. https://codereview.chromium.org/422503004/diff/320001/remoting/host/single_wi... File remoting/host/single_window_input_injector_mac.cc (right): https://codereview.chromium.org/422503004/diff/320001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.cc:71: CGRect windowRect = FindCGRectOfWindow(); On 2014/08/12 18:18:00, Sergey Ulanov wrote: > window_rect Done. https://codereview.chromium.org/422503004/diff/320001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.cc:83: webrtc::DesktopVector window_mouse_pos(windowRect.origin.x, On 2014/08/12 18:18:00, Sergey Ulanov wrote: > call this window_pos? Done. https://codereview.chromium.org/422503004/diff/320001/remoting/host/single_wi... remoting/host/single_window_input_injector_mac.cc:168: On 2014/08/12 18:18:00, Sergey Ulanov wrote: > remove extra empty lines. Done. https://codereview.chromium.org/422503004/diff/320001/remoting/host/window_ca... File remoting/host/window_capturer_screen_wrapper.h (right): https://codereview.chromium.org/422503004/diff/320001/remoting/host/window_ca... remoting/host/window_capturer_screen_wrapper.h:21: class WindowCapturerScreenWrapper : public webrtc::ScreenCapturer { On 2014/08/12 18:18:00, Sergey Ulanov wrote: > You will not need this class after Wez's CL: > https://codereview.chromium.org/455073004/ Done. https://codereview.chromium.org/422503004/diff/320001/remoting/remoting_host.... File remoting/remoting_host.gypi (right): https://codereview.chromium.org/422503004/diff/320001/remoting/remoting_host.... remoting/remoting_host.gypi:237: 'host/ingle_window_desktop_environment.h', On 2014/08/12 18:18:00, Sergey Ulanov wrote: > typo > > (gyp effectively ignores .h files in the list of files, that's why it still > compiles) Done.
https://codereview.chromium.org/422503004/diff/320001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/320001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:356: self_(this), On 2014/08/12 18:18:00, Sergey Ulanov wrote: > Add initializers for enable_window_capture_ and window_id_ here. Please address this comment https://codereview.chromium.org/422503004/diff/400001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/400001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:470: #if defined(OS_LINUX) #if doesn't need to be indented. https://codereview.chromium.org/422503004/diff/400001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:474: #if defined(OS_WIN) nit: No need to have two separate messages. It's better to use the same warning for both platforms (just to make code simpler). Just replace "on Linux" with "on Linux and Windows" in the message above.
https://codereview.chromium.org/422503004/diff/400001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/400001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:470: #if defined(OS_LINUX) On 2014/08/13 21:27:14, Sergey Ulanov wrote: > #if doesn't need to be indented. Done. https://codereview.chromium.org/422503004/diff/400001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:474: #if defined(OS_WIN) On 2014/08/13 21:27:14, Sergey Ulanov wrote: > nit: No need to have two separate messages. It's better to use the same warning > for both platforms (just to make code simpler). Just replace "on Linux" with "on > Linux and Windows" in the message above. Done.
LGTM when my comment is addressed https://codereview.chromium.org/422503004/diff/420001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/420001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:473: LOG(WARNING) << "Window capturing is not fully supported on Linux or Windows."; this still needs to be indented 4 spaces, i.e. #if defined(OS_LINUX) || defined(OS_WIN) LOG(WARNING) << .. #endif
https://codereview.chromium.org/422503004/diff/420001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/422503004/diff/420001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:473: LOG(WARNING) << "Window capturing is not fully supported on Linux or Windows."; On 2014/08/13 22:06:22, Sergey Ulanov wrote: > this still needs to be indented 4 spaces, i.e. > > #if defined(OS_LINUX) || defined(OS_WIN) > LOG(WARNING) << .. > #endif Done.
lgtm, just some tiny nits. https://codereview.chromium.org/422503004/diff/440001/remoting/host/single_wi... File remoting/host/single_window_desktop_environment.cc (right): https://codereview.chromium.org/422503004/diff/440001/remoting/host/single_wi... remoting/host/single_window_desktop_environment.cc:57: SingleWindowDesktopEnvironment::CreateInputInjector() { nit: I don't think this line should be indented - CreateVideoCapturer() above looks correct (assuming it doesn't all fit on one line). https://codereview.chromium.org/422503004/diff/440001/remoting/host/single_wi... File remoting/host/single_window_input_injector_win.cc (right): https://codereview.chromium.org/422503004/diff/440001/remoting/host/single_wi... remoting/host/single_window_input_injector_win.cc:10: webrtc::WindowId window_id, nit: 4-space indent https://codereview.chromium.org/422503004/diff/440001/remoting/remoting_host.... File remoting/remoting_host.gypi (right): https://codereview.chromium.org/422503004/diff/440001/remoting/remoting_host.... remoting/remoting_host.gypi:248: 'host/single_window_input_injector.h', nit: I think the .h should come before the _linux, _mac,.. files, to be consistent with host/usage_stats_consent.. ordering below.
https://codereview.chromium.org/422503004/diff/440001/remoting/host/single_wi... File remoting/host/single_window_desktop_environment.cc (right): https://codereview.chromium.org/422503004/diff/440001/remoting/host/single_wi... remoting/host/single_window_desktop_environment.cc:57: SingleWindowDesktopEnvironment::CreateInputInjector() { On 2014/08/13 22:32:51, Lambros wrote: > nit: I don't think this line should be indented - CreateVideoCapturer() above > looks correct (assuming it doesn't all fit on one line). Done. https://codereview.chromium.org/422503004/diff/440001/remoting/host/single_wi... File remoting/host/single_window_input_injector_win.cc (right): https://codereview.chromium.org/422503004/diff/440001/remoting/host/single_wi... remoting/host/single_window_input_injector_win.cc:10: webrtc::WindowId window_id, On 2014/08/13 22:32:51, Lambros wrote: > nit: 4-space indent Done. https://codereview.chromium.org/422503004/diff/440001/remoting/remoting_host.... File remoting/remoting_host.gypi (right): https://codereview.chromium.org/422503004/diff/440001/remoting/remoting_host.... remoting/remoting_host.gypi:248: 'host/single_window_input_injector.h', On 2014/08/13 22:32:51, Lambros wrote: > nit: I think the .h should come before the _linux, _mac,.. files, to be > consistent with host/usage_stats_consent.. ordering below. Done.
The CQ bit was checked by ronakvora@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ronakvora@google.com/422503004/460001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by ronakvora@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ronakvora@google.com/422503004/480001
The CQ bit was checked by ronakvora@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ronakvora@google.com/422503004/500001
The CQ bit was checked by ronakvora@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ronakvora@google.com/422503004/520001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
The CQ bit was checked by lambroslambrou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ronakvora@google.com/422503004/520001
Message was sent while issue was closed.
Committed patchset #27 (520001) as 289825 |
