Cross-platform plumbing for resize-to-client

remoting/host/resizing_host_observer.cc

Index: remoting/host/resizing_host_observer.cc
diff --git a/remoting/host/resizing_host_observer.cc b/remoting/host/resizing_host_observer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b168676c65a64dffc62cca88ef09939b6200c963
--- /dev/null
+++ b/remoting/host/resizing_host_observer.cc
@@ -0,0 +1,129 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/host/resizing_host_observer.h"
+#include "remoting/host/desktop_resizer.h"
+
+#include <set>
+
+#include "base/logging.h"
+
+namespace remoting {
+
+ResizingHostObserver::ResizingHostObserver(
+    DesktopResizer* desktop_resizer)
+    : desktop_resizer_(desktop_resizer),
+      original_size_(SkISize::Make(0, 0)) {
+}
+
+ResizingHostObserver::~ResizingHostObserver() {
+}
+
+void ResizingHostObserver::OnClientAuthenticated(const std::string& jid) {
+  // Resize-to-client doesn't make sense if multiple clients are connected,
+  // and this is not currently supported anyway.
+  DCHECK(client_jid_.empty());
+  original_size_ = desktop_resizer_->GetCurrentSize();
+}
+
+void ResizingHostObserver::OnClientDisconnected(const std::string& jid) {
+  if (!original_size_.isZero()) {

[Wez, 2012/09/20 21:35:15]

nit: isZero -> isEmpty

[Jamie, 2012/09/20 22:59:59]

Done.

+    desktop_resizer_->SetSize(original_size_);

[Wez, 2012/09/20 21:35:15]

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.

[Jamie, 2012/09/20 22:59:59]

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.

[Wez, 2012/09/21 00:59:32]

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.

[Jamie, 2012/09/24 22:49:15]

That's a good point. I've added an explicit RestoreSize method to the interface,
with a comment explaining why it's needed.

+    original_size_.set(0, 0);
+  }
+  client_jid_.clear();
+}
+
+class SupportedSize {

[Wez, 2012/09/20 21:35:15]

SupportedSize -> CandidateSize

[Jamie, 2012/09/20 22:59:59]

Done.

+ public:
+  SupportedSize(const SkISize& actual, const SkISize& preferred)
+      : size_(actual) {
+    // The client scale factor is the smaller of the ratios between the actual
+    // and preferred widths and heights.

[Wez, 2012/09/20 21:35:15]

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.

[Wez, 2012/09/20 21:35:15]

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.

[Jamie, 2012/09/20 22:59:59]

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.

[Jamie, 2012/09/20 22:59:59]

(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.

[Wez, 2012/09/21 00:59:32]

Actually, I think swapping "actual" for "supported" would help, too.

[Jamie, 2012/09/24 22:49:15]

I've gone with |candidate|, since that's the new name for the class.

+    if ((actual.width() > preferred.width()) ||
+        (actual.height() > preferred.height())) {
+      float ratio_width =
+          static_cast<float>(preferred.width()) / actual.width();
+      float ratio_height =
+          static_cast<float>(preferred.height()) / actual.height();
+      client_scale_factor_ = std::min(ratio_width, ratio_height);
+    } else {
+      client_scale_factor_ = 1.0;
+    }
+    // The aspect ratio "goodness" is defined as being the ratio of the smaller

[Wez, 2012/09/20 21:35:15]

nit: blank line before this comment

[Wez, 2012/09/20 21:35:15]

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".

[Jamie, 2012/09/20 22:59:59]

Done.

[Jamie, 2012/09/20 22:59:59]

Done.

+    // of the actual and preferred aspect ratios to the larger. It allows two
+    // sizes to be easily compared in terms of how well the match a requested
+    // aspect ratio.
+    float actual_aspect_ratio =
+        static_cast<float>(actual.width()) / actual.height();
+    float preferred_aspect_ratio =
+        static_cast<float>(preferred.width()) / preferred.height();
+    if (actual_aspect_ratio > preferred_aspect_ratio) {
+      aspect_ratio_goodness_ = preferred_aspect_ratio / actual_aspect_ratio;
+    } else {
+      aspect_ratio_goodness_ = actual_aspect_ratio / preferred_aspect_ratio;
+    }
+  }
+
+  const SkISize& size() const { return size_; }
+  float client_scale_factor() const { return client_scale_factor_; }
+  float aspect_ratio_goodness() const { return aspect_ratio_goodness_; }
+  int64 area() const {
+    return static_cast<int64>(size_.width()) * size_.height();
+  }
+
+  bool IsBetterThan(const SupportedSize& other) const {
+    // First consider which size results in the largest scale factor at the
+    // client. Since the scale factor is at most 1.0, this implicitly prefers
+    // sizes with both width and height no larger than requested.

[Wez, 2012/09/20 21:35:15]

nit: Reword: If either size would require down-scaling, prefer the one that
down-scales the least.

[Jamie, 2012/09/20 22:59:59]

Done.

+    if (client_scale_factor() < other.client_scale_factor()) {
+      return false;
+    } else if (client_scale_factor() > other.client_scale_factor()) {
+      return true;
+    }
+    // If the scale factors are the same, pick the size with the largest area.

[Wez, 2012/09/20 21:35:15]

nit: blank before this comment

[Jamie, 2012/09/20 22:59:59]

Done.

+    if (area() < other.area()) {
+      return false;
+    } else if (area() > other.area()) {
+      return true;
+    }
+    // If the areas are equal, pick the size with the "best" aspect ratio.

[Wez, 2012/09/20 21:35:15]

nit: blank before this comment

[Jamie, 2012/09/20 22:59:59]

Done.

+    if (aspect_ratio_goodness() < other.aspect_ratio_goodness()) {
+      return false;
+    } else if (aspect_ratio_goodness() > other.aspect_ratio_goodness()) {
+      return true;
+    }
+    // If the aspect ratios are equally good (for example, comparing 640x480 to

[Wez, 2012/09/20 21:35:15]

nit: blank before this comment

[Jamie, 2012/09/20 22:59:59]

Done.

+    // 480x640 w.r.t. 640x640), just pick the widest size. This assumes that
+    // landscape aspect ratios are more common than portrait.

[Wez, 2012/09/20 21:35:15]

nit: Reword: ... pick the widest, since desktop user interfaces tend to be
designed to fit landscape aspect ratios.

[Jamie, 2012/09/20 22:59:59]

Done.

+    return size().width() > other.size().width();
+  }
+
+ private:
+  float client_scale_factor_;
+  float aspect_ratio_goodness_;
+  SkISize size_;
+};
+
+void ResizingHostObserver::OnClientDimensionsChanged(
+    const std::string& jid, const SkISize& preferred_size) {
+  std::list<SkISize> sizes =
+      desktop_resizer_->GetSupportedSizes(preferred_size);
+  if (sizes.empty()) {
+    return;
+  }
+  // If the implementation returned any sizes, pick the best one according to

[Wez, 2012/09/20 21:35:15]

nit: blank line before this comment

[Jamie, 2012/09/20 22:59:59]

Done.

+  // the algorithm described in SupportedSize::IsBetterThen.
+  SupportedSize best_size(sizes.front(), preferred_size);
+  std::list<SkISize>::iterator i = sizes.begin();
+  for (++i; i != sizes.end(); ++i) {
+    SupportedSize candidate_size(*i, preferred_size);
+    if (candidate_size.IsBetterThan(best_size)) {
+      best_size = candidate_size;
+    }
+  }
+  desktop_resizer_->SetSize(best_size.size());
+}
+
+}  // namespace remoting