Reworked the plumbing required to pass the client resolution to the desktop resizer.

Reworked the plumbing required to pass the client resolution to the desktop resizer.

This is mostly a renaming CL:
  - DesktopSessionParams -> ScreenResolution
  - OnClientResolutionChanged() -> SetScreenResolution().

Also added validation of ScreenResolution before passing it to session controllers.

BUG=196316
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188661

[alexeypa (please no reviews), 2013-03-15 16:27:31]

Jamie, PTAL.

Chris, please take a look at remoting/host/chromoting_messages.h. It is
basically a rename CL, plus the validation logic has been moved to a different
file.

[Jamie, 2013-03-15 18:33:24]

lgtm with nits. If you make any non-trivial changes in response to my comments,
I'll take another look.

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/daemon_p...
File remoting/host/daemon_process.cc (right):

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/daemon_p...
remoting/host/daemon_process.cc:193: LOG(ERROR) << "Invalid DPI of the remote
screen specified: "
"DPI/resolution" instead of "DPI"?

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_proxy.cc (right):

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_proxy.cc:283: void
DesktopSessionProxy::SetScreenResolution(
For a CL that's primarily a global rename, I was surprised not to see a
corresponding OnClientResolutionChanged method removed from the LHS. Was this
message not being proxied previously?

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/resizing...
File remoting/host/resizing_host_observer.cc (right):

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/resizing...
remoting/host/resizing_host_observer.cc:114: if (resolution.IsNull())
Why not IsValid() here?

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/screen_r...
File remoting/host/screen_resolution.cc (right):

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/screen_r...
remoting/host/screen_resolution.cc:10: // Default dots per inch is 96 DPI.
I think the default DPI on Mac is 72, but we ignore that. It might be worth a
comment here; talk to Wez about it.

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/screen_r...
File remoting/host/screen_resolution.h (right):

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/screen_r...
remoting/host/screen_resolution.h:34: bool IsValid() const;
I think it's worth doing a careful audit (perhaps in a separate CL) to determine
whether or not we really need both of these methods.

[alexeypa (please no reviews), 2013-03-15 20:30:41]

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/daemon_p...
File remoting/host/daemon_process.cc (right):

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/daemon_p...
remoting/host/daemon_process.cc:193: LOG(ERROR) << "Invalid DPI of the remote
screen specified: "
On 2013/03/15 18:33:24, Jamie wrote:
> "DPI/resolution" instead of "DPI"?

Done.

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_proxy.cc (right):

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_proxy.cc:283: void
DesktopSessionProxy::SetScreenResolution(
On 2013/03/15 18:33:24, Jamie wrote:
> For a CL that's primarily a global rename, I was surprised not to see a
> corresponding OnClientResolutionChanged method removed from the LHS. Was this
> message not being proxied previously?

This leaked from the follow up CL. I moved this code to that CL.

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/resizing...
File remoting/host/resizing_host_observer.cc (right):

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/resizing...
remoting/host/resizing_host_observer.cc:114: if (resolution.IsNull())
On 2013/03/15 18:33:24, Jamie wrote:
> Why not IsValid() here?

IsValid() checks for negative sizes only. I.e. it checks whether the structure
completely bogus.

InNull() additionally checks if the dimensions is an empty rectangle. 

This method expects the dimensions to be not empty.

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/screen_r...
File remoting/host/screen_resolution.cc (right):

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/screen_r...
remoting/host/screen_resolution.cc:10: // Default dots per inch is 96 DPI.
On 2013/03/15 18:33:24, Jamie wrote:
> I think the default DPI on Mac is 72, but we ignore that. It might be worth a
> comment here; talk to Wez about it.

Talked to Wez. He said it is better to have this class to always specify DPI.
Done.

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/screen_r...
File remoting/host/screen_resolution.h (right):

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/screen_r...
remoting/host/screen_resolution.h:34: bool IsValid() const;
On 2013/03/15 18:33:24, Jamie wrote:
> I think it's worth doing a careful audit (perhaps in a separate CL) to
determine
> whether or not we really need both of these methods.

We need both. We need to be able to check if the structure is valid (obviously).
There are two valid states:

- |dimensions_| is an empty rectangle. I.e. the caller does not specify what the
resolution should be. 
- |dimensions_| is a non-empty rectangle. In this case the caller knows the
exact resolution.

An example when the "empty rectangle" state is needed: when a client connects
its resolution is not yet knows, but we need to create a session already.

[Jamie, 2013-03-15 22:06:27]

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_proxy.h (right):

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_proxy.h:125: // API used to implement the
SessionController interface.
Nit: indent

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/screen_r...
File remoting/host/screen_resolution.h (right):

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/screen_r...
remoting/host/screen_resolution.h:34: bool IsValid() const;
On 2013/03/15 20:30:41, alexeypa wrote:
> On 2013/03/15 18:33:24, Jamie wrote:
> > I think it's worth doing a careful audit (perhaps in a separate CL) to
> determine
> > whether or not we really need both of these methods.
> 
> We need both. We need to be able to check if the structure is valid
(obviously).
> There are two valid states:
> 
> - |dimensions_| is an empty rectangle. I.e. the caller does not specify what
the
> resolution should be. 
> - |dimensions_| is a non-empty rectangle. In this case the caller knows the
> exact resolution.
> 
> An example when the "empty rectangle" state is needed: when a client connects
> its resolution is not yet knows, but we need to create a session already.

Fair enough; thanks for the clarification. However, I'd suggest calling it
IsEmpty instead of IsNull, since null has  a specific meaning in other contexts
that's not appropriate here.

https://chromiumcodereview.appspot.com/12678008/diff/8001/remoting/host/clien...
File remoting/host/client_session.cc (right):

https://chromiumcodereview.appspot.com/12678008/diff/8001/remoting/host/clien...
remoting/host/client_session.cc:31: const int kDefaultDPI = 96;
Why lose the anonymous namespace?

https://chromiumcodereview.appspot.com/12678008/diff/8001/remoting/host/resiz...
File remoting/host/resizing_host_observer_unittest.cc (right):

https://chromiumcodereview.appspot.com/12678008/diff/8001/remoting/host/resiz...
remoting/host/resizing_host_observer_unittest.cc:19: const int kDefaultDPI = 96;
Anonymous namespace?

https://chromiumcodereview.appspot.com/12678008/diff/8001/remoting/host/scree...
File remoting/host/screen_resolution.cc (right):

https://chromiumcodereview.appspot.com/12678008/diff/8001/remoting/host/scree...
remoting/host/screen_resolution.cc:24: // Scale the screen dimensions to new
DPI.
DCHECK dpi_ to make sure you're not going to divide by zero (since the default
ctor initializes to that)?

[palmer, 2013-03-15 22:20:29]

IPC security LGTM

[alexeypa (please no reviews), 2013-03-15 22:32:12]

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_proxy.h (right):

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_proxy.h:125: // API used to implement the
SessionController interface.
On 2013/03/15 22:06:27, Jamie wrote:
> Nit: indent

This code was removed.

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/screen_r...
File remoting/host/screen_resolution.h (right):

https://chromiumcodereview.appspot.com/12678008/diff/1/remoting/host/screen_r...
remoting/host/screen_resolution.h:34: bool IsValid() const;
On 2013/03/15 22:06:27, Jamie wrote:
> On 2013/03/15 20:30:41, alexeypa wrote:
> > On 2013/03/15 18:33:24, Jamie wrote:
> > > I think it's worth doing a careful audit (perhaps in a separate CL) to
> > determine
> > > whether or not we really need both of these methods.
> > 
> > We need both. We need to be able to check if the structure is valid
> (obviously).
> > There are two valid states:
> > 
> > - |dimensions_| is an empty rectangle. I.e. the caller does not specify what
> the
> > resolution should be. 
> > - |dimensions_| is a non-empty rectangle. In this case the caller knows the
> > exact resolution.
> > 
> > An example when the "empty rectangle" state is needed: when a client
connects
> > its resolution is not yet knows, but we need to create a session already.
> 
> Fair enough; thanks for the clarification. However, I'd suggest calling it
> IsEmpty instead of IsNull, since null has  a specific meaning in other
contexts
> that's not appropriate here.

Done.

https://chromiumcodereview.appspot.com/12678008/diff/8001/remoting/host/clien...
File remoting/host/client_session.cc (right):

https://chromiumcodereview.appspot.com/12678008/diff/8001/remoting/host/clien...
remoting/host/client_session.cc:31: const int kDefaultDPI = 96;
On 2013/03/15 22:06:27, Jamie wrote:
> Why lose the anonymous namespace?

It is not needed. Constants have file scope by default.

https://chromiumcodereview.appspot.com/12678008/diff/8001/remoting/host/resiz...
File remoting/host/resizing_host_observer_unittest.cc (right):

https://chromiumcodereview.appspot.com/12678008/diff/8001/remoting/host/resiz...
remoting/host/resizing_host_observer_unittest.cc:19: const int kDefaultDPI = 96;
On 2013/03/15 22:06:27, Jamie wrote:
> Anonymous namespace?

Not needed. See my previous comment.

https://chromiumcodereview.appspot.com/12678008/diff/8001/remoting/host/scree...
File remoting/host/screen_resolution.cc (right):

https://chromiumcodereview.appspot.com/12678008/diff/8001/remoting/host/scree...
remoting/host/screen_resolution.cc:24: // Scale the screen dimensions to new
DPI.
On 2013/03/15 22:06:27, Jamie wrote:
> DCHECK dpi_ to make sure you're not going to divide by zero (since the default
> ctor initializes to that)?

It will crash nicely even without a DCHECK(). Why bother?

[Jamie, 2013-03-15 22:33:35]

lgtm

[commit-bot: I haz the power, 2013-03-15 22:43:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/12678008/27001

[Wez, 2013-03-16 00:44:53]

nit: CL description is missing an "of" in the final line.

[Wez, 2013-03-16 01:03:25]

https://chromiumcodereview.appspot.com/12678008/diff/27001/remoting/host/scre...
File remoting/host/screen_resolution.h (right):

https://chromiumcodereview.appspot.com/12678008/diff/27001/remoting/host/scre...
remoting/host/screen_resolution.h:15: // This structure describes the screen's
dimensions and DPI.
nit: the -> a

https://chromiumcodereview.appspot.com/12678008/diff/27001/remoting/host/scre...
remoting/host/screen_resolution.h:15: // This structure describes the screen's
dimensions and DPI.
nit: his isn't actually specific to screens - it's really just a general
dimensions & density structure.  Call it e.g. ImageResolution?

https://chromiumcodereview.appspot.com/12678008/diff/27001/remoting/host/scre...
remoting/host/screen_resolution.h:16: struct ScreenResolution {
As per style-guide this should be a class, not a struct, since
ScaleDimensionsToDpi() isn't a simple initializer, setter, or validator.

https://chromiumcodereview.appspot.com/12678008/diff/27001/remoting/host/scre...
remoting/host/screen_resolution.h:21: // Returns the screen dimensions scaled
assording to the passed |new_dpi|.
typo: assording

https://chromiumcodereview.appspot.com/12678008/diff/27001/remoting/host/scre...
remoting/host/screen_resolution.h:22: SkISize ScaleDimensionsToDpi(const
SkIPoint& new_dpi) const;
The problem with general scale to DPI is that it may result in fractional
coordinates, and if applied to actually imagery it could make things blurry.  We
really only use this to normalize from potentially-high-DPI to 96dpi, and we
expect to always be called with some multiple like 192dpi, so can we instead
have a more specific function that will only scale up or down by integer amounts
to get as close as possible to 96dpi?

https://chromiumcodereview.appspot.com/12678008/diff/27001/remoting/host/scre...
remoting/host/screen_resolution.h:33: SkISize dimensions_;
As per style-guide, struct members shouldn't have trailing _.

[alexeypa (please no reviews), 2013-03-16 22:20:09]

https://chromiumcodereview.appspot.com/12678008/diff/27001/remoting/host/scre...
File remoting/host/screen_resolution.h (right):

https://chromiumcodereview.appspot.com/12678008/diff/27001/remoting/host/scre...
remoting/host/screen_resolution.h:15: // This structure describes the screen's
dimensions and DPI.
On 2013/03/16 01:03:25, Wez wrote:
> nit: the -> a

Done.

https://chromiumcodereview.appspot.com/12678008/diff/27001/remoting/host/scre...
remoting/host/screen_resolution.h:15: // This structure describes the screen's
dimensions and DPI.
On 2013/03/16 01:03:25, Wez wrote:
> nit: his isn't actually specific to screens - it's really just a general
> dimensions & density structure.  Call it e.g. ImageResolution?

I prefer to leave it as it is. I don't think is it ready to be a general-purpose
structure.

https://chromiumcodereview.appspot.com/12678008/diff/27001/remoting/host/scre...
remoting/host/screen_resolution.h:16: struct ScreenResolution {
On 2013/03/16 01:03:25, Wez wrote:
> As per style-guide this should be a class, not a struct, since
> ScaleDimensionsToDpi() isn't a simple initializer, setter, or validator.

Done.

https://chromiumcodereview.appspot.com/12678008/diff/27001/remoting/host/scre...
remoting/host/screen_resolution.h:21: // Returns the screen dimensions scaled
assording to the passed |new_dpi|.
On 2013/03/16 01:03:25, Wez wrote:
> typo: assording

Done.

https://chromiumcodereview.appspot.com/12678008/diff/27001/remoting/host/scre...
remoting/host/screen_resolution.h:22: SkISize ScaleDimensionsToDpi(const
SkIPoint& new_dpi) const;
On 2013/03/16 01:03:25, Wez wrote:
> The problem with general scale to DPI is that it may result in fractional
> coordinates, and if applied to actually imagery it could make things blurry. 
We
> really only use this to normalize from potentially-high-DPI to 96dpi, and we
> expect to always be called with some multiple like 192dpi, so can we instead
> have a more specific function that will only scale up or down by integer
amounts
> to get as close as possible to 96dpi?

This change does not belong to this CL.

https://chromiumcodereview.appspot.com/12678008/diff/27001/remoting/host/scre...
remoting/host/screen_resolution.h:33: SkISize dimensions_;
On 2013/03/16 01:03:25, Wez wrote:
> As per style-guide, struct members shouldn't have trailing _.

It is a class now.

[commit-bot: I haz the power, 2013-03-16 22:21:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/12678008/60001

[alexeypa (please no reviews), 2013-03-18 01:04:10]

Committed patchset #4 manually as r188661 (presubmit successful).