Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10918224:
Cross-platform plumbing for resize-to-client (Closed)
|
Created:
8 years, 3 months ago by Jamie Modified:
8 years, 2 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, pam+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCross-platform plumbing for resize-to-client and Linux implementation.
BUG=145049
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158718
Patch Set 1 #
Total comments: 2
Patch Set 2 : Cleaned up SkISize zero check. Fixed XFlush bug. #
Total comments: 24
Patch Set 3 : Fixed Clang errors. #
Total comments: 13
Patch Set 4 : Reviewer comments. #
Total comments: 24
Patch Set 5 : Fixed comment. #Patch Set 6 : Fixed comment. #Patch Set 7 : Reviewer comments and removed Linux impl (will re-add in follow-up). #Patch Set 8 : Added missing files. #Patch Set 9 : Missed reviewer comments and trybot fixes. #
Total comments: 57
Patch Set 10 : Reviewer comments. #
Total comments: 7
Patch Set 11 : Reviewer comments, plus disable resize-to-client if overridden by user. #
Total comments: 2
Patch Set 12 : Removed anonymous namespace. #Patch Set 13 : Actually use RestoreSize API. #
Total comments: 16
Patch Set 14 : Fixed non-registration bug and addressed reviewer comments. #Patch Set 15 : Fixed typo. #
Messages
Total messages: 25 (0 generated)
Wez, please check the plumbing. Lambros, please check the Linux implementation. Note that there are no unit tests for this yet. I plan on adding some for the plumbing (at least) before submitting, but I wanted to get some feedback first. https://codereview.chromium.org/10918224/diff/1/remoting/host/client_session.h File remoting/host/client_session.h (right): https://codereview.chromium.org/10918224/diff/1/remoting/host/client_session.... remoting/host/client_session.h:80: // change. This is not currently true. The initial client size is not communicated to the host. I plan to fix this in a follow-up CL since this one currently doesn't touch anything client-side and is big enough anyway. https://codereview.chromium.org/10918224/diff/1/remoting/host/desktop_resizer... File remoting/host/desktop_resizer_linux.cc (right): https://codereview.chromium.org/10918224/diff/1/remoting/host/desktop_resizer... remoting/host/desktop_resizer_linux.cc:235: mode.name = const_cast<char*>(name); Stupid non-const APIs...
https://codereview.chromium.org/10918224/diff/13001/remoting/tools/me2me_virt... File remoting/tools/me2me_virtual_host.py (right): https://codereview.chromium.org/10918224/diff/13001/remoting/tools/me2me_virt... remoting/tools/me2me_virtual_host.py:259: xvfb = "/usr/bin/Xvfb-randr" Not directly related to this CL: maybe we can just pack Xvfb-randr in our dpk and put it in /opt/google/chrome-remote-desktop? That way we won't need to worry about installing this dependency.
A few driveby comments https://codereview.chromium.org/10918224/diff/13001/remoting/host/desktop_res... File remoting/host/desktop_resizer_linux.cc (right): https://codereview.chromium.org/10918224/diff/13001/remoting/host/desktop_res... remoting/host/desktop_resizer_linux.cc:27: // client size"). It doesn't make the code significantly more complex. Nit:Stray ) https://codereview.chromium.org/10918224/diff/13001/remoting/host/desktop_res... remoting/host/desktop_resizer_linux.cc:232: XRRModeInfo mode; These all-zero modes are only valid for Xvfb. Do we care about me2me to non-Xvfb displays? https://codereview.chromium.org/10918224/diff/13001/remoting/remoting.gyp File remoting/remoting.gyp (right): https://codereview.chromium.org/10918224/diff/13001/remoting/remoting.gyp#new... remoting/remoting.gyp:1406: '-lXext' add -lXrandr https://codereview.chromium.org/10918224/diff/13001/remoting/tools/me2me_virt... File remoting/tools/me2me_virtual_host.py (right): https://codereview.chromium.org/10918224/diff/13001/remoting/tools/me2me_virt... remoting/tools/me2me_virtual_host.py:616: parser.add_option("-s", "--size", dest="size", action="append", TODO: For this feature to be fully functional we need another way for users to specify the maximum screen size (the default size is rather small, and with the new /etc/init.d way it's hard to pass the size as a cmd line arg).
http://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resiz... File remoting/host/desktop_resizer_linux.cc (right): http://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:7: #include <X11/Xlib.h> nit: #include <string.h> for strcmp http://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:8: #include <X11/extensions/Xrandr.h> nit: alphabetize http://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:52: Handler previous_handler_; nit: DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:66: Display* display_; nit: DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:145: // Switch the primary output to the specified mode. Please could you explain what this does if |name| is NULL? I don't understand what happens in this case, or why you do it, or what the ramifications are. http://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:154: } // namespace nit: Blank line before } // namespace ? http://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:190: ScopedXGrabServer grabber(display_); nit: I think it's slightly better to put the scoped error handler inside the scoped GrabServer. That way, we change and restore the error-handler before anyone else sees anything. http://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:198: if (size.width() < min_width) { nit: How about width = std::max(width, min_width); width = std::min(width, max_width); ? In any case, s/size.width()/width/ for readability. http://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:218: // that we have to detatch the output from any mode in order to resize it nit: detatch -> detach http://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:220: // seems safe to do it regardless). Could you explain why we need both SwitchToMode and XRRSetScreenSize? Maybe SwitchToMode could be renamed SetCrtc if it only sets a new Crtc without actually switching to the new resolution? http://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:227: SwitchToMode(kModeName); Does this actually work if kModeName is the same resolution as kTempModeName? I thought the X Server would stay on kTempModeName if the sizes matched, and then we wouldn't be able to delete the temporary mode. Am I missing something? http://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:262: int number_of_outputs = 0; Not that it matters, but I think num_outputs would be fine. I know we're not supposed to abbreviate, but Chrome uses num_ prefix all over the place :) http://codereview.chromium.org/10918224/diff/13001/remoting/tools/me2me_virtu... File remoting/tools/me2me_virtual_host.py (right): http://codereview.chromium.org/10918224/diff/13001/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:257: # Xvf-randr, but that's no longer the case. Fix this once we have typo: Xvfb-randr
ptal https://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resi... File remoting/host/desktop_resizer_linux.cc (right): https://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resi... remoting/host/desktop_resizer_linux.cc:7: #include <X11/Xlib.h> On 2012/09/13 22:00:35, Lambros wrote: > nit: #include <string.h> for strcmp Done. https://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resi... remoting/host/desktop_resizer_linux.cc:8: #include <X11/extensions/Xrandr.h> On 2012/09/13 22:00:35, Lambros wrote: > nit: alphabetize Done. https://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resi... remoting/host/desktop_resizer_linux.cc:52: Handler previous_handler_; On 2012/09/13 22:00:35, Lambros wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resi... remoting/host/desktop_resizer_linux.cc:66: Display* display_; On 2012/09/13 22:00:35, Lambros wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resi... remoting/host/desktop_resizer_linux.cc:145: // Switch the primary output to the specified mode. On 2012/09/13 22:00:35, Lambros wrote: > Please could you explain what this does if |name| is NULL? I don't understand > what happens in this case, or why you do it, or what the ramifications are. TBH, neither do I, but xrandr does it, and you get Xlib errors if you don't. There's a comment below where I call this with NULL, but I'll add something here as well. https://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resi... remoting/host/desktop_resizer_linux.cc:154: } // namespace On 2012/09/13 22:00:35, Lambros wrote: > nit: Blank line before } // namespace ? Done. https://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resi... remoting/host/desktop_resizer_linux.cc:190: ScopedXGrabServer grabber(display_); On 2012/09/13 22:00:35, Lambros wrote: > nit: I think it's slightly better to put the scoped error handler inside the > scoped GrabServer. That way, we change and restore the error-handler before > anyone else sees anything. I did it this way so that errors from XGrabServer are trapped as well. I don't expect to see any, but the failure mode is severe enough that I wanted to guard against it. https://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resi... remoting/host/desktop_resizer_linux.cc:198: if (size.width() < min_width) { On 2012/09/13 22:00:35, Lambros wrote: > nit: How about > width = std::max(width, min_width); > width = std::min(width, max_width); > ? > In any case, s/size.width()/width/ for readability. Done. https://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resi... remoting/host/desktop_resizer_linux.cc:218: // that we have to detatch the output from any mode in order to resize it On 2012/09/13 22:00:35, Lambros wrote: > nit: detatch -> detach Done. https://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resi... remoting/host/desktop_resizer_linux.cc:220: // seems safe to do it regardless). On 2012/09/13 22:00:35, Lambros wrote: > Could you explain why we need both SwitchToMode and XRRSetScreenSize? Maybe > SwitchToMode could be renamed SetCrtc if it only sets a new Crtc without > actually switching to the new resolution? I don't understand why both are required, and there's no documentation. This is how xrandr does it. If you don't call XRRSetScreenSize, then the status bars all resize, but the desktop itself does not. If you call XRRSetScreenSize without setting the mode, nothing appears to happen. https://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resi... remoting/host/desktop_resizer_linux.cc:227: SwitchToMode(kModeName); On 2012/09/13 22:00:35, Lambros wrote: > Does this actually work if kModeName is the same resolution as kTempModeName? I > thought the X Server would stay on kTempModeName if the sizes matched, and then > we wouldn't be able to delete the temporary mode. Am I missing something? Yes, this works because you set the mode by explicit index. https://codereview.chromium.org/10918224/diff/2001/remoting/host/desktop_resi... remoting/host/desktop_resizer_linux.cc:262: int number_of_outputs = 0; On 2012/09/13 22:00:35, Lambros wrote: > Not that it matters, but I think num_outputs would be fine. I know we're not > supposed to abbreviate, but Chrome uses num_ prefix all over the place :) It doesn't cause any unnecessary word-wrap, so I'll leave it for now. https://codereview.chromium.org/10918224/diff/13001/remoting/host/desktop_res... File remoting/host/desktop_resizer_linux.cc (right): https://codereview.chromium.org/10918224/diff/13001/remoting/host/desktop_res... remoting/host/desktop_resizer_linux.cc:27: // client size"). It doesn't make the code significantly more complex. On 2012/09/13 21:59:05, rmsousa wrote: > Nit:Stray ) Done. https://codereview.chromium.org/10918224/diff/13001/remoting/host/desktop_res... remoting/host/desktop_resizer_linux.cc:232: XRRModeInfo mode; On 2012/09/13 21:59:05, rmsousa wrote: > These all-zero modes are only valid for Xvfb. Do we care about me2me to non-Xvfb > displays? Good point. I'll add some code to explicitly check for Xvfb-randr; for other X servers we'll want a different algorithm anyway, so I'll disable resize for now. https://codereview.chromium.org/10918224/diff/13001/remoting/remoting.gyp File remoting/remoting.gyp (right): https://codereview.chromium.org/10918224/diff/13001/remoting/remoting.gyp#new... remoting/remoting.gyp:1406: '-lXext' On 2012/09/13 21:59:05, rmsousa wrote: > add -lXrandr It must be being included somehow, otherwise it wouldn't link. But I think you're right, we should be explicit about it. https://codereview.chromium.org/10918224/diff/13001/remoting/tools/me2me_virt... File remoting/tools/me2me_virtual_host.py (right): https://codereview.chromium.org/10918224/diff/13001/remoting/tools/me2me_virt... remoting/tools/me2me_virtual_host.py:257: # Xvf-randr, but that's no longer the case. Fix this once we have On 2012/09/13 22:00:35, Lambros wrote: > typo: Xvfb-randr Done. https://codereview.chromium.org/10918224/diff/13001/remoting/tools/me2me_virt... remoting/tools/me2me_virtual_host.py:259: xvfb = "/usr/bin/Xvfb-randr" On 2012/09/13 20:52:43, sergeyu wrote: > Not directly related to this CL: maybe we can just pack Xvfb-randr in our dpk > and put it in /opt/google/chrome-remote-desktop? That way we won't need to worry > about installing this dependency. Lambros had some reason for preferring to keep the two packages separate--he can probably elaborate further, but it does seem like a cleaner way of doing it. https://codereview.chromium.org/10918224/diff/13001/remoting/tools/me2me_virt... remoting/tools/me2me_virtual_host.py:616: parser.add_option("-s", "--size", dest="size", action="append", On 2012/09/13 21:59:05, rmsousa wrote: > TODO: For this feature to be fully functional we need another way for users to > specify the maximum screen size (the default size is rather small, and with the > new /etc/init.d way it's hard to pass the size as a cmd line arg). I forgot to increase the default maximum size. You're right that we ought to provide a way to change the default, but for now I'll just increase it.
https://codereview.chromium.org/10918224/diff/13001/remoting/host/desktop_res... File remoting/host/desktop_resizer_linux.cc (right): https://codereview.chromium.org/10918224/diff/13001/remoting/host/desktop_res... remoting/host/desktop_resizer_linux.cc:232: XRRModeInfo mode; On 2012/09/14 21:07:34, Jamie wrote: > On 2012/09/13 21:59:05, rmsousa wrote: > > These all-zero modes are only valid for Xvfb. Do we care about me2me to > non-Xvfb > > displays? > > Good point. I'll add some code to explicitly check for Xvfb-randr; for other X > servers we'll want a different algorithm anyway, so I'll disable resize for now. Do you know any way to do that check reliably? If you do, can you make that a separate class (as I'll also need it for the curtain mode enforcement) FWIW, I couldn't find any way to directly test it... so I was going with the approach in http://codereview.chromium.org/10914298/patch/1/4, but that's fairly hacky (and I'm not sure if it fits here) In your particular case, I figure testing if the current mode is already all-zeroes should be enough for safety (even if it doesn't directly test for xvfb).
On 2012/09/14 21:37:05, rmsousa wrote: > https://codereview.chromium.org/10918224/diff/13001/remoting/host/desktop_res... > File remoting/host/desktop_resizer_linux.cc (right): > > https://codereview.chromium.org/10918224/diff/13001/remoting/host/desktop_res... > remoting/host/desktop_resizer_linux.cc:232: XRRModeInfo mode; > On 2012/09/14 21:07:34, Jamie wrote: > > On 2012/09/13 21:59:05, rmsousa wrote: > > > These all-zero modes are only valid for Xvfb. Do we care about me2me to > > non-Xvfb > > > displays? > > > > Good point. I'll add some code to explicitly check for Xvfb-randr; for other X > > servers we'll want a different algorithm anyway, so I'll disable resize for > now. > > Do you know any way to do that check reliably? If you do, can you make that a > separate class (as I'll also need it for the curtain mode enforcement) > > FWIW, I couldn't find any way to directly test it... so I was going with the > approach in http://codereview.chromium.org/10914298/patch/1/4, but that's fairly > hacky (and I'm not sure if it fits here) I think Lambros wrote that code. It seems like a good test for your case, but I think it's over-the-top for mine, where a command-line option is simpler.
Linux implementation lgtm http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... File remoting/host/desktop_resizer_linux.cc (right): http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:152: // primary output is disabled instead, which is required in order before nit: "in order to change" or "required before" or ...?
fyi http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... File remoting/host/desktop_resizer_linux.cc (right): http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:152: // primary output is disabled instead, which is required in order before On 2012/09/14 23:21:22, Lambros wrote: > nit: "in order to change" or "required before" or ...? Done.
Some initial comments, but mostly looks good; will re-review when the changes we discussed are in place. http://codereview.chromium.org/10918224/diff/6005/remoting/host/chromoting_ho... File remoting/host/chromoting_host.h (right): http://codereview.chromium.org/10918224/diff/6005/remoting/host/chromoting_ho... remoting/host/chromoting_host.h:119: nit: remove blank line http://codereview.chromium.org/10918224/diff/6005/remoting/host/chromoting_ho... File remoting/host/chromoting_host_unittest.cc (right): http://codereview.chromium.org/10918224/diff/6005/remoting/host/chromoting_ho... remoting/host/chromoting_host_unittest.cc:16: #include "remoting/host/desktop_resizer.h" DesktopResizer isn't referred to by this file, so either this include isn't needed, or some other file is missing the include? http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... File remoting/host/desktop_resizer.cc (right): http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... remoting/host/desktop_resizer.cc:29: desktop_resizer_->SetSize(original_size_); What happens if the user resizes the desktop themselves mid-session? Why not have the restore logic per-platform, since e.g. on Windows there's a specific flag that lets you restore it safely? http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... remoting/host/desktop_resizer.cc:35: const SkISize& size) { nit: With the client JID passed to this function you can DCHECK that there's only one client we're being asked to match at any given time. http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... File remoting/host/desktop_resizer.h (right): http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... remoting/host/desktop_resizer.h:16: class DesktopResizer { Add a comment explaining how to use this interface/class. http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... remoting/host/desktop_resizer.h:23: virtual void SetSize(const SkISize& size) = 0; As discussed, swap this for a GetSizes(preferred_size)/SetSize(size) API pair. http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... remoting/host/desktop_resizer.h:25: class HostStatusObserver : public remoting::HostStatusObserver { I think it would make sense to keep the cross-platform resizing logic (e.g. searching for a suitable dimensions to resize to from those available) in this and make it ResizingHostStatusObserver, and keep DesktopResizer as a pure interface with just the basic platform query/resize code in it. http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... File remoting/host/desktop_resizer_linux.cc (right): http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:33: namespace { nit: blank line http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:59: class ScopedXGrabServer { nit: Add comment describing what this does? http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:109: // For now, assume we're only ever interested in the first output. First output = first display? http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:110: RROutput GetOutput() { nit: GetFirstOutput? http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:178: ScopedXErrorHandler handler(ScopedXErrorHandler::Ignore); nit: blank line after this http://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resiz... remoting/host/desktop_resizer_linux.cc:224: const char* kTempModeName = "Chrome Remote Desktop temporary mode"; nit: Mode names without whitespace in them will be easier for the user to type, e.g. xrandr -s FitToClient http://codereview.chromium.org/10918224/diff/6005/remoting/host/host_status_o... File remoting/host/host_status_observer.h (right): http://codereview.chromium.org/10918224/diff/6005/remoting/host/host_status_o... remoting/host/host_status_observer.h:50: virtual void OnClientDimensionsChanged(const SkISize& size) {} Include the client jid here, so that in future we can cope with multiple clients.
PTAL. I've removed the Linux implementation and will follow-up with that once this has landed. https://codereview.chromium.org/10918224/diff/6005/remoting/host/chromoting_h... File remoting/host/chromoting_host.h (right): https://codereview.chromium.org/10918224/diff/6005/remoting/host/chromoting_h... remoting/host/chromoting_host.h:119: On 2012/09/15 22:39:08, Wez wrote: > nit: remove blank line Done. https://codereview.chromium.org/10918224/diff/6005/remoting/host/chromoting_h... File remoting/host/chromoting_host_unittest.cc (right): https://codereview.chromium.org/10918224/diff/6005/remoting/host/chromoting_h... remoting/host/chromoting_host_unittest.cc:16: #include "remoting/host/desktop_resizer.h" On 2012/09/15 22:39:08, Wez wrote: > DesktopResizer isn't referred to by this file, so either this include isn't > needed, or some other file is missing the include? Done. https://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resi... File remoting/host/desktop_resizer.cc (right): https://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resi... remoting/host/desktop_resizer.cc:29: desktop_resizer_->SetSize(original_size_); On 2012/09/15 22:39:08, Wez wrote: > What happens if the user resizes the desktop themselves mid-session? Why not > have the restore logic per-platform, since e.g. on Windows there's a specific > flag that lets you restore it safely? I think it makes more sense to restore the pre-connection resolution and having this logic in one place allows us to unit test it more easily. https://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resi... remoting/host/desktop_resizer.cc:35: const SkISize& size) { On 2012/09/15 22:39:08, Wez wrote: > nit: With the client JID passed to this function you can DCHECK that there's > only one client we're being asked to match at any given time. Done. https://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resi... File remoting/host/desktop_resizer.h (right): https://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resi... remoting/host/desktop_resizer.h:16: class DesktopResizer { On 2012/09/15 22:39:08, Wez wrote: > Add a comment explaining how to use this interface/class. Done. https://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resi... remoting/host/desktop_resizer.h:23: virtual void SetSize(const SkISize& size) = 0; On 2012/09/15 22:39:08, Wez wrote: > As discussed, swap this for a GetSizes(preferred_size)/SetSize(size) API pair. Done. https://codereview.chromium.org/10918224/diff/6005/remoting/host/desktop_resi... remoting/host/desktop_resizer.h:25: class HostStatusObserver : public remoting::HostStatusObserver { On 2012/09/15 22:39:08, Wez wrote: > I think it would make sense to keep the cross-platform resizing logic (e.g. > searching for a suitable dimensions to resize to from those available) in this > and make it ResizingHostStatusObserver, and keep DesktopResizer as a pure > interface with just the basic platform query/resize code in it. Done. https://codereview.chromium.org/10918224/diff/6005/remoting/host/host_status_... File remoting/host/host_status_observer.h (right): https://codereview.chromium.org/10918224/diff/6005/remoting/host/host_status_... remoting/host/host_status_observer.h:50: virtual void OnClientDimensionsChanged(const SkISize& size) {} On 2012/09/15 22:39:08, Wez wrote: > Include the client jid here, so that in future we can cope with multiple > clients. Done.
https://codereview.chromium.org/10918224/diff/24002/remoting/host/desktop_res... File remoting/host/desktop_resizer.h (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/desktop_res... remoting/host/desktop_resizer.h:23: // disconnection. This interface has no concept of disconnection; do you mean destruction? If so then rename the call GetOriginalSize(), since it will return the original size that will be restored, not the current size in the case that SetSize() has already been used to change it. https://codereview.chromium.org/10918224/diff/24002/remoting/host/desktop_res... remoting/host/desktop_resizer.h:32: // there are no constraints imposed by the underlying video driver. 3. Return no sizes if resize is not supported. https://codereview.chromium.org/10918224/diff/24002/remoting/host/desktop_res... remoting/host/desktop_resizer.h:35: // Set the size of the desktop. |size| is guaranteed to be one of the sizes |size| must be one of the sizes previously returned; implementations should check that. https://codereview.chromium.org/10918224/diff/24002/remoting/host/remoting_me... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/remoting_me... remoting/host/remoting_me2me_host.cc:580: host_->AddStatusObserver(new ResizingHostObserver(desktop_resizer_.get())); You also need to remove the observer at some point. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... File remoting/host/resizing_host_observer.cc (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:31: if (!original_size_.isZero()) { nit: isZero -> isEmpty https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:32: desktop_resizer_->SetSize(original_size_); This will re-set the size the host had when the client connected to it, ignoring any change that the user may have made while connected. The Windows implementation of DesktopResizer will most likely set the size with the "fullscreen" flag, which marks the change as transient and makes it easy to restore the size to whatever the current configured size is; to support that you really need an explicit RestoreOriginalSize() API on DesktopResizer. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:38: class SupportedSize { SupportedSize -> CandidateSize https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:43: // and preferred widths and heights. nit: Referring to ratio is confusing here, since one thinks of ratios as 1:2, 3:2, etc; what you're actually minimizing is the scaling _factor_ from actual to preferred width/height. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:43: // and preferred widths and heights. nit: You're clamping the scale factor at 1.0 to avoid having to explicitly check for that case in IsBetterThan(); I think it's more readable to cap here and to just add the check there. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:54: // The aspect ratio "goodness" is defined as being the ratio of the smaller nit: blank line before this comment https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:54: // The aspect ratio "goodness" is defined as being the ratio of the smaller Explain that we the best aspect ratio is the one that most closely matches the preferred aspect ratio, i.e. where the ratio between the two is 1.0. Then explain that we're arrange that ratios that diverge from that have lower "goodness". https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:79: // sizes with both width and height no larger than requested. nit: Reword: If either size would require down-scaling, prefer the one that down-scales the least. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:85: // If the scale factors are the same, pick the size with the largest area. nit: blank before this comment https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:91: // If the areas are equal, pick the size with the "best" aspect ratio. nit: blank before this comment https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:97: // If the aspect ratios are equally good (for example, comparing 640x480 to nit: blank before this comment https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:99: // landscape aspect ratios are more common than portrait. nit: Reword: ... pick the widest, since desktop user interfaces tend to be designed to fit landscape aspect ratios. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:116: // If the implementation returned any sizes, pick the best one according to nit: blank line before this comment https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... File remoting/host/resizing_host_observer.h (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.h:20: class ResizingHostObserver : public remoting::HostStatusObserver { Summarize the semantics implemented by the ResizingHostObserver in a comment here. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... File remoting/host/resizing_host_observer_unittest.cc (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer_unittest.cc:15: std::ostream& operator<<(std::ostream& os, const SkISize& size) { nit: Wrap this in an anonymous namespace. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer_unittest.cc:93: // Check that if the implementation supports exact size matching, it is used. nit: This test is no different from including all the exact-match sizes you're about to test in the list of returned preferred sizes, so I don't think the mock needs special case for it. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer_unittest.cc:97: SkISize client_sizes[] = { SkISize::Make(200, 100), SkISize::Make(100, 200), nit: The members of SkISize are public, so I think you can use simple struct initialization here, rather than SkISize::Make()? https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer_unittest.cc:103: // Check that if the implementation supports a size that is no larger than nit: ... supports sizes smaller than ... https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer_unittest.cc:119: // the requested size, then the one that maximizes the resultant scale factor nit: ... the one closest to the requested size is used ... or ... the one that would require the least down-scaling is used.
https://codereview.chromium.org/10918224/diff/24002/remoting/host/desktop_res... File remoting/host/desktop_resizer.h (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/desktop_res... remoting/host/desktop_resizer.h:23: // disconnection. On 2012/09/20 21:35:15, Wez wrote: > This interface has no concept of disconnection; do you mean destruction? If so > then rename the call GetOriginalSize(), since it will return the original size > that will be restored, not the current size in the case that SetSize() has > already been used to change it. No, I meant disconnection. I was trying to give an explanation of how it might be used, but I can remove it if you think it makes things less clear. I'd prefer not to to call it GetOriginalSize, because it's less clear what the implementation should do. https://codereview.chromium.org/10918224/diff/24002/remoting/host/desktop_res... remoting/host/desktop_resizer.h:32: // there are no constraints imposed by the underlying video driver. On 2012/09/20 21:35:15, Wez wrote: > 3. Return no sizes if resize is not supported. Done. https://codereview.chromium.org/10918224/diff/24002/remoting/host/desktop_res... remoting/host/desktop_resizer.h:35: // Set the size of the desktop. |size| is guaranteed to be one of the sizes On 2012/09/20 21:35:15, Wez wrote: > |size| must be one of the sizes previously returned; implementations should > check that. I'm not sure what you're suggesting here. Are you asking that the comment specify that implementations should check the supplied size? That would require that they cache the sizes they previously returned, which is not something they would necessarily do otherwise. Since monitor configurations can change on the fly anyway, the implementations will need to fail gracefully if they're asked to set a size that isn't supported anyway, so I'm not sure what such a check buys us. https://codereview.chromium.org/10918224/diff/24002/remoting/host/remoting_me... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/remoting_me... remoting/host/remoting_me2me_host.cc:580: host_->AddStatusObserver(new ResizingHostObserver(desktop_resizer_.get())); On 2012/09/20 21:35:15, Wez wrote: > You also need to remove the observer at some point. I don't think it ever needs to be removed, but this will leak memory as written. I've fixed the leak. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... File remoting/host/resizing_host_observer.cc (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:31: if (!original_size_.isZero()) { On 2012/09/20 21:35:15, Wez wrote: > nit: isZero -> isEmpty Done. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:32: desktop_resizer_->SetSize(original_size_); On 2012/09/20 21:35:15, Wez wrote: > This will re-set the size the host had when the client connected to it, ignoring > any change that the user may have made while connected. > > The Windows implementation of DesktopResizer will most likely set the size with > the "fullscreen" flag, which marks the change as transient and makes it easy to > restore the size to whatever the current configured size is; to support that you > really need an explicit RestoreOriginalSize() API on DesktopResizer. I don't think it's obvious that the user setting a specific size while connected means that they want to keep that size when they disconnect. For example, the client view might be too small to be useful, so I might make it a little bigger, but not full-size. I wouldn't want that change to persist when I disconnect. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:38: class SupportedSize { On 2012/09/20 21:35:15, Wez wrote: > SupportedSize -> CandidateSize Done. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:43: // and preferred widths and heights. On 2012/09/20 21:35:15, Wez wrote: > nit: Referring to ratio is confusing here, since one thinks of ratios as 1:2, > 3:2, etc; what you're actually minimizing is the scaling _factor_ from actual to > preferred width/height. I think it's clearer to use ratio for the individual measurements and factor for the overall scale factor, but I've reworded to hopefully make it clearer. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:43: // and preferred widths and heights. On 2012/09/20 21:35:15, Wez wrote: > nit: You're clamping the scale factor at 1.0 to avoid having to explicitly check > for that case in IsBetterThan(); I think it's more readable to cap here and to > just add the check there. (Assuming you mean "more readable NOT to cap here"). Since the client doesn't scale up, I think it makes more sense to clamp here. It also means that we're making the test based on integer comparisons, which feels safer. I've added a comment to explain what's going on here. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:54: // The aspect ratio "goodness" is defined as being the ratio of the smaller On 2012/09/20 21:35:15, Wez wrote: > nit: blank line before this comment Done. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:54: // The aspect ratio "goodness" is defined as being the ratio of the smaller On 2012/09/20 21:35:15, Wez wrote: > Explain that we the best aspect ratio is the one that most closely matches the > preferred aspect ratio, i.e. where the ratio between the two is 1.0. Then > explain that we're arrange that ratios that diverge from that have lower > "goodness". Done. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:79: // sizes with both width and height no larger than requested. On 2012/09/20 21:35:15, Wez wrote: > nit: Reword: If either size would require down-scaling, prefer the one that > down-scales the least. Done. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:85: // If the scale factors are the same, pick the size with the largest area. On 2012/09/20 21:35:15, Wez wrote: > nit: blank before this comment Done. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:91: // If the areas are equal, pick the size with the "best" aspect ratio. On 2012/09/20 21:35:15, Wez wrote: > nit: blank before this comment Done. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:97: // If the aspect ratios are equally good (for example, comparing 640x480 to On 2012/09/20 21:35:15, Wez wrote: > nit: blank before this comment Done. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:99: // landscape aspect ratios are more common than portrait. On 2012/09/20 21:35:15, Wez wrote: > nit: Reword: ... pick the widest, since desktop user interfaces tend to be > designed to fit landscape aspect ratios. Done. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:116: // If the implementation returned any sizes, pick the best one according to On 2012/09/20 21:35:15, Wez wrote: > nit: blank line before this comment Done. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... File remoting/host/resizing_host_observer.h (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.h:20: class ResizingHostObserver : public remoting::HostStatusObserver { On 2012/09/20 21:35:15, Wez wrote: > Summarize the semantics implemented by the ResizingHostObserver in a comment > here. Done. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... File remoting/host/resizing_host_observer_unittest.cc (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer_unittest.cc:15: std::ostream& operator<<(std::ostream& os, const SkISize& size) { On 2012/09/20 21:35:15, Wez wrote: > nit: Wrap this in an anonymous namespace. Done. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer_unittest.cc:93: // Check that if the implementation supports exact size matching, it is used. On 2012/09/20 21:35:15, Wez wrote: > nit: This test is no different from including all the exact-match sizes you're > about to test in the list of returned preferred sizes, so I don't think the mock > needs special case for it. It's different insofar as the same DesktopResizer returns something different each time you call SupportedSizes. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer_unittest.cc:97: SkISize client_sizes[] = { SkISize::Make(200, 100), SkISize::Make(100, 200), On 2012/09/20 21:35:15, Wez wrote: > nit: The members of SkISize are public, so I think you can use simple struct > initialization here, rather than SkISize::Make()? Sweet! https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer_unittest.cc:103: // Check that if the implementation supports a size that is no larger than On 2012/09/20 21:35:15, Wez wrote: > nit: ... supports sizes smaller than ... The distinction between "no larger" and "smaller" is important here. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer_unittest.cc:119: // the requested size, then the one that maximizes the resultant scale factor On 2012/09/20 21:35:15, Wez wrote: > nit: ... the one closest to the requested size is used ... or ... the one that > would require the least down-scaling is used. Done.
https://codereview.chromium.org/10918224/diff/24002/remoting/host/desktop_res... File remoting/host/desktop_resizer.h (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/desktop_res... remoting/host/desktop_resizer.h:23: // disconnection. On 2012/09/20 22:59:59, Jamie wrote: > On 2012/09/20 21:35:15, Wez wrote: > > This interface has no concept of disconnection; do you mean destruction? If > so > > then rename the call GetOriginalSize(), since it will return the original size > > that will be restored, not the current size in the case that SetSize() has > > already been used to change it. > > No, I meant disconnection. I was trying to give an explanation of how it might > be used, but I can remove it if you think it makes things less clear. I'd prefer > not to to call it GetOriginalSize, because it's less clear what the > implementation should do. As I commented elsewhere, I think you need an explicit RestoreOriginalSize() rather than fetching the original size in the resizer. https://codereview.chromium.org/10918224/diff/24002/remoting/host/desktop_res... remoting/host/desktop_resizer.h:35: // Set the size of the desktop. |size| is guaranteed to be one of the sizes On 2012/09/20 22:59:59, Jamie wrote: > On 2012/09/20 21:35:15, Wez wrote: > > |size| must be one of the sizes previously returned; implementations should > > check that. > > I'm not sure what you're suggesting here. Are you asking that the comment > specify that implementations should check the supplied size? That would require > that they cache the sizes they previously returned, which is not something they > would necessarily do otherwise. Since monitor configurations can change on the > fly anyway, the implementations will need to fail gracefully if they're asked to > set a size that isn't supported anyway, so I'm not sure what such a check buys > us. No, I'm asking that the comment state that the caller must supply one of the previously-returned sizes. And yes, implementations should check that the size is valid in the current context. https://codereview.chromium.org/10918224/diff/24002/remoting/host/remoting_me... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/remoting_me... remoting/host/remoting_me2me_host.cc:580: host_->AddStatusObserver(new ResizingHostObserver(desktop_resizer_.get())); On 2012/09/20 22:59:59, Jamie wrote: > On 2012/09/20 21:35:15, Wez wrote: > > You also need to remove the observer at some point. > > I don't think it ever needs to be removed, but this will leak memory as written. > I've fixed the leak. If it isn't removed then the ChromotingHost may outlive it, and notify a dead object, surely? https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... File remoting/host/resizing_host_observer.cc (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:32: desktop_resizer_->SetSize(original_size_); On 2012/09/20 22:59:59, Jamie wrote: > On 2012/09/20 21:35:15, Wez wrote: > > This will re-set the size the host had when the client connected to it, > ignoring > > any change that the user may have made while connected. > > > > The Windows implementation of DesktopResizer will most likely set the size > with > > the "fullscreen" flag, which marks the change as transient and makes it easy > to > > restore the size to whatever the current configured size is; to support that > you > > really need an explicit RestoreOriginalSize() API on DesktopResizer. > > I don't think it's obvious that the user setting a specific size while connected > means that they want to keep that size when they disconnect. For example, the > client view might be too small to be useful, so I might make it a little bigger, > but not full-size. I wouldn't want that change to persist when I disconnect. I can see situations where both expectations are valid. Another specific situation where this resizing is bad is exact-fit Xvfb desktops - if you resize it to its "original" size then you're resizing it every time a client connects and disconnects, which will mess up the user's window arrangement. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:43: // and preferred widths and heights. On 2012/09/20 22:59:59, Jamie wrote: > On 2012/09/20 21:35:15, Wez wrote: > > nit: Referring to ratio is confusing here, since one thinks of ratios as 1:2, > > 3:2, etc; what you're actually minimizing is the scaling _factor_ from actual > to > > preferred width/height. > > I think it's clearer to use ratio for the individual measurements and factor for > the overall scale factor, but I've reworded to hopefully make it clearer. Actually, I think swapping "actual" for "supported" would help, too. https://codereview.chromium.org/10918224/diff/39001/remoting/host/resizing_ho... File remoting/host/resizing_host_observer.cc (right): https://codereview.chromium.org/10918224/diff/39001/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:52: // Since clients do not scale up, 1.0 is the maximum. nit: up-scale (for consistency w/ use of "down-scale" elsewhere in our code) https://codereview.chromium.org/10918224/diff/39001/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:60: // the values < 1.0, it allows ratios the differ in opposite directions to typo: the -> that https://codereview.chromium.org/10918224/diff/39001/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:106: // are typically design for landscape aspect ratios. typo: design -> designed
http://codereview.chromium.org/10918224/diff/39001/remoting/host/resizing_hos... File remoting/host/resizing_host_observer.h (right): http://codereview.chromium.org/10918224/diff/39001/remoting/host/resizing_hos... remoting/host/resizing_host_observer.h:23: class ResizingHostObserver : public remoting::HostStatusObserver { nit: remoting:: here is unnecessary, as we are in remoting namespace already. http://codereview.chromium.org/10918224/diff/39001/remoting/host/resizing_hos... remoting/host/resizing_host_observer.h:40: nit: rm extra blank line.
ptal https://codereview.chromium.org/10918224/diff/24002/remoting/host/desktop_res... File remoting/host/desktop_resizer.h (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/desktop_res... remoting/host/desktop_resizer.h:23: // disconnection. On 2012/09/21 00:59:32, Wez wrote: > On 2012/09/20 22:59:59, Jamie wrote: > > On 2012/09/20 21:35:15, Wez wrote: > > > This interface has no concept of disconnection; do you mean destruction? If > > so > > > then rename the call GetOriginalSize(), since it will return the original > size > > > that will be restored, not the current size in the case that SetSize() has > > > already been used to change it. > > > > No, I meant disconnection. I was trying to give an explanation of how it might > > be used, but I can remove it if you think it makes things less clear. I'd > prefer > > not to to call it GetOriginalSize, because it's less clear what the > > implementation should do. > > As I commented elsewhere, I think you need an explicit RestoreOriginalSize() > rather than fetching the original size in the resizer. I think the method should stay, so that we can turn off auto-resize in a cross-platform way if the user changes the size mid-session. I'll remove the part about disconnection, since it's no longer used solely for that purpose. https://codereview.chromium.org/10918224/diff/24002/remoting/host/desktop_res... remoting/host/desktop_resizer.h:35: // Set the size of the desktop. |size| is guaranteed to be one of the sizes On 2012/09/21 00:59:32, Wez wrote: > On 2012/09/20 22:59:59, Jamie wrote: > > On 2012/09/20 21:35:15, Wez wrote: > > > |size| must be one of the sizes previously returned; implementations should > > > check that. > > > > I'm not sure what you're suggesting here. Are you asking that the comment > > specify that implementations should check the supplied size? That would > require > > that they cache the sizes they previously returned, which is not something > they > > would necessarily do otherwise. Since monitor configurations can change on the > > fly anyway, the implementations will need to fail gracefully if they're asked > to > > set a size that isn't supported anyway, so I'm not sure what such a check buys > > us. > > No, I'm asking that the comment state that the caller must supply one of the > previously-returned sizes. And yes, implementations should check that the size > is valid in the current context. Done. https://codereview.chromium.org/10918224/diff/24002/remoting/host/remoting_me... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/remoting_me... remoting/host/remoting_me2me_host.cc:580: host_->AddStatusObserver(new ResizingHostObserver(desktop_resizer_.get())); On 2012/09/21 00:59:32, Wez wrote: > On 2012/09/20 22:59:59, Jamie wrote: > > On 2012/09/20 21:35:15, Wez wrote: > > > You also need to remove the observer at some point. > > > > I don't think it ever needs to be removed, but this will leak memory as > written. > > I've fixed the leak. > > If it isn't removed then the ChromotingHost may outlive it, and notify a dead > object, surely? I'd forgotten that the host is reference-counted. I've changed it to use do the Add/RemoveStatusObserver in the ctor/dtor like our other observers. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... File remoting/host/resizing_host_observer.cc (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:32: desktop_resizer_->SetSize(original_size_); On 2012/09/21 00:59:32, Wez wrote: > On 2012/09/20 22:59:59, Jamie wrote: > > On 2012/09/20 21:35:15, Wez wrote: > > > This will re-set the size the host had when the client connected to it, > > ignoring > > > any change that the user may have made while connected. > > > > > > The Windows implementation of DesktopResizer will most likely set the size > > with > > > the "fullscreen" flag, which marks the change as transient and makes it easy > > to > > > restore the size to whatever the current configured size is; to support that > > you > > > really need an explicit RestoreOriginalSize() API on DesktopResizer. > > > > I don't think it's obvious that the user setting a specific size while > connected > > means that they want to keep that size when they disconnect. For example, the > > client view might be too small to be useful, so I might make it a little > bigger, > > but not full-size. I wouldn't want that change to persist when I disconnect. > > I can see situations where both expectations are valid. > > Another specific situation where this resizing is bad is exact-fit Xvfb desktops > - if you resize it to its "original" size then you're resizing it every time a > client connects and disconnects, which will mess up the user's window > arrangement. That's a good point. I've added an explicit RestoreSize method to the interface, with a comment explaining why it's needed. https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:43: // and preferred widths and heights. On 2012/09/21 00:59:32, Wez wrote: > On 2012/09/20 22:59:59, Jamie wrote: > > On 2012/09/20 21:35:15, Wez wrote: > > > nit: Referring to ratio is confusing here, since one thinks of ratios as > 1:2, > > > 3:2, etc; what you're actually minimizing is the scaling _factor_ from > actual > > to > > > preferred width/height. > > > > I think it's clearer to use ratio for the individual measurements and factor > for > > the overall scale factor, but I've reworded to hopefully make it clearer. > > Actually, I think swapping "actual" for "supported" would help, too. I've gone with |candidate|, since that's the new name for the class. https://codereview.chromium.org/10918224/diff/39001/remoting/host/resizing_ho... File remoting/host/resizing_host_observer.h (right): https://codereview.chromium.org/10918224/diff/39001/remoting/host/resizing_ho... remoting/host/resizing_host_observer.h:23: class ResizingHostObserver : public remoting::HostStatusObserver { On 2012/09/23 02:37:41, tfarina wrote: > nit: remoting:: here is unnecessary, as we are in remoting namespace already. Done. https://codereview.chromium.org/10918224/diff/39001/remoting/host/resizing_ho... remoting/host/resizing_host_observer.h:40: On 2012/09/23 02:37:41, tfarina wrote: > nit: rm extra blank line. Done.
https://codereview.chromium.org/10918224/diff/45001/remoting/host/desktop_res... File remoting/host/desktop_resizer_linux.cc (right): https://codereview.chromium.org/10918224/diff/45001/remoting/host/desktop_res... remoting/host/desktop_resizer_linux.cc:16: this is missing a virtual dtor.
https://codereview.chromium.org/10918224/diff/45001/remoting/host/desktop_res... File remoting/host/desktop_resizer_linux.cc (right): https://codereview.chromium.org/10918224/diff/45001/remoting/host/desktop_res... remoting/host/desktop_resizer_linux.cc:16: On 2012/09/24 22:52:07, tfarina wrote: > this is missing a virtual dtor. The base class has one. Is it necessary to re-declare it here? I couldn't find anything in the style guide requiring that.
On 2012/09/24 23:11:20, Jamie wrote: > The base class has one. Is it necessary to re-declare it here? I couldn't find > anything in the style guide requiring that. if you had objects to be destroyed I'd require you to add one (override the one from the base-class), but since that is not the case, I think it's fine to leave it out.
https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... File remoting/host/resizing_host_observer_unittest.cc (right): https://codereview.chromium.org/10918224/diff/24002/remoting/host/resizing_ho... remoting/host/resizing_host_observer_unittest.cc:15: std::ostream& operator<<(std::ostream& os, const SkISize& size) { On 2012/09/20 22:59:59, Jamie wrote: > On 2012/09/20 21:35:15, Wez wrote: > > nit: Wrap this in an anonymous namespace. > > Done. Undone. It seems that gtest can't see the definition if it's in an anonymous namespace.
LGTM w/ nits. https://codereview.chromium.org/10918224/diff/46017/remoting/host/desktop_res... File remoting/host/desktop_resizer.h (right): https://codereview.chromium.org/10918224/diff/46017/remoting/host/desktop_res... remoting/host/desktop_resizer.h:22: // Return the current size of the desktop, or 0x0 if resize is not supported. nit: Is the implementation _required_ to return 0x0 if resize is not supported, or can it / should it return the actual desktop size? https://codereview.chromium.org/10918224/diff/46017/remoting/host/desktop_res... remoting/host/desktop_resizer.h:36: // returned by |GetSupportedSizes|. Note that, since monitor configurations nit: Move the "since ... fly" part to the end of the sentence. https://codereview.chromium.org/10918224/diff/46017/remoting/host/desktop_res... remoting/host/desktop_resizer.h:41: // Restore the original desktop size. This is separate from |SetSize| nit: Clarify what |original| is for, e.g: "Restore the original desktop size. The caller must provide the current size reported by the DesktopResizer in |original|, as a hint. Implementations are free to ignore this, for example virtual hosts typically ignore the call to avoid unnecessary resize operations." https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... File remoting/host/resizing_host_observer.cc (right): https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:33: // and this is not currently supported anyway. nit: Suggest "This implementation assumes a single connected client, which is what the host currently supports"; in future we might support multiple clients but assume that e.g. the first to connect sets the size. https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:130: if (previous_size_.isZero()) { nit: I'd prefer that this check be above this comment, so that the comment appears directly before the |new_size != previous_size_| check, or that it come immediately after that check, which would match the order of the comment description. https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... File remoting/host/resizing_host_observer.h (right): https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... remoting/host/resizing_host_observer.h:21: // Use the specified DesktopResizer to keep the match host desktop size to the typo: Remove "keep the" https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... remoting/host/resizing_host_observer.h:22: // client view size as far as is possible. When the connection closes, restore nit: "as far as is" -> "as closely as" https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... File remoting/host/resizing_host_observer_unittest.cc (right): https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... remoting/host/resizing_host_observer_unittest.cc:157: TEST_F(ResizingHostObserverTest, ManualResize) { nit: Add a test for there being no sizes reported by the implementation, or for GetCurrentSize() returning 0x0.
fyi https://codereview.chromium.org/10918224/diff/46017/remoting/host/desktop_res... File remoting/host/desktop_resizer.h (right): https://codereview.chromium.org/10918224/diff/46017/remoting/host/desktop_res... remoting/host/desktop_resizer.h:22: // Return the current size of the desktop, or 0x0 if resize is not supported. On 2012/09/25 20:48:29, Wez wrote: > nit: Is the implementation _required_ to return 0x0 if resize is not supported, > or can it / should it return the actual desktop size? It doesn't actually matter. If it returns non-zero, then it will get asked to reset to that size when the connection closes, which it will presumably ignore. I'll change the comment to require it. https://codereview.chromium.org/10918224/diff/46017/remoting/host/desktop_res... remoting/host/desktop_resizer.h:36: // returned by |GetSupportedSizes|. Note that, since monitor configurations On 2012/09/25 20:48:29, Wez wrote: > nit: Move the "since ... fly" part to the end of the sentence. Done. https://codereview.chromium.org/10918224/diff/46017/remoting/host/desktop_res... remoting/host/desktop_resizer.h:41: // Restore the original desktop size. This is separate from |SetSize| On 2012/09/25 20:48:29, Wez wrote: > nit: Clarify what |original| is for, e.g: "Restore the original desktop size. > The caller must provide the current size reported by the DesktopResizer in > |original|, as a hint. Implementations are free to ignore this, for example > virtual hosts typically ignore the call to avoid unnecessary resize operations." Done. https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... File remoting/host/resizing_host_observer.cc (right): https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:33: // and this is not currently supported anyway. On 2012/09/25 20:48:29, Wez wrote: > nit: Suggest "This implementation assumes a single connected client, which is > what the host currently supports"; in future we might support multiple clients > but assume that e.g. the first to connect sets the size. Done. https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... remoting/host/resizing_host_observer.cc:130: if (previous_size_.isZero()) { On 2012/09/25 20:48:29, Wez wrote: > nit: I'd prefer that this check be above this comment, so that the comment > appears directly before the |new_size != previous_size_| check, or that it come > immediately after that check, which would match the order of the comment > description. I want to do this check first because it avoids an unnecessary call to GetCurrentSize. I've moved the comment. https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... File remoting/host/resizing_host_observer.h (right): https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... remoting/host/resizing_host_observer.h:21: // Use the specified DesktopResizer to keep the match host desktop size to the On 2012/09/25 20:48:29, Wez wrote: > typo: Remove "keep the" Done. https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... remoting/host/resizing_host_observer.h:22: // client view size as far as is possible. When the connection closes, restore On 2012/09/25 20:48:29, Wez wrote: > nit: "as far as is" -> "as closely as" Done. https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... File remoting/host/resizing_host_observer_unittest.cc (right): https://codereview.chromium.org/10918224/diff/46017/remoting/host/resizing_ho... remoting/host/resizing_host_observer_unittest.cc:157: TEST_F(ResizingHostObserverTest, ManualResize) { On 2012/09/25 20:48:29, Wez wrote: > nit: Add a test for there being no sizes reported by the implementation, or for > GetCurrentSize() returning 0x0. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/10918224/57020
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/10918224/57020
Change committed as 158718 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
